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 <[email protected]> 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-000...@email.amazonses.com?part=11
