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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZFFfhhwibxHKwDbZ@nvidia.com>
Date:   Tue, 2 May 2023 16:07:50 -0300
From:   Jason Gunthorpe <jgg@...dia.com>
To:     Lorenzo Stoakes <lstoakes@...il.com>
Cc:     David Hildenbrand <david@...hat.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 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.

> +	 * 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?

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.

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

Jason