Message-ID: <6jrtl2vc4dmi5b6db6tte2ckiyjmiwezbtlwrtmm464v65wkhj@znzv2mwjfgsk>
Date: Tue, 27 Feb 2024 18:59:36 -0600
From: John Groves <John@...ves.net>
To: Christian Brauner <brauner@...nel.org>
Cc: 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 24/02/27 02:41PM, Christian Brauner wrote:
> 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

<snip>

> > +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.

I wasn't aware of the new fsconfig interface. Is there documentation or a
file sytsem that already uses it that I should refer to? I didn't find an
obvious candidate, but it might be me. If it should be obvious from the
example above, tell me and I'll try harder.

My famfs code above was copied from ramfs. If you point me to 
documentation I might send you a ramfs fsconfig patch too :D.

> 
> > +
> > +		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?

Thanks... That should probaby be EBUSY. But the whole famfs_context_list
should probably also be removed. More below...

> 
> 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.
> 

And I suspect this is related to the get_tree issue you noticed below.

This famfs code was working in 6.5 without keeping the linked list of devices,
but in 6.6/6.7/6.8 it works provided you don't try to repeat a mount command
that has already succeeded. I'm not sure why 6.5 protected me from that,
but the later versions don't. In 6.6+ That hits a BUG_ON (have specifics on 
that but not handy right now).

So for a while we just removed repeated mount requests from the famfs smoke
tests, but eventually I implemented the list above, which - though you're right
it would be easy to circumvent and therefore is not right - it did solve the
problem that we were testing for.

I suspect that correctly handling get_tree might solve this problem.

Please assume that linked list will be removed - it was not the right solution.

More below...

> > +		}
> > +	}
> > +
> > +	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.

Clearly that's a bug in my code. That get_tree_nodev() is from ramfs, which
was the starting point for famfs.

I'm wondering if doing this correctly (get_tree_bdev() when it's pmem) would
have solved my double mount problem on 6.6 onward.

However, there's another wrinkle: I'm concluding
(see https://lore.kernel.org/linux-fsdevel/ups6cvjw6bx5m3hotn452brbbcgemnarsasre6ep2lbe4tpjsy@ezp6oh5c72ur/)
that famfs should drop block support and just work with /dev/dax. So famfs 
may be the first file system to be hosted on a character device? Certainly 
first on character dax. 

Given that, what variant of get_tree() should it call? Should it add 
get_tree_dax()? I'm not yet familiar enough with that code to have a worthy 
opinion on this.

Please let me know what you think.

Thank you for the serious review!
John

Powered by blists - more mailing lists