Re: [PATCH V11 8/9] famfs_fuse: Add DAX address_space_operations with noop_dirty_folio

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] AB-BA self-deadlock on inode_lock(inode) in fuse_open() when opening a famfs file with O_TRUNC and writeback caching enabled.
- [High] Dynamic transition of an inode to DAX via S_DAX reassignment without page cache invalidation violates DAX invariants.
- [High] Premature publication of fi->famfs_meta allows a concurrent open() to return early and issue I/O before the inode's DAX state is initialized, causing spurious -ENXIO or SIGBUS.
- [High] Unvalidated dev_index from untrusted FUSE server causes Undefined Behavior via BIT_ULL() shift.
- [Medium] famfs_dax_aops is missing the .direct_IO callback, which will cause open() with O_DIRECT to fail with -EINVAL.
- [Medium] Data race on meta->error inside famfs_file_bad().
--

--- Patch [8]: [PATCH V11 8/9] famfs_fuse: Add DAX address_space_operations with noop_dirty_folio ---
commit f34b116803453f431c8f566dd550b747920ca217
Author: John Groves <John@Groves.net>

famfs_fuse: Add DAX address_space_operations with noop_dirty_folio

This commit adds a DAX-specific address_space_operations struct for famfs
using noop_dirty_folio. Since famfs is memory-backed and has no writeback,
these operations are assigned to the inode during DAX initialization.

> diff --git a/fs/fuse/famfs.c b/fs/fuse/famfs.c
> index 8d13282e8949f..aea0bceef774f 100644
> --- a/fs/fuse/famfs.c
> +++ b/fs/fuse/famfs.c

[ ... ]

> +/*
> + * DAX address_space_operations for famfs.
> + * famfs doesn't need dirty tracking - writes go directly to
> + * memory with no writeback required.
> + */
> +static const struct address_space_operations famfs_dax_aops = {
> +	.dirty_folio	= noop_dirty_folio,
> +};

[Severity: Medium]
Is it intentional to omit the .direct_IO callback here?

If applications attempt to open a DAX or memory-backed file with O_DIRECT,
VFS do_dentry_open() checks f_mapping->a_ops->direct_IO. Since it is
missing, will this cause the open to fail with -EINVAL and break
expected API functionality?

Should this include a .direct_IO callback, such as noop_direct_IO?

>  /*****************************************************************************/
>  
>  /*
> @@ -564,6 +574,7 @@ famfs_file_init_dax(
>  	if (famfs_meta_set(fi, meta) == NULL) {

[Severity: High]
Does publishing the metadata here allow a concurrent open to return early
before the inode's DAX state is fully initialized?

If Thread A executes famfs_file_init_dax(), publishes fi->famfs_meta via
cmpxchg here, and is preempted before setting S_DAX, Thread B could
concurrently open the same file.

Thread B would locklessly see fi->famfs_meta != NULL in fuse_get_fmap(),
return 0 immediately, and execute I/O before S_DAX is set. Would this
trigger the IS_DAX() checks in the I/O paths, resulting in spurious -ENXIO
errors or SIGBUS crashes for the application?

[Severity: High]
Can this lead to an AB-BA self-deadlock during truncation?

When opening a famfs file with O_TRUNC and writeback caching enabled,
fuse_open() evaluates is_wb_truncate to true and acquires the non-recursive
inode_lock(inode).

It then calls fuse_get_fmap(), which invokes famfs_file_init_dax().
In famfs_file_init_dax(), it unconditionally attempts to acquire
inode_lock(inode) again before we reach this point. Will this cause the
thread to self-deadlock?

>  		i_size_write(inode, meta->file_size);
>  		inode->i_flags |= S_DAX;

[Severity: High]
Does transitioning the inode to DAX mode here violate core DAX invariants if
the page cache is not invalidated?

If a file is opened with O_CREAT, it bypasses DAX initialization and writes
populate the standard page cache. If the file is subsequently reopened without
O_CREAT, it routes to fuse_open() and famfs_file_init_dax(), dynamically
setting S_DAX here.

Since fuse_open() explicitly skips page cache invalidation for famfs files,
does this leave stale, dirty pages in the VFS page cache? Could this lead
to kernel crashes or data corruption upon subsequent accesses or writeback?

> +		inode->i_data.a_ops = &famfs_dax_aops;
>  	} else {
>  		pr_debug("%s: file already had metadata\n", __func__);
>  		__famfs_meta_free(meta);

I also have a couple of questions about related functions:

[Severity: High]
Does this code overflow when shifting with an untrusted index?

In famfs_fuse_meta_alloc(), the dev_index from the FUSE server is used
directly in a bitwise shift:

fs/fuse/famfs.c:famfs_fuse_meta_alloc() {
    ...
    /* Record bitmap of referenced daxdev indices */
    meta->dev_bitmap |= BIT_ULL(meta->se[i].dev_index);
    ...
}

If a malicious FUSE server returns an fmap_buf with a dev_index of 64 or
greater, this will shift a 64-bit integer by 64 or more before any bounds
validation occurs. Can this cause undefined behavior and trigger a kernel
panic in kernels compiled with UBSAN?

[Severity: Medium]
Is there a data race when updating the error state locklessly?

When multiple threads perform concurrent I/O, famfs_file_bad() can
detect an i_size mismatch and update the state:

fs/fuse/famfs.c:famfs_file_bad() {
    ...
    meta->error = true;
    ...
}

Since this is a concurrent read and write of the shared boolean variable
without proper synchronization, will this violate the memory model and
trigger KCSAN warnings?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019f7d9fbe81-6cb16662-2522-47ea-a152-fab0ee3d9b35-000000@email.amazonses.com?part=8