Re: [PATCH V12 04/12] famfs: Introduce inode_operations and super_operations

[sashiko-bot@kernel.org]

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 <john@groves.net>

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-000000@email.amazonses.com?part=4