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 <[email protected]> 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/[email protected]?part=5
