Message-ID: <ZFGh+UUd3IMZk0lb@nvidia.com>
Date:   Tue, 2 May 2023 20:51:21 -0300
From:   Jason Gunthorpe <jgg@...dia.com>
To:     David Hildenbrand <david@...hat.com>
Cc:     Lorenzo Stoakes <lstoakes@...il.com>,
        Peter Zijlstra <peterz@...radead.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jens Axboe <axboe@...nel.dk>,
        Matthew Wilcox <willy@...radead.org>,
        Dennis Dalessandro <dennis.dalessandro@...nelisnetworks.com>,
        Leon Romanovsky <leon@...nel.org>,
        Christian Benvenuti <benve@...co.com>,
        Nelson Escobar <neescoba@...co.com>,
        Bernard Metzler <bmt@...ich.ibm.com>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Ian Rogers <irogers@...gle.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Bjorn Topel <bjorn@...nel.org>,
        Magnus Karlsson <magnus.karlsson@...el.com>,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Christian Brauner <brauner@...nel.org>,
        Richard Cochran <richardcochran@...il.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        linux-fsdevel@...r.kernel.org, linux-perf-users@...r.kernel.org,
        netdev@...r.kernel.org, bpf@...r.kernel.org,
        Oleg Nesterov <oleg@...hat.com>,
        John Hubbard <jhubbard@...dia.com>, Jan Kara <jack@...e.cz>,
        "Kirill A . Shutemov" <kirill@...temov.name>,
        Pavel Begunkov <asml.silence@...il.com>,
        Mika Penttila <mpenttil@...hat.com>,
        Dave Chinner <david@...morbit.com>,
        Theodore Ts'o <tytso@....edu>, Peter Xu <peterx@...hat.com>,
        Matthew Rosato <mjrosato@...ux.ibm.com>,
        "Paul E . McKenney" <paulmck@...nel.org>,
        Christian Borntraeger <borntraeger@...ux.ibm.com>,
        Mike Rapoport <rppt@...ux.ibm.com>
Subject: Re: [PATCH v7 3/3] mm/gup: disallow FOLL_LONGTERM GUP-fast writing
 to file-backed mappings

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

Powered by blists - more mailing lists