Re: [PATCH 3.10 268/268] mm: larger stack guard gap, between vmas

[Willy Tarreau]

On Wed, Jun 21, 2017 at 12:05:07AM -0700, Hugh Dickins wrote:
> On Mon, 19 Jun 2017, Willy Tarreau wrote:
> 
> > From: Hugh Dickins <hughd@xxxxxxxxxx>
> > 
> > commit 1be7107fbe18eed3e319a6c3e83c78254b693acb upstream.
> 
> Some of these suggested adjustments below are just what comparing mine
> and yours showed up, and I'm being anal in passing them on e.g. I do
> like your blank line in mm.h, but Michal chose to leave it out, and
> I think that the closer we keep these sources to each other,
> the less trouble we shall have patching on top in future.

I totally agree, that's what I generally focus on as well.

> Which is particularly true in expand_upwards() and expand_downwards()
> (and you're thinking of backporting Helge's TASK_SIZE enhancement
> on top of that, though I don't think it's strictly necessary for a
> stable tree).

I thought it was a fix for a corner case on PARISC, so just in case
I'd rather stick as close as possible to mainline : at least we want
to ensure the same bugs are met everywhere so that we can benefit
from developers' help when issues are met.

> Your patch is not wrong there: though odd to be trying
> anon_vma_prepare() twice in expand_downwards(),

Ah crap, the second one is a leftover from initial code that I missed.

> and tiresome to have to unlock at each error exit.

Oh I'm seeing that you could move it later, I wasn't sure about this
one. Thanks. I think I did the same stuff in the 3.16 backport.

> But I'd already decided in one of our
> internal trees just to factor in some of Konstantin's change, that
> made the ordering much more sensible there, and the two more like
> each other; so recommend that 3.10 do the same, keeping it closer
> to the final 4.12 code.  But you may have different priorities and
> disagree with that: just suggesting.

No, I perfectly agree with you. As I mentionned, my patches were
proposals based on what I understood from the code, I'm really glad
to receive your help and fixes here!

> And there is the possibility that we shall want another patch or
> two on top there.  I've left a question as to whether we should be
> comparing anon_vmas.  And there's a potential (but I think ignorable)
> locking issue, in the case of an architecture that supports both
> VM_GROWSUP and VM_GROWSDOWN: if they expand towards each other at the
> same instant, they could gobble up the gap between them (they almost
> certainly have different anon_vmas, so the anon_vma locking does not
> protect against that).  When it gets to updating the vma tree, it is
> careful to use page_table_lock to maintain the consistency of the
> tree in such a case, but maybe we should do that earlier.

OK.

> Then there's the FOLL_MLOCK thing, and the WARN_ON (phew, remembered
> in time that you don't have VM_WARN_ON) - but keep in mind that I
> have not even built this tree, let alone tested it.

I'll take care of building it, don't worry.

> Sorry if I'm being annoying, Willy: you must be heartily sick of
> these patches by now!  Or, being a longtime longterm maintainer,
> perhaps it's all joy for you ;-?

No, rest assured it's never a full joy :-)  But it's much better
when I get help from the people who know how this stuff works than
when I have to invent the backport by myself!

Thanks a lot, I'll include your patch and will test it again. And
yes, I intend to merge Helge's fix once it lands into mainline (maybe
it is right now, I didn't check) and possibly other ones you might be
working on depending on various feedback.

Willy