linux-kernel - Re: [PATCH v6 22/26] device/dax: Properly refcount device dax pages when mapping

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: <7tmxffcwudefbm7bva4y54bnyp2le2fvtqkmm2ofdhyjvgjqly@4tkut3oxgjvh>
Date: Mon, 3 Feb 2025 22:29:00 +1100
From: Alistair Popple <apopple@...dia.com>
To: Dan Williams <dan.j.williams@...el.com>
Cc: akpm@...ux-foundation.org, linux-mm@...ck.org, 
	alison.schofield@...el.com, lina@...hilina.net, zhang.lyra@...il.com, 
	gerald.schaefer@...ux.ibm.com, vishal.l.verma@...el.com, dave.jiang@...el.com, 
	logang@...tatee.com, bhelgaas@...gle.com, jack@...e.cz, jgg@...pe.ca, 
	catalin.marinas@....com, will@...nel.org, mpe@...erman.id.au, npiggin@...il.com, 
	dave.hansen@...ux.intel.com, ira.weiny@...el.com, willy@...radead.org, djwong@...nel.org, 
	tytso@....edu, linmiaohe@...wei.com, david@...hat.com, peterx@...hat.com, 
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linuxppc-dev@...ts.ozlabs.org, nvdimm@...ts.linux.dev, 
	linux-cxl@...r.kernel.org, linux-fsdevel@...r.kernel.org, linux-ext4@...r.kernel.org, 
	linux-xfs@...r.kernel.org, jhubbard@...dia.com, hch@....de, david@...morbit.com, 
	chenhuacai@...nel.org, kernel@...0n.name, loongarch@...ts.linux.dev
Subject: Re: [PATCH v6 22/26] device/dax: Properly refcount device dax pages
 when mapping

On Mon, Jan 13, 2025 at 10:12:41PM -0800, Dan Williams wrote:
> Alistair Popple wrote:
> > Device DAX pages are currently not reference counted when mapped,
> > instead relying on the devmap PTE bit to ensure mapping code will not
> > get/put references. This requires special handling in various page
> > table walkers, particularly GUP, to manage references on the
> > underlying pgmap to ensure the pages remain valid.
> > 
> > However there is no reason these pages can't be refcounted properly at
> > map time. Doning so eliminates the need for the devmap PTE bit,
> > freeing up a precious PTE bit. It also simplifies GUP as it no longer
> > needs to manage the special pgmap references and can instead just
> > treat the pages normally as defined by vm_normal_page().
> > 
> > Signed-off-by: Alistair Popple <apopple@...dia.com>
> > ---
> >  drivers/dax/device.c | 15 +++++++++------
> >  mm/memremap.c        | 13 ++++++-------
> >  2 files changed, 15 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/dax/device.c b/drivers/dax/device.c
> > index 6d74e62..fd22dbf 100644
> > --- a/drivers/dax/device.c
> > +++ b/drivers/dax/device.c
> > @@ -126,11 +126,12 @@ static vm_fault_t __dev_dax_pte_fault(struct dev_dax *dev_dax,
> >  		return VM_FAULT_SIGBUS;
> >  	}
> >  
> > -	pfn = phys_to_pfn_t(phys, PFN_DEV|PFN_MAP);
> > +	pfn = phys_to_pfn_t(phys, 0);
> >  
> >  	dax_set_mapping(vmf, pfn, fault_size);
> >  
> > -	return vmf_insert_mixed(vmf->vma, vmf->address, pfn);
> > +	return vmf_insert_page_mkwrite(vmf, pfn_t_to_page(pfn),
> > +					vmf->flags & FAULT_FLAG_WRITE);
> >  }
> >  
> >  static vm_fault_t __dev_dax_pmd_fault(struct dev_dax *dev_dax,
> > @@ -169,11 +170,12 @@ static vm_fault_t __dev_dax_pmd_fault(struct dev_dax *dev_dax,
> >  		return VM_FAULT_SIGBUS;
> >  	}
> >  
> > -	pfn = phys_to_pfn_t(phys, PFN_DEV|PFN_MAP);
> > +	pfn = phys_to_pfn_t(phys, 0);
> >  
> >  	dax_set_mapping(vmf, pfn, fault_size);
> >  
> > -	return vmf_insert_pfn_pmd(vmf, pfn, vmf->flags & FAULT_FLAG_WRITE);
> > +	return vmf_insert_folio_pmd(vmf, page_folio(pfn_t_to_page(pfn)),
> > +				vmf->flags & FAULT_FLAG_WRITE);
> 
> This looks suspect without initializing the compound page metadata.

I initially wondered about this too, however I think the compound page metadata
should be initialised by memmap_init_zone_device(). That said I kind of get lost
in all the namespace/CXL/PMEM/DAX drivers in the stack so maybe I've overlooked
something.
 
> This might be getting compound pages by default with
> CONFIG_ARCH_WANT_OPTIMIZE_DAX_VMEMMAP. The device-dax unit tests are ok
> so far, but that is not super comforting until I can think about this a
> bit more... but not tonight.

>From my reading of the code I don't _think_
CONFIG_ARCH_WANT_OPTIMIZE_DAX_VMEMMAP would change whether or not we got
compound pages by default, just that if we did some of the (tail?) pages may
refer to the same physical struct page.

> Might as well fix up device-dax refcounts in this series too, but I
> won't ask you to do that, will send you something to include.

Eh. That should be relatively straight forward. But then I thought that about FS
DAX too :-)