Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Setting `inode->i_fop` to `NULL` for regular files triggers a kernel WARN_ON during `open()`, leading to a denial of service. - [High] The `FAMFS_OPT_UNLINK` and `FAMFS_OPT_RMDIR` policies can be trivially bypassed by renaming an entry over existing files or directories. - [Medium] Creation operations (create, mkdir, mknod) fail to update the parent directory's timestamps, and racily update the child inode's timestamps after publishing. --
--- Patch [4]: [PATCH V12 04/12] famfs: Introduce inode_operations and super_operations --- commit 1062e22926010cfce092f1770e05033249b3ab2f Author: John Groves <[email protected]> famfs: Introduce inode_operations and super_operations The famfs inode and super operations are generic other than show_options, evict_inode and setattr. This commit builds but is still too incomplete to run. > diff --git a/fs/famfs/famfs_inode.c b/fs/famfs/famfs_inode.c > index ad71e5e7a8e39..efc6b852eca00 100644 > --- a/fs/famfs/famfs_inode.c > +++ b/fs/famfs/famfs_inode.c [ ... ] > @@ -54,11 +57,11 @@ static struct inode *famfs_get_inode( > init_special_inode(inode, mode, dev); > break; > case S_IFREG: > - inode->i_op = NULL /* famfs_file_inode_operations */; > + inode->i_op = &famfs_file_inode_operations; > inode->i_fop = NULL /* &famfs_file_operations */; [Severity: High] Will this trigger a kernel warning during open()? When VFS attempts to open a regular file, do_dentry_open() calls fops_get(inode->i_fop). Passing a NULL f_op pointer triggers a WARN_ON(!f->f_op), which could lead to a denial of service on systems with panic_on_warn enabled. > break; > case S_IFDIR: > - inode->i_op = NULL /* famfs_dir_inode_operations */; > + inode->i_op = &famfs_dir_inode_operations; > inode->i_fop = &simple_dir_operations; [ ... ] > +static int > +famfs_mknod(struct mnt_idmap *idmap, struct inode *dir, struct dentry > *dentry, > + umode_t mode, dev_t dev) > +{ > + struct famfs_fs_info *fsi = dir->i_sb->s_fs_info; > + struct timespec64 tv; > + struct inode *inode; > + > + if (fsi->deverror) > + return -ENODEV; > + > + inode = famfs_get_inode(dir->i_sb, dir, mode, dev); > + if (!inode) > + return -ENOSPC; > + > + d_make_persistent(dentry, inode); > + tv = inode_set_ctime_current(inode); > + inode_set_mtime_to_ts(inode, tv); > + inode_set_atime_to_ts(inode, tv); [Severity: Medium] Is there a typo here with 'inode' instead of 'dir'? Creation operations should update the parent directory's mtime and ctime. Updating the newly created child inode's timestamps here seems redundant since they are already initialized in famfs_get_inode(). Additionally, modifying the child inode's timestamps after calling d_make_persistent() publishes it to the dcache creates a race condition against lockless lookups. > + > + return 0; > +} [ ... ] > +static int > +famfs_rename( > + struct mnt_idmap *idmap, > + struct inode *old_dir, > + struct dentry *old_dentry, > + struct inode *new_dir, > + struct dentry *new_dentry, > + unsigned int flags) > +{ > + struct famfs_fs_info *fsi = old_dir->i_sb->s_fs_info; > + > + if (!famfs_opt_enabled(fsi, FAMFS_OPT_RENAME)) > + return -EPERM; > + > + return simple_rename(idmap, old_dir, old_dentry, new_dir, new_dentry, > + flags); [Severity: High] Can this implementation bypass the protections in famfs_unlink() and famfs_rmdir()? The filesystem implements safety checks in famfs_unlink() to prevent the removal of files with active memory maps (where inode->i_private is set). However, delegating directly to simple_rename() will internally call simple_unlink() or simple_rmdir() on an existing target without invoking the custom policy checks. Could this allow a policy bypass by renaming an entry over a protected target, potentially destroying active shared memory states? > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/0100019fc572ca94-ec363dd7-3a77-484b-b4b7-f2503a0931a6-000...@email.amazonses.com?part=4
