Message-ID: <20240227-mammut-tastatur-d791ca2f556b@brauner>
Date: Tue, 27 Feb 2024 14:41:44 +0100
From: Christian Brauner <brauner@...nel.org>
To: John Groves <John@...ves.net>
Cc: Christian Brauner <brauner@...nel.org>,
	John Groves <jgroves@...ron.com>,
	Jonathan Corbet <corbet@....net>,
	Dan Williams <dan.j.williams@...el.com>,
	Vishal Verma <vishal.l.verma@...el.com>,
	Dave Jiang <dave.jiang@...el.com>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Jan Kara <jack@...e.cz>,
	Matthew Wilcox <willy@...radead.org>,
	linux-cxl@...r.kernel.org,
	linux-fsdevel@...r.kernel.org,
	linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	nvdimm@...ts.linux.dev,
	john@...alactic.com,
	Dave Chinner <david@...morbit.com>,
	Christoph Hellwig <hch@...radead.org>,
	dave.hansen@...ux.intel.com,
	gregory.price@...verge.com
Subject: Re: [RFC PATCH 11/20] famfs: Add fs_context_operations

On Fri, Feb 23, 2024 at 11:41:55AM -0600, John Groves wrote:
> This commit introduces the famfs fs_context_operations and
> famfs_get_inode() which is used by the context operations.
> 
> Signed-off-by: John Groves <john@...ves.net>
> ---
>  fs/famfs/famfs_inode.c | 178 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 178 insertions(+)
> 
> diff --git a/fs/famfs/famfs_inode.c b/fs/famfs/famfs_inode.c
> index 82c861998093..f98f82962d7b 100644
> --- a/fs/famfs/famfs_inode.c
> +++ b/fs/famfs/famfs_inode.c
> @@ -41,6 +41,50 @@ static const struct super_operations famfs_ops;
>  static const struct inode_operations famfs_file_inode_operations;
>  static const struct inode_operations famfs_dir_inode_operations;
>  
> +static struct inode *famfs_get_inode(
> +	struct super_block *sb,
> +	const struct inode *dir,
> +	umode_t             mode,
> +	dev_t               dev)
> +{
> +	struct inode *inode = new_inode(sb);
> +
> +	if (inode) {
> +		struct timespec64       tv;
> +
> +		inode->i_ino = get_next_ino();
> +		inode_init_owner(&nop_mnt_idmap, inode, dir, mode);
> +		inode->i_mapping->a_ops = &ram_aops;
> +		mapping_set_gfp_mask(inode->i_mapping, GFP_HIGHUSER);
> +		mapping_set_unevictable(inode->i_mapping);
> +		tv = inode_set_ctime_current(inode);
> +		inode_set_mtime_to_ts(inode, tv);
> +		inode_set_atime_to_ts(inode, tv);
> +
> +		switch (mode & S_IFMT) {
> +		default:
> +			init_special_inode(inode, mode, dev);
> +			break;
> +		case S_IFREG:
> +			inode->i_op = &famfs_file_inode_operations;
> +			inode->i_fop = &famfs_file_operations;
> +			break;
> +		case S_IFDIR:
> +			inode->i_op = &famfs_dir_inode_operations;
> +			inode->i_fop = &simple_dir_operations;
> +
> +			/* Directory inodes start off with i_nlink == 2 (for "." entry) */
> +			inc_nlink(inode);
> +			break;
> +		case S_IFLNK:
> +			inode->i_op = &page_symlink_inode_operations;
> +			inode_nohighmem(inode);
> +			break;
> +		}
> +	}
> +	return inode;
> +}
> +
>  /**********************************************************************************
>   * famfs super_operations
>   *
> @@ -150,6 +194,140 @@ famfs_open_device(
>  	return 0;
>  }
>  
> +/*****************************************************************************************
> + * fs_context_operations
> + */
> +static int
> +famfs_fill_super(
> +	struct super_block *sb,
> +	struct fs_context  *fc)
> +{
> +	struct famfs_fs_info *fsi = sb->s_fs_info;
> +	struct inode *inode;
> +	int rc = 0;
> +
> +	sb->s_maxbytes		= MAX_LFS_FILESIZE;
> +	sb->s_blocksize		= PAGE_SIZE;
> +	sb->s_blocksize_bits	= PAGE_SHIFT;
> +	sb->s_magic		= FAMFS_MAGIC;
> +	sb->s_op		= &famfs_ops;
> +	sb->s_time_gran		= 1;
> +
> +	rc = famfs_open_device(sb, fc);
> +	if (rc)
> +		goto out;
> +
> +	inode = famfs_get_inode(sb, NULL, S_IFDIR | fsi->mount_opts.mode, 0);
> +	sb->s_root = d_make_root(inode);
> +	if (!sb->s_root)
> +		rc = -ENOMEM;
> +
> +out:
> +	return rc;
> +}
> +
> +enum famfs_param {
> +	Opt_mode,
> +	Opt_dax,
> +};
> +
> +const struct fs_parameter_spec famfs_fs_parameters[] = {
> +	fsparam_u32oct("mode",	  Opt_mode),
> +	fsparam_string("dax",     Opt_dax),
> +	{}
> +};
> +
> +static int famfs_parse_param(
> +	struct fs_context   *fc,
> +	struct fs_parameter *param)
> +{
> +	struct famfs_fs_info *fsi = fc->s_fs_info;
> +	struct fs_parse_result result;
> +	int opt;
> +
> +	opt = fs_parse(fc, famfs_fs_parameters, param, &result);
> +	if (opt == -ENOPARAM) {
> +		opt = vfs_parse_fs_param_source(fc, param);
> +		if (opt != -ENOPARAM)
> +			return opt;

I'm not sure I understand this. But in any case add, you should add
Opt_source to enum famfs_param and then add

        fsparam_string("source",        Opt_source),

to famfs_fs_parameters. Then you can add:

famfs_parse_source(fc, param);

You might want to consider validating your devices right away. So think
about:

fd_fs = fsopen("famfs", ...);
ret = fsconfig(fd_fs, FSCONFIG_SET_STRING, "source", "/definitely/not/valid/device", ...) // succeeds
ret = fsconfig(fd_fs, FSCONFIG_SET_FLAG, "OPTION_1", ...) // succeeds
ret = fsconfig(fd_fs, FSCONFIG_SET_FLAG, "OPTION_2", ...) // succeeds 
ret = fsconfig(fd_fs, FSCONFIG_SET_FLAG, "OPTION_3", ...) // succeeds 
ret = fsconfig(fd_fs, FSCONFIG_SET_FLAG, "OPTION_N", ...) // succeeds 
ret = fsconfig(fd_fs, FSCONFIG_CMD_CREATE, ...) // superblock creation failed

So what failed exactly? Yes, you can log into the fscontext and dmesg
that it's @source that's the issue but it's annoying for userspace to
setup a whole mount context only to figure out that some option was
wrong at the end of it.

So validating

famfs_parse_source(...)
{
	if (fc->source)
		return invalfc(fc, "Uhm, we already have a source....
	
       lookup_bdev(fc->source, &dev)
       // validate it's a device you're actually happy to use

       fc->source = param->string;
       param->string = NULL;
}

Your ->get_tree implementation that actually creates/finds the
superblock will validate fc->source again and yes, there's a race here
in so far as the path that fc->source points to could change in between
validating this in famfs_parse_source() and ->get_tree() superblock
creation. This is fixable even right now but then you couldn't reuse
common infrastrucute so I would just accept that race for now and we
should provide a nicer mechanism on the vfs layer.

> +
> +		return 0;
> +	}
> +	if (opt < 0)
> +		return opt;
> +
> +	switch (opt) {
> +	case Opt_mode:
> +		fsi->mount_opts.mode = result.uint_32 & S_IALLUGO;
> +		break;
> +	case Opt_dax:
> +		if (strcmp(param->string, "always"))
> +			pr_notice("%s: invalid dax mode %s\n",
> +				  __func__, param->string);
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static DEFINE_MUTEX(famfs_context_mutex);
> +static LIST_HEAD(famfs_context_list);
> +
> +static int famfs_get_tree(struct fs_context *fc)
> +{
> +	struct famfs_fs_info *fsi_entry;
> +	struct famfs_fs_info *fsi = fc->s_fs_info;
> +
> +	fsi->rootdev = kstrdup(fc->source, GFP_KERNEL);
> +	if (!fsi->rootdev)
> +		return -ENOMEM;
> +
> +	/* Fail if famfs is already mounted from the same device */
> +	mutex_lock(&famfs_context_mutex);
> +	list_for_each_entry(fsi_entry, &famfs_context_list, fsi_list) {
> +		if (strcmp(fsi_entry->rootdev, fc->source) == 0) {
> +			mutex_unlock(&famfs_context_mutex);
> +			pr_err("%s: already mounted from rootdev %s\n", __func__, fc->source);
> +			return -EALREADY;

What errno is EALREADY? Isn't that socket stuff. In any case, it seems
you want EBUSY?

But bigger picture I'm lost. And why do you keep that list based on
strings? What if I do:

mount -t famfs /dev/pmem1234 /mnt # succeeds

mount -t famfs /dev/pmem1234 /opt # ah, fsck me, this fails.. But wait a minute....

mount --bind /dev/pmem1234 /evil-masterplan

mount -t famfs /evil-masterplan /opt # succeeds. YAY

I believe that would trivially defeat your check.

> +		}
> +	}
> +
> +	list_add(&fsi->fsi_list, &famfs_context_list);
> +	mutex_unlock(&famfs_context_mutex);
> +
> +	return get_tree_nodev(fc, famfs_fill_super);

So why isn't this using get_tree_bdev()? Note that a while ago I
added FSCONFIG_CMD_CREAT_EXCL which prevents silent superblock reuse. To
implement that I added fs_context->exclusive. If you unconditionally set
fc->exclusive = 1 in your famfs_init_fs_context() and use
get_tree_bdev() it will give you EBUSY if fc->source is already in use -
including other famfs instances.

I also fail to yet understand how that function which actually opens the block
device and gets the dax device figures into this. It's a bit hard to follow
what's going on since you add all those unused functions and types so there's
never a wider context to see that stuff in.

> +
> +}
> +
> +static void famfs_free_fc(struct fs_context *fc)
> +{
> +	struct famfs_fs_info *fsi = fc->s_fs_info;
> +
> +	if (fsi && fsi->rootdev)
> +		kfree(fsi->rootdev);
> +
> +	kfree(fsi);
> +}
> +
> +static const struct fs_context_operations famfs_context_ops = {
> +	.free		= famfs_free_fc,
> +	.parse_param	= famfs_parse_param,
> +	.get_tree	= famfs_get_tree,
> +};
> +
> +static int famfs_init_fs_context(struct fs_context *fc)
> +{
> +	struct famfs_fs_info *fsi;
> +
> +	fsi = kzalloc(sizeof(*fsi), GFP_KERNEL);
> +	if (!fsi)
> +		return -ENOMEM;
> +
> +	fsi->mount_opts.mode = FAMFS_DEFAULT_MODE;
> +	fc->s_fs_info        = fsi;
> +	fc->ops              = &famfs_context_ops;
> +	return 0;
> +}
>  
>  
>  MODULE_LICENSE("GPL");
> -- 
> 2.43.0
> 

Powered by blists - more mailing lists