From: Hugh Dickins <hughd@google.com> To: Oleg Nesterov <oleg@redhat.com> Cc: Hugh Dickins <hughd@google.com>, Linus Torvalds <torvalds@linux-foundation.org>, Cyrill Gorcunov <gorcunov@gmail.com>, Andrey Vagin <avagin@openvz.org>, Pavel Emelyanov <xemul@virtuozzo.com>, Dmitry Safonov <dsafonov@virtuozzo.com>, Andrew Morton <akpm@linux-foundation.org>, Adrian Reber <areber@redhat.com>, Michael Kerrisk <mtk@man7.org>, Willy Tarreau <w@1wt.eu>, kernel test robot <xiaolong.ye@intel.com>, Michal Hocko <mhocko@suse.com>, LKML <linux-kernel@vger.kernel.org>, LKP <lkp@01.org>, Larry Woodman <lwoodman@redhat.com>, Rik van Riel <riel@redhat.com> Subject: Re: [lkp-robot] [mm] 1be7107fbe: kernel_BUG_at_mm/mmap.c Date: Thu, 22 Jun 2017 11:04:18 -0700 (PDT) [thread overview] Message-ID: <alpine.LSU.2.11.1706221042550.20265@eggly.anvils> (raw) In-Reply-To: <20170622151623.GB762@redhat.com> On Thu, 22 Jun 2017, Oleg Nesterov wrote: > On 06/21, Hugh Dickins wrote: > > > > On Wed, 21 Jun 2017, Linus Torvalds wrote: > > > On Wed, Jun 21, 2017 at 1:56 PM, Oleg Nesterov <oleg@redhat.com> wrote: > > > > > > > > I understand. My point is that this check was invalidated by stack-guard-page > > > > a long ago, and this means that we add the user-visible change now. > > > > > > Yeah. I guess we could consider it an *old* regression that got fixed, > > > but if people started relying on the regression... > > > > > > >> Do you have a pointer to the report for this regression? I must have missed it. > > > > > > > > See http://marc.info/?t=149794523000001&r=1&w=2 > > > > > > Ok. > > > > > > And thinking about it, while that is a silly test-case, the notion of > > > "create top-down segment, then start populating it _before_ moving the > > > stack pointer into it" is actually perfectly valid. > > > > > > So I guess checking against the stack pointer is wrong in that case - > > > at least if the stack pointer isn't inside that vma to begin with. > > > > > > So yes, removing that check looks like the right thing to do for now. > > > > > > Do you want to send me the patch if you already have a commit message etc? > > > > I have a bit of a bad feeling about this. > > > > Perhaps it's just sentimental attachment to all those weird > > and ancient stack pointer checks in arch/<some>/fault.c. > > > > We have been inconsistent: cris frv m32r m68k microblaze mn10300 > > openrisc powerpc tile um x86 have such checks, the others don't. > > So that's a good reason to delete them. > > OK, I didn't bother to check other acrhitectures, thanks... > > > But at least at the moment those checks impose some sanity: > > just a page less than we had imagined for several years. > > Once we remove them, they cannot go back. Should we now > > complicate them with an extra page of slop? > > Something like the patch below? Yes, I thought about this too. Yes, that patch (times 11 for all the architectures) would be a good conservative choice: imposing the traditional sanity check, but weakened by one page to match what we've inadvertently been doing for the last four years. Would deserve a comment (since it makes no sense in any tree by itself), but unfair to ask you to write that: I must get this mail off before a meeting, can't think what to say now. But my own preference this morning is to do nothing, until we hear more complaints and can classify them as genuine userspace breakage, as opposed to testcases surprised by a new kernel implementation. Hugh > > I simply do not know. Honestly, I do not even know why MAP_GROWSDOWN > exists. I mean, I do not understand how user-space can actually use it > to get auto-growing, the usage of MAP_GROWSDOWN in (say) criu is clear. > The main thread's stack can grow, but this is only because it is placed > at the right place, above mm->mmap_base in case of top-down layout. > > > I'm not entirely persuaded by your pre-population argument: > > it's perfectly possible to prepare a MAP_GROWSDOWN area with > > an initial size, that's populated in a normal way, before handing > > off for stack expansion - isn't it? > > Exactly. > > > I'd be interested to hear more about that (redhat internal) bug > > report that Oleg mentions: whether it gives stronger grounds for > > making this sudden change than the CRIU testcase. > > Probably not. Well, the customer reported multiple problems, but most > of them were caused by rhel-specific bugs. As for "MAP_GROWSDOWN does > not grow", most probably this was another test-case, not the real > application. I will ask and report back if this is not true. > > In short, I agree with any decision. Even with "we do not care if we > break some artificial test-cases". > > Oleg. > --- > > --- a/arch/x86/mm/fault.c > +++ b/arch/x86/mm/fault.c > @@ -1409,7 +1409,7 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code, > bad_area(regs, error_code, address); > return; > } > - if (error_code & PF_USER) { > + if ((error_code & PF_USER) && (address + PAGE_SIZE < vma->vm_start)) { > /* > * Accessing the stack below %sp is always a bug. > * The large cushion allows instructions like enter
WARNING: multiple messages have this Message-ID (diff)
From: Hugh Dickins <hughd@google.com> To: lkp@lists.01.org Subject: Re: [lkp-robot] [mm] 1be7107fbe: kernel_BUG_at_mm/mmap.c Date: Thu, 22 Jun 2017 11:04:18 -0700 [thread overview] Message-ID: <alpine.LSU.2.11.1706221042550.20265@eggly.anvils> (raw) In-Reply-To: <20170622151623.GB762@redhat.com> [-- Attachment #1: Type: text/plain, Size: 4340 bytes --] On Thu, 22 Jun 2017, Oleg Nesterov wrote: > On 06/21, Hugh Dickins wrote: > > > > On Wed, 21 Jun 2017, Linus Torvalds wrote: > > > On Wed, Jun 21, 2017 at 1:56 PM, Oleg Nesterov <oleg@redhat.com> wrote: > > > > > > > > I understand. My point is that this check was invalidated by stack-guard-page > > > > a long ago, and this means that we add the user-visible change now. > > > > > > Yeah. I guess we could consider it an *old* regression that got fixed, > > > but if people started relying on the regression... > > > > > > >> Do you have a pointer to the report for this regression? I must have missed it. > > > > > > > > See http://marc.info/?t=149794523000001&r=1&w=2 > > > > > > Ok. > > > > > > And thinking about it, while that is a silly test-case, the notion of > > > "create top-down segment, then start populating it _before_ moving the > > > stack pointer into it" is actually perfectly valid. > > > > > > So I guess checking against the stack pointer is wrong in that case - > > > at least if the stack pointer isn't inside that vma to begin with. > > > > > > So yes, removing that check looks like the right thing to do for now. > > > > > > Do you want to send me the patch if you already have a commit message etc? > > > > I have a bit of a bad feeling about this. > > > > Perhaps it's just sentimental attachment to all those weird > > and ancient stack pointer checks in arch/<some>/fault.c. > > > > We have been inconsistent: cris frv m32r m68k microblaze mn10300 > > openrisc powerpc tile um x86 have such checks, the others don't. > > So that's a good reason to delete them. > > OK, I didn't bother to check other acrhitectures, thanks... > > > But at least at the moment those checks impose some sanity: > > just a page less than we had imagined for several years. > > Once we remove them, they cannot go back. Should we now > > complicate them with an extra page of slop? > > Something like the patch below? Yes, I thought about this too. Yes, that patch (times 11 for all the architectures) would be a good conservative choice: imposing the traditional sanity check, but weakened by one page to match what we've inadvertently been doing for the last four years. Would deserve a comment (since it makes no sense in any tree by itself), but unfair to ask you to write that: I must get this mail off before a meeting, can't think what to say now. But my own preference this morning is to do nothing, until we hear more complaints and can classify them as genuine userspace breakage, as opposed to testcases surprised by a new kernel implementation. Hugh > > I simply do not know. Honestly, I do not even know why MAP_GROWSDOWN > exists. I mean, I do not understand how user-space can actually use it > to get auto-growing, the usage of MAP_GROWSDOWN in (say) criu is clear. > The main thread's stack can grow, but this is only because it is placed > at the right place, above mm->mmap_base in case of top-down layout. > > > I'm not entirely persuaded by your pre-population argument: > > it's perfectly possible to prepare a MAP_GROWSDOWN area with > > an initial size, that's populated in a normal way, before handing > > off for stack expansion - isn't it? > > Exactly. > > > I'd be interested to hear more about that (redhat internal) bug > > report that Oleg mentions: whether it gives stronger grounds for > > making this sudden change than the CRIU testcase. > > Probably not. Well, the customer reported multiple problems, but most > of them were caused by rhel-specific bugs. As for "MAP_GROWSDOWN does > not grow", most probably this was another test-case, not the real > application. I will ask and report back if this is not true. > > In short, I agree with any decision. Even with "we do not care if we > break some artificial test-cases". > > Oleg. > --- > > --- a/arch/x86/mm/fault.c > +++ b/arch/x86/mm/fault.c > @@ -1409,7 +1409,7 @@ __do_page_fault(struct pt_regs *regs, unsigned long error_code, > bad_area(regs, error_code, address); > return; > } > - if (error_code & PF_USER) { > + if ((error_code & PF_USER) && (address + PAGE_SIZE < vma->vm_start)) { > /* > * Accessing the stack below %sp is always a bug. > * The large cushion allows instructions like enter
next prev parent reply other threads:[~2017-06-22 18:04 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-06-21 2:35 [lkp-robot] [mm] 1be7107fbe: kernel_BUG_at_mm/mmap.c kernel test robot 2017-06-21 2:35 ` kernel test robot 2017-06-21 2:41 ` Hugh Dickins 2017-06-21 2:41 ` Hugh Dickins 2017-06-21 18:29 ` Linus Torvalds 2017-06-21 18:29 ` Linus Torvalds 2017-06-21 19:33 ` Oleg Nesterov 2017-06-21 19:33 ` Oleg Nesterov 2017-06-21 19:39 ` Linus Torvalds 2017-06-21 19:39 ` Linus Torvalds 2017-06-21 20:27 ` Oleg Nesterov 2017-06-21 20:27 ` Oleg Nesterov 2017-06-21 20:30 ` Linus Torvalds 2017-06-21 20:30 ` Linus Torvalds 2017-06-21 20:56 ` Oleg Nesterov 2017-06-21 20:56 ` Oleg Nesterov 2017-06-21 22:19 ` Linus Torvalds 2017-06-21 22:19 ` Linus Torvalds 2017-06-22 1:07 ` Hugh Dickins 2017-06-22 1:07 ` Hugh Dickins 2017-06-22 10:58 ` Dmitry Safonov 2017-06-22 10:58 ` Dmitry Safonov 2017-06-22 15:16 ` Oleg Nesterov 2017-06-22 15:16 ` Oleg Nesterov 2017-06-22 18:04 ` Hugh Dickins [this message] 2017-06-22 18:04 ` Hugh Dickins 2017-06-22 20:51 ` Oleg Nesterov 2017-06-22 20:51 ` Oleg Nesterov 2017-06-22 4:23 ` Hugh Dickins 2017-06-22 4:23 ` Hugh Dickins 2017-06-21 19:39 ` Hugh Dickins 2017-06-21 19:39 ` Hugh Dickins
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=alpine.LSU.2.11.1706221042550.20265@eggly.anvils \ --to=hughd@google.com \ --cc=akpm@linux-foundation.org \ --cc=areber@redhat.com \ --cc=avagin@openvz.org \ --cc=dsafonov@virtuozzo.com \ --cc=gorcunov@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=lkp@01.org \ --cc=lwoodman@redhat.com \ --cc=mhocko@suse.com \ --cc=mtk@man7.org \ --cc=oleg@redhat.com \ --cc=riel@redhat.com \ --cc=torvalds@linux-foundation.org \ --cc=w@1wt.eu \ --cc=xemul@virtuozzo.com \ --cc=xiaolong.ye@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.