Re: [PATCH v2 5/8] fuse: add support for opening dax device as backing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] mm: missing a_ops methods on passthrough DAX inodes
- [High] mm: uninitialized memory in fuse_backing_new for dax_error
- [High] mm: NULL pointer dereference in fuse_dax_notify_failure race
- [High] mm: virtiofs DAX cache consistency broken by IS_DAX check
- [Medium] mm: RCU data race on backing_id in fuse_backing_close_64
--

commit db1b69a7f08e8b3b4063c0a40f3bd08ae1c2dbc6
Author: Miklos Szeredi <mszeredi@redhat.com>
Subject: fuse: add support for opening dax device as backing

This commit adds support for using a DAX device as a backing file for FUSE.
It marks the inode with S_DAX when FUSE_ATTR_DAX is set during lookup, and
prepares the backing object structures and ops for DAX devices.

> --- a/fs/fuse/backing.c
> +++ b/fs/fuse/backing.c
[ ... ]
> @@ -103,39 +111,83 @@ int fuse_backing_close_64(struct fuse_conn *fc, u64 backing_id)

[Severity: Medium]
Could modifying fb->backing_id to 0 outside of the rhashtable lock while
lockless readers might still be inspecting it cause an RCU data race?
RCU readers could be concurrently comparing the key in rhashtable_lookup_fast,
potentially leading to KCSAN warnings.

>  	return 0;
>  }
>  
> -static struct fuse_backing *fuse_backing_new(struct fuse_conn *fc, int fd)
> +static int fuse_dax_notify_failure(struct dax_device *daxdev, u64 offset, u64 len, int mf_flags)
>  {
> -	struct fuse_backing *fb;
> -	struct super_block *backing_sb;
> -	struct file *file;
> +	struct fuse_backing *fb = dax_holder(daxdev);
>  
> -	/* TODO: relax CAP_SYS_ADMIN once backing files are visible to lsof */
> -	if (!fc->passthrough || !capable(CAP_SYS_ADMIN))
> -		return ERR_PTR(-EPERM);
> +	fb->dax_error = true;

[Severity: High]
Does this code risk a NULL pointer dereference if it races with fs_put_dax?
If a memory failure occurs concurrently with fuse_backing_free clearing
the DAX holder data, dax_holder could return NULL. Should we verify fb
is not NULL before accessing dax_error?

> +
> +	return 0;
> +}
[ ... ]
> +static struct fuse_backing *fuse_backing_new(struct fuse_conn *fc, int fd)
> +{
> +	struct fuse_backing *fb __free(kfree) = kmalloc_obj(*fb);

[Severity: High]
Does this allocation leave fb->dax_error uninitialized? Since kmalloc_obj
does not zero memory, the adjacent union member dax_error might contain
garbage when the dax_dev is explicitly initialized for S_IFCHR later. If
subsequent patches in this series read dax_error (such as in
fuse_ext_map_iomap_begin), it could falsely trigger an error path and cause
erratic failures.

> +	int err;
>  
> -	fb = kmalloc_obj(struct fuse_backing);
>  	if (!fb)
>  		return ERR_PTR(-ENOMEM);
[ ... ]
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -297,7 +297,7 @@ static int fuse_open(struct inode *inode, struct file *file)
>  	if (!err) {
>  		if (is_truncate)
>  			truncate_pagecache(inode, 0);
> -		else if (!(ff->open_flags & FOPEN_KEEP_CACHE))
> +		else if (!(ff->open_flags & FOPEN_KEEP_CACHE) && !IS_DAX(inode))
>  			invalidate_inode_pages2(inode->i_mapping);

[Severity: High]
Could skipping invalidate_inode_pages2 for DAX inodes break virtiofs DAX
cache consistency? Virtiofs DAX relies on invalidate_inode_pages2 to clear
stale DAX exceptional entries from the page cache XArray. Without this
invalidation, stale mappings might remain resident and cause data corruption.

>  	}
>  out_unlock:
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
[ ... ]
> @@ -403,6 +403,10 @@ static void fuse_init_submount_lookup(struct fuse_submount_lookup *sl,
>  	refcount_set(&sl->count, 1);
>  }
>  
> +static const struct address_space_operations fuse_dax_aops = {
> +	.dirty_folio	= noop_dirty_folio,
> +};
> +

[Severity: High]
Is it intentional that this struct lacks read_folio and write_begin? If a
DAX-enabled inode is opened without passthrough (e.g., FOPEN_PASSTHROUGH is
missing) or direct I/O, normal non-passthrough reads might be routed to the
page cache. The VFS could then dereference mapping->a_ops->read_folio which
is NULL, potentially causing a kernel panic.

>  static void fuse_init_inode(struct inode *inode, struct fuse_attr *attr,
>  			    struct fuse_conn *fc)
>  {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001150935.655979-1-mszeredi@redhat.com?part=5