Message-ID: <9fb4761d-db28-4271-afb7-9e3cafa89438@lucifer.local>
Date: Mon, 9 Dec 2024 14:28:21 +0000
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: David Hildenbrand <david@...hat.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>,
        Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] MAINTAINERS: group all VMA-related files into the VMA
 section

On Mon, Dec 09, 2024 at 03:09:26PM +0100, David Hildenbrand wrote:
> On 09.12.24 11:06, Lorenzo Stoakes wrote:
> > On Mon, Dec 09, 2024 at 10:16:21AM +0100, David Hildenbrand wrote:
> > > On 06.12.24 20:16, Lorenzo Stoakes wrote:
> > > > There are a number of means of interacting with VMA operations within mm,
> > > > and we have on occasion not been made aware of impactful changes due to
> > > > these sitting in different files, most recently in [0].
> > > >
> > > > Correct this by bringing all VMA operations under the same section in
> > > > MAINTAINERS. Additionally take the opportunity to combine MEMORY MAPPING
> > > > with VMA as there needn't be two entries as they amount to the same thing.
> > > >
> > > > [0]:https://lore.kernel.org/linux-mm/CAG48ez0siYGB8GP5+Szgj2ovBZAkL6Zi4n6GUAjzzjFV9LTkRQ@mail.gmail.com/
> > > >
> > > > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> > > > ---
> > > >    MAINTAINERS | 19 +++++++------------
> > > >    1 file changed, 7 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 1e930c7a58b1..95db20c26f5f 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -15060,18 +15060,6 @@ F:	tools/mm/
> > > >    F:	tools/testing/selftests/mm/
> > > >    N:	include/linux/page[-_]*
> > > >
> > > > -MEMORY MAPPING
> > > > -M:	Andrew Morton <akpm@...ux-foundation.org>
> > > > -M:	Liam R. Howlett <Liam.Howlett@...cle.com>
> > > > -M:	Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> > > > -R:	Vlastimil Babka <vbabka@...e.cz>
> > > > -R:	Jann Horn <jannh@...gle.com>
> > > > -L:	linux-mm@...ck.org
> > > > -S:	Maintained
> > > > -W:	http://www.linux-mm.org
> > > > -T:	git git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
> > > > -F:	mm/mmap.c
> > > > -
> > > >    MEMORY TECHNOLOGY DEVICES (MTD)
> > > >    M:	Miquel Raynal <miquel.raynal@...tlin.com>
> > > >    M:	Richard Weinberger <richard@....at>
> > > > @@ -25028,6 +25016,13 @@ L:	linux-mm@...ck.org
> > > >    S:	Maintained
> > > >    W:	https://www.linux-mm.org
> > > >    T:	git git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
> > > > +F:	mm/madvise.c
> > > > +F:	mm/mlock.c
> > > > +F:	mm/mmap.c
> > > > +F:	mm/mprotect.c
> > > > +F:	mm/mremap.c
> > > > +F:	mm/mseal.c
> > > > +F:	mm/msync.c
> > >
> > > Not sure about mprotect.c, mlock.c and madvise.c, though. I'd claim that the
> > > real "magic" they perform is in page table handling and not primarily VMA
> > > handling (yes, both do VMA changes, but they are the "easy" part ;) ).
> >
> > And large parts of the VMA logic interface with page tables, see - the entire
> > 6.12 cycle - around recent changes in mmap() MAP_FIXED - which... the VMA
> > maintainers fixed :)
> >
> > And then there were the issues around VMA and mm locking relating to page
> > table work which... oh right yeah we had to fix again... :>)
> >
> > I mean you can make this argument about probably all of these files (mremap
> > has -tons- of page table-specific stuff), and then we get back to not being
> > notified about key changes that interface with memory mapping/VMA which we
> > end up having to deal with anyway.
> >
> > A lot of the reason we have 'magic' in these files anyway is because we
> > don't have a decent generic page table handler. Not sure I'd actually use
> > the word 'magic' for that though.
> >
> > I am planning to make significant changes to mprotect/mlock soon, which
> > have some terribly duplicated horrible handling logic, and both are key
> > considerations in VMA logic as a whole.
> >
> > Anyway, as far as I'm concerned page table manipulation after the point of
> > faulting is completely within the purvue of VMA manipulation and a side
> > product of it.
> >
> > However, can concede mm/madvise.c if you feel strongly about that as that's
> > a bit blurry, but of course contains a whole bunch of VMA and... page table
> > manipulation :) I mean it still to me seems very pertinent.
> >
>
> And then we have mprotect.c being heavily used by uffd-wp and NUMA hinting,
> which don't perform any VMA modification.
>
> That's why I don't think the change proposed here is really the right step.
>
> >>>> They have much more in common with memory.c, which I wouldn't want to
> see in
> > > here either. Hm.
> >
> > No, memory.c is really dedicated to fault handling. This is really
> > different from manipulating page tables in specific cases in my opinion.
>
> And fork and such stuff. And if you look into huge_memory.c, we actually
> moved all of the THP logic for mprotect()/madvise()/... in there.
>
> Not sure if something similar should have been done for memory.c, or if the
> THP stuff should actually also have gone into the respective files.
>
> To me it sounds wrong to have VMA maintainers maintain a lot of the code in
> these files code because these files somehow modify VMAs, sorry.

This isn't what I said, I said that de facto we (that is the MEMORY MAPPING
maintainers as well as VMA) were dealing with a great many issues around
page tables and page table manipulation which are rather inseparable from
one another.

I even went to the lengths of writing a detailed set of documentation on
locking behaviour in and around page table manipulation and solved
security-sensitive issues in relation to page table teardown over the 6.12
rc cycle.

To me, the idea that mprotect() and mlock(), operations that are explicitly
about manipulating VMAs (_and of course consqeuent page table
manipulation_), are somehow separate is really bizarre to me, but I respect
your opinion even if I disagree.

But unfortunately your arguments apply equally as well to mremap.c (more
than half of which is dedicated to page table manipulation), so I will have
to drop the whole patch then.

If issues arise there in future, I guess others will have to deal with them
if we don't notice them (luckily Jann did and pinged this time, hopefully
will in future).

>
> --
> Cheers,
>
> David / dhildenb
>

To be clear, I made this change in the interests of the community and
contributing. It seems to me that within mm has far too little sharing of
the maintainership burden and I only wanted to help with that and make
explicit what I work on day-to-day.

I am glad you at least don't object to my doing so with respect to at least
some parts of the VMA logic.

Powered by blists - more mailing lists