linux-kernel - Re: [RFC V2 03/18] dev_dax_iomap: Save the kva from memremap

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: <20250704121119.00002846@huawei.com>
Date: Fri, 4 Jul 2025 12:11:19 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: John Groves <John@...ves.net>
CC: Dan Williams <dan.j.williams@...el.com>, Miklos Szeredi
	<miklos@...redb.hu>, Bernd Schubert <bschubert@....com>, John Groves
	<jgroves@...ron.com>, Jonathan Corbet <corbet@....net>, Vishal Verma
	<vishal.l.verma@...el.com>, Dave Jiang <dave.jiang@...el.com>, "Matthew
 Wilcox" <willy@...radead.org>, Jan Kara <jack@...e.cz>, Alexander Viro
	<viro@...iv.linux.org.uk>, Christian Brauner <brauner@...nel.org>, "Darrick J
 . Wong" <djwong@...nel.org>, Randy Dunlap <rdunlap@...radead.org>, "Jeff
 Layton" <jlayton@...nel.org>, Kent Overstreet <kent.overstreet@...ux.dev>,
	<linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<nvdimm@...ts.linux.dev>, <linux-cxl@...r.kernel.org>,
	<linux-fsdevel@...r.kernel.org>, Amir Goldstein <amir73il@...il.com>, "Stefan
 Hajnoczi" <shajnocz@...hat.com>, Joanne Koong <joannelkoong@...il.com>, Josef
 Bacik <josef@...icpanda.com>, Aravind Ramesh <arramesh@...ron.com>, Ajay
 Joshi <ajayjoshi@...ron.com>
Subject: Re: [RFC V2 03/18] dev_dax_iomap: Save the kva from memremap

On Thu,  3 Jul 2025 13:50:17 -0500
John Groves <John@...ves.net> wrote:

> Save the kva from memremap because we need it for iomap rw support.
> 
> Prior to famfs, there were no iomap users of /dev/dax - so the virtual
> address from memremap was not needed.
> 
> Also: in some cases dev_dax_probe() is called with the first
> dev_dax->range offset past the start of pgmap[0].range. In those cases
> we need to add the difference to virt_addr in order to have the physaddr's
> in dev_dax->ranges match dev_dax->virt_addr.
> 
> This happens with devdax devices that started as pmem and got converted
> to devdax. I'm not sure whether the offset is due to label storage, or
> page tables, but this works in all known cases.

Clearly a question we need to resolve to understand if this is correct
handling.

> 
> Signed-off-by: John Groves <john@...ves.net>
> ---
>  drivers/dax/dax-private.h |  1 +
>  drivers/dax/device.c      | 15 +++++++++++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/dax/dax-private.h b/drivers/dax/dax-private.h
> index 0867115aeef2..2a6b07813f9f 100644
> --- a/drivers/dax/dax-private.h
> +++ b/drivers/dax/dax-private.h
> @@ -81,6 +81,7 @@ struct dev_dax_range {
>  struct dev_dax {
>  	struct dax_region *region;
>  	struct dax_device *dax_dev;
> +	void *virt_addr;
>  	unsigned int align;
>  	int target_node;
>  	bool dyn_id;
> diff --git a/drivers/dax/device.c b/drivers/dax/device.c
> index 29f61771fef0..583150478dcc 100644
> --- a/drivers/dax/device.c
> +++ b/drivers/dax/device.c
> @@ -372,6 +372,7 @@ static int dev_dax_probe(struct dev_dax *dev_dax)
>  	struct dax_device *dax_dev = dev_dax->dax_dev;
>  	struct device *dev = &dev_dax->dev;
>  	struct dev_pagemap *pgmap;
> +	u64 data_offset = 0;
>  	struct inode *inode;
>  	struct cdev *cdev;
>  	void *addr;
> @@ -426,6 +427,20 @@ static int dev_dax_probe(struct dev_dax *dev_dax)
>  	if (IS_ERR(addr))
>  		return PTR_ERR(addr);
>  
> +	/* Detect whether the data is at a non-zero offset into the memory */
> +	if (pgmap->range.start != dev_dax->ranges[0].range.start) {

Using pgmap->range.start here but then getting to the same (I think)
with  dev_dax->pgmap[0].range.start is rather inconsistent.

Also, perhaps drag the assignment of phys and pgmap_phys out of this
scope so that you can use them for the condition check above and
then reuse the same in here.


> +		u64 phys = dev_dax->ranges[0].range.start;
> +		u64 pgmap_phys = dev_dax->pgmap[0].range.start;
> +		u64 vmemmap_shift = dev_dax->pgmap[0].vmemmap_shift;
> +
> +		if (!WARN_ON(pgmap_phys > phys))
> +			data_offset = phys - pgmap_phys;

In the event of the condition above being false.
phys == pgmap_phys and data_offset == 0.

So why not do this unconditionally replacing this block with something like

	/* Apply necessary offset */

	dev_dax->virt_addr = addr +
		(dev_dax->ranges[0].range.start - pgmap->range.start);
> +
> +		pr_debug("%s: offset detected phys=%llx pgmap_phys=%llx offset=%llx shift=%llx\n",
> +		       __func__, phys, pgmap_phys, data_offset, vmemmap_shift);

If it's only used in the print, I'd just put the path to vmemmap_shift directly in here
and probably get to it via pgmap->vmemmap_shift



> +	}
> +	dev_dax->virt_addr = addr + data_offset;
> +
>  	inode = dax_inode(dax_dev);
>  	cdev = inode->i_cdev;
>  	cdev_init(cdev, &dax_fops);