Re: [PATCH] mm: Update do_vmi_align_munmap() return semantics

[Liam R. Howlett]

 * Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> [230630 13:21]:
> On Fri, 30 Jun 2023 at 09:41, Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> wrote:
> >
> > I hesitate to ask considering how much trouble I've caused with the
> > 32bit map flag, but I also wonder about the stack guard now that the
> > write lock is taken for stack expansion?
> 
> Which part of it? We have a lot of special code wrt the whole thing
> that came from how we did the expansion that probably can - and should
> - be cleaned up.
> 
> For example I didn't want to go remove our ad-hoc locking, so we still
> do that "mm->page_table_lock" thing.
> 
> And I think the stack expansion does several things differently from
> the "normal" vma games in general, because it explicitly didn't want
> to use the normal "merge vma" code because it didn't do real locking.

I think this is probably rare enough to not be worth the added
complexity?

> 
> But you're talking about the general issue of having a stack guard
> area at all, _that_ isn't affected by the locking.

I was thinking about the searching for free area, which falls into this
category so the locking change doesn't help much to clean this up.

> 
> That was always a real semantic issue of "we don't want user space
> stack growth to possibly grow into another vma, and because the stack
> growing isn't strictly predictable, we need to have that guard area in
> between to catch things when they get too close".
> 
> So the stack guard isn't there to protect stack vma's from merging.
> It's there to protect users from mistakes.
> 
> And then we have all those very rare situations where we *do* want
> stacks to merge, and the guard goes away, but we currently do *not*
> call vma_merge(), and just leave it as two adjacent vma's because we
> used to only have a read-lock.
> 
> End result: I do think that doing the locking right means that we may
> be able to clean up some other code. The odd do_vmi_align_munmap()
> case is just one of the more egregious special cases.

Thanks for the insight.

Anywhere that asks for a downgrade can be changed to an unlock so here's
v2 that makes that change as well.

Thanks,
Liam