Re: [PATCH V14 11/12] famfs: Report device capacity via statfs so df works

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Use-after-free race condition in `dax_fsdev_size()` due to missing SRCU read lock (`dax_read_lock()`).
- [Medium] The statfs `used_capacity` counter grows monotonically but is never decremented when files are evicted, causing `df` to permanently show 0 available space.
- [Low] Mixing goto-based error handling with scope-based cleanup helpers (`scoped_guard`) in the same function violates kernel cleanup guidelines.
--

commit dd30d0d60f5f3c8708d001e5e982ce4519316fdc
Author: John Groves <John@Groves.net>

    famfs: Report device capacity via statfs so df works
    
    Replace simple_statfs(), which reports zero blocks (so df omits the mount),
    with famfs_statfs() reporting real capacity and usage.

> diff --git a/drivers/dax/fsdev.c b/drivers/dax/fsdev.c
> index 188b2526bee45..a5b4b2d794286 100644
> --- a/drivers/dax/fsdev.c
> +++ b/drivers/dax/fsdev.c
[ ... ]
> @@ -104,6 +104,25 @@ static size_t fsdev_dax_recovery_write(struct dax_device *dax_dev, pgoff_t pgoff
>  	return _copy_from_iter_flushcache(addr, bytes, i);
>  }
>  
> +/**
> + * dax_fsdev_size() - total size in bytes of an fsdev dax device
> + * @dax_dev: the dax device (must be bound to this driver)
> + *
> + * Returns the size cached at probe time (sum of all ranges); it cannot change
> + * while the driver is bound. Only valid for fsdev dax devices - callers
> + * ensure that (e.g. fs_dax_get() enforces DAXDRV_FSDEV_TYPE). Returns 0 if the
> + * device is not alive.
> + */
> +u64 dax_fsdev_size(struct dax_device *dax_dev)
> +{
> +	struct dev_dax *dev_dax = dax_get_private(dax_dev);
> +
> +	if (!dev_dax)
> +		return 0;
> +	return dev_dax->cached_size;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this code introduce a use-after-free race condition?

dax_fsdev_size() retrieves the dev_dax pointer via dax_get_private(dax_dev)
and accesses dev_dax->cached_size without acquiring the SRCU read lock
(dax_read_lock()).

If an asynchronous device unplug or sysfs unbind occurs concurrently,
kill_dax() could clear the DAXDEV_ALIVE flag and call synchronize_srcu().
Since the SRCU read lock is not held here, synchronize_srcu() would return
immediately, and the unplug path could free the dev_dax structure before
cached_size is read.

> +}
> +EXPORT_SYMBOL_GPL(dax_fsdev_size);
> +
>  static const struct dax_operations dev_dax_ops = {
>  	.direct_access = fsdev_dax_direct_access,
>  	.zero_page_range = fsdev_dax_zero_page_range,
[ ... ]
> diff --git a/fs/famfs/famfs_file.c b/fs/famfs/famfs_file.c
> index 0a6d0392794c5..965b6732f52c4 100644
> --- a/fs/famfs/famfs_file.c
> +++ b/fs/famfs/famfs_file.c
[ ... ]
> @@ -344,6 +344,11 @@ famfs_file_init_dax(struct file *file, void __user *arg)
>  	}
>  	inode_unlock(inode);
>  
> +	/* Account the mapped device bytes for statfs (only on success) */
> +	if (!rc) {
> +		scoped_guard(rwsem_write, &fsi->stats_sem)
> +			fsi->used_capacity += extent_total;
> +	}
        ^^^^^

[Severity: Low]
This isn't a bug, but does mixing scope-based cleanup helpers (scoped_guard)
with goto-based error handling in this function violate kernel cleanup
guidelines?

famfs_file_init_dax() heavily relies on goto labels (out:) for error
handling. The kernel guidelines suggest either converting all resources
that need cleanup to scope-based cleanup, or converting none of them, to
avoid confusing ownership semantics.

>  out:
>  	kvfree(fmap_buf);
>  	if (meta)
[ ... ]
> diff --git a/fs/famfs/famfs_inode.c b/fs/famfs/famfs_inode.c
> index 2f1938adbfbc3..61b668b299ef5 100644
> --- a/fs/famfs/famfs_inode.c
> +++ b/fs/famfs/famfs_inode.c
[ ... ]
> @@ -348,8 +349,38 @@ famfs_evict_inode(struct inode *inode)
>  	clear_inode(inode);
>  }
        ^^^^^^^^^^^^^^^^^^

[Severity: Medium]
Will this cause the fsi->used_capacity counter to grow monotonically and
eventually report zero available space?

When a user creates and maps a famfs file, fsi->used_capacity is permanently
incremented in famfs_file_init_dax(). However, famfs_evict_inode() frees the
metadata but does not appear to decrement fsi->used_capacity to reflect the
freed space.

If users repeatedly create and unlink files, won't fsi->used_capacity
double-count the space until df permanently shows 0 available blocks?

>  
> +/*
> + * famfs_statfs() - report device capacity and consumption so 'df' works.
> + * @total_capacity is the sum of installed daxdev sizes; @used_capacity is the
> + * sum of device bytes mapped by fmaps (superblock + log + data files). Free is
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/010001a04e801a4e-8eb212cd-b263-4043-ab65-33e480d2d7d4-000000@email.amazonses.com?part=11