[PATCH 1/3] arm/syscalls: Move address limit check in loop

[Thomas Garnier]

The work pending loop can call set_fs after addr_limit_user_check
removed the _TIF_FSCHECK flag. To prevent the infinite loop, move
the addr_limit_user_check call at the beginning of the loop.

Fixes: 73ac5d6a2b6a ("arm/syscalls: Check address limit on user-mode return")
Reported-by: Leonard Crestez <***@nxp.com>
Signed-off-by: Thomas Garnier <***@google.com>
---
arch/arm/kernel/signal.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index 3a48b54c6405..f4574287d14b 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -573,10 +573,10 @@ do_work_pending(struct pt_regs *regs, unsigned int thread_flags, int syscall)
*/
trace_hardirqs_off();

- /* Check valid user FS if needed */
- addr_limit_user_check();
-
do {
+ /* Check valid user FS if needed */
+ addr_limit_user_check();
+
if (likely(thread_flags & _TIF_NEED_RESCHED)) {
schedule();
} else {

[Thomas Garnier]

Remove the double branch and use tsteq instead.

Suggested-by: Russell King <***@armlinux.org.uk>
Signed-off-by: Thomas Garnier <***@google.com>
---
arch/arm/kernel/entry-common.S | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index e33c32d56193..6e094b639d83 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -42,8 +42,7 @@ ret_fast_syscall:
disable_irq_notrace @ disable interrupts
ldr r1, [tsk, #TI_FLAGS] @ re-check for syscall tracing
tst r1, #_TIF_SYSCALL_WORK
- bne fast_work_pending
- tst r1, #_TIF_WORK_MASK
+ tsteq r1, #_TIF_WORK_MASK
bne fast_work_pending

/* perform architecture specific actions before user return */
@@ -70,14 +69,12 @@ ret_fast_syscall:
disable_irq_notrace @ disable interrupts
ldr r1, [tsk, #TI_FLAGS] @ re-check for syscall tracing
tst r1, #_TIF_SYSCALL_WORK
- bne fast_work_pending
- tst r1, #_TIF_WORK_MASK
+ tsteq r1, #_TIF_WORK_MASK
beq no_work_pending
UNWIND(.fnend )
ENDPROC(ret_fast_syscall)

/* Slower path - fall through to work_pending */
-fast_work_pending:
#endif

tst r1, #_TIF_SYSCALL_WORK

[Thomas Garnier]

The original bug was reported on arm but I am fixing arm64 too because
it has a similar code pattern.

The work pending loop can call set_fs after addr_limit_user_check
removed the _TIF_FSCHECK flag. To prevent the infinite loop, move the
addr_limit_user_check call at the beginning of the loop.

Fixes: cf7de27ab351 ("arm64/syscalls: Check address limit on user-mode return")
Reported-by: Leonard Crestez <***@nxp.com>
Signed-off-by: Thomas Garnier <***@google.com>
---
arch/arm64/kernel/signal.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index e3e3293d1123..8e2705983e1d 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -751,10 +751,10 @@ asmlinkage void do_notify_resume(struct pt_regs *regs,
*/
trace_hardirqs_off();

- /* Check valid user FS if needed */
- addr_limit_user_check();
-
do {
+ /* Check valid user FS if needed */
+ addr_limit_user_check();
+
if (thread_flags & _TIF_NEED_RESCHED) {
schedule();
} else {

[Thomas Garnier]

Post by Thomas Garnier
The work pending loop can call set_fs after addr_limit_user_check
removed the _TIF_FSCHECK flag. To prevent the infinite loop, move
the addr_limit_user_check call at the beginning of the loop.
Fixes: 73ac5d6a2b6a ("arm/syscalls: Check address limit on user-mode return")

Any comments on this patch set?

Post by Thomas Garnier
---
arch/arm/kernel/signal.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index 3a48b54c6405..f4574287d14b 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -573,10 +573,10 @@ do_work_pending(struct pt_regs *regs, unsigned int thread_flags, int syscall)
*/
trace_hardirqs_off();
- /* Check valid user FS if needed */
- addr_limit_user_check();
-
do {
+ /* Check valid user FS if needed */
+ addr_limit_user_check();
+
if (likely(thread_flags & _TIF_NEED_RESCHED)) {
schedule();
} else {
--
2.14.0.rc0.284.gd933b75aa4-goog

[Leonard Crestez]

Post by Thomas Garnier

Post by Thomas Garnier
The work pending loop can call set_fs after addr_limit_user_check
removed the _TIF_FSCHECK flag. To prevent the infinite loop, move
the addr_limit_user_check call at the beginning of the loop.
Fixes: 73ac5d6a2b6a ("arm/syscalls: Check address limit on user-
mode return")

Any comments on this patch set?

Tested-by: Leonard Crestez <***@nxp.com>

This appears to fix the original issue of failing to boot from NFS when
there are lots of alignment faults. But this is a very basic test
relative to the reach of this change.

However the original patch has been in linux-next for a while and
apparently nobody else noticed system calls randomly hanging on arm.

I assume maintainers need to give their opinion.

[Russell King - ARM Linux]

Post by Leonard Crestez

Post by Thomas Garnier

Post by Thomas Garnier
The work pending loop can call set_fs after addr_limit_user_check
removed the _TIF_FSCHECK flag. To prevent the infinite loop, move
the addr_limit_user_check call at the beginning of the loop.
Fixes: 73ac5d6a2b6a ("arm/syscalls: Check address limit on user-
mode return")

Any comments on this patch set?

This appears to fix the original issue of failing to boot from NFS when
there are lots of alignment faults. But this is a very basic test
relative to the reach of this change.
However the original patch has been in linux-next for a while and
apparently nobody else noticed system calls randomly hanging on arm.
I assume maintainers need to give their opinion.

I've already stated my opinion, which is different from what Linus has
requested of Thomas. IMHO, the current approach is going to keep on
causing problems along the lines that I've already pointed out.

[Thomas Garnier]

On Tue, Jul 25, 2017 at 3:38 AM, Russell King - ARM Linux

Post by Russell King - ARM Linux

Post by Leonard Crestez

Post by Thomas Garnier

Post by Thomas Garnier
The work pending loop can call set_fs after addr_limit_user_check
removed the _TIF_FSCHECK flag. To prevent the infinite loop, move
the addr_limit_user_check call at the beginning of the loop.
Fixes: 73ac5d6a2b6a ("arm/syscalls: Check address limit on user-
mode return")

Any comments on this patch set?

This appears to fix the original issue of failing to boot from NFS when
there are lots of alignment faults. But this is a very basic test
relative to the reach of this change.
However the original patch has been in linux-next for a while and
apparently nobody else noticed system calls randomly hanging on arm.
I assume maintainers need to give their opinion.

I've already stated my opinion, which is different from what Linus has
requested of Thomas. IMHO, the current approach is going to keep on
causing problems along the lines that I've already pointed out.

I understand. Do you think this problem apply to arm64 as well?

Post by Russell King - ARM Linux
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.