UML/hostfs - mount failure at tip of tree

UML/hostfs - mount failure at tip of tree

[Maciej Żenczykowski]

Reverting the following 3 patches:
- 104eef133fd9 hostfs: Add const qualifier to host_root in hostfs_fill_super()
- cd140ce9f611 hostfs: convert hostfs to use the new mount API
- e3ec0fe944d2 hostfs: Convert hostfs_read_folio() to use a folio

appears to be necessary to get the Android net test framework to boot
with tip of tree,
*without* the reverts we get:
  mount: /host: special device hostfs does not exist.
(if I don't revert the folio change then it mounts, but appears to not
actually work)

This is likely related to having an old debian mount binary...
but it is not *that* old... the base OS image was built in October 2022.

root@uml-x86-64:/# mount --version
mount from util-linux 2.36.1 (libmount 2.36.1: selinux, smack, btrfs,
namespaces, assert, debug)
root@uml-x86-64:/# dpkg -l | egrep mount
ii  libmount1:amd64               2.36.1-8+deb11u1             amd64
     device mounting library
ii  mount                         2.36.1-8+deb11u1             amd64
     tools for mounting and manipulating filesystems
which is from https://packages.debian.org/bullseye/mount [oldstable]

https://www.debian.org/releases/bullseye/
The Debian 11 life cycle encompasses five years: the initial three
years of full Debian support, until August 14th, 2024, and two years
of Long Term Support (LTS), until August 31st, 2026.

Thoughts?

--
Maciej Żenczykowski, Kernel Networking Developer @ Google

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

[Linus Torvalds]

On Tue, 23 Jul 2024 at 15:33, Maciej Żenczykowski <maze@google.com> wrote:
>
> Reverting the following 3 patches:
> - 104eef133fd9 hostfs: Add const qualifier to host_root in hostfs_fill_super()
> - cd140ce9f611 hostfs: convert hostfs to use the new mount API
> - e3ec0fe944d2 hostfs: Convert hostfs_read_folio() to use a folio
>
> appears to be necessary to get the Android net test framework to boot
> with tip of tree,
> *without* the reverts we get:
>   mount: /host: special device hostfs does not exist.
> (if I don't revert the folio change then it mounts, but appears to not
> actually work)

Interesting. That folio change was clearly supposed to be a no-op, but
isn't. Which makes a revert the right thing to do regardless.

That code was odd before too, but clearly that commit is completely broken.

I think this part is buggy:

                buffer = folio_zero_tail(folio, bytes_read, buffer);

because while the documentation for folio_zero_tail() does imply that
usage, the third argument is supposed really looks like it should be
"buffer + bytes_read".

So  instead of reverting that commit, does it help to just do that instead:

-               buffer = folio_zero_tail(folio, bytes_read, buffer);
+               buffer = folio_zero_tail(folio, bytes_read, buffer +
bytes_read);

Willy, that function is really bad. It's not helpful when it
apparently confused even you, and the calling convention really is
broken. I think that folio_zero_tail() needs to be rewritten to have
sane calling conventions (like matching the docs!) or just die.

The mount API change is somethign else. Again, it wasn't supposed to
break anything, but clearly does, and so reverting it sounds sane
unless somebody sees what the problem is.

I'm not even guessing at what might have been wrong in that mount API
conversion.

           Linus

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

[Hongbo Li]

On 2024/7/24 9:14, Linus Torvalds wrote:
> On Tue, 23 Jul 2024 at 15:33, Maciej Żenczykowski <maze@google.com> wrote:
>>
>> Reverting the following 3 patches:
>> - 104eef133fd9 hostfs: Add const qualifier to host_root in hostfs_fill_super()
>> - cd140ce9f611 hostfs: convert hostfs to use the new mount API
>> - e3ec0fe944d2 hostfs: Convert hostfs_read_folio() to use a folio
>>
>> appears to be necessary to get the Android net test framework to boot
>> with tip of tree,
>> *without* the reverts we get:
>>    mount: /host: special device hostfs does not exist.
>> (if I don't revert the folio change then it mounts, but appears to not
>> actually work)
> 
> Interesting. That folio change was clearly supposed to be a no-op, but
> isn't. Which makes a revert the right thing to do regardless.
> 
> That code was odd before too, but clearly that commit is completely broken.
> 
> I think this part is buggy:
> 
>                  buffer = folio_zero_tail(folio, bytes_read, buffer);
> 
> because while the documentation for folio_zero_tail() does imply that
> usage, the third argument is supposed really looks like it should be
> "buffer + bytes_read".
> 
> So  instead of reverting that commit, does it help to just do that instead:
> 
> -               buffer = folio_zero_tail(folio, bytes_read, buffer);
> +               buffer = folio_zero_tail(folio, bytes_read, buffer +
> bytes_read);
> 
> Willy, that function is really bad. It's not helpful when it
> apparently confused even you, and the calling convention really is
> broken. I think that folio_zero_tail() needs to be rewritten to have
> sane calling conventions (like matching the docs!) or just die.
> 
> The mount API change is somethign else. Again, it wasn't supposed to
> break anything, but clearly does, and so reverting it sounds sane
> unless somebody sees what the problem is.
> 
I apologize for causing this issue. I am currently tracking it down.  If 
reverting this can solve the problem, you can revert it first.

Thanks,
Hongbo

> I'm not even guessing at what might have been wrong in that mount API
> conversion.
> 
>             Linus

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

[Linus Torvalds]

On Tue, 23 Jul 2024 at 18:35, Hongbo Li <lihongbo22@huawei.com> 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.

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

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

[Maciej Żenczykowski]

On Tue, Jul 23, 2024 at 7:22 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Tue, 23 Jul 2024 at 18:35, Hongbo Li <lihongbo22@huawei.com> 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 ;-)

> 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

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

[Maciej Żenczykowski]

On Tue, Jul 23, 2024 at 7:55 PM Maciej Żenczykowski <maze@google.com> wrote:
>
> On Tue, Jul 23, 2024 at 7:22 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Tue, 23 Jul 2024 at 18:35, Hongbo Li <lihongbo22@huawei.com> 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 = "/";

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

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

[Hongbo Li]

On 2024/7/24 11:59, Maciej Żenczykowski wrote:
> On Tue, Jul 23, 2024 at 7:55 PM Maciej Żenczykowski <maze@google.com> wrote:
>>
>> On Tue, Jul 23, 2024 at 7:22 PM Linus Torvalds
>> <torvalds@linux-foundation.org> wrote:
>>>
>>> On Tue, 23 Jul 2024 at 18:35, Hongbo Li <lihongbo22@huawei.com> 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@huawei.com>
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@huawei.com>
---
  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 = "";
+		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
> 

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@google.com> wrote:
> > > 
> > > On Tue, Jul 23, 2024 at 7:22 PM Linus Torvalds
> > > <torvalds@linux-foundation.org> wrote:
> > > > 
> > > > On Tue, 23 Jul 2024 at 18:35, Hongbo Li <lihongbo22@huawei.com> 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.:

I see you sent the patch. I looked at it yesterday but didn't really dig
into it. Let me go do that now. I'll have a pr ready for Linus latest
tomorrow.

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@google.com> wrote:
> > > 
> > > On Tue, Jul 23, 2024 at 7:22 PM Linus Torvalds
> > > <torvalds@linux-foundation.org> wrote:
> > > > 
> > > > On Tue, 23 Jul 2024 at 18:35, Hongbo Li <lihongbo22@huawei.com> 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@huawei.com>
> 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@huawei.com>
> ---
>  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
> > 

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

[Hongbo Li]

On 2024/7/26 15:52, Christian Brauner wrote:
> 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@google.com> wrote:
>>>>
>>>> On Tue, Jul 23, 2024 at 7:22 PM Linus Torvalds
>>>> <torvalds@linux-foundation.org> wrote:
>>>>>
>>>>> On Tue, 23 Jul 2024 at 18:35, Hongbo Li <lihongbo22@huawei.com> 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@huawei.com>
>> 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@huawei.com>
>> ---
>>   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.
> 
Thanks for reviewing!
Yeah, param->string is checked at the begin of fsconfig syscall, here is 
just for double check (may be redundant). The empty string is allowed 
for host_root(also ""), or just remove this?

Thanks,
Hongbo

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

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

[Hongbo Li]

On 2024/7/24 6:33, Maciej Żenczykowski wrote:
>    mount: /host: special device hostfs does not exist.
Hi, Maciej,

Sorry to bother you.

According to this output and the source code, I found the `src` is 
hostfs which seems like the fstype.

Can you provide the mount command? Is it like "mount none /mnt/home -t 
hostfs -o /home" or embedded in the boot command"./linux xxx 
rootfstype=hostfs hostfs=.."?


Thanks,
Hongbo