Re: [PATCH 3/3] mm/mmap: Regression fix for unmapped_area{_topdown}

[Liam R. Howlett]

* Edgecombe, Rick P <rick.p.edgecombe@xxxxxxxxx> [230414 13:53]:
> On Fri, 2023-04-14 at 13:29 -0400, Liam R. Howlett wrote:
> > * Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> [230414 13:26]:
> > > * Edgecombe, Rick P <rick.p.edgecombe@xxxxxxxxx> [230414 12:27]:
> > > > On Fri, 2023-04-14 at 10:57 -0400, Liam R. Howlett wrote:<br>
> > > > > +       tmp = mas_next(&mas, ULONG_MAX);
> > > > > +       if (tmp && (tmp->vm_flags & VM_GROWSDOWN)) {
> > > > 
> > > > Why also check VM_GROWSDOWN here (and VM_GROWSUP below)?
> > > > vm_start/end_gap() already have checks inside.
> > > 
> > > An artifact of a plan that was later abandoned.
> > > 
> > > > 
> > > > > +               if (vm_start_gap(tmp) < gap + length - 1) {
> > > > > +                       low_limit = tmp->vm_end;
> > > > > +                       mas_reset(&mas);
> > > > > +                       goto retry;
> > > > > +               }
> > > > > +       } else {
> > > > > +               tmp = mas_prev(&mas, 0);
> > > > > +               if (tmp && (tmp->vm_flags & VM_GROWSUP) &&
> > > > > +                   vm_end_gap(tmp) > gap) {
> > > > > +                       low_limit = vm_end_gap(tmp); 
> > > > > +                       mas_reset(&mas);
> > > > > +                       goto retry; 
> > > > > +               }
> > > > > +       } 
> > > > > +
> > > > 
> > > > Could it be like this?
> > > 
> > > Yes, I'll make this change.  Thanks for the suggestion.
> > 
> > 
> > Wait, I like how it is.
> > 
> > In my version, if there is a stack that is VM_GROWSDOWN there, but
> > does
> > not intercept the gap, then I won't check the prev.. in yours, we
> > will
> > never avoid checking prev.
> 
> Hmm, I see. I guess I'm thinking ahead a bit to adding the shadow stack
> guard gap, but I can always add to these vm_flags checks.
> 
> But are you sure this optimization is even possible? The old
> vma_compute_gap() had this comment:
> /*
>  * Note: in the rare case of a VM_GROWSDOWN above a VM_GROWSUP, we
>  * allow two stack_guard_gaps between them here, and when choosing
>  * an unmapped area; whereas when expanding we only require one.
>  * That's a little inconsistent, but keeps the code here simpler.
>  */

I didn't think this was possible.  ia64 (orphaned in 96ec72a3425d) did
this.

> 
> Assuming this is a real scenario, if you have VM_GROWSDOWN above and
> VM_GROWSUP below, don't you need to check the gaps for above and below?
> Again thinking about adding shadow stack guard pages, something like
> that could be a more common scenario. Not that you need to fix my out
> of tree issues, but I would probably need to adjust it to check both
> directions.
> 
> I guess there is no way to embed this inside maple tree search so we
> don't need to retry? (sorry if this is a dumb question, it's an opaque
> box to me).

Absolutely, and I'm working on this as well, but right now I'm trying
to fix my regression for this and past releases.  Handling this in the
maple tree is more involved and so there's more risk.