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

[Jason Gunthorpe]

On Tue, May 02, 2023 at 09:33:45PM +0200, David Hildenbrand wrote:

> I'll just stress again that I think there are cases where we unmap and free
> a page without synchronizing against GUP-fast using an IPI or RCU.

AFAIK this is true on ARM64 and other arches that do not use IPIs for
TLB shootdown.

Eg the broadcast TLBI described here:

https://developer.arm.com/documentation/101811/0102/Translation-Lookaside-Buffer-maintenance

TLB invalidation of remote CPUs Is done at the interconnect level and
does not trigger any interrupt.

So, arches that don't use IPI have to RCU free the page table entires
to work with GUP fast. They will set MMU_GATHER_RCU_TABLE_FREE. The
whole interrupt disable thing in GUP turns into nothing more than a
hacky RCU on those arches.

The ugly bit is the comment:

 * We do not adopt an rcu_read_lock() here as we also want to block IPIs
 * that come from THPs splitting.

Which, I think, today can be summarized in today's code base as
serializing with split_huge_page_to_list().

I don't know this code well, but the comment there says "Only caller
must hold pin on the @page" which is only possible if all the PTEs
have been replaced with migration entries or otherwise before we get
here. So the IPI the comment talks about, I suppose, is from the
installation of the migration entry?

However gup_huge_pmd() does the usual read, ref, check pattern, and
split_huge_page_to_list() uses page_ref_freeze() which is cmpxchg

So the three races seem to resolve themselves without IPI magic
 - GUP sees the THP folio before the migration entry and +1's the ref
   split_huge_page_to_list() bails beacuse the ref is elevated
 - GUP fails to +1 the ref because it is zero and bails,
   split_huge_page_to_list() made it zero, so it runs
 - GUP sees the THP folio, split_huge_page_to_list() ran to
   completion, and then GUP +1's a 4k page. The recheck of pmd_val
   will see the migration entry, or the new PTE table pointer, but
   never the original THP folio. So GUP will bail. The speculative
   ref on the 4k page is harmless.

I can't figure out what this 2014 comment means in today's code.

Or where ARM64 hid the "IPI broadcast on THP split" :)

See commit 2667f50e8b81457fcb4a3dbe6aff3e81ea009e13

> That's one of the reasons why we recheck if the PTE changed to back off, so
> I've been told.

Yes, with no synchronizing point the refcount in GUP fast could be
taken on the page after it has been free'd and reallocated, but this
is only possible on RCU

> I'm happy if someone proves me wrong and a page we just (temporarily) pinned
> cannot have been freed+reused in the meantime.

Sadly I think no.. We'd need to RCU free the page itself as well to
make this true.

Jason