Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
New issues: - [High] Missing read memory barrier in lockless daxdev lookup causes NULL pointer dereferences and uninitialized iomap returns. - [High] Huge user-provided extent lengths can bypass overflow checks, wrap to negative `loff_t`, and cause `iomap_begin` to return a negative length. - [High] Concurrent lockless modification of latching error state (meta->error) from page fault paths. - [High] Page fault path can observe partially initialized inode state due to missing synchronization. - [Medium] Concurrent read of `dd->name` in `famfs_dax_err` while `dd->valid == 0` can dereference uncommitted or unmapped memory. - [Medium] Un-ratelimited pr_warn in the I/O path can cause kernel log flooding. - [Low] Native 64-bit division and modulo operations cause build failures on 32-bit architectures. Pre-existing issues: - [High] famfs_dax_read_iter manually advances iocb->ki_pos after dax_iomap_rw already advanced it. -- commit b15edec08ab799f362ddce177190dc4582c58868 Author: John Groves <[email protected]> Subject: famfs: iomap_begin and file-to-dax offset resolution Add the iomap resolver that maps a file offset to a (daxdev, offset) pair. Wires it into the read, write and fault paths by replacing their NULL stub with &famfs_iomap_ops, so dax_iomap_rw() and dax_iomap_fault() now resolve through famfs. > diff --git a/fs/famfs/famfs_file.c b/fs/famfs/famfs_file.c > index f099e0d08673f..2200f168aa79d 100644 > --- a/fs/famfs/famfs_file.c > +++ b/fs/famfs/famfs_file.c > @@ -384,6 +384,305 @@ famfs_file_ioctl(struct file *file, unsigned int cmd, > unsigned long arg) [ ... ] > +static int > +famfs_dax_err(struct famfs_daxdev *dd) > +{ > + if (!dd->valid) { > + pr_debug("%s: daxdev=%s invalid\n", __func__, dd->name); [Severity: Medium] Can this race with daxdev initialization? A concurrent famfs_install_daxdev() allocates dd->name via kstrdup() before setting dd->valid = 1. Could a reader here observe the pointer but dereference uncommitted string memory? > + return -EIO; > + } [ ... ] > +static struct dax_device * > +famfs_daxdev_from_index(struct famfs_fs_info *fsi, u64 dev_index, int *errp) > +{ [ ... ] > + dd = &devlist->devlist[dev_index]; > + rc = famfs_dax_err(dd); > + if (rc) { > + *errp = rc; > + return NULL; > + } > + *errp = 0; > + return dd->devp; [Severity: High] Does this lockless read of dd->devp need an smp_rmb()? The writer famfs_install_daxdev() uses a wmb() to order stores before setting dd->valid = 1. If the reader reads dd->valid == 1 in famfs_dax_err(), could CPU reordering cause it to fetch a stale NULL for dd->devp, returning NULL while setting *errp = 0? > +} > + > +static int > +famfs_meta_to_dax_offset_interleaved( [ ... ] > + /* Is the data is in this striped extent? */ > + if (local_offset < ext_size) { > + u64 chunk_num = local_offset / chunk_size; > + u64 chunk_offset = local_offset % chunk_size; > + u64 stripe_num = chunk_num / nstrips; > + u64 strip_num = chunk_num % nstrips; [Severity: Low] Will these native 64-bit division and modulo operations cause build failures on 32-bit architectures? Should this use kernel math helpers like div64_u64()? > + u64 chunk_remainder = chunk_size - chunk_offset; > + u64 strip_offset = chunk_offset + (stripe_num * > chunk_size); > + struct famfs_meta_simple_ext *strip = > &fei->ie_strips[strip_num]; > + struct dax_device *daxdev; [ ... ] > + iomap->addr = strip->ext_offset + strip_offset; > + iomap->offset = file_offset; > + iomap->length = min_t(loff_t, len, chunk_remainder); > + iomap->length = min_t(loff_t, iomap->length, > + strip->ext_len - strip_offset); [Severity: High] If a user provides a very large strip->ext_len that exceeds the maximum signed loff_t (e.g. > 0x7FFFFFFFFFFFFFFF), will casting it to loff_t in min_t() result in a negative number? Could this cause iomap->length to become negative and break VFS iomap iterator invariants? > + iomap->dax_dev = daxdev; > + iomap->type = IOMAP_MAPPED; > + > + return 0; [ ... ] > +static int > +famfs_meta_to_dax_offset( [ ... ] > + for (i = 0; i < meta->fm_nextents; i++) { > + loff_t dax_ext_offset = meta->se[i].ext_offset; > + loff_t dax_ext_len = meta->se[i].ext_len; > + > + if ((dax_ext_offset == 0) && > + (meta->file_type != FAMFS_SUPERBLOCK)) > + pr_warn("%s: zero offset on non-superblock file!!\n", > + __func__); [Severity: Medium] Since this is on the hot I/O path for every upcall, could an unprivileged user trivially flood the kernel log by reading a mapped file with ext_offset set to 0? Should this use a ratelimited printk? > + > + /* local_offset is the offset minus the size of extents skipped > + * so far; If local_offset < dax_ext_len, the data of interest > + * starts in this extent > + */ > + if (local_offset < dax_ext_len) { > + loff_t ext_len_remainder = dax_ext_len - local_offset; > + struct dax_device *daxdev; > + > + daxdev = famfs_daxdev_from_index(fsi, > + meta->se[i].dev_index, &rc); > + if (!daxdev) { > + meta->error = true; [Severity: High] If reading a file races with famfs_daxdev_open(), could failing to find the daxdev prematurely latch meta->error = true? Since this flag is persistent, does this permanently deny access to the file? Also, is it safe to write to meta->error locklessly from concurrent fault paths? Additionally, if famfs_daxdev_from_index() returns NULL with rc = 0 due to reordering (as asked earlier), does this return 0 to the VFS while leaving iomap->length uninitialized, causing iterators to malfunction? > + return rc; > + } [ ... ] > + iomap->addr = dax_ext_offset + local_offset; > + iomap->offset = file_offset; > + iomap->length = min_t(loff_t, len, ext_len_remainder); [Severity: High] Similar to the interleaved case, if dax_ext_len is extremely large, could it cast to a negative loff_t, resulting in a negative ext_len_remainder and causing iomap->length to become negative? > + iomap->dax_dev = daxdev; > + iomap->type = IOMAP_MAPPED; > + > + return 0; > + } [ ... ] > @@ -412,7 +711,7 @@ __famfs_filemap_fault( > file_update_time(vmf->vma->vm_file); > } > > - ret = dax_iomap_fault(vmf, order, &pfn, NULL, NULL /*&famfs_iomap_ops > */); > + ret = dax_iomap_fault(vmf, order, &pfn, NULL, &famfs_iomap_ops); [Severity: High] Looking at __famfs_filemap_fault(), it locklessly checks IS_DAX(inode). Since famfs_file_init_dax() sets inode->i_private, updates i_size, and sets S_DAX without memory barriers: famfs_file_init_dax() { inode->i_private = meta; i_size_write(inode, meta->file_size); inode->i_flags |= S_DAX; } Could the page fault path observe IS_DAX being true, but read a stale i_size of 0? If so, would famfs_file_invalid() incorrectly latch meta->error = true because i_size != meta->file_size? > if (ret & VM_FAULT_NEEDDSYNC) > ret = dax_finish_sync_fault(vmf, order, pfn); > > @@ -534,7 +833,7 @@ famfs_dax_read_iter(struct kiocb *iocb, struct iov_iter > *to) > return rc; > } > > - rc = dax_iomap_rw(iocb, to, NULL /*&famfs_iomap_ops */); > + rc = dax_iomap_rw(iocb, to, &famfs_iomap_ops); > inode_unlock_shared(inode); > > if (rc > 0) [Severity: High] This isn't a bug introduced by this patch, but by replacing the NULL ops with &famfs_iomap_ops, does dax_iomap_rw() now succeed and inherently update iocb->ki_pos? If so, does the subsequent manual advance of iocb->ki_pos via iocb->ki_pos += rc (just below this diff) cause a double-advance of the file offset during sequential reads? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/010001a04e801a4e-8eb212cd-b263-4043-ab65-33e480d2d7d4-000...@email.amazonses.com?part=8
