From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751538AbdGZMCD (ORCPT ); Wed, 26 Jul 2017 08:02:03 -0400 Received: from foss.arm.com ([217.140.101.70]:59870 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751089AbdGZMCC (ORCPT ); Wed, 26 Jul 2017 08:02:02 -0400 Date: Wed, 26 Jul 2017 13:02:08 +0100 From: Will Deacon To: Thomas Garnier Cc: Russell King - ARM Linux , Leonard Crestez , Thomas Gleixner , Catalin Marinas , Dave Martin , Chris Metcalf , Pratyush Anand , linux-arm-kernel@lists.infradead.org, LKML , Kernel Hardening Subject: Re: [PATCH 1/3] arm/syscalls: Move address limit check in loop Message-ID: <20170726120208.GB2115@arm.com> References: <20170719175900.124074-1-thgarnie@google.com> <1500978481.30745.10.camel@nxp.com> <20170725103828.GJ31807@n2100.armlinux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > 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 >> > > 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 > >> > > Signed-off-by: Thomas Garnier > >> > >> > Any comments on this patch set? > >> > >> Tested-by: Leonard Crestez > >> > >> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: will.deacon@arm.com (Will Deacon) Date: Wed, 26 Jul 2017 13:02:08 +0100 Subject: [PATCH 1/3] arm/syscalls: Move address limit check in loop In-Reply-To: References: <20170719175900.124074-1-thgarnie@google.com> <1500978481.30745.10.camel@nxp.com> <20170725103828.GJ31807@n2100.armlinux.org.uk> Message-ID: <20170726120208.GB2115@arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 > 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 >> > > 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 > >> > > Signed-off-by: Thomas Garnier > >> > >> > Any comments on this patch set? > >> > >> Tested-by: Leonard Crestez > >> > >> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Wed, 26 Jul 2017 13:02:08 +0100 From: Will Deacon Message-ID: <20170726120208.GB2115@arm.com> References: <20170719175900.124074-1-thgarnie@google.com> <1500978481.30745.10.camel@nxp.com> <20170725103828.GJ31807@n2100.armlinux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: [kernel-hardening] Re: [PATCH 1/3] arm/syscalls: Move address limit check in loop To: Thomas Garnier Cc: Russell King - ARM Linux , Leonard Crestez , Thomas Gleixner , Catalin Marinas , Dave Martin , Chris Metcalf , Pratyush Anand , linux-arm-kernel@lists.infradead.org, LKML , Kernel Hardening List-ID: 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 > 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 >> > > 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 > >> > > Signed-off-by: Thomas Garnier > >> > >> > Any comments on this patch set? > >> > >> Tested-by: Leonard Crestez > >> > >> 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