Received-SPF: pass (google.com: domain of linux-kernel+bounces-50490-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 2 Feb 2024 11:41:40 -0800
From: Dan Williams <dan.j.williams@intel.com>
To: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>, Dan Williams
	<dan.j.williams@intel.com>, Arnd Bergmann <arnd@arndb.de>, Dave Chinner
	<david@fromorbit.com>
CC: <linux-kernel@vger.kernel.org>, Andrew Morton <akpm@linux-foundation.org>,
	Linus Torvalds <torvalds@linux-foundation.org>, <linux-mm@kvack.org>,
	<linux-arch@vger.kernel.org>, Vishal Verma <vishal.l.verma@intel.com>, "Dave
 Jiang" <dave.jiang@intel.com>, Matthew Wilcox <willy@infradead.org>, "Russell
 King" <linux@armlinux.org.uk>, <nvdimm@lists.linux.dev>,
	<linux-cxl@vger.kernel.org>, <linux-fsdevel@vger.kernel.org>,
	<dm-devel@lists.linux.dev>
Subject: Re: [RFC PATCH v3 2/4] dax: Check for data cache aliasing at runtime
Message-ID: <65bd457460fb1_719322942@dwillia2-mobl3.amr.corp.intel.com.notmuch>
References: <20240131162533.247710-1-mathieu.desnoyers@efficios.com>
 <20240131162533.247710-3-mathieu.desnoyers@efficios.com>
 <65bab567665f3_37ad2943c@dwillia2-xfh.jf.intel.com.notmuch>
 <0a38176b-c453-4be0-be83-f3e1bb897973@efficios.com>
 <65bac71a9659b_37ad29428@dwillia2-xfh.jf.intel.com.notmuch>
 <f1d14941-2d22-452a-99e6-42db806b6d7f@efficios.com>
 <65bd284165177_2d43c29443@dwillia2-mobl3.amr.corp.intel.com.notmuch>
 <6bdf6085-101d-47ef-86f4-87936622345a@efficios.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <6bdf6085-101d-47ef-86f4-87936622345a@efficios.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?VlfO2LhDJMr88m1bR6HKiaMkt731pIoB+BdqHgew3b5Au+ZVWotDGcf1PTC3?=
 =?us-ascii?Q?l8WPxWRf9YclHZ4LRjvx+NWeQndY+014EoDbaLf8jR5S7Rq8yGrBKg/f0RqQ?=
 =?us-ascii?Q?xvEcplPTEtAv+2DEUIORqOQbwnC7aASTxePXze89S8V/fAcMORRDjm/s2+mj?=
 =?us-ascii?Q?XEaxicfrp+wePaxSrSiQLDihKEkIeBZ/1ZKCmrOk52J5j1on7Kvuv7Sti1k7?=
 =?us-ascii?Q?hyeIw1VAqlalXtsP7cjeHkTfz1GbZ15rO8L+Xz9h7Njhihgg3fa33O0bXhcF?=
 =?us-ascii?Q?VFrPrL+5MCQ5lFMRkJ5c+zQhe3me+1cpJJ4GGHdYrLuWR+ThJ9PGYETXlnSD?=
 =?us-ascii?Q?//C2exzI0GTQaW6uwGBRlR98tN5Tj9KUy6YiQRZk7Y7OqBmnPO6JLA8/3xaG?=
 =?us-ascii?Q?LRz8cXJ9gAbuzMvEFPceoX4vDkuduMyQ31Avh2gqyH3ihvSoTgyw/Jit0kSN?=
 =?us-ascii?Q?DCaYXLPYBPK6nJXICypcbF6Ffy9Oz18qRL6voPXFpn0CfqXe3wQGXuHCERPZ?=
 =?us-ascii?Q?KRvOO9Zb7Nplht+g7m6sJtvhVTYXs1ijz76ls0uICUvF1Ne//02uUviWFrTU?=
 =?us-ascii?Q?YnTI06k+rv++LaO5HEj5f+nV2zHnysE6Kp5TSms+XT3t2EVJ4Fv+im6oO96Q?=
 =?us-ascii?Q?m3o3VVuR5zCoyd8B1ORtbfw8NMCF1N6v3DQBzFI+yhcBQUlWxxvLzGCANmbW?=
 =?us-ascii?Q?69qTBsEf7fkZ0j/4OFOQ+OKCC9APjFk0+O9OivjSvuaasibY4ht+JItQeYQN?=
 =?us-ascii?Q?4GoaCOsLwCBjwqXSAydlgyVe37gU8HZFvzIlr5Hf3sNchwvrsLw8hlP/d9xm?=
 =?us-ascii?Q?fvSmnLnYQwpbzkHjkVqB/0/QP7C+QyP2DNf0iMtCqTN1u/joBpvz5HjPb2dw?=
 =?us-ascii?Q?z5SfQfxHdYyAj5PfHwaMWFdadmyTsm/U2huN7Xzkpk81ugMDaJm+HaLBWp4R?=
 =?us-ascii?Q?4lrq6BGF0pi+OqwyfOC9hYcoeK39N/3DbA53o9ZbAlAwKLrVZTby7Co61jAO?=
 =?us-ascii?Q?0TqHWM0sr5xHEJqYTnDgJcTV9XU3YdTTSu19v06BQxXKqEu0V3DBVgMD7Q+D?=
 =?us-ascii?Q?i8a0MMiKATmh7BqdtU/syVthDUIgXtQGDrkxytrnkjobzeCnZn8LktRFp+Nw?=
 =?us-ascii?Q?pUI7LW4+w0CpXMWxdHh44ulkm/4zgC3GxSUG6oMqw9bD4dcaYuY8ftg7Pusd?=
 =?us-ascii?Q?BZRAYJ2CQ2XoHGKRFMDKtTaMmHAAhPfZv+o1vTXmMKXnL7khVJwmyvqQb49c?=
 =?us-ascii?Q?hMZfrMdeXzQj6YjftIK5fKZENGAUuQej45RDPGmngjcNDNXXOxEHJ5dPfX/G?=
 =?us-ascii?Q?VvBz3fBfe4XCILO0nx6Yh5MlXj8uZnxhlNOvyN9AduX0a0wk9c4IWb3zJb9U?=
 =?us-ascii?Q?s1YsCX8jDZKkhrpalYFl1l2ca564YLNBAkF7ST8kzIDs3G9sp85ySebQqccU?=
 =?us-ascii?Q?FjkwRJH7p+sPztxhQ5J1aUOAkRt+s3HV/Y5j04PSq+KDkW/Fs9Y/HoLn7aH1?=
 =?us-ascii?Q?93G0DoZNUB1aTHlK4Ar1nqqSdP+S6xHBfh4twbn6DJ/OBuH8C+5Aga/AySCi?=
 =?us-ascii?Q?u5+Ds/eMgLs/uPWTLdL+I/RvV9imGcl4QrWouQhs2/9W5kCMT5xUhnxKp78M?=
 =?us-ascii?Q?Iw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 86e61088-d60a-40b1-a762-08dc2426fc63
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Feb 2024 19:41:44.4512
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: KPdjlzEaWjnKRIwh3WZTz5zfoqcANtfPvaMUT2L6O1GIqqb0VTKSG+bUALpG/uHIp3VCit/IQxprWjS1cmdYWI2YdC9Yq69tT1GUOqoKtss=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR11MB7377
X-OriginatorOrg: intel.com

Mathieu Desnoyers wrote:
> On 2024-02-02 12:37, Dan Williams wrote:
> > Mathieu Desnoyers wrote:
> [...]
> >>
> > 
> >> The alternative route I intend to take is to audit all callers
> >> of alloc_dax() and make sure they all save the alloc_dax() return
> >> value in a struct dax_device * local variable first for the sake
> >> of checking for IS_ERR(). This will leave the xyz->dax_dev pointer
> >> initialized to NULL in the error case and simplify the rest of
> >> error checking.
> > 
> > I could maybe get on board with that, but it needs a comment somewhere
> > about the asymmetric subtlety.
> 
> Is this "somewhere" at every alloc_dax() call site, or do you have
> something else in mind ?

At least kill_dax() should mention the asymmetry I think.

> 
> > 
> >>
> >>
> >>>    		return;
> >>>    
> >>>    	if (dax_dev->holder_data != NULL)
> >>> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> >>> index 4e8fdcb3f1c8..b69c9e442cf4 100644
> >>> --- a/drivers/nvdimm/pmem.c
> >>> +++ b/drivers/nvdimm/pmem.c
> >>> @@ -560,17 +560,19 @@ static int pmem_attach_disk(struct device *dev,
> >>>    	dax_dev = alloc_dax(pmem, &pmem_dax_ops);
> >>>    	if (IS_ERR(dax_dev)) {
> >>>    		rc = PTR_ERR(dax_dev);
> >>> -		goto out;
> >>> +		if (rc != -EOPNOTSUPP)
> >>> +			goto out;
> >>
> >> If I compare the before / after this change, if previously
> >> pmem_attach_disk() was called in a configuration with FS_DAX=n, it would
> >> result in a NULL pointer dereference.
> > 
> > No, alloc_dax() only returns NULL CONFIG_DAX=n case, not the
> > CONFIG_FS_DAX=n case.
> 
> Indeed, I was wrong there.
> 
> > So that means that pmem devices on ARM have been
> > possible without FS_DAX. So, in order for alloc_dax() returning
> > ERR_PTR(-EOPNOTSUPP) to not regress pmem device availability this error
> > path needs to be changed.
> Good point. We're moving the depends on !(ARM || MIPS |PARC) from FS_DAX
> Kconfig to a runtime check in alloc_dax(), which is used whenever DAX=y,
> which includes configurations that had FS_DAX=n previously.
> 
> I'll change the error path in pmem_attack_disk to treat -EOPNOTSUPP
> alloc_dax() return value as non-fatal.
> 
> > 
> >> This would be an error handling fix all by itself. Do we really want
> >> to return successfully if dax is unsupported, or should we return
> >> an error here ?
> > 
> > Per above, there is no error handling fix, and pmem block device
> > available should not depend on alloc_dax() succeeding.
> 
> I agree on treating alloc_dax() failure as non-fatal. There is
> however one error handling fix to nvdimm/pmem which I plan to
> introduce as an initial patch before this change:
> 
>      nvdimm/pmem: Fix leak on dax_add_host() failure
>      
>      Fix a leak on dax_add_host() error, where "goto out_cleanup_dax" is done
>      before setting pmem->dax_dev, which therefore issues the two following
>      calls on NULL pointers:
>      
>      out_cleanup_dax:
>              kill_dax(pmem->dax_dev);
>              put_dax(pmem->dax_dev);
>      
>      Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> 
> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index 4e8fdcb3f1c8..9fe358090720 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
> @@ -566,12 +566,11 @@ static int pmem_attach_disk(struct device *dev,
>   	set_dax_nomc(dax_dev);
>   	if (is_nvdimm_sync(nd_region))
>   		set_dax_synchronous(dax_dev);
> +	pmem->dax_dev = dax_dev;
>   	rc = dax_add_host(dax_dev, disk);
>   	if (rc)
>   		goto out_cleanup_dax;
>   	dax_write_cache(dax_dev, nvdimm_has_cache(nd_region));
> -	pmem->dax_dev = dax_dev;
> -
>   	rc = device_add_disk(dev, disk, pmem_attribute_groups);
>   	if (rc)
>   		goto out_remove_host;

Yup, looks good.

> > The real question is what to do about device-dax. I *think* it is not
> > affected by cpu_dcache aliasing because it never accesses user mappings
> > through a kernel alias. I doubt device-dax is in use on these platforms,
> > but we might need another fixup for that if someone screams about the
> > alloc_dax() behavior change making them lose device-dax access.
> 
> By "device-dax", I understand you mean drivers/dax/Kconfig:DEV_DAX.
> 
> Based on your analysis, is alloc_dax() still the right spot where
> to place this runtime check ? Which call sites are responsible
> for invoking alloc_dax() for device-dax ?

That is in devm_create_dev_dax().

> If we know which call sites do not intend to use the kernel linear
> mapping, we could introduce a flag (or a new variant of the alloc_dax()
> API) that would either enforce or skip the check.

Hmmm, it looks like there is already a natural flag for that. If
alloc_dax() is passed a NULL operations pointer it means there are no
kernel usages of the aliased mapping. That actually fits rather nicely.

[..]
> >>> @@ -804,6 +808,15 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
> >>>    	if (!IS_ENABLED(CONFIG_FUSE_DAX))
> >>>    		return 0;
> >>>    
> >>> +	dax_dev = alloc_dax(fs, &virtio_fs_dax_ops);
> >>> +	if (IS_ERR(dax_dev)) {
> >>> +		int rc = PTR_ERR(dax_dev);
> >>> +
> >>> +		if (rc == -EOPNOTSUPP)
> >>> +			return 0;
> >>> +		return rc;
> >>> +	}
> >>
> >> What is gained by moving this allocation here ?
> > 
> > The gain is to fail early in virtio_fs_setup_dax() since the fundamental
> > dependency of alloc_dax() success is not met. For example why let the
> > setup progress to devm_memremap_pages() when alloc_dax() is going to
> > return ERR_PTR(-EOPNOTSUPP).
> 
> What I don't know is whether there is a dependency requiring to do
> devm_request_mem_region(), devm_kzalloc(), devm_memremap_pages()
> before calling alloc_dax() ?
> 
> Those 3 calls are used to populate:
> 
>          fs->window_phys_addr = (phys_addr_t) cache_reg.addr;
>          fs->window_len = (phys_addr_t) cache_reg.len;
> 
> and then alloc_dax() takes "fs" as private data parameter. So it's
> unclear to me whether we can swap the invocation order. I suspect
> that it is not an issue because it is only used to populate
> dax_dev->private, but I prefer to confirm this with you just to be
> on the safe side.

Thanks for that. All of those need to be done before the fs goes live
later in virtio_device_ready(), but before that point nothing should be
calling into virtio_fs_dax_ops, so as far as I can see it is safe to
change the order.