Re: [PATCH v7 3/3] mm/gup: disallow FOLL_LONGTERM GUP-fast writing to file-backed mappings

[Lorenzo Stoakes]

On Tue, May 02, 2023 at 04:07:50PM -0300, Jason Gunthorpe wrote:
> On Tue, May 02, 2023 at 07:17:14PM +0100, Lorenzo Stoakes wrote:
>
> > On a specific point - if mapping turns out to be NULL after we confirm
> > stable PTE, I'd be inclined to reject and let the slow path take care of
> > it, would you agree that that's the correct approach?
>
> I think in general if GUP fast detects any kind of race it should bail
> to the slow path.
>
> The races it tries to resolve itself should have really safe and
> obvious solutions.
>
> I think this comment is misleading:
>
> > +	/*
> > +	 * GUP-fast disables IRQs - this prevents IPIs from causing page tables
> > +	 * to disappear from under us, as well as preventing RCU grace periods
> > +	 * from making progress (i.e. implying rcu_read_lock()).
>
> True, but that is not important here since we are not reading page
> tables
>
> > +	 * This means we can rely on the folio remaining stable for all
> > +	 * architectures, both those that set CONFIG_MMU_GATHER_RCU_TABLE_FREE
> > +	 * and those that do not.
>
> Not really clear. We have a valid folio refcount here, that is all.

Some of this is a product of mixed signals from different commenters and
my being perhaps a little _too_ willing to just go with the flow.

With interrupts disabled and IPI blocked, plus the assurances that
interrupts being disabled implied the RCU version of page table
manipulation is also blocked, my understanding was that remapping in this
process to another page could not occur.

Of course the folio is 'stable' in the sense we have a refcount on it, but
it is unlocked so things can change.

I'm guessing the RCU guarantees in the TLB logic are not as solid as IPI,
because in the IPI case it seems to me you couldn't even clear the PTE
entry before getting to the page table case.

Otherwise, I'm a bit uncertain actually as to how we can get to the point
where the folio->mapping is being manipulated. Is this why?

>
> > +	 * We get the added benefit that given inodes, and thus address_space,
> > +	 * objects are RCU freed, we can rely on the mapping remaining stable
> > +	 * here with no risk of a truncation or similar race.
>
> Which is the real point:
>
> 1) GUP-fast disables IRQs which means this is the same context as rcu_read_lock()
> 2) We have a valid ref on the folio due to normal GUP fast operation
>    Thus derefing struct folio is OK
> 3) folio->mapping can be deref'd safely under RCU since mapping is RCU free'd
>    It may be zero if we are racing a page free path
>    Can it be zero for other reasons?

Zero? You mean NULL?

In any case, I will try to clarify these, I do agree the _key_ point is
that we can rely on safely derefing the mapping, at least READ_ONCE()'d, as
use-after-free or dereffing garbage is the fear here.

>
> If it can't be zero for any other reason then go to GUP slow and let
> it sort it out
>
> Otherwise you have to treat NULL as a success.
>

Well that was literally the question :) and I've got somewhat contradictory
feedback. My instinct aligns with yours in that, just fallback to slow
path, so that's what I'll do. But just wanted to confirm.

> Really what you are trying to do here is remove the folio lock which
> would normally protect folio->mapping. Ie this test really boils down
> to just 'folio_get_mapping_a_ops_rcu() == shmem_aops'
>
> The hugetlb test is done on a page flag which should be stable under
> the pageref.
>
> So.. Your function really ought to be doing this logic:
>
>         // Should be impossible for a slab page to be in a VMA
> 	if (unlikely(folio_test_slab(folio)))
> 	   return do gup slow;
>
> 	// Can a present PTE even be a swap cache?
>    	if (unlikely(folio_test_swapcache(folio)))
> 	   return do gup slow;
>
> 	if (folio_test_hugetlb(folio))
> 	   return safe for fast
>
> 	// Safe without the folio lock ?
>    	struct address_space *mapping = READ_ONCE(folio->mapping)
> 	if ((mapping & PAGE_MAPPING_FLAGS) == PAGE_MAPPING_ANON)
> 	   return safe for fast
> 	if ((mapping & PAGE_MAPPING_FLAGS) == 0 && mapping)
> 	   return mapping->a_ops == shmem_aops;
>
>         // Depends on what mapping = NULL means
> 	return do gup slow
>

Yeah this is how I was planning to implement it, or something along these
lines. The only question was whether my own view aligned with others to
avoid more spam :)

The READ_ONCE() approach is precisely how I wanted to do it in thet first
instance, but feared feedback about duplication and wondered if it made
much difference, but now it's clear this is ther ight way.

> Jason