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

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 kern=
el WARN_ON during `open()`, leading to a denial of service.
- [High] The `FAMFS_OPT_UNLINK` and `FAMFS_OPT_RMDIR` policies can be trivi=
ally bypassed by renaming an entry over existing files or directories.
- [Medium] Creation operations (create, mkdir, mknod) fail to update the pa=
rent directory's timestamps, and racily update the child inode's timestamps=
 after publishing.
--

--- Patch [4]: [PATCH V12 04/12] famfs: Introduce inode_operations and supe=
r_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 =3D NULL /* famfs_file_inode_operations */;
> +		inode->i_op =3D &famfs_file_inode_operations;
>  		inode->i_fop =3D 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 =3D NULL /* famfs_dir_inode_operations */;
> +		inode->i_op =3D &famfs_dir_inode_operations;
>  		inode->i_fop =3D &simple_dir_operations;
[ ... ]
> +static int
> +famfs_mknod(struct mnt_idmap *idmap, struct inode *dir, struct dentry *d=
entry,
> +	    umode_t mode, dev_t dev)
> +{
> +	struct famfs_fs_info *fsi =3D dir->i_sb->s_fs_info;
> +	struct timespec64 tv;
> +	struct inode *inode;
> +
> +	if (fsi->deverror)
> +		return -ENODEV;
> +
> +	inode =3D famfs_get_inode(dir->i_sb, dir, mode, dev);
> +	if (!inode)
> +		return -ENOSPC;
> +
> +	d_make_persistent(dentry, inode);
> +	tv =3D 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 si=
nce
they are already initialized in famfs_get_inode().=20

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 =3D 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?

> +}

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/0100019fc572ca94-ec=
[email protected]?part=3D4