linux-kernel - Re: [RFC PATCH 1/3] mm: refactor rmap_walk_file() to separate out traversal logic

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: <0aefd0bb-7897-4a7c-af18-c2b95547f492@lucifer.local>
Date: Wed, 8 Jan 2025 19:23:36 +0000
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Matthew Wilcox <willy@...radead.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
        Jaya Kumar <jayalk@...works.biz>, Simona Vetter <simona@...ll.ch>,
        Helge Deller <deller@....de>, linux-fbdev@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, David Hildenbrand <david@...hat.com>
Subject: Re: [RFC PATCH 1/3] mm: refactor rmap_walk_file() to separate out
 traversal logic

On Wed, Jan 08, 2025 at 04:38:57PM +0000, Matthew Wilcox wrote:
> On Wed, Jan 08, 2025 at 04:18:40PM +0000, Lorenzo Stoakes wrote:
> > +/*
> > + * rmap_walk_file - do something to file page using the object-based rmap method
> > + * @folio: the folio to be handled
> > + * @rwc: control variable according to each walk type
> > + * @locked: caller holds relevant rmap lock
> > + *
> > + * Find all the mappings of a folio using the mapping pointer and the vma chains
> > + * contained in the address_space struct it points to.
> > + */
> > +static void rmap_walk_file(struct folio *folio,
> > +		struct rmap_walk_control *rwc, bool locked)
> > +{
> > +	struct address_space *mapping = folio_mapping(folio);
>
> I'm unconvinced this shouldn't be just folio->mapping.  On the face of
> it, we're saying that we're walking a file, and file folios just want
> to use folio->mapping.  But let's dig a little deeper.
>
> The folio passed in is locked, so it can't be changed during this call.
> In folio_mapping(), folio_test_slab() is guaranteed untrue.
> folio_test_swapcache() doesn't seem likely to be true either; unless
> it's shmem, it can't be in the swapcache, and if it's shmem and in the
> swap cache, it can't be mapped to userspace (they're swizzled back from
> the swapcache to the pagecache before being mapped).  And then the
> check for PAGE_MAPPING_FLAGS is guaranteed to be untrue (we know it's
> not anon/ksm/movable).  So I think this should just be folio->mapping.

Ack, and we assert that it is indeed locked first. We will have checked
that this is not anon, and with the lock we shouldn't see it disappear
under us to be slab, we have also explicitly checked for ksm so that's out.

Wasn't aware of that swizzling actually... good to know! But I guess that
makes sense since you'd hit a swap entry in the fault code and trigger all
that fun stuff (hm let me go read the swap chapter in my book again :P)

TL;DR - will change. But will add a comment saying we can do it safely.

>
> > +	/*
> > +	 * The page lock not only makes sure that page->mapping cannot
> > +	 * suddenly be NULLified by truncation, it makes sure that the
> > +	 * structure at mapping cannot be freed and reused yet,
> > +	 * so we can safely take mapping->i_mmap_rwsem.
> > +	 */
>
> I know you only moved this comment, but please fix it to refer to
> folios, not pages.

Ack will do.

>
> > +	VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
> > +
> > +	if (!mapping)
> > +		return;
>
> Maybe make this a WARN_ON_ONCE?

I'm not sure if this isn't actually a vaguely possible scenario? Though
hm. I'm not 100% certain it's  not expected to happen _sometimes_.

Perhaps one to do as a follow up in case it turns out this is sometimes
expected due to timing issues with a truncate?

But I may be wrong and this should demonstrably not happen other than in
case of programmatic error?

>
> > +	__rmap_walk_file(folio, mapping, folio_pgoff(folio),
> > +			 folio_nr_pages(folio), rwc, locked);
>
> folio_pgoff() can go too.  Just use folio->index.
>

Ack. Will change.