Re: UML/hostfs - mount failure at tip of tree

[Christian Brauner]

On Wed, Jul 24, 2024 at 05:49:16PM GMT, Hongbo Li wrote:
> 
> 
> On 2024/7/24 11:59, Maciej Żenczykowski wrote:
> > On Tue, Jul 23, 2024 at 7:55 PM Maciej Żenczykowski <maze@xxxxxxxxxx> wrote:
> > > 
> > > On Tue, Jul 23, 2024 at 7:22 PM Linus Torvalds
> > > <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> > > > 
> > > > On Tue, 23 Jul 2024 at 18:35, Hongbo Li <lihongbo22@xxxxxxxxxx> wrote:
> > > > > 
> > > > > I apologize for causing this issue. I am currently tracking it down.  If
> > > > > reverting this can solve the problem, you can revert it first.
> > > > 
> > > > I don't get the feeling that this is _so_ urgent that it needs to be
> > > > reverted immediately - let's give it at least a few days and see if
> > > > you (or somebody else) figures out the bug.
> > > > 
> > > > Maciej - if you can verify that folio conversion fix suggestion of
> > > > mine (or alternatively report that it doesn't help and I was barking
> > > > up the wrong tree), that would be great.
> > > 
> > > That appears to fix the folio patch indeed (ie. I no longer need to revert it).
> > > 
> > > The tests are still super unhappy, but I've yet to fix our tests very
> > > broken netlink parser for changes that released in 6.10, so that may
> > > be unrelated ;-)
> > 
> > +++ fs/hostfs/hostfs_kern.c:
> >   static int hostfs_fill_super(struct super_block *sb, struct fs_context *fc)
> >   {
> >          struct hostfs_fs_info *fsi = sb->s_fs_info;
> > -       const char *host_root = fc->source;
> > +       const char *host_root = "/";
> > 
> This doesn't work in case where the host directory is designated (such as
> mount -t hostfs hostfs -o /home /host).
> 
> I can fix this by the following patch, the root cause of this issue is the
> incorrect parsing of the host directory. The original mount path will use
> `parse_monolithic` to parse the host directory. For the new mount api, it
> use `parse_param` directly. So we should call `fsconfig(fd,
> FSCONFIG_SET_STRING, "hostfs", "xxx", 0)` to mount the hostfs(I think may be
> we should add hostfs as the key for host directory.). This may need
> Christian's reviews.:
> 
> ```
> From e7cc3be86a01b8382e9510f6ae1a2764942c7cba Mon Sep 17 00:00:00 2001
> From: Hongbo Li <lihongbo22@xxxxxxxxxx>
> Date: Wed, 24 Jul 2024 16:08:32 +0800
> Subject: [PATCH] hostfs: fix the host directory parse when mounting.
> 
> hostfs not keep the host directory when mounting. When the host
> directory is none (default), fc->source is used as the host root
> directory, and this is wrong. Here we use `parse_monolithic` to
> handle the old mount path for parsing the root directory. For new
> mount path, The `parse_param` is used for the host directory parse.
> 
> Fixes: cd140ce9f611 ("hostfs: convert hostfs to use the new mount API")
> Signed-off-by: Hongbo Li <lihongbo22@xxxxxxxxxx>
> ---
>  fs/hostfs/hostfs_kern.c | 64 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 54 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
> index 3eb747d26924..205c3700a035 100644
> --- a/fs/hostfs/hostfs_kern.c
> +++ b/fs/hostfs/hostfs_kern.c
> @@ -17,6 +17,7 @@
>  #include <linux/writeback.h>
>  #include <linux/mount.h>
>  #include <linux/fs_context.h>
> +#include <linux/fs_parser.h>
>  #include <linux/namei.h>
>  #include "hostfs.h"
>  #include <init.h>
> @@ -927,7 +928,6 @@ static const struct inode_operations hostfs_link_iops =
> {
>  static int hostfs_fill_super(struct super_block *sb, struct fs_context *fc)
>  {
>  	struct hostfs_fs_info *fsi = sb->s_fs_info;
> -	const char *host_root = fc->source;
>  	struct inode *root_inode;
>  	int err;
> 
> @@ -941,15 +941,6 @@ static int hostfs_fill_super(struct super_block *sb,
> struct fs_context *fc)
>  	if (err)
>  		return err;
> 
> -	/* NULL is printed as '(null)' by printf(): avoid that. */
> -	if (fc->source == NULL)
> -		host_root = "";
> -
> -	fsi->host_root_path =
> -		kasprintf(GFP_KERNEL, "%s/%s", root_ino, host_root);
> -	if (fsi->host_root_path == NULL)
> -		return -ENOMEM;
> -
>  	root_inode = hostfs_iget(sb, fsi->host_root_path);
>  	if (IS_ERR(root_inode))
>  		return PTR_ERR(root_inode);
> @@ -975,6 +966,57 @@ static int hostfs_fill_super(struct super_block *sb,
> struct fs_context *fc)
>  	return 0;
>  }
> 
> +enum hostfs_parma {
> +	Opt_hostfs,
> +};
> +
> +static const struct fs_parameter_spec hostfs_param_specs[] = {
> +	fsparam_string_empty("hostfs",		Opt_hostfs),
> +	{}
> +};
> +
> +static int hostfs_parse_param(struct fs_context *fc, struct fs_parameter
> *param)
> +{
> +	struct hostfs_fs_info *fsi = fc->s_fs_info;
> +	struct fs_parse_result result;
> +	char *host_root;
> +	int opt;
> +
> +	opt = fs_parse(fc, hostfs_param_specs, param, &result);
> +	if (opt < 0)
> +		return opt;
> +
> +	switch (opt) {
> +	case Opt_hostfs:
> +		host_root = param->string;
> +		if (!host_root)
> +			host_root = "";

That should be:

host_root = param->string;
if (!*host_root)
	host_root = "";

as param->string is never NULL but can be an empty string. I'll fix that
up though.

Otherwise overall looks sane to me.

I'm a bit puzzled that hostfs allowed to specify an option without a key like:

mount("hostfs", "/mnt", "/home");

but ok. So in the new mount api you did:

fsconfig(fd, FSCONFIG_SET_STRING, "hostfs", "/home", 0);

which I think is a lot saner.

> +		fsi->host_root_path =
> +			kasprintf(GFP_KERNEL, "%s/%s", root_ino, host_root);
> +		if (fsi->host_root_path == NULL)
> +			return -ENOMEM;
> +		break;
> +	}
> +
> +	return 0;
> +}
> +static int hostfs_parse_monolithic(struct fs_context *fc, void *data)
> +{
> +	struct hostfs_fs_info *fsi = fc->s_fs_info;
> +	char *host_root = (char *)data;
> +
> +	/* NULL is printed as '(null)' by printf(): avoid that. */
> +	if (host_root == NULL)
> +		host_root = "";
> +
> +	fsi->host_root_path =
> +		kasprintf(GFP_KERNEL, "%s/%s", root_ino, host_root);
> +	if (fsi->host_root_path == NULL)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
>  static int hostfs_fc_get_tree(struct fs_context *fc)
>  {
>  	return get_tree_nodev(fc, hostfs_fill_super);
> @@ -992,6 +1034,8 @@ static void hostfs_fc_free(struct fs_context *fc)
>  }
> 
>  static const struct fs_context_operations hostfs_context_ops = {
> +	.parse_monolithic = hostfs_parse_monolithic,
> +	.parse_param	= hostfs_parse_param,
>  	.get_tree	= hostfs_fc_get_tree,
>  	.free		= hostfs_fc_free,
>  };
> -- 
> 2.34.1
> ```
> 
> Thanks,
> Hongbo
> 
> > appears to fix the problem (when combined with Linus' folio fix).
> > 
> > I think fc->source is just the 'block device' passed to mount, and
> > thus for a virtual filesystem like hostfs, it is just garbage...
> > 
> > (and with the appropriate netlink fixes all the tests now pass at tip-of-tree:
> > 87f3073c2871 (HEAD) hostfs_fill_super(): host_root := "/" (not fc->source)
> > 2743a4aabac6 fs/hostfs/hostfs_kern.c:445 buffer =
> > folio_zero_tail(folio, bytes_read, buffer + bytes_read);
> > a2caf678d7e1 neighbour: add RTNL_FLAG_DUMP_SPLIT_NLM_DONE to RTM_GETNEIGH
> > 3bb0c5772acf net: add RTNL_FLAG_DUMP_SPLIT_NLM_DONE to RTM_GET(RULE|ROUTE)
> > 786c8248dbd3 (linux/master) Merge tag
> > 'perf-tools-fixes-for-v6.11-2024-07-23' of
> > git://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools
> > )
> > 
> > > > And perhaps remind me about this mount API thing too if it doesn't
> > > > seem to be resolved by the end of the week when I'm starting to get
> > > > ready to do the rc1?
> > > > 
> > > >               Linus
> > > 
> > > --
> > > Maciej Żenczykowski, Kernel Networking Developer @ Google
> > 
> > --
> > Maciej Żenczykowski, Kernel Networking Developer @ Google
> >