linux-kernel - RE: [PATCH v5 5/8] virtio: Treat alloc_dax() -EOPNOTSUPP failure as non-fatal

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: <65ca95d086dfd_d2d429470@dwillia2-xfh.jf.intel.com.notmuch>
Date: Mon, 12 Feb 2024 14:04:00 -0800
From: Dan Williams <dan.j.williams@...el.com>
To: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>, Dan Williams
	<dan.j.williams@...el.com>, Arnd Bergmann <arnd@...db.de>, Dave Chinner
	<david@...morbit.com>
CC: <linux-kernel@...r.kernel.org>, Mathieu Desnoyers
	<mathieu.desnoyers@...icios.com>, Andrew Morton <akpm@...ux-foundation.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>, Vishal Verma
	<vishal.l.verma@...el.com>, Dave Jiang <dave.jiang@...el.com>, Matthew Wilcox
	<willy@...radead.org>, Russell King <linux@...linux.org.uk>,
	<linux-arch@...r.kernel.org>, <linux-cxl@...r.kernel.org>,
	<linux-fsdevel@...r.kernel.org>, <linux-mm@...ck.org>,
	<linux-xfs@...r.kernel.org>, <dm-devel@...ts.linux.dev>,
	<nvdimm@...ts.linux.dev>, <linux-s390@...r.kernel.org>, Alasdair Kergon
	<agk@...hat.com>, Mike Snitzer <snitzer@...nel.org>, Mikulas Patocka
	<mpatocka@...hat.com>
Subject: RE: [PATCH v5 5/8] virtio: Treat alloc_dax() -EOPNOTSUPP failure as
 non-fatal

Mathieu Desnoyers wrote:
> In preparation for checking whether the architecture has data cache
> aliasing within alloc_dax(), modify the error handling of virtio
> virtio_fs_setup_dax() to treat alloc_dax() -EOPNOTSUPP failure as
> non-fatal.
> 
> Co-developed-by: Dan Williams <dan.j.williams@...el.com>
> Signed-off-by: Dan Williams <dan.j.williams@...el.com>
> Fixes: d92576f1167c ("dax: does not work correctly with virtual aliasing caches")
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
> Cc: Alasdair Kergon <agk@...hat.com>
> Cc: Mike Snitzer <snitzer@...nel.org>
> Cc: Mikulas Patocka <mpatocka@...hat.com>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Linus Torvalds <torvalds@...ux-foundation.org>
> Cc: Dan Williams <dan.j.williams@...el.com>
> Cc: Vishal Verma <vishal.l.verma@...el.com>
> Cc: Dave Jiang <dave.jiang@...el.com>
> Cc: Matthew Wilcox <willy@...radead.org>
> Cc: Arnd Bergmann <arnd@...db.de>
> Cc: Russell King <linux@...linux.org.uk>
> Cc: linux-arch@...r.kernel.org
> Cc: linux-cxl@...r.kernel.org
> Cc: linux-fsdevel@...r.kernel.org
> Cc: linux-mm@...ck.org
> Cc: linux-xfs@...r.kernel.org
> Cc: dm-devel@...ts.linux.dev
> Cc: nvdimm@...ts.linux.dev
> ---
>  fs/fuse/virtio_fs.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index 5f1be1da92ce..f9acd9972af2 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -16,6 +16,7 @@
>  #include <linux/fs_context.h>
>  #include <linux/fs_parser.h>
>  #include <linux/highmem.h>
> +#include <linux/cleanup.h>
>  #include <linux/uio.h>
>  #include "fuse_i.h"
>  
> @@ -795,8 +796,11 @@ static void virtio_fs_cleanup_dax(void *data)
>  	put_dax(dax_dev);
>  }
>  
> +DEFINE_FREE(cleanup_dax, struct dax_dev *, if (!IS_ERR(_T)) virtio_fs_cleanup_dax(_T))
> +
>  static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
>  {
> +	struct dax_device *dax_dev __free(cleanup_dax) = ERR_PTR(-EOPNOTSUPP);
>  	struct virtio_shm_region cache_reg;
>  	struct dev_pagemap *pgmap;
>  	bool have_cache;
> @@ -804,6 +808,12 @@ 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);
> +		return rc == -EOPNOTSUPP ? 0 : rc;
> +	}
> +
>  	/* Get cache region */
>  	have_cache = virtio_get_shm_region(vdev, &cache_reg,
>  					   (u8)VIRTIO_FS_SHMCAP_ID_CACHE);
> @@ -849,10 +859,7 @@ static int virtio_fs_setup_dax(struct virtio_device *vdev, struct virtio_fs *fs)
>  	dev_dbg(&vdev->dev, "%s: window kaddr 0x%px phys_addr 0x%llx len 0x%llx\n",
>  		__func__, fs->window_kaddr, cache_reg.addr, cache_reg.len);
>  
> -	fs->dax_dev = alloc_dax(fs, &virtio_fs_dax_ops);
> -	if (IS_ERR(fs->dax_dev))
> -		return PTR_ERR(fs->dax_dev);
> -
> +	fs->dax_dev = no_free_ptr(dax_dev);

This works because the internals of virtio_fs_cleanup_dax(), "kill_dax()
and put_dax()", know how to handle a NULL @dax_dev. It is still early
days with the "cleanup" helpers, but I wonder if anyone else cares that
the DEFINE_FREE() above does not check for NULL?

I think it is ok, but wanted to point that out for the virtiofs folks
and wonder what the style should be for things like this going forward.

Other growing pains with "cleanup.h" and ERR_PTR is happening over here:

http://lore.kernel.org/r/65ca861e14779_5a7f2949e@dwillia2-xfh.jf.intel.com.notmuch

..and that arrived at a similar style as is being used in this patch.
In both cases the cleanup function is called on exit with a NULL
argument.