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

[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 <leonard.crestez@nxp.com>
Signed-off-by: Thomas Garnier <thgarnie@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 {
-- 
2.14.0.rc0.284.gd933b75aa4-goog

[PATCH 2/3] arm/syscalls: Optimize work flags assembly check

[Thomas Garnier]

Remove the double branch and use tsteq instead.

Suggested-by: Russell King <linux@armlinux.org.uk>
Signed-off-by: Thomas Garnier <thgarnie@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
-- 
2.14.0.rc0.284.gd933b75aa4-goog

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

[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 <leonard.crestez@nxp.com>
Signed-off-by: Thomas Garnier <thgarnie@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 {
-- 
2.14.0.rc0.284.gd933b75aa4-goog

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

[Thomas Garnier]

On Wed, Jul 19, 2017 at 10:58 AM, Thomas Garnier <thgarnie@google.com> wrote:
> 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 <leonard.crestez@nxp.com>
> Signed-off-by: Thomas Garnier <thgarnie@google.com>

Any comments on this patch set?

> ---
>  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
>



-- 
Thomas

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

[Leonard Crestez]

On Mon, 2017-07-24 at 10:07 -0700, Thomas Garnier wrote:
> On Wed, Jul 19, 2017 at 10:58 AM, Thomas Garnier <thgarnie@google.com
> > wrote:
> > 
> > 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 <leonard.crestez@nxp.com>
> > Signed-off-by: Thomas Garnier <thgarnie@google.com>

> Any comments on this patch set?

Tested-by: Leonard Crestez <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.

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

[Russell King - ARM Linux]

On Tue, Jul 25, 2017 at 01:28:01PM +0300, Leonard Crestez wrote:
> On Mon, 2017-07-24 at 10:07 -0700, Thomas Garnier wrote:
> > On Wed, Jul 19, 2017 at 10:58 AM, Thomas Garnier <thgarnie@google.com
> > > wrote:
> > > 
> > > 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 <leonard.crestez@nxp.com>
> > > Signed-off-by: Thomas Garnier <thgarnie@google.com>
> 
> > Any comments on this patch set?
> 
> Tested-by: Leonard Crestez <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.

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.

-- 
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.

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

[Thomas Garnier]

On Tue, Jul 25, 2017 at 3:38 AM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Tue, Jul 25, 2017 at 01:28:01PM +0300, Leonard Crestez wrote:
>> On Mon, 2017-07-24 at 10:07 -0700, Thomas Garnier wrote:
>> > On Wed, Jul 19, 2017 at 10:58 AM, Thomas Garnier <thgarnie@google.com
>> > > wrote:
>> > >
>> > > 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 <leonard.crestez@nxp.com>
>> > > Signed-off-by: Thomas Garnier <thgarnie@google.com>
>>
>> > Any comments on this patch set?
>>
>> Tested-by: Leonard Crestez <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.
>
> 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?

>
> --
> 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.



-- 
Thomas

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

[Will Deacon]

On Tue, Jul 25, 2017 at 01:01:17PM -0700, Thomas Garnier wrote:
> On Tue, Jul 25, 2017 at 3:38 AM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > On Tue, Jul 25, 2017 at 01:28:01PM +0300, Leonard Crestez wrote:
> >> On Mon, 2017-07-24 at 10:07 -0700, Thomas Garnier wrote:
> >> > On Wed, Jul 19, 2017 at 10:58 AM, Thomas Garnier <thgarnie@google.com
> >> > > wrote:
> >> > >
> >> > > 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 <leonard.crestez@nxp.com>
> >> > > Signed-off-by: Thomas Garnier <thgarnie@google.com>
> >>
> >> > Any comments on this patch set?
> >>
> >> Tested-by: Leonard Crestez <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.
> >
> > 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?

It's probably less of an issue for arm64 because we don't take alignment
faults from the kernel and I think the perf case would resolve itself by
throttling the event. However, I also don't see the advantage of doing
this in the work loop as opposed to leaving it until we're actually doing
the return to userspace.

I looked to see what you've done for x86, but it looks like you check/clear
the flag before the work pending loop (exit_to_usermode_loop), which
subsequently re-enables interrupts and exits when
EXIT_TO_USERMODE_LOOP_FLAGS are all clear. Since TIF_FSCHECK isn't included
in those flags, what stops it being set again by an irq and remaining set
for the return to userspace?

Will

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

[Thomas Garnier]

On Wed, Jul 26, 2017 at 5:02 AM, Will Deacon <will.deacon@arm.com> wrote:
> On Tue, Jul 25, 2017 at 01:01:17PM -0700, Thomas Garnier wrote:
>> On Tue, Jul 25, 2017 at 3:38 AM, Russell King - ARM Linux
>> <linux@armlinux.org.uk> wrote:
>> > On Tue, Jul 25, 2017 at 01:28:01PM +0300, Leonard Crestez wrote:
>> >> On Mon, 2017-07-24 at 10:07 -0700, Thomas Garnier wrote:
>> >> > On Wed, Jul 19, 2017 at 10:58 AM, Thomas Garnier <thgarnie@google.com
>> >> > > wrote:
>> >> > >
>> >> > > 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 <leonard.crestez@nxp.com>
>> >> > > Signed-off-by: Thomas Garnier <thgarnie@google.com>
>> >>
>> >> > Any comments on this patch set?
>> >>
>> >> Tested-by: Leonard Crestez <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.
>> >
>> > 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?
>
> It's probably less of an issue for arm64 because we don't take alignment
> faults from the kernel and I think the perf case would resolve itself by
> throttling the event. However, I also don't see the advantage of doing
> this in the work loop as opposed to leaving it until we're actually doing
> the return to userspace.

I think the idea is doing the check as early as possible to catch any
error before any additional logic.

>
> I looked to see what you've done for x86, but it looks like you check/clear
> the flag before the work pending loop (exit_to_usermode_loop), which
> subsequently re-enables interrupts and exits when
> EXIT_TO_USERMODE_LOOP_FLAGS are all clear. Since TIF_FSCHECK isn't included
> in those flags, what stops it being set again by an irq and remaining set
> for the return to userspace?

Nothing, I plan to improve the x86 logic later. I focused on ARM/ARM64
right now based on Leonard report.

For the next iteration, I plan on having an updated version of the
previous implementation for ARM. I will send it soon.

>
> Will



-- 
Thomas

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

[Russell King - ARM Linux]

On Wed, Jul 26, 2017 at 07:20:22AM -0700, Thomas Garnier wrote:
> On Wed, Jul 26, 2017 at 5:02 AM, Will Deacon <will.deacon@arm.com> wrote:
> > I looked to see what you've done for x86, but it looks like you check/clear
> > the flag before the work pending loop (exit_to_usermode_loop), which
> > subsequently re-enables interrupts and exits when
> > EXIT_TO_USERMODE_LOOP_FLAGS are all clear. Since TIF_FSCHECK isn't included
> > in those flags, what stops it being set again by an irq and remaining set
> > for the return to userspace?
> 
> Nothing, I plan to improve the x86 logic later. I focused on ARM/ARM64
> right now based on Leonard report.

Hmm.  In this case, I'd suggest concentrating on x86 and getting the
implementation correct there before porting it to other architectures.

If x86 were to check TIF_FSCHECK in the loop, and repeat until clear,
would x86 also end up in these infinite loops that have been reported
on ARM as well?

I strongly suggest testing the behaviour with kprobes/tracing enabled
for a function called from the work pending loop, and checking how
that behaves.

-- 
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.

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

[Thomas Garnier]

On Wed, Jul 26, 2017 at 11:25 AM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Wed, Jul 26, 2017 at 07:20:22AM -0700, Thomas Garnier wrote:
>> On Wed, Jul 26, 2017 at 5:02 AM, Will Deacon <will.deacon@arm.com> wrote:
>> > I looked to see what you've done for x86, but it looks like you check/clear
>> > the flag before the work pending loop (exit_to_usermode_loop), which
>> > subsequently re-enables interrupts and exits when
>> > EXIT_TO_USERMODE_LOOP_FLAGS are all clear. Since TIF_FSCHECK isn't included
>> > in those flags, what stops it being set again by an irq and remaining set
>> > for the return to userspace?
>>
>> Nothing, I plan to improve the x86 logic later. I focused on ARM/ARM64
>> right now based on Leonard report.
>
> Hmm.  In this case, I'd suggest concentrating on x86 and getting the
> implementation correct there before porting it to other architectures.

I think the ARM architecture is different.

>
> If x86 were to check TIF_FSCHECK in the loop, and repeat until clear,
> would x86 also end up in these infinite loops that have been reported
> on ARM as well?

If for every loop set_fs was called. I think the idea is that at some
point only the TIF_FSCHECK remain. I don't think x86 suffers from the
same issue than ARM.

>
> I strongly suggest testing the behaviour with kprobes/tracing enabled
> for a function called from the work pending loop, and checking how
> that behaves.
>
> --
> 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.



-- 
Thomas