linux-ext4 - Re: [RFC 04/10] fs/dax: Don't track page mapping/index

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 for Android: free password hash cracker in your pocket

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

Message-ID: <878r1f2jko.fsf@nvdebian.thelocal>
Date: Mon, 15 Apr 2024 17:03:48 +1000
From: Alistair Popple <apopple@...dia.com>
To: Dan Williams <dan.j.williams@...el.com>
Cc: linux-mm@...ck.org, david@...morbit.com, jhubbard@...dia.com,
 rcampbell@...dia.com, willy@...radead.org, jgg@...dia.com,
 linux-fsdevel@...r.kernel.org, jack@...e.cz, djwong@...nel.org,
 hch@....de, david@...hat.com, ruansy.fnst@...itsu.com,
 nvdimm@...ts.linux.dev, linux-xfs@...r.kernel.org,
 linux-ext4@...r.kernel.org, jglisse@...hat.com
Subject: Re: [RFC 04/10] fs/dax: Don't track page mapping/index

Dan Williams <dan.j.williams@...el.com> writes:

> Jan Kara wrote:
>> On Thu 11-04-24 10:57:25, Alistair Popple wrote:
>> > The page->mapping and page->index fields are normally used by the
>> > pagecache and rmap for looking up virtual mappings of pages. FS DAX
>> > implements it's own kind of page cache and rmap look ups so these
>> > fields are unnecessary. They are currently only used to detect
>> > error/warning conditions which should never occur.
>> > 
>> > A future change will change the way shared mappings are detected by
>> > doing normal page reference counting instead, so remove the
>> > unnecessary checks.
>> > 
>> > Signed-off-by: Alistair Popple <apopple@...dia.com>
>> ...
>> > -/*
>> > - * When it is called in dax_insert_entry(), the shared flag will indicate that
>> > - * whether this entry is shared by multiple files.  If so, set the page->mapping
>> > - * PAGE_MAPPING_DAX_SHARED, and use page->share as refcount.
>> > - */
>> > -static void dax_associate_entry(void *entry, struct address_space *mapping,
>> > -		struct vm_area_struct *vma, unsigned long address, bool shared)
>> > -{
>> > -	unsigned long size = dax_entry_size(entry), pfn, index;
>> > -	int i = 0;
>> > -
>> > -	if (IS_ENABLED(CONFIG_FS_DAX_LIMITED))
>> > -		return;
>> > -
>> > -	index = linear_page_index(vma, address & ~(size - 1));
>> > -	for_each_mapped_pfn(entry, pfn) {
>> > -		struct page *page = pfn_to_page(pfn);
>> > -
>> > -		if (shared) {
>> > -			dax_page_share_get(page);
>> > -		} else {
>> > -			WARN_ON_ONCE(page->mapping);
>> > -			page->mapping = mapping;
>> > -			page->index = index + i++;
>> > -		}
>> > -	}
>> > -}
>> 
>> Hum, but what about existing uses of folio->mapping and folio->index in
>> fs/dax.c? AFAICT this patch breaks them. What am I missing? How can this
>> ever work?

I did feel I was missing something here as well, but nothing obviously
breaks with this change from a test perspective (ie. ndctl tests, manual
tests). Somehow I missed how this was used in code, but Dan provided
enough of a hint below though so now I see the errors of my ways :-)

> Right, as far as I can see every fsdax filesystem would need to be
> converted to use dax_holder_operations() so that the fs can backfill
> ->mapping and ->index.

Oh, that was the hint I needed. Thanks. So basically it's just used for
memory failure like so:

memory_failure()
 -> memory_failure_dev_pagemap()
  -> mf_generic_kill_procs()
   -> dax_lock_page()
    -> mapping = READ_ONCE(page->mapping);

Somehow I had missed that bleatingly obvious usage of page->mapping. I
also couldn't understand how it was important if it was safe for it to
be just randomly overwritten in the shared case.

But I think I understand now - shared fs dax pages are only supported on
xfs and the mapping/index fields aren't used there because xfs provides
it's own look up for memory failure using dax_holder_operations.

I was initially concerned about these cases because I was wondering if
folio subpages could ever get different mappings and the shared case
implied they could. But it seems that's xfs specific and there is a
separate mechanism to deal with looking up ->mapping/index for that. So
I guess we should still be able to safely store this on the folio
head. I will double check and update this change.