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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The `used_capacity` counter is permanently leaked when mapped files are unlinked or their inodes are evicted.
- [High] dax_fsdev_size() accesses the private dev_dax structure without holding the required dax_read_lock(), leading to a use-after-free race condition with concurrent device unbinds.
- [High] Missing read memory barrier when locklessly checking `daxdev->valid` allows reading an uninitialized or stale `daxdev->devp` pointer.
- [Low] famfs_file_init_dax() mixes goto-based error handling with scope-based cleanup helpers, violating the subsystem rules.
--

--- Patch [11]: [PATCH V12 11/12] famfs: Report device capacity via statfs so df works ---
commit 72bb530cad0a65ccec5c1eea93f055e5015f64ec
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.

Add dax_fsdev_size() in drivers/dax/fsdev.c, returning the size fsdev
caches at probe (dev_dax->cached_size - the sum of the device's ranges,
stable while bound), exported. It lives in fsdev.c because cached_size is
set only by the fsdev driver, and famfs only ever holds fsdev-mode daxdevs
(fs_dax_get() enforces DAXDRV_FSDEV_TYPE); famfs.ko therefore depends on
fsdev_dax.ko.

famfs tracks two byte counters under a new stats_sem:
 - total_capacity: summed in famfs_install_daxdev() from dax_fsdev_size(),
   covering the mount primary and every DAXDEV_OPEN secondary, counted once
   per daxdev (on the valid 0->1 transition).
 - used_capacity: summed in famfs_file_init_dax() from the fmap's mapped
   device bytes (superblock + log + data files).

famfs_statfs() reports total and free (total - used). Free is an
approximation of the userspace allocator's free space (it ignores allocator
gaps and reserved regions), which is adequate for df.

(Side note: I am the maintainer of drivers/dax/fsdev.c)

Signed-off-by: John Groves <john@groves.net>

> 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 dax_get_private() require holding the dax_read_lock() here?

In the DAX subsystem, accessing dev_dax from a dax_device requires holding the
SRCU-based dax_read_lock(), because a concurrent driver unbind can call
kill_dax(), which clears DAXDEV_ALIVE, calls synchronize_srcu(&dax_srcu), and
frees the dev_dax structure.

Since dax_fsdev_size() and its caller famfs_install_daxdev() do not hold
dax_read_lock(), could a concurrent unbind result in dev_dax being freed
before dev_dax->cached_size is dereferenced?

> +EXPORT_SYMBOL_GPL(dax_fsdev_size);
> +
>  static const struct dax_operations dev_dax_ops = {
> diff --git a/fs/famfs/famfs_file.c b/fs/famfs/famfs_file.c
> index abf049b32a4b9..5be39d6770899 100644
> --- a/fs/famfs/famfs_file.c
> +++ b/fs/famfs/famfs_file.c
> @@ -280,6 +280,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: High]
Is fsi->used_capacity ever decremented when files are unlinked?

If famfs_file_init_dax() increments fsi->used_capacity here, should there be a
matching decrement path when the mapping is torn down and its private metadata
is freed in famfs_evict_inode()?

If FAMFS_OPT_UNLINK is enabled via ioctl, userspace could repeatedly create,
map, and delete files, causing used_capacity to monotonically grow until it
exceeds total_capacity. Would this permanently break the statfs free space
accounting for the mount?

> +	}
>  out:
>  	kvfree(fmap_buf);
>  	if (meta)
>  		famfs_meta_free(meta);

[Severity: Low]
This isn't a bug, but does this code mix goto-based error handling with
scope-based cleanup helpers?

Using the out label for manual cleanup (freeing fmap_buf and meta) while also
using scoped_guard() in the same function violates the documented cleanup
guidelines, which state that functions must not mix goto-based cleanup labels
and __free()/guard() declarations.

> diff --git a/fs/famfs/famfs_inode.c b/fs/famfs/famfs_inode.c
> index 6cbd7d657fd80..3c0d1094d6532 100644
> --- a/fs/famfs/famfs_inode.c
> +++ b/fs/famfs/famfs_inode.c
> @@ -462,6 +493,15 @@ int famfs_install_daxdev(
>  
>  		wmb(); /* All other fields must be visible before valid */
>  		daxdev->valid = 1;
> +		devp = daxdev->devp;
> +	}

[Severity: High]
Is there a matching read memory barrier on the consumer side?

Here the producer uses wmb() before setting daxdev->valid = 1 to make the new
devp visible.

On the IO/fault path, famfs_daxdev_from_index() calls famfs_dax_err() to
locklessly read dd->valid, and if true, proceeds to read dd->devp:

fs/famfs/famfs_file.c:famfs_dax_err() {
    if (!dd->valid) {
        ...
}

fs/famfs/famfs_file.c:famfs_daxdev_from_index() {
    ...
    *errp = 0;
    return dd->devp;
}

If there is no paired read memory barrier (e.g., smp_rmb() or
smp_load_acquire()) on the consumer side, could the CPU reorder the loads?

A concurrent thread might observe valid == 1 but still read a stale or NULL
value for devp, resulting in a NULL pointer dereference during IO.

> +
> +	/* Freshly installed: add its capacity to the statfs accounting */
> +	if (devp) {
> +		u64 sz = dax_fsdev_size(devp);
> +
> +		scoped_guard(rwsem_write, &fsi->stats_sem)
> +			fsi->total_capacity += sz;
>  	}
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/0100019fc572ca94-ec363dd7-3a77-484b-b4b7-f2503a0931a6-000000@email.amazonses.com?part=11