Message-ID: <5ixvg4tnwj53ixh2fx26dxlctgqtayydqryqxhns6bwj3q3ies@6sttjti5dxt7>
Date: Thu, 3 Jul 2025 13:39:04 -0400
From: "Liam R. Howlett" <Liam.Howlett@...cle.com>
To: Peter Xu <peterx@...hat.com>
Cc: Nikita Kalyazin <kalyazin@...zon.com>,
        Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
        Suren Baghdasaryan <surenb@...gle.com>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, Vlastimil Babka <vbabka@...e.cz>,
        Muchun Song <muchun.song@...ux.dev>, Mike Rapoport <rppt@...nel.org>,
        Hugh Dickins <hughd@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        James Houghton <jthoughton@...gle.com>, Michal Hocko <mhocko@...e.com>,
        David Hildenbrand <david@...hat.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Oscar Salvador <osalvador@...e.de>,
        Axel Rasmussen <axelrasmussen@...gle.com>,
        Ujwal Kundur <ujwal.kundur@...il.com>
Subject: Re: [PATCH v2 1/4] mm: Introduce vm_uffd_ops API

* Peter Xu <peterx@...hat.com> [250703 11:24]:
> On Wed, Jul 02, 2025 at 10:00:51PM -0400, Liam R. Howlett wrote:
> > * Peter Xu <peterx@...hat.com> [250702 17:36]:
> > > On Wed, Jul 02, 2025 at 05:24:02PM -0400, Liam R. Howlett wrote:
> > > > That's because the entry point is from a function pointer, so [3] won't
> > > > help at all.
> > > > 
> > > > It is recreating the situation that existed for the vma through the
> > > > vm_ops in mmap, but for uffd.  And at a lower level (page tables).  I do not
> > > > want to relive that experience.
> > > > 
> > > > We are not doing this.  It is for the benefit of everyone that we are
> > > > not doing this.
> > > 
> > > Is the vma issue about "allowing vma->vm_flags to be modified anywhere"
> > > issue?  Or is there a pointer to the issue being discussed if not?
> > 
> > The issue is passing pointers of structs that are protected by locks or
> > ref counters into modules to do what they please.
> > 
> > vma->vm_flags was an example of where we learned how wrong this can go.
> > 
> > There is also the concern of the state of the folio on return from the
> > callback.  The error handling gets messy quick.
> > 
> > Now, imagine we have something that gets a folio, but then we find a
> > solution for contention of a lock or ref count (whatever is next), but
> > it doesn't work because the mm code has been bleeding into random
> > modules and we have no clue what that module is supposed to be doing, or
> > we can't make the necessary change because this module will break
> > userspace, or cause a performance decrease, or any other random thing
> > that we cannot work around without rewriting (probably suboptimally)
> > something we don't maintain.
> > 
> > Again, these are examples of how this can go bad but not an exhaustive
> > list by any means.
> > 
> > So the issue is with allowing modules to play with the folio and page
> > tables on their own.
> 
> I understand the concern, however IMHO that's really why mm can be hard and
> important at the same time..

I'm glad you understand, but I think you do not understand the severity
of the concern and the repercussions.

These patches, as they exist today, are not going to be accepted.

I am not okay with it going in, and I don't see many saying differently.

I am not okay with you pushing for it any longer.

Several people have told you it is not a good idea, the people who have
to deal with the fallout of this when it goes bad - and it _will_ go
bad.

You have been given ample reasons why, technical reasons as well as real
examples of failures - and security bugs - that have resulted from the
exact same pattern in the exact same structure where you are reproducing
the patter.

Please stop trying to push this plan.  It is a waste of time and energy.

We need a new plan.

> 
> We definitely have driver code manipulating pgtables.  We also have folios
> or pages that can be directly accessible from drivers.
> 
> After all mm is the core function provider for those and there needs to be
> some API accessing them from outside.

But that's not what you are doing, you are handing out pointers and
expecting the modules to do the core function.

And the core function that was suggested in the example user already had
a ref counting issue.  Bugs happen, but that sort of thing is going to
be difficult to find and it won't be the driver author hunting it down,
potentially under an embargo.  And good luck validating what was done on
return from the module.

> 
> I agree some protection would be nice, like what Suren did with the
> vm_flags using __private, even though it's unfortunate it only works with
> sparse not a warn/error when compiling, as vm_flags is not a pointer.
> OTOH, forbid exposing anything might be an overkill, IMHO.  It stops mm
> from growing in healthy ways.

This isn't healthy, it is repeating the errors of the past and expecting
a different result.

This isn't about balance or forbidding any exposure, it's about finding
a less bug-prone way of getting your feature to work.

I'm not saying it's wrong TO do this. I'm saying these patches are the
wrong way OF doing this - from experience of dealing with the same
pattern.

Anyways, I am not spending more time fighting about this.  Let me know
when you come up with an alternative approach.

Regards,
Liam

Powered by blists - more mailing lists