Re: Linux 4.12-rc6

[Hugh Dickins <hughd@google.com>]

On Tue, 20 Jun 2017, Linus Torvalds wrote:
> On Tue, Jun 20, 2017 at 8:26 AM, Dave Jones <davej@codemonkey.org.uk> wrote:
> >  > Hugh Dickins (1):
> >  >       mm: larger stack guard gap, between vmas
> >
> > This seems to be buggered.
> >
> > 002331 00000396712307 0 2 kernel BUG at mm/mmap.c:1963!
> > 002332 00000396712414 0 4 invalid opcode: 0000 [#1] SMP DEBUG_PAGEALLOC
> > 002333 00000396712541 0 4 CPU: 0 PID: 4572 Comm: trinity-c41 Not tainted 4.12.0-rc6-think+ #1
> > 002336 00000396712959 0 4 RIP: 0010:unmapped_area_topdown+0xa5/0x170
> 
> Dave, do you have instructions for Hugh to recreate that with trinity
> (or perhaps some way to generate a test-case from trinity?). Or does
> it trigger easily by just running trinity?
> 
> I'm in China right now, and will be traveling again this afternoon, so
> I probably can't look at it myself until later, but hopefully Hugh has
> the cycles to follow up in it..
> 
> Hugh? The changes to unmapped_area_topdown() look trivial, but
> obviously there's something wrong there.  The code decodes to
> 
>         49 39 c0                cmp    %rax,%r8
>         76 d0                   jbe    0xfffffffffffffffb
> *       0f 0b                   ud2             <-- trapping instruction
> 
> so from the
> 
>         VM_BUG_ON(gap_end < gap_start);
> 
> we have gap_start/end in %r8 and %rax respectively, which are:
> 
>   R08: 00007f7d54673000
>   RAX: 00007f7d543d6000
> 
> so yes, gap_start is bigger than gap_end there by quite a degree (more
> than the 1MB of the gap size unless I looked at it wrong).
> 
> Hmm. Maybe it's this:
> 
>                 /* Check if current node has a suitable gap */
>                 gap_end = vm_start_gap(vma);
>                 if (gap_end < low_limit)
>                         return -ENOMEM;
>                 if (gap_start <= high_limit && gap_end - gap_start >= length)
>                         goto found;
> 
> where it used to be that gap_end was guaranteed to be after gap_start,
> but that's no longer true. We have
> 
>     gap_start = vma->vm_prev ? vm_end_gap(vma->vm_prev) : 0;
>     gap_end = vm_start_gap(vma);
> 
> and by using MAP_FIXED, you can end up in the situation that
> "vma->vm_prev" is closer to vma than the gap size.
> 
> So now gap_end - gap_start will underflow, and then the logic that
> does "goto found" thinks it  found a hole that is larger than
> "length", when in actual fact it found a "negative-size" hole.
> 
> So maybe that "goto found" condition should have an additional test
> for "gap_end > gap_start"?
> 
> Or maybe I'm just hallucinating and missed something. Hugh, Oleg,
> Michal, can you take another look and double-check this logic?

My first impression is that you've got right to the heart of it,
before I even started looking.  I'll go over that area more carefully
now, in case there are other such instances, and post a test patch
for Dave perhaps to try - but probably he's shut down now, so I'll
then grab a trinity, and see what luck I have with it.

Hugh