support booting of arbitrary non-blockdevice file systems v2

support booting of arbitrary non-blockdevice file systems v2

[Christoph Hellwig]

Hi all,

this series adds support to boot off arbitrary non-blockdevice root file
systems, based off an earlier patch from Vivek.

Chances since v1:
 - don't try to mount every registered file system if none is specified
 - fix various null pointer dereferences when certain kernel paramters are
   not set
 - general refactoring.

[Virtio-fs] support booting of arbitrary non-blockdevice file systems v2

[Christoph Hellwig]

Hi all,

this series adds support to boot off arbitrary non-blockdevice root file
systems, based off an earlier patch from Vivek.

Chances since v1:
 - don't try to mount every registered file system if none is specified
 - fix various null pointer dereferences when certain kernel paramters are
   not set
 - general refactoring.

[PATCH 1/2] init: split get_fs_names

[Christoph Hellwig]

Split get_fs_names into one function that splits up the command line
argument, and one that gets the list of all registered file systems.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 init/do_mounts.c | 48 ++++++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/init/do_mounts.c b/init/do_mounts.c
index 74aede860de7..ec32de3ad52b 100644
--- a/init/do_mounts.c
+++ b/init/do_mounts.c
@@ -338,30 +338,31 @@ __setup("rootflags=", root_data_setup);
 __setup("rootfstype=", fs_names_setup);
 __setup("rootdelay=", root_delay_setup);
 
-static void __init get_fs_names(char *page)
+static void __init split_fs_names(char *page, char *names)
 {
-	char *s = page;
-
-	if (root_fs_names) {
-		strcpy(page, root_fs_names);
-		while (*s++) {
-			if (s[-1] == ',')
-				s[-1] = '\0';
-		}
-	} else {
-		int len = get_filesystem_list(page);
-		char *p, *next;
+	strcpy(page, root_fs_names);
+	while (*page++) {
+		if (page[-1] == ',')
+			page[-1] = '\0';
+	}
+	*page = '\0';
+}
 
-		page[len] = '\0';
-		for (p = page-1; p; p = next) {
-			next = strchr(++p, '\n');
-			if (*p++ != '\t')
-				continue;
-			while ((*s++ = *p++) != '\n')
-				;
-			s[-1] = '\0';
-		}
+static void __init get_all_fs_names(char *page)
+{
+	int len = get_filesystem_list(page);
+	char *s = page, *p, *next;
+
+	page[len] = '\0';
+	for (p = page - 1; p; p = next) {
+		next = strchr(++p, '\n');
+		if (*p++ != '\t')
+			continue;
+		while ((*s++ = *p++) != '\n')
+			;
+		s[-1] = '\0';
 	}
+
 	*s = '\0';
 }
 
@@ -411,7 +412,10 @@ void __init mount_block_root(char *name, int flags)
 
 	scnprintf(b, BDEVNAME_SIZE, "unknown-block(%u,%u)",
 		  MAJOR(ROOT_DEV), MINOR(ROOT_DEV));
-	get_fs_names(fs_names);
+	if (root_fs_names)
+		split_fs_names(fs_names, root_fs_names);
+	else
+		get_all_fs_names(fs_names);
 retry:
 	for (p = fs_names; *p; p += strlen(p)+1) {
 		int err = do_mount_root(name, p, flags, root_mount_data);
-- 
2.30.2

[Virtio-fs] [PATCH 1/2] init: split get_fs_names

[Christoph Hellwig]

Split get_fs_names into one function that splits up the command line
argument, and one that gets the list of all registered file systems.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 init/do_mounts.c | 48 ++++++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/init/do_mounts.c b/init/do_mounts.c
index 74aede860de7..ec32de3ad52b 100644
--- a/init/do_mounts.c
+++ b/init/do_mounts.c
@@ -338,30 +338,31 @@ __setup("rootflags=", root_data_setup);
 __setup("rootfstype=", fs_names_setup);
 __setup("rootdelay=", root_delay_setup);
 
-static void __init get_fs_names(char *page)
+static void __init split_fs_names(char *page, char *names)
 {
-	char *s = page;
-
-	if (root_fs_names) {
-		strcpy(page, root_fs_names);
-		while (*s++) {
-			if (s[-1] == ',')
-				s[-1] = '\0';
-		}
-	} else {
-		int len = get_filesystem_list(page);
-		char *p, *next;
+	strcpy(page, root_fs_names);
+	while (*page++) {
+		if (page[-1] == ',')
+			page[-1] = '\0';
+	}
+	*page = '\0';
+}
 
-		page[len] = '\0';
-		for (p = page-1; p; p = next) {
-			next = strchr(++p, '\n');
-			if (*p++ != '\t')
-				continue;
-			while ((*s++ = *p++) != '\n')
-				;
-			s[-1] = '\0';
-		}
+static void __init get_all_fs_names(char *page)
+{
+	int len = get_filesystem_list(page);
+	char *s = page, *p, *next;
+
+	page[len] = '\0';
+	for (p = page - 1; p; p = next) {
+		next = strchr(++p, '\n');
+		if (*p++ != '\t')
+			continue;
+		while ((*s++ = *p++) != '\n')
+			;
+		s[-1] = '\0';
 	}
+
 	*s = '\0';
 }
 
@@ -411,7 +412,10 @@ void __init mount_block_root(char *name, int flags)
 
 	scnprintf(b, BDEVNAME_SIZE, "unknown-block(%u,%u)",
 		  MAJOR(ROOT_DEV), MINOR(ROOT_DEV));
-	get_fs_names(fs_names);
+	if (root_fs_names)
+		split_fs_names(fs_names, root_fs_names);
+	else
+		get_all_fs_names(fs_names);
 retry:
 	for (p = fs_names; *p; p += strlen(p)+1) {
 		int err = do_mount_root(name, p, flags, root_mount_data);
-- 
2.30.2

Re: [PATCH 1/2] init: split get_fs_names

[Al Viro]

On Mon, Jun 21, 2021 at 08:26:56AM +0200, Christoph Hellwig wrote:
> Split get_fs_names into one function that splits up the command line
> argument, and one that gets the list of all registered file systems.

> +static void __init get_all_fs_names(char *page)
> +{
> +	int len = get_filesystem_list(page);
> +	char *s = page, *p, *next;
> +
> +	page[len] = '\0';
> +	for (p = page - 1; p; p = next) {
> +		next = strchr(++p, '\n');
> +		if (*p++ != '\t')
> +			continue;
> +		while ((*s++ = *p++) != '\n')
> +			;
> +		s[-1] = '\0';
>  	}
> +
>  	*s = '\0';
>  }

TBH, I would rather take that one into fs/filesystems.c.  Rationale:
get_filesystem_list(), for all its resemblance to /proc/filesystems
contents, is used only by init/*.c and it's not a big deal to make
it

int __init get_filesystem_list(char *buf, bool is_dev)
{
	int f = is_dev ? FS_REQUIRES_DEV : 0;
        int left = PAGE_SIZE, count = 0;
        struct file_system_type *p;

        read_lock(&file_systems_lock);
	for (p = file_systems; p; p = p->next) {
		if ((p->fs_flags & FS_REQUIRES_DEV) == f) {
			size_t len = strlen(p->name) + 1;
			if (len > left)
				break;
			memcpy(buf, p->name, len);
			buf += len;
			left -= len;
			count++;
		}
	}
        read_unlock(&file_systems_lock);
	return count;
}

Generates NUL-separated list, returns the number of list elements,
the second argument is "what kind do you want"...

Re: [Virtio-fs] [PATCH 1/2] init: split get_fs_names

[Al Viro]

On Mon, Jun 21, 2021 at 08:26:56AM +0200, Christoph Hellwig wrote:
> Split get_fs_names into one function that splits up the command line
> argument, and one that gets the list of all registered file systems.

> +static void __init get_all_fs_names(char *page)
> +{
> +	int len = get_filesystem_list(page);
> +	char *s = page, *p, *next;
> +
> +	page[len] = '\0';
> +	for (p = page - 1; p; p = next) {
> +		next = strchr(++p, '\n');
> +		if (*p++ != '\t')
> +			continue;
> +		while ((*s++ = *p++) != '\n')
> +			;
> +		s[-1] = '\0';
>  	}
> +
>  	*s = '\0';
>  }

TBH, I would rather take that one into fs/filesystems.c.  Rationale:
get_filesystem_list(), for all its resemblance to /proc/filesystems
contents, is used only by init/*.c and it's not a big deal to make
it

int __init get_filesystem_list(char *buf, bool is_dev)
{
	int f = is_dev ? FS_REQUIRES_DEV : 0;
        int left = PAGE_SIZE, count = 0;
        struct file_system_type *p;

        read_lock(&file_systems_lock);
	for (p = file_systems; p; p = p->next) {
		if ((p->fs_flags & FS_REQUIRES_DEV) == f) {
			size_t len = strlen(p->name) + 1;
			if (len > left)
				break;
			memcpy(buf, p->name, len);
			buf += len;
			left -= len;
			count++;
		}
	}
        read_unlock(&file_systems_lock);
	return count;
}

Generates NUL-separated list, returns the number of list elements,
the second argument is "what kind do you want"...

Re: [PATCH 1/2] init: split get_fs_names

[Al Viro]

On Mon, Jun 21, 2021 at 02:46:37PM +0000, Al Viro wrote:

> int __init get_filesystem_list(char *buf, bool is_dev)
> {
> 	int f = is_dev ? FS_REQUIRES_DEV : 0;
>         int left = PAGE_SIZE, count = 0;
>         struct file_system_type *p;
> 
>         read_lock(&file_systems_lock);
> 	for (p = file_systems; p; p = p->next) {
> 		if ((p->fs_flags & FS_REQUIRES_DEV) == f) {
> 			size_t len = strlen(p->name) + 1;
> 			if (len > left)
> 				break;
> 			memcpy(buf, p->name, len);
> 			buf += len;
> 			left -= len;
> 			count++;
> 		}
> 	}
>         read_unlock(&file_systems_lock);
> 	return count;
> }
> 
> Generates NUL-separated list, returns the number of list elements,
> the second argument is "what kind do you want"...

Actually, looking at the rest of the queue, that's only used for
!nodev ones, so might as well drop the flag here...

Re: [Virtio-fs] [PATCH 1/2] init: split get_fs_names

[Al Viro]

On Mon, Jun 21, 2021 at 02:46:37PM +0000, Al Viro wrote:

> int __init get_filesystem_list(char *buf, bool is_dev)
> {
> 	int f = is_dev ? FS_REQUIRES_DEV : 0;
>         int left = PAGE_SIZE, count = 0;
>         struct file_system_type *p;
> 
>         read_lock(&file_systems_lock);
> 	for (p = file_systems; p; p = p->next) {
> 		if ((p->fs_flags & FS_REQUIRES_DEV) == f) {
> 			size_t len = strlen(p->name) + 1;
> 			if (len > left)
> 				break;
> 			memcpy(buf, p->name, len);
> 			buf += len;
> 			left -= len;
> 			count++;
> 		}
> 	}
>         read_unlock(&file_systems_lock);
> 	return count;
> }
> 
> Generates NUL-separated list, returns the number of list elements,
> the second argument is "what kind do you want"...

Actually, looking at the rest of the queue, that's only used for
!nodev ones, so might as well drop the flag here...

Re: [PATCH 1/2] init: split get_fs_names

[Christoph Hellwig]

On Mon, Jun 21, 2021 at 02:46:37PM +0000, Al Viro wrote:
> TBH, I would rather take that one into fs/filesystems.c.  Rationale:
> get_filesystem_list(), for all its resemblance to /proc/filesystems
> contents, is used only by init/*.c and it's not a big deal to make
> it

Yeah, unwinding this mess actually is a good idea.  I didn't really
look outside of do_mounts.c, but once doing that it becomes completely
obvious.

> int __init get_filesystem_list(char *buf, bool is_dev)

As-is we don't even really need the is_dev argument, as the only
callers wants block device file systems anyway.  In fact it would
much rather have a cursor based iteration so that we can skip the
allocation, but that is probaby overengineering the problem.

Re: [Virtio-fs] [PATCH 1/2] init: split get_fs_names

[Christoph Hellwig]

On Mon, Jun 21, 2021 at 02:46:37PM +0000, Al Viro wrote:
> TBH, I would rather take that one into fs/filesystems.c.  Rationale:
> get_filesystem_list(), for all its resemblance to /proc/filesystems
> contents, is used only by init/*.c and it's not a big deal to make
> it

Yeah, unwinding this mess actually is a good idea.  I didn't really
look outside of do_mounts.c, but once doing that it becomes completely
obvious.

> int __init get_filesystem_list(char *buf, bool is_dev)

As-is we don't even really need the is_dev argument, as the only
callers wants block device file systems anyway.  In fact it would
much rather have a cursor based iteration so that we can skip the
allocation, but that is probaby overengineering the problem.

Re: [PATCH 1/2] init: split get_fs_names

[Al Viro]

On Mon, Jun 21, 2021 at 04:53:09PM +0200, Christoph Hellwig wrote:
> On Mon, Jun 21, 2021 at 02:46:37PM +0000, Al Viro wrote:
> > TBH, I would rather take that one into fs/filesystems.c.  Rationale:
> > get_filesystem_list(), for all its resemblance to /proc/filesystems
> > contents, is used only by init/*.c and it's not a big deal to make
> > it
> 
> Yeah, unwinding this mess actually is a good idea.  I didn't really
> look outside of do_mounts.c, but once doing that it becomes completely
> obvious.
> 
> > int __init get_filesystem_list(char *buf, bool is_dev)
> 
> As-is we don't even really need the is_dev argument, as the only
> callers wants block device file systems anyway.

*nod*

> In fact it would
> much rather have a cursor based iteration so that we can skip the
> allocation, but that is probaby overengineering the problem.

Very much so.

Sigh...  I really wish we had more uniform syntax, though -
e.g.  root=nfs(<options>) or root=xfs(sdb11,noatime), etc.
Oh, well...

Re: [Virtio-fs] [PATCH 1/2] init: split get_fs_names

[Al Viro]

On Mon, Jun 21, 2021 at 04:53:09PM +0200, Christoph Hellwig wrote:
> On Mon, Jun 21, 2021 at 02:46:37PM +0000, Al Viro wrote:
> > TBH, I would rather take that one into fs/filesystems.c.  Rationale:
> > get_filesystem_list(), for all its resemblance to /proc/filesystems
> > contents, is used only by init/*.c and it's not a big deal to make
> > it
> 
> Yeah, unwinding this mess actually is a good idea.  I didn't really
> look outside of do_mounts.c, but once doing that it becomes completely
> obvious.
> 
> > int __init get_filesystem_list(char *buf, bool is_dev)
> 
> As-is we don't even really need the is_dev argument, as the only
> callers wants block device file systems anyway.

*nod*

> In fact it would
> much rather have a cursor based iteration so that we can skip the
> allocation, but that is probaby overengineering the problem.

Very much so.

Sigh...  I really wish we had more uniform syntax, though -
e.g.  root=nfs(<options>) or root=xfs(sdb11,noatime), etc.
Oh, well...

Re: [PATCH 1/2] init: split get_fs_names

[Matthew Wilcox]

On Mon, Jun 21, 2021 at 08:26:56AM +0200, Christoph Hellwig wrote:
> -static void __init get_fs_names(char *page)
> +static void __init split_fs_names(char *page, char *names)

If you're going to respin it anyway, can you rename 'page' to 'buf'
or something?  Kind of confusing to have a char * called 'page'.

>  {
> +	strcpy(page, root_fs_names);
> +	while (*page++) {
> +		if (page[-1] == ',')
> +			page[-1] = '\0';
> +	}
> +	*page = '\0';
> +}

is it really worth doing a strcpy() followed by a custom strtok()?
would this work better?

	char c;

	do {
		c =  *root_fs_names++;
		*buf++ = c;
		if (c == ',')
			buf[-1] = '\0';
	} while (c);

> +static void __init get_all_fs_names(char *page)
> +{
> +	int len = get_filesystem_list(page);

it occurs to me that get_filesystem_list() fails silently.  if you build
every linux filesystem in, and want your root on zonefs (assuming
they're alphabetical), we'll fail to find it without a message
indicating that we overflowed the buffer.

Re: [Virtio-fs] [PATCH 1/2] init: split get_fs_names

[Matthew Wilcox]

On Mon, Jun 21, 2021 at 08:26:56AM +0200, Christoph Hellwig wrote:
> -static void __init get_fs_names(char *page)
> +static void __init split_fs_names(char *page, char *names)

If you're going to respin it anyway, can you rename 'page' to 'buf'
or something?  Kind of confusing to have a char * called 'page'.

>  {
> +	strcpy(page, root_fs_names);
> +	while (*page++) {
> +		if (page[-1] == ',')
> +			page[-1] = '\0';
> +	}
> +	*page = '\0';
> +}

is it really worth doing a strcpy() followed by a custom strtok()?
would this work better?

	char c;

	do {
		c =  *root_fs_names++;
		*buf++ = c;
		if (c == ',')
			buf[-1] = '\0';
	} while (c);

> +static void __init get_all_fs_names(char *page)
> +{
> +	int len = get_filesystem_list(page);

it occurs to me that get_filesystem_list() fails silently.  if you build
every linux filesystem in, and want your root on zonefs (assuming
they're alphabetical), we'll fail to find it without a message
indicating that we overflowed the buffer.

Re: [PATCH 1/2] init: split get_fs_names

[Christoph Hellwig]

On Mon, Jun 21, 2021 at 04:09:16PM +0100, Matthew Wilcox wrote:
> On Mon, Jun 21, 2021 at 08:26:56AM +0200, Christoph Hellwig wrote:
> > -static void __init get_fs_names(char *page)
> > +static void __init split_fs_names(char *page, char *names)
> 
> If you're going to respin it anyway, can you rename 'page' to 'buf'
> or something?  Kind of confusing to have a char * called 'page'.

I was hoping that we did not need a respin.  While Al's suggestion is
nice, and I've added a variant of it to my local tree it really
is just incremental improvements.

I actually thing that page name is not too bad, as it implements the
implicit assumptions that it is a PAGE_SIZE allocation (which is a bad
idea to start with, but we're digging a deeper and deeper hole here..)

> is it really worth doing a strcpy() followed by a custom strtok()?
> would this work better?
> 
> 	char c;
> 
> 	do {
> 		c =  *root_fs_names++;
> 		*buf++ = c;
> 		if (c == ',')
> 			buf[-1] = '\0';
> 	} while (c);

Maybe.  Then again all this is age old rarely used code, so it might be
better to not stirr it too much..

> > +static void __init get_all_fs_names(char *page)
> > +{
> > +	int len = get_filesystem_list(page);
> 
> it occurs to me that get_filesystem_list() fails silently.  if you build
> every linux filesystem in, and want your root on zonefs (assuming
> they're alphabetical), we'll fail to find it without a message
> indicating that we overflowed the buffer.

Yes.  The only sensible way to fix this would be some kind of cursor.
We could use an xarray for the file_systems list, but unless we want
to assume file systems can't go away at init type (which might be an
ok assumption) we'd then need to get into refcount, and and and..

Re: [Virtio-fs] [PATCH 1/2] init: split get_fs_names

[Christoph Hellwig]

On Mon, Jun 21, 2021 at 04:09:16PM +0100, Matthew Wilcox wrote:
> On Mon, Jun 21, 2021 at 08:26:56AM +0200, Christoph Hellwig wrote:
> > -static void __init get_fs_names(char *page)
> > +static void __init split_fs_names(char *page, char *names)
> 
> If you're going to respin it anyway, can you rename 'page' to 'buf'
> or something?  Kind of confusing to have a char * called 'page'.

I was hoping that we did not need a respin.  While Al's suggestion is
nice, and I've added a variant of it to my local tree it really
is just incremental improvements.

I actually thing that page name is not too bad, as it implements the
implicit assumptions that it is a PAGE_SIZE allocation (which is a bad
idea to start with, but we're digging a deeper and deeper hole here..)

> is it really worth doing a strcpy() followed by a custom strtok()?
> would this work better?
> 
> 	char c;
> 
> 	do {
> 		c =  *root_fs_names++;
> 		*buf++ = c;
> 		if (c == ',')
> 			buf[-1] = '\0';
> 	} while (c);

Maybe.  Then again all this is age old rarely used code, so it might be
better to not stirr it too much..

> > +static void __init get_all_fs_names(char *page)
> > +{
> > +	int len = get_filesystem_list(page);
> 
> it occurs to me that get_filesystem_list() fails silently.  if you build
> every linux filesystem in, and want your root on zonefs (assuming
> they're alphabetical), we'll fail to find it without a message
> indicating that we overflowed the buffer.

Yes.  The only sensible way to fix this would be some kind of cursor.
We could use an xarray for the file_systems list, but unless we want
to assume file systems can't go away at init type (which might be an
ok assumption) we'd then need to get into refcount, and and and..

[PATCH 2/2] init: allow mounting arbitrary non-blockdevice filesystems as root

[Christoph Hellwig]

Currently the only non-blockdevice filesystems that can be used as the
initial root filesystem are NFS and CIFS, which use the magic
"root=/dev/nfs" and "root=/dev/cifs" syntax that requires the root
device file system details to come from filesystem specific kernel
command line options.

Add a little bit of new code that allows to just pass arbitrary
string mount options to any non-blockdevice filesystems so that it can
be mounted as the root file system.

For example a virtiofs root file system can be mounted using the
following syntax:

"root=myfs rootfstype=virtiofs rw"

Based on an earlier patch from Vivek Goyal <vgoyal@redhat.com>.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 init/do_mounts.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/init/do_mounts.c b/init/do_mounts.c
index ec32de3ad52b..bdeb90b8d669 100644
--- a/init/do_mounts.c
+++ b/init/do_mounts.c
@@ -534,6 +534,45 @@ static int __init mount_cifs_root(void)
 }
 #endif
 
+static bool __init fs_is_nodev(char *fstype)
+{
+	struct file_system_type *fs = get_fs_type(fstype);
+	bool ret = false;
+
+	if (fs) {
+		ret = !(fs->fs_flags & FS_REQUIRES_DEV);
+		put_filesystem(fs);
+	}
+
+	return ret;
+}
+
+static int __init mount_nodev_root(void)
+{
+	char *fs_names, *fstype;
+	int err = -EINVAL;
+
+	fs_names = (void *)__get_free_page(GFP_KERNEL);
+	if (!fs_names)
+		return -EINVAL;
+	split_fs_names(fs_names, root_fs_names);
+
+	for (fstype = fs_names; *fstype; fstype += strlen(fstype) + 1) {
+		if (!fs_is_nodev(fstype))
+			continue;
+		err = do_mount_root(root_device_name, fstype, root_mountflags,
+				    root_mount_data);
+		if (!err)
+			break;
+		if (err != -EACCES && err != -EINVAL)
+			panic("VFS: Unable to mount root \"%s\" (%s), err=%d\n",
+			      root_device_name, fstype, err);
+	}
+
+	free_page((unsigned long)fs_names);
+	return err;
+}
+
 void __init mount_root(void)
 {
 #ifdef CONFIG_ROOT_NFS
@@ -550,6 +589,10 @@ void __init mount_root(void)
 		return;
 	}
 #endif
+	if (ROOT_DEV == 0 && root_device_name && root_fs_names) {
+		if (mount_nodev_root() == 0)
+			return;
+	}
 #ifdef CONFIG_BLOCK
 	{
 		int err = create_dev("/dev/root", ROOT_DEV);
-- 
2.30.2

[Virtio-fs] [PATCH 2/2] init: allow mounting arbitrary non-blockdevice filesystems as root

[Christoph Hellwig]

Currently the only non-blockdevice filesystems that can be used as the
initial root filesystem are NFS and CIFS, which use the magic
"root=/dev/nfs" and "root=/dev/cifs" syntax that requires the root
device file system details to come from filesystem specific kernel
command line options.

Add a little bit of new code that allows to just pass arbitrary
string mount options to any non-blockdevice filesystems so that it can
be mounted as the root file system.

For example a virtiofs root file system can be mounted using the
following syntax:

"root=myfs rootfstype=virtiofs rw"

Based on an earlier patch from Vivek Goyal <vgoyal@redhat.com>.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 init/do_mounts.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/init/do_mounts.c b/init/do_mounts.c
index ec32de3ad52b..bdeb90b8d669 100644
--- a/init/do_mounts.c
+++ b/init/do_mounts.c
@@ -534,6 +534,45 @@ static int __init mount_cifs_root(void)
 }
 #endif
 
+static bool __init fs_is_nodev(char *fstype)
+{
+	struct file_system_type *fs = get_fs_type(fstype);
+	bool ret = false;
+
+	if (fs) {
+		ret = !(fs->fs_flags & FS_REQUIRES_DEV);
+		put_filesystem(fs);
+	}
+
+	return ret;
+}
+
+static int __init mount_nodev_root(void)
+{
+	char *fs_names, *fstype;
+	int err = -EINVAL;
+
+	fs_names = (void *)__get_free_page(GFP_KERNEL);
+	if (!fs_names)
+		return -EINVAL;
+	split_fs_names(fs_names, root_fs_names);
+
+	for (fstype = fs_names; *fstype; fstype += strlen(fstype) + 1) {
+		if (!fs_is_nodev(fstype))
+			continue;
+		err = do_mount_root(root_device_name, fstype, root_mountflags,
+				    root_mount_data);
+		if (!err)
+			break;
+		if (err != -EACCES && err != -EINVAL)
+			panic("VFS: Unable to mount root \"%s\" (%s), err=%d\n",
+			      root_device_name, fstype, err);
+	}
+
+	free_page((unsigned long)fs_names);
+	return err;
+}
+
 void __init mount_root(void)
 {
 #ifdef CONFIG_ROOT_NFS
@@ -550,6 +589,10 @@ void __init mount_root(void)
 		return;
 	}
 #endif
+	if (ROOT_DEV == 0 && root_device_name && root_fs_names) {
+		if (mount_nodev_root() == 0)
+			return;
+	}
 #ifdef CONFIG_BLOCK
 	{
 		int err = create_dev("/dev/root", ROOT_DEV);
-- 
2.30.2

Re: [Virtio-fs] support booting of arbitrary non-blockdevice file systems v2

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 649 bytes --]

On Mon, Jun 21, 2021 at 08:26:55AM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> this series adds support to boot off arbitrary non-blockdevice root file
> systems, based off an earlier patch from Vivek.
> 
> Chances since v1:
>  - don't try to mount every registered file system if none is specified
>  - fix various null pointer dereferences when certain kernel paramters are
>    not set
>  - general refactoring.
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://listman.redhat.com/mailman/listinfo/virtio-fs
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Virtio-fs] support booting of arbitrary non-blockdevice file systems v2

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 649 bytes --]

On Mon, Jun 21, 2021 at 08:26:55AM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> this series adds support to boot off arbitrary non-blockdevice root file
> systems, based off an earlier patch from Vivek.
> 
> Chances since v1:
>  - don't try to mount every registered file system if none is specified
>  - fix various null pointer dereferences when certain kernel paramters are
>    not set
>  - general refactoring.
> 
> _______________________________________________
> Virtio-fs mailing list
> Virtio-fs@redhat.com
> https://listman.redhat.com/mailman/listinfo/virtio-fs
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: support booting of arbitrary non-blockdevice file systems v2

[Vivek Goyal]

On Mon, Jun 21, 2021 at 08:26:55AM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> this series adds support to boot off arbitrary non-blockdevice root file
> systems, based off an earlier patch from Vivek.
> 
> Chances since v1:
>  - don't try to mount every registered file system if none is specified
>  - fix various null pointer dereferences when certain kernel paramters are
>    not set
>  - general refactoring.

Thanks Christoph. This version looks good to me. I tested both with
virtiofs and 9p and it works for me.

Reviewed-by: Vivek Goyal <vgoyal@redhat.com>

Thanks
Vivek

Re: [Virtio-fs] support booting of arbitrary non-blockdevice file systems v2

[Vivek Goyal]

On Mon, Jun 21, 2021 at 08:26:55AM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> this series adds support to boot off arbitrary non-blockdevice root file
> systems, based off an earlier patch from Vivek.
> 
> Chances since v1:
>  - don't try to mount every registered file system if none is specified
>  - fix various null pointer dereferences when certain kernel paramters are
>    not set
>  - general refactoring.

Thanks Christoph. This version looks good to me. I tested both with
virtiofs and 9p and it works for me.

Reviewed-by: Vivek Goyal <vgoyal@redhat.com>

Thanks
Vivek

[PATCH 3/2] fs: simplify get_filesystem_list / get_all_fs_names

[Christoph Hellwig]

Just output the '\0' separate list of supported file systems for block
devices directly rather than going through a pointless round of string
manipulation.

Based on an earlier patch from Al Viro <viro@zeniv.linux.org.uk>.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/filesystems.c   | 24 ++++++++++++++----------
 include/linux/fs.h |  2 +-
 init/do_mounts.c   | 20 +-------------------
 3 files changed, 16 insertions(+), 30 deletions(-)

diff --git a/fs/filesystems.c b/fs/filesystems.c
index 90b8d879fbaf..7c136251607a 100644
--- a/fs/filesystems.c
+++ b/fs/filesystems.c
@@ -209,21 +209,25 @@ SYSCALL_DEFINE3(sysfs, int, option, unsigned long, arg1, unsigned long, arg2)
 }
 #endif
 
-int __init get_filesystem_list(char *buf)
+void __init list_bdev_fs_names(char *buf, size_t size)
 {
-	int len = 0;
-	struct file_system_type * tmp;
+	struct file_system_type *p;
+	size_t len;
 
 	read_lock(&file_systems_lock);
-	tmp = file_systems;
-	while (tmp && len < PAGE_SIZE - 80) {
-		len += sprintf(buf+len, "%s\t%s\n",
-			(tmp->fs_flags & FS_REQUIRES_DEV) ? "" : "nodev",
-			tmp->name);
-		tmp = tmp->next;
+	for (p = file_systems; p; p = p->next) {
+		if (!(p->fs_flags & FS_REQUIRES_DEV))
+			continue;
+		len = strlen(p->name) + 1;
+		if (len > size) {
+			pr_warn("%s: truncating file system list\n", __func__);
+			break;
+		}
+		memcpy(buf, p->name, len);
+		buf += len;
+		size -= len;
 	}
 	read_unlock(&file_systems_lock);
-	return len;
 }
 
 #ifdef CONFIG_PROC_FS
diff --git a/include/linux/fs.h b/include/linux/fs.h
index f4ed7bf1130d..cdcd7f2a2c3f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3624,7 +3624,7 @@ int proc_nr_dentry(struct ctl_table *table, int write,
 		  void *buffer, size_t *lenp, loff_t *ppos);
 int proc_nr_inodes(struct ctl_table *table, int write,
 		   void *buffer, size_t *lenp, loff_t *ppos);
-int __init get_filesystem_list(char *buf);
+void __init list_bdev_fs_names(char *buf, size_t size);
 
 #define __FMODE_EXEC		((__force int) FMODE_EXEC)
 #define __FMODE_NONOTIFY	((__force int) FMODE_NONOTIFY)
diff --git a/init/do_mounts.c b/init/do_mounts.c
index bdeb90b8d669..ffbe4deeb274 100644
--- a/init/do_mounts.c
+++ b/init/do_mounts.c
@@ -348,24 +348,6 @@ static void __init split_fs_names(char *page, char *names)
 	*page = '\0';
 }
 
-static void __init get_all_fs_names(char *page)
-{
-	int len = get_filesystem_list(page);
-	char *s = page, *p, *next;
-
-	page[len] = '\0';
-	for (p = page - 1; p; p = next) {
-		next = strchr(++p, '\n');
-		if (*p++ != '\t')
-			continue;
-		while ((*s++ = *p++) != '\n')
-			;
-		s[-1] = '\0';
-	}
-
-	*s = '\0';
-}
-
 static int __init do_mount_root(const char *name, const char *fs,
 				 const int flags, const void *data)
 {
@@ -415,7 +397,7 @@ void __init mount_block_root(char *name, int flags)
 	if (root_fs_names)
 		split_fs_names(fs_names, root_fs_names);
 	else
-		get_all_fs_names(fs_names);
+		list_bdev_fs_names(fs_names, PAGE_SIZE);
 retry:
 	for (p = fs_names; *p; p += strlen(p)+1) {
 		int err = do_mount_root(name, p, flags, root_mount_data);
-- 
2.30.2

[Virtio-fs] [PATCH 3/2] fs: simplify get_filesystem_list / get_all_fs_names

[Christoph Hellwig]

Just output the '\0' separate list of supported file systems for block
devices directly rather than going through a pointless round of string
manipulation.

Based on an earlier patch from Al Viro <viro@zeniv.linux.org.uk>.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/filesystems.c   | 24 ++++++++++++++----------
 include/linux/fs.h |  2 +-
 init/do_mounts.c   | 20 +-------------------
 3 files changed, 16 insertions(+), 30 deletions(-)

diff --git a/fs/filesystems.c b/fs/filesystems.c
index 90b8d879fbaf..7c136251607a 100644
--- a/fs/filesystems.c
+++ b/fs/filesystems.c
@@ -209,21 +209,25 @@ SYSCALL_DEFINE3(sysfs, int, option, unsigned long, arg1, unsigned long, arg2)
 }
 #endif
 
-int __init get_filesystem_list(char *buf)
+void __init list_bdev_fs_names(char *buf, size_t size)
 {
-	int len = 0;
-	struct file_system_type * tmp;
+	struct file_system_type *p;
+	size_t len;
 
 	read_lock(&file_systems_lock);
-	tmp = file_systems;
-	while (tmp && len < PAGE_SIZE - 80) {
-		len += sprintf(buf+len, "%s\t%s\n",
-			(tmp->fs_flags & FS_REQUIRES_DEV) ? "" : "nodev",
-			tmp->name);
-		tmp = tmp->next;
+	for (p = file_systems; p; p = p->next) {
+		if (!(p->fs_flags & FS_REQUIRES_DEV))
+			continue;
+		len = strlen(p->name) + 1;
+		if (len > size) {
+			pr_warn("%s: truncating file system list\n", __func__);
+			break;
+		}
+		memcpy(buf, p->name, len);
+		buf += len;
+		size -= len;
 	}
 	read_unlock(&file_systems_lock);
-	return len;
 }
 
 #ifdef CONFIG_PROC_FS
diff --git a/include/linux/fs.h b/include/linux/fs.h
index f4ed7bf1130d..cdcd7f2a2c3f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3624,7 +3624,7 @@ int proc_nr_dentry(struct ctl_table *table, int write,
 		  void *buffer, size_t *lenp, loff_t *ppos);
 int proc_nr_inodes(struct ctl_table *table, int write,
 		   void *buffer, size_t *lenp, loff_t *ppos);
-int __init get_filesystem_list(char *buf);
+void __init list_bdev_fs_names(char *buf, size_t size);
 
 #define __FMODE_EXEC		((__force int) FMODE_EXEC)
 #define __FMODE_NONOTIFY	((__force int) FMODE_NONOTIFY)
diff --git a/init/do_mounts.c b/init/do_mounts.c
index bdeb90b8d669..ffbe4deeb274 100644
--- a/init/do_mounts.c
+++ b/init/do_mounts.c
@@ -348,24 +348,6 @@ static void __init split_fs_names(char *page, char *names)
 	*page = '\0';
 }
 
-static void __init get_all_fs_names(char *page)
-{
-	int len = get_filesystem_list(page);
-	char *s = page, *p, *next;
-
-	page[len] = '\0';
-	for (p = page - 1; p; p = next) {
-		next = strchr(++p, '\n');
-		if (*p++ != '\t')
-			continue;
-		while ((*s++ = *p++) != '\n')
-			;
-		s[-1] = '\0';
-	}
-
-	*s = '\0';
-}
-
 static int __init do_mount_root(const char *name, const char *fs,
 				 const int flags, const void *data)
 {
@@ -415,7 +397,7 @@ void __init mount_block_root(char *name, int flags)
 	if (root_fs_names)
 		split_fs_names(fs_names, root_fs_names);
 	else
-		get_all_fs_names(fs_names);
+		list_bdev_fs_names(fs_names, PAGE_SIZE);
 retry:
 	for (p = fs_names; *p; p += strlen(p)+1) {
 		int err = do_mount_root(name, p, flags, root_mount_data);
-- 
2.30.2

Re: [Virtio-fs] [PATCH 3/2] fs: simplify get_filesystem_list / get_all_fs_names

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1791 bytes --]

On Tue, Jun 22, 2021 at 10:12:17AM +0200, Christoph Hellwig wrote:
> Just output the '\0' separate list of supported file systems for block
> devices directly rather than going through a pointless round of string
> manipulation.
> 
> Based on an earlier patch from Al Viro <viro@zeniv.linux.org.uk>.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/filesystems.c   | 24 ++++++++++++++----------
>  include/linux/fs.h |  2 +-
>  init/do_mounts.c   | 20 +-------------------
>  3 files changed, 16 insertions(+), 30 deletions(-)
> 
> diff --git a/fs/filesystems.c b/fs/filesystems.c
> index 90b8d879fbaf..7c136251607a 100644
> --- a/fs/filesystems.c
> +++ b/fs/filesystems.c
> @@ -209,21 +209,25 @@ SYSCALL_DEFINE3(sysfs, int, option, unsigned long, arg1, unsigned long, arg2)
>  }
>  #endif
>  
> -int __init get_filesystem_list(char *buf)
> +void __init list_bdev_fs_names(char *buf, size_t size)
>  {
> -	int len = 0;
> -	struct file_system_type * tmp;
> +	struct file_system_type *p;
> +	size_t len;
>  
>  	read_lock(&file_systems_lock);
> -	tmp = file_systems;
> -	while (tmp && len < PAGE_SIZE - 80) {
> -		len += sprintf(buf+len, "%s\t%s\n",
> -			(tmp->fs_flags & FS_REQUIRES_DEV) ? "" : "nodev",
> -			tmp->name);
> -		tmp = tmp->next;
> +	for (p = file_systems; p; p = p->next) {
> +		if (!(p->fs_flags & FS_REQUIRES_DEV))
> +			continue;
> +		len = strlen(p->name) + 1;
> +		if (len > size) {
> +			pr_warn("%s: truncating file system list\n", __func__);
> +			break;
> +		}
> +		memcpy(buf, p->name, len);
> +		buf += len;
> +		size -= len;
>  	}
>  	read_unlock(&file_systems_lock);
> -	return len;
>  }

I don't see the extra NUL terminator byte being added that's required by
the loop in mount_block_root()?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Virtio-fs] [PATCH 3/2] fs: simplify get_filesystem_list / get_all_fs_names

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1791 bytes --]

On Tue, Jun 22, 2021 at 10:12:17AM +0200, Christoph Hellwig wrote:
> Just output the '\0' separate list of supported file systems for block
> devices directly rather than going through a pointless round of string
> manipulation.
> 
> Based on an earlier patch from Al Viro <viro@zeniv.linux.org.uk>.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/filesystems.c   | 24 ++++++++++++++----------
>  include/linux/fs.h |  2 +-
>  init/do_mounts.c   | 20 +-------------------
>  3 files changed, 16 insertions(+), 30 deletions(-)
> 
> diff --git a/fs/filesystems.c b/fs/filesystems.c
> index 90b8d879fbaf..7c136251607a 100644
> --- a/fs/filesystems.c
> +++ b/fs/filesystems.c
> @@ -209,21 +209,25 @@ SYSCALL_DEFINE3(sysfs, int, option, unsigned long, arg1, unsigned long, arg2)
>  }
>  #endif
>  
> -int __init get_filesystem_list(char *buf)
> +void __init list_bdev_fs_names(char *buf, size_t size)
>  {
> -	int len = 0;
> -	struct file_system_type * tmp;
> +	struct file_system_type *p;
> +	size_t len;
>  
>  	read_lock(&file_systems_lock);
> -	tmp = file_systems;
> -	while (tmp && len < PAGE_SIZE - 80) {
> -		len += sprintf(buf+len, "%s\t%s\n",
> -			(tmp->fs_flags & FS_REQUIRES_DEV) ? "" : "nodev",
> -			tmp->name);
> -		tmp = tmp->next;
> +	for (p = file_systems; p; p = p->next) {
> +		if (!(p->fs_flags & FS_REQUIRES_DEV))
> +			continue;
> +		len = strlen(p->name) + 1;
> +		if (len > size) {
> +			pr_warn("%s: truncating file system list\n", __func__);
> +			break;
> +		}
> +		memcpy(buf, p->name, len);
> +		buf += len;
> +		size -= len;
>  	}
>  	read_unlock(&file_systems_lock);
> -	return len;
>  }

I don't see the extra NUL terminator byte being added that's required by
the loop in mount_block_root()?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Virtio-fs] [PATCH 3/2] fs: simplify get_filesystem_list / get_all_fs_names

[Vivek Goyal]

On Tue, Jun 22, 2021 at 09:36:33AM +0100, Stefan Hajnoczi wrote:
> On Tue, Jun 22, 2021 at 10:12:17AM +0200, Christoph Hellwig wrote:
> > Just output the '\0' separate list of supported file systems for block
> > devices directly rather than going through a pointless round of string
> > manipulation.
> > 
> > Based on an earlier patch from Al Viro <viro@zeniv.linux.org.uk>.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  fs/filesystems.c   | 24 ++++++++++++++----------
> >  include/linux/fs.h |  2 +-
> >  init/do_mounts.c   | 20 +-------------------
> >  3 files changed, 16 insertions(+), 30 deletions(-)
> > 
> > diff --git a/fs/filesystems.c b/fs/filesystems.c
> > index 90b8d879fbaf..7c136251607a 100644
> > --- a/fs/filesystems.c
> > +++ b/fs/filesystems.c
> > @@ -209,21 +209,25 @@ SYSCALL_DEFINE3(sysfs, int, option, unsigned long, arg1, unsigned long, arg2)
> >  }
> >  #endif
> >  
> > -int __init get_filesystem_list(char *buf)
> > +void __init list_bdev_fs_names(char *buf, size_t size)
> >  {
> > -	int len = 0;
> > -	struct file_system_type * tmp;
> > +	struct file_system_type *p;
> > +	size_t len;
> >  
> >  	read_lock(&file_systems_lock);
> > -	tmp = file_systems;
> > -	while (tmp && len < PAGE_SIZE - 80) {
> > -		len += sprintf(buf+len, "%s\t%s\n",
> > -			(tmp->fs_flags & FS_REQUIRES_DEV) ? "" : "nodev",
> > -			tmp->name);
> > -		tmp = tmp->next;
> > +	for (p = file_systems; p; p = p->next) {
> > +		if (!(p->fs_flags & FS_REQUIRES_DEV))
> > +			continue;
> > +		len = strlen(p->name) + 1;
> > +		if (len > size) {
> > +			pr_warn("%s: truncating file system list\n", __func__);
> > +			break;
> > +		}
> > +		memcpy(buf, p->name, len);
> > +		buf += len;
> > +		size -= len;
> >  	}
> >  	read_unlock(&file_systems_lock);
> > -	return len;
> >  }
> 
> I don't see the extra NUL terminator byte being added that's required by
> the loop in mount_block_root()?

May be we should modify mount_block_root() code so that it does not
require that extra "\0". Possibly zero initialize page and that should
make sure list_bdev_fs_names() does not have to worry about it.

It is possible that a page gets full from the list of filesystems, and
last byte on page is terminating null. In that case just zeroing page
will not help. We can keep track of some sort of end pointer and make
sure we are not searching beyond that for valid filesystem types.

end = page + PAGE_SIZE - 1;

mount_block_root()
{
	for (p = fs_names; p < end && *p; p += strlen(p)+1) {
	}
}

Thanks
Vivek

Re: [Virtio-fs] [PATCH 3/2] fs: simplify get_filesystem_list / get_all_fs_names

[Vivek Goyal]

On Tue, Jun 22, 2021 at 09:36:33AM +0100, Stefan Hajnoczi wrote:
> On Tue, Jun 22, 2021 at 10:12:17AM +0200, Christoph Hellwig wrote:
> > Just output the '\0' separate list of supported file systems for block
> > devices directly rather than going through a pointless round of string
> > manipulation.
> > 
> > Based on an earlier patch from Al Viro <viro@zeniv.linux.org.uk>.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  fs/filesystems.c   | 24 ++++++++++++++----------
> >  include/linux/fs.h |  2 +-
> >  init/do_mounts.c   | 20 +-------------------
> >  3 files changed, 16 insertions(+), 30 deletions(-)
> > 
> > diff --git a/fs/filesystems.c b/fs/filesystems.c
> > index 90b8d879fbaf..7c136251607a 100644
> > --- a/fs/filesystems.c
> > +++ b/fs/filesystems.c
> > @@ -209,21 +209,25 @@ SYSCALL_DEFINE3(sysfs, int, option, unsigned long, arg1, unsigned long, arg2)
> >  }
> >  #endif
> >  
> > -int __init get_filesystem_list(char *buf)
> > +void __init list_bdev_fs_names(char *buf, size_t size)
> >  {
> > -	int len = 0;
> > -	struct file_system_type * tmp;
> > +	struct file_system_type *p;
> > +	size_t len;
> >  
> >  	read_lock(&file_systems_lock);
> > -	tmp = file_systems;
> > -	while (tmp && len < PAGE_SIZE - 80) {
> > -		len += sprintf(buf+len, "%s\t%s\n",
> > -			(tmp->fs_flags & FS_REQUIRES_DEV) ? "" : "nodev",
> > -			tmp->name);
> > -		tmp = tmp->next;
> > +	for (p = file_systems; p; p = p->next) {
> > +		if (!(p->fs_flags & FS_REQUIRES_DEV))
> > +			continue;
> > +		len = strlen(p->name) + 1;
> > +		if (len > size) {
> > +			pr_warn("%s: truncating file system list\n", __func__);
> > +			break;
> > +		}
> > +		memcpy(buf, p->name, len);
> > +		buf += len;
> > +		size -= len;
> >  	}
> >  	read_unlock(&file_systems_lock);
> > -	return len;
> >  }
> 
> I don't see the extra NUL terminator byte being added that's required by
> the loop in mount_block_root()?

May be we should modify mount_block_root() code so that it does not
require that extra "\0". Possibly zero initialize page and that should
make sure list_bdev_fs_names() does not have to worry about it.

It is possible that a page gets full from the list of filesystems, and
last byte on page is terminating null. In that case just zeroing page
will not help. We can keep track of some sort of end pointer and make
sure we are not searching beyond that for valid filesystem types.

end = page + PAGE_SIZE - 1;

mount_block_root()
{
	for (p = fs_names; p < end && *p; p += strlen(p)+1) {
	}
}

Thanks
Vivek

Re: [Virtio-fs] [PATCH 3/2] fs: simplify get_filesystem_list / get_all_fs_names

[Christoph Hellwig]

On Tue, Jun 29, 2021 at 04:50:48PM -0400, Vivek Goyal wrote:
> May be we should modify mount_block_root() code so that it does not
> require that extra "\0". Possibly zero initialize page and that should
> make sure list_bdev_fs_names() does not have to worry about it.
> 
> It is possible that a page gets full from the list of filesystems, and
> last byte on page is terminating null. In that case just zeroing page
> will not help. We can keep track of some sort of end pointer and make
> sure we are not searching beyond that for valid filesystem types.
> 
> end = page + PAGE_SIZE - 1;
> 
> mount_block_root()
> {
> 	for (p = fs_names; p < end && *p; p += strlen(p)+1) {
> 	}
> }

Maybe.  To honest I'd prefer to not even touch this unrelated code given
how full of landmines it is :)

Re: [Virtio-fs] [PATCH 3/2] fs: simplify get_filesystem_list / get_all_fs_names

[Christoph Hellwig]

On Tue, Jun 29, 2021 at 04:50:48PM -0400, Vivek Goyal wrote:
> May be we should modify mount_block_root() code so that it does not
> require that extra "\0". Possibly zero initialize page and that should
> make sure list_bdev_fs_names() does not have to worry about it.
> 
> It is possible that a page gets full from the list of filesystems, and
> last byte on page is terminating null. In that case just zeroing page
> will not help. We can keep track of some sort of end pointer and make
> sure we are not searching beyond that for valid filesystem types.
> 
> end = page + PAGE_SIZE - 1;
> 
> mount_block_root()
> {
> 	for (p = fs_names; p < end && *p; p += strlen(p)+1) {
> 	}
> }

Maybe.  To honest I'd prefer to not even touch this unrelated code given
how full of landmines it is :)

Re: [Virtio-fs] [PATCH 3/2] fs: simplify get_filesystem_list / get_all_fs_names

[Vivek Goyal]

On Wed, Jun 30, 2021 at 07:36:01AM +0200, Christoph Hellwig wrote:
> On Tue, Jun 29, 2021 at 04:50:48PM -0400, Vivek Goyal wrote:
> > May be we should modify mount_block_root() code so that it does not
> > require that extra "\0". Possibly zero initialize page and that should
> > make sure list_bdev_fs_names() does not have to worry about it.
> > 
> > It is possible that a page gets full from the list of filesystems, and
> > last byte on page is terminating null. In that case just zeroing page
> > will not help. We can keep track of some sort of end pointer and make
> > sure we are not searching beyond that for valid filesystem types.
> > 
> > end = page + PAGE_SIZE - 1;
> > 
> > mount_block_root()
> > {
> > 	for (p = fs_names; p < end && *p; p += strlen(p)+1) {
> > 	}
> > }
> 
> Maybe.  To honest I'd prefer to not even touch this unrelated code given
> how full of landmines it is :)

Agreed. It probably is better to make such changes incrementally. 

Given third patch is nice to have cleanup kind of thing, can we first
just merge first two patches to support non-block device filesystems as
rootfs.

We will really like to have a method to properly boot virtiofs as rootfs.

Thanks
Vivek

Re: [Virtio-fs] [PATCH 3/2] fs: simplify get_filesystem_list / get_all_fs_names

[Vivek Goyal]

On Wed, Jun 30, 2021 at 07:36:01AM +0200, Christoph Hellwig wrote:
> On Tue, Jun 29, 2021 at 04:50:48PM -0400, Vivek Goyal wrote:
> > May be we should modify mount_block_root() code so that it does not
> > require that extra "\0". Possibly zero initialize page and that should
> > make sure list_bdev_fs_names() does not have to worry about it.
> > 
> > It is possible that a page gets full from the list of filesystems, and
> > last byte on page is terminating null. In that case just zeroing page
> > will not help. We can keep track of some sort of end pointer and make
> > sure we are not searching beyond that for valid filesystem types.
> > 
> > end = page + PAGE_SIZE - 1;
> > 
> > mount_block_root()
> > {
> > 	for (p = fs_names; p < end && *p; p += strlen(p)+1) {
> > 	}
> > }
> 
> Maybe.  To honest I'd prefer to not even touch this unrelated code given
> how full of landmines it is :)

Agreed. It probably is better to make such changes incrementally. 

Given third patch is nice to have cleanup kind of thing, can we first
just merge first two patches to support non-block device filesystems as
rootfs.

We will really like to have a method to properly boot virtiofs as rootfs.

Thanks
Vivek

Re: [Virtio-fs] [PATCH 3/2] fs: simplify get_filesystem_list / get_all_fs_names

[Vivek Goyal]

On Wed, Jun 30, 2021 at 07:36:01AM +0200, Christoph Hellwig wrote:
> On Tue, Jun 29, 2021 at 04:50:48PM -0400, Vivek Goyal wrote:
> > May be we should modify mount_block_root() code so that it does not
> > require that extra "\0". Possibly zero initialize page and that should
> > make sure list_bdev_fs_names() does not have to worry about it.
> > 
> > It is possible that a page gets full from the list of filesystems, and
> > last byte on page is terminating null. In that case just zeroing page
> > will not help. We can keep track of some sort of end pointer and make
> > sure we are not searching beyond that for valid filesystem types.
> > 
> > end = page + PAGE_SIZE - 1;
> > 
> > mount_block_root()
> > {
> > 	for (p = fs_names; p < end && *p; p += strlen(p)+1) {
> > 	}
> > }
> 
> Maybe.  To honest I'd prefer to not even touch this unrelated code given
> how full of landmines it is :)

Hi Christoph,

How about following patch. This applies on top of your patches. I noticed
that Al had suggested to return number of filesystems from helper
functions. I just did that and used that to iterate in the loop.

I tested it with a virtual block device (root=/dev/vda1) and it works.
I also filled page with garbage after allocation to make sure natually
occurring null is not there in the middle of page to terminate string.

If you like it, can you please incorporate it in your patches.

Thanks
Vivek

---
 fs/filesystems.c   |    5 ++++-
 include/linux/fs.h |    2 +-
 init/do_mounts.c   |    7 ++++---
 3 files changed, 9 insertions(+), 5 deletions(-)

Index: redhat-linux/fs/filesystems.c
===================================================================
--- redhat-linux.orig/fs/filesystems.c	2021-07-07 16:12:08.890562576 -0400
+++ redhat-linux/fs/filesystems.c	2021-07-07 16:27:51.197620063 -0400
@@ -209,10 +209,11 @@ SYSCALL_DEFINE3(sysfs, int, option, unsi
 }
 #endif
 
-void __init list_bdev_fs_names(char *buf, size_t size)
+int __init list_bdev_fs_names(char *buf, size_t size)
 {
 	struct file_system_type *p;
 	size_t len;
+	int count = 0;
 
 	read_lock(&file_systems_lock);
 	for (p = file_systems; p; p = p->next) {
@@ -226,8 +227,10 @@ void __init list_bdev_fs_names(char *buf
 		memcpy(buf, p->name, len);
 		buf += len;
 		size -= len;
+		count++;
 	}
 	read_unlock(&file_systems_lock);
+	return count;
 }
 
 #ifdef CONFIG_PROC_FS
Index: redhat-linux/include/linux/fs.h
===================================================================
--- redhat-linux.orig/include/linux/fs.h	2021-07-07 15:36:43.224418935 -0400
+++ redhat-linux/include/linux/fs.h	2021-07-07 16:12:18.232949807 -0400
@@ -3622,7 +3622,7 @@ int proc_nr_dentry(struct ctl_table *tab
 		  void *buffer, size_t *lenp, loff_t *ppos);
 int proc_nr_inodes(struct ctl_table *table, int write,
 		   void *buffer, size_t *lenp, loff_t *ppos);
-void __init list_bdev_fs_names(char *buf, size_t size);
+int __init list_bdev_fs_names(char *buf, size_t size);
 
 #define __FMODE_EXEC		((__force int) FMODE_EXEC)
 #define __FMODE_NONOTIFY	((__force int) FMODE_NONOTIFY)
Index: redhat-linux/init/do_mounts.c
===================================================================
--- redhat-linux.orig/init/do_mounts.c	2021-07-07 16:12:08.890562576 -0400
+++ redhat-linux/init/do_mounts.c	2021-07-07 16:23:32.308889444 -0400
@@ -391,15 +391,16 @@ void __init mount_block_root(char *name,
 	char *fs_names = page_address(page);
 	char *p;
 	char b[BDEVNAME_SIZE];
+	int num_fs, i;
 
 	scnprintf(b, BDEVNAME_SIZE, "unknown-block(%u,%u)",
 		  MAJOR(ROOT_DEV), MINOR(ROOT_DEV));
 	if (root_fs_names)
 		split_fs_names(fs_names, root_fs_names);
 	else
-		list_bdev_fs_names(fs_names, PAGE_SIZE);
+		num_fs = list_bdev_fs_names(fs_names, PAGE_SIZE);
 retry:
-	for (p = fs_names; *p; p += strlen(p)+1) {
+	for (p = fs_names, i = 0; i < num_fs; p += strlen(p)+1, i++) {
 		int err = do_mount_root(name, p, flags, root_mount_data);
 		switch (err) {
 			case 0:
@@ -432,7 +433,7 @@ retry:
 	printk("List of all partitions:\n");
 	printk_all_partitions();
 	printk("No filesystem could mount root, tried: ");
-	for (p = fs_names; *p; p += strlen(p)+1)
+	for (p = fs_names, i = 0; i < num_fs; p += strlen(p)+1, i++)
 		printk(" %s", p);
 	printk("\n");
 	panic("VFS: Unable to mount root fs on %s", b);

Re: [Virtio-fs] [PATCH 3/2] fs: simplify get_filesystem_list / get_all_fs_names

[Vivek Goyal]

On Wed, Jun 30, 2021 at 07:36:01AM +0200, Christoph Hellwig wrote:
> On Tue, Jun 29, 2021 at 04:50:48PM -0400, Vivek Goyal wrote:
> > May be we should modify mount_block_root() code so that it does not
> > require that extra "\0". Possibly zero initialize page and that should
> > make sure list_bdev_fs_names() does not have to worry about it.
> > 
> > It is possible that a page gets full from the list of filesystems, and
> > last byte on page is terminating null. In that case just zeroing page
> > will not help. We can keep track of some sort of end pointer and make
> > sure we are not searching beyond that for valid filesystem types.
> > 
> > end = page + PAGE_SIZE - 1;
> > 
> > mount_block_root()
> > {
> > 	for (p = fs_names; p < end && *p; p += strlen(p)+1) {
> > 	}
> > }
> 
> Maybe.  To honest I'd prefer to not even touch this unrelated code given
> how full of landmines it is :)

Hi Christoph,

How about following patch. This applies on top of your patches. I noticed
that Al had suggested to return number of filesystems from helper
functions. I just did that and used that to iterate in the loop.

I tested it with a virtual block device (root=/dev/vda1) and it works.
I also filled page with garbage after allocation to make sure natually
occurring null is not there in the middle of page to terminate string.

If you like it, can you please incorporate it in your patches.

Thanks
Vivek

---
 fs/filesystems.c   |    5 ++++-
 include/linux/fs.h |    2 +-
 init/do_mounts.c   |    7 ++++---
 3 files changed, 9 insertions(+), 5 deletions(-)

Index: redhat-linux/fs/filesystems.c
===================================================================
--- redhat-linux.orig/fs/filesystems.c	2021-07-07 16:12:08.890562576 -0400
+++ redhat-linux/fs/filesystems.c	2021-07-07 16:27:51.197620063 -0400
@@ -209,10 +209,11 @@ SYSCALL_DEFINE3(sysfs, int, option, unsi
 }
 #endif
 
-void __init list_bdev_fs_names(char *buf, size_t size)
+int __init list_bdev_fs_names(char *buf, size_t size)
 {
 	struct file_system_type *p;
 	size_t len;
+	int count = 0;
 
 	read_lock(&file_systems_lock);
 	for (p = file_systems; p; p = p->next) {
@@ -226,8 +227,10 @@ void __init list_bdev_fs_names(char *buf
 		memcpy(buf, p->name, len);
 		buf += len;
 		size -= len;
+		count++;
 	}
 	read_unlock(&file_systems_lock);
+	return count;
 }
 
 #ifdef CONFIG_PROC_FS
Index: redhat-linux/include/linux/fs.h
===================================================================
--- redhat-linux.orig/include/linux/fs.h	2021-07-07 15:36:43.224418935 -0400
+++ redhat-linux/include/linux/fs.h	2021-07-07 16:12:18.232949807 -0400
@@ -3622,7 +3622,7 @@ int proc_nr_dentry(struct ctl_table *tab
 		  void *buffer, size_t *lenp, loff_t *ppos);
 int proc_nr_inodes(struct ctl_table *table, int write,
 		   void *buffer, size_t *lenp, loff_t *ppos);
-void __init list_bdev_fs_names(char *buf, size_t size);
+int __init list_bdev_fs_names(char *buf, size_t size);
 
 #define __FMODE_EXEC		((__force int) FMODE_EXEC)
 #define __FMODE_NONOTIFY	((__force int) FMODE_NONOTIFY)
Index: redhat-linux/init/do_mounts.c
===================================================================
--- redhat-linux.orig/init/do_mounts.c	2021-07-07 16:12:08.890562576 -0400
+++ redhat-linux/init/do_mounts.c	2021-07-07 16:23:32.308889444 -0400
@@ -391,15 +391,16 @@ void __init mount_block_root(char *name,
 	char *fs_names = page_address(page);
 	char *p;
 	char b[BDEVNAME_SIZE];
+	int num_fs, i;
 
 	scnprintf(b, BDEVNAME_SIZE, "unknown-block(%u,%u)",
 		  MAJOR(ROOT_DEV), MINOR(ROOT_DEV));
 	if (root_fs_names)
 		split_fs_names(fs_names, root_fs_names);
 	else
-		list_bdev_fs_names(fs_names, PAGE_SIZE);
+		num_fs = list_bdev_fs_names(fs_names, PAGE_SIZE);
 retry:
-	for (p = fs_names; *p; p += strlen(p)+1) {
+	for (p = fs_names, i = 0; i < num_fs; p += strlen(p)+1, i++) {
 		int err = do_mount_root(name, p, flags, root_mount_data);
 		switch (err) {
 			case 0:
@@ -432,7 +433,7 @@ retry:
 	printk("List of all partitions:\n");
 	printk_all_partitions();
 	printk("No filesystem could mount root, tried: ");
-	for (p = fs_names; *p; p += strlen(p)+1)
+	for (p = fs_names, i = 0; i < num_fs; p += strlen(p)+1, i++)
 		printk(" %s", p);
 	printk("\n");
 	panic("VFS: Unable to mount root fs on %s", b);

Re: [Virtio-fs] [PATCH 3/2] fs: simplify get_filesystem_list / get_all_fs_names

[Vivek Goyal]

On Wed, Jul 07, 2021 at 05:04:04PM -0400, Vivek Goyal wrote:
> On Wed, Jun 30, 2021 at 07:36:01AM +0200, Christoph Hellwig wrote:
> > On Tue, Jun 29, 2021 at 04:50:48PM -0400, Vivek Goyal wrote:
> > > May be we should modify mount_block_root() code so that it does not
> > > require that extra "\0". Possibly zero initialize page and that should
> > > make sure list_bdev_fs_names() does not have to worry about it.
> > > 
> > > It is possible that a page gets full from the list of filesystems, and
> > > last byte on page is terminating null. In that case just zeroing page
> > > will not help. We can keep track of some sort of end pointer and make
> > > sure we are not searching beyond that for valid filesystem types.
> > > 
> > > end = page + PAGE_SIZE - 1;
> > > 
> > > mount_block_root()
> > > {
> > > 	for (p = fs_names; p < end && *p; p += strlen(p)+1) {
> > > 	}
> > > }
> > 
> > Maybe.  To honest I'd prefer to not even touch this unrelated code given
> > how full of landmines it is :)
> 
> Hi Christoph,
> 
> How about following patch. This applies on top of your patches. I noticed
> that Al had suggested to return number of filesystems from helper
> functions. I just did that and used that to iterate in the loop.
> 
> I tested it with a virtual block device (root=/dev/vda1) and it works.
> I also filled page with garbage after allocation to make sure natually
> occurring null is not there in the middle of page to terminate string.
> 
> If you like it, can you please incorporate it in your patches.

I noticed this will break with "root_fs_names=". Sorry, will have to
fix split_fs_names() as well. Will do.

Vivek

> 
> Thanks
> Vivek
> 
> ---
>  fs/filesystems.c   |    5 ++++-
>  include/linux/fs.h |    2 +-
>  init/do_mounts.c   |    7 ++++---
>  3 files changed, 9 insertions(+), 5 deletions(-)
> 
> Index: redhat-linux/fs/filesystems.c
> ===================================================================
> --- redhat-linux.orig/fs/filesystems.c	2021-07-07 16:12:08.890562576 -0400
> +++ redhat-linux/fs/filesystems.c	2021-07-07 16:27:51.197620063 -0400
> @@ -209,10 +209,11 @@ SYSCALL_DEFINE3(sysfs, int, option, unsi
>  }
>  #endif
>  
> -void __init list_bdev_fs_names(char *buf, size_t size)
> +int __init list_bdev_fs_names(char *buf, size_t size)
>  {
>  	struct file_system_type *p;
>  	size_t len;
> +	int count = 0;
>  
>  	read_lock(&file_systems_lock);
>  	for (p = file_systems; p; p = p->next) {
> @@ -226,8 +227,10 @@ void __init list_bdev_fs_names(char *buf
>  		memcpy(buf, p->name, len);
>  		buf += len;
>  		size -= len;
> +		count++;
>  	}
>  	read_unlock(&file_systems_lock);
> +	return count;
>  }
>  
>  #ifdef CONFIG_PROC_FS
> Index: redhat-linux/include/linux/fs.h
> ===================================================================
> --- redhat-linux.orig/include/linux/fs.h	2021-07-07 15:36:43.224418935 -0400
> +++ redhat-linux/include/linux/fs.h	2021-07-07 16:12:18.232949807 -0400
> @@ -3622,7 +3622,7 @@ int proc_nr_dentry(struct ctl_table *tab
>  		  void *buffer, size_t *lenp, loff_t *ppos);
>  int proc_nr_inodes(struct ctl_table *table, int write,
>  		   void *buffer, size_t *lenp, loff_t *ppos);
> -void __init list_bdev_fs_names(char *buf, size_t size);
> +int __init list_bdev_fs_names(char *buf, size_t size);
>  
>  #define __FMODE_EXEC		((__force int) FMODE_EXEC)
>  #define __FMODE_NONOTIFY	((__force int) FMODE_NONOTIFY)
> Index: redhat-linux/init/do_mounts.c
> ===================================================================
> --- redhat-linux.orig/init/do_mounts.c	2021-07-07 16:12:08.890562576 -0400
> +++ redhat-linux/init/do_mounts.c	2021-07-07 16:23:32.308889444 -0400
> @@ -391,15 +391,16 @@ void __init mount_block_root(char *name,
>  	char *fs_names = page_address(page);
>  	char *p;
>  	char b[BDEVNAME_SIZE];
> +	int num_fs, i;
>  
>  	scnprintf(b, BDEVNAME_SIZE, "unknown-block(%u,%u)",
>  		  MAJOR(ROOT_DEV), MINOR(ROOT_DEV));
>  	if (root_fs_names)
>  		split_fs_names(fs_names, root_fs_names);
>  	else
> -		list_bdev_fs_names(fs_names, PAGE_SIZE);
> +		num_fs = list_bdev_fs_names(fs_names, PAGE_SIZE);
>  retry:
> -	for (p = fs_names; *p; p += strlen(p)+1) {
> +	for (p = fs_names, i = 0; i < num_fs; p += strlen(p)+1, i++) {
>  		int err = do_mount_root(name, p, flags, root_mount_data);
>  		switch (err) {
>  			case 0:
> @@ -432,7 +433,7 @@ retry:
>  	printk("List of all partitions:\n");
>  	printk_all_partitions();
>  	printk("No filesystem could mount root, tried: ");
> -	for (p = fs_names; *p; p += strlen(p)+1)
> +	for (p = fs_names, i = 0; i < num_fs; p += strlen(p)+1, i++)
>  		printk(" %s", p);
>  	printk("\n");
>  	panic("VFS: Unable to mount root fs on %s", b);
> 
> 

Re: [Virtio-fs] [PATCH 3/2] fs: simplify get_filesystem_list / get_all_fs_names

[Vivek Goyal]

On Wed, Jul 07, 2021 at 05:04:04PM -0400, Vivek Goyal wrote:
> On Wed, Jun 30, 2021 at 07:36:01AM +0200, Christoph Hellwig wrote:
> > On Tue, Jun 29, 2021 at 04:50:48PM -0400, Vivek Goyal wrote:
> > > May be we should modify mount_block_root() code so that it does not
> > > require that extra "\0". Possibly zero initialize page and that should
> > > make sure list_bdev_fs_names() does not have to worry about it.
> > > 
> > > It is possible that a page gets full from the list of filesystems, and
> > > last byte on page is terminating null. In that case just zeroing page
> > > will not help. We can keep track of some sort of end pointer and make
> > > sure we are not searching beyond that for valid filesystem types.
> > > 
> > > end = page + PAGE_SIZE - 1;
> > > 
> > > mount_block_root()
> > > {
> > > 	for (p = fs_names; p < end && *p; p += strlen(p)+1) {
> > > 	}
> > > }
> > 
> > Maybe.  To honest I'd prefer to not even touch this unrelated code given
> > how full of landmines it is :)
> 
> Hi Christoph,
> 
> How about following patch. This applies on top of your patches. I noticed
> that Al had suggested to return number of filesystems from helper
> functions. I just did that and used that to iterate in the loop.
> 
> I tested it with a virtual block device (root=/dev/vda1) and it works.
> I also filled page with garbage after allocation to make sure natually
> occurring null is not there in the middle of page to terminate string.
> 
> If you like it, can you please incorporate it in your patches.

I noticed this will break with "root_fs_names=". Sorry, will have to
fix split_fs_names() as well. Will do.

Vivek

> 
> Thanks
> Vivek
> 
> ---
>  fs/filesystems.c   |    5 ++++-
>  include/linux/fs.h |    2 +-
>  init/do_mounts.c   |    7 ++++---
>  3 files changed, 9 insertions(+), 5 deletions(-)
> 
> Index: redhat-linux/fs/filesystems.c
> ===================================================================
> --- redhat-linux.orig/fs/filesystems.c	2021-07-07 16:12:08.890562576 -0400
> +++ redhat-linux/fs/filesystems.c	2021-07-07 16:27:51.197620063 -0400
> @@ -209,10 +209,11 @@ SYSCALL_DEFINE3(sysfs, int, option, unsi
>  }
>  #endif
>  
> -void __init list_bdev_fs_names(char *buf, size_t size)
> +int __init list_bdev_fs_names(char *buf, size_t size)
>  {
>  	struct file_system_type *p;
>  	size_t len;
> +	int count = 0;
>  
>  	read_lock(&file_systems_lock);
>  	for (p = file_systems; p; p = p->next) {
> @@ -226,8 +227,10 @@ void __init list_bdev_fs_names(char *buf
>  		memcpy(buf, p->name, len);
>  		buf += len;
>  		size -= len;
> +		count++;
>  	}
>  	read_unlock(&file_systems_lock);
> +	return count;
>  }
>  
>  #ifdef CONFIG_PROC_FS
> Index: redhat-linux/include/linux/fs.h
> ===================================================================
> --- redhat-linux.orig/include/linux/fs.h	2021-07-07 15:36:43.224418935 -0400
> +++ redhat-linux/include/linux/fs.h	2021-07-07 16:12:18.232949807 -0400
> @@ -3622,7 +3622,7 @@ int proc_nr_dentry(struct ctl_table *tab
>  		  void *buffer, size_t *lenp, loff_t *ppos);
>  int proc_nr_inodes(struct ctl_table *table, int write,
>  		   void *buffer, size_t *lenp, loff_t *ppos);
> -void __init list_bdev_fs_names(char *buf, size_t size);
> +int __init list_bdev_fs_names(char *buf, size_t size);
>  
>  #define __FMODE_EXEC		((__force int) FMODE_EXEC)
>  #define __FMODE_NONOTIFY	((__force int) FMODE_NONOTIFY)
> Index: redhat-linux/init/do_mounts.c
> ===================================================================
> --- redhat-linux.orig/init/do_mounts.c	2021-07-07 16:12:08.890562576 -0400
> +++ redhat-linux/init/do_mounts.c	2021-07-07 16:23:32.308889444 -0400
> @@ -391,15 +391,16 @@ void __init mount_block_root(char *name,
>  	char *fs_names = page_address(page);
>  	char *p;
>  	char b[BDEVNAME_SIZE];
> +	int num_fs, i;
>  
>  	scnprintf(b, BDEVNAME_SIZE, "unknown-block(%u,%u)",
>  		  MAJOR(ROOT_DEV), MINOR(ROOT_DEV));
>  	if (root_fs_names)
>  		split_fs_names(fs_names, root_fs_names);
>  	else
> -		list_bdev_fs_names(fs_names, PAGE_SIZE);
> +		num_fs = list_bdev_fs_names(fs_names, PAGE_SIZE);
>  retry:
> -	for (p = fs_names; *p; p += strlen(p)+1) {
> +	for (p = fs_names, i = 0; i < num_fs; p += strlen(p)+1, i++) {
>  		int err = do_mount_root(name, p, flags, root_mount_data);
>  		switch (err) {
>  			case 0:
> @@ -432,7 +433,7 @@ retry:
>  	printk("List of all partitions:\n");
>  	printk_all_partitions();
>  	printk("No filesystem could mount root, tried: ");
> -	for (p = fs_names; *p; p += strlen(p)+1)
> +	for (p = fs_names, i = 0; i < num_fs; p += strlen(p)+1, i++)
>  		printk(" %s", p);
>  	printk("\n");
>  	panic("VFS: Unable to mount root fs on %s", b);
> 
> 

Re: [Virtio-fs] [PATCH 3/2] fs: simplify get_filesystem_list / get_all_fs_names

[Vivek Goyal]

On Wed, Jul 07, 2021 at 05:06:36PM -0400, Vivek Goyal wrote:
> On Wed, Jul 07, 2021 at 05:04:04PM -0400, Vivek Goyal wrote:
> > On Wed, Jun 30, 2021 at 07:36:01AM +0200, Christoph Hellwig wrote:
> > > On Tue, Jun 29, 2021 at 04:50:48PM -0400, Vivek Goyal wrote:
> > > > May be we should modify mount_block_root() code so that it does not
> > > > require that extra "\0". Possibly zero initialize page and that should
> > > > make sure list_bdev_fs_names() does not have to worry about it.
> > > > 
> > > > It is possible that a page gets full from the list of filesystems, and
> > > > last byte on page is terminating null. In that case just zeroing page
> > > > will not help. We can keep track of some sort of end pointer and make
> > > > sure we are not searching beyond that for valid filesystem types.
> > > > 
> > > > end = page + PAGE_SIZE - 1;
> > > > 
> > > > mount_block_root()
> > > > {
> > > > 	for (p = fs_names; p < end && *p; p += strlen(p)+1) {
> > > > 	}
> > > > }
> > > 
> > > Maybe.  To honest I'd prefer to not even touch this unrelated code given
> > > how full of landmines it is :)
> > 
> > Hi Christoph,
> > 
> > How about following patch. This applies on top of your patches. I noticed
> > that Al had suggested to return number of filesystems from helper
> > functions. I just did that and used that to iterate in the loop.
> > 
> > I tested it with a virtual block device (root=/dev/vda1) and it works.
> > I also filled page with garbage after allocation to make sure natually
> > occurring null is not there in the middle of page to terminate string.
> > 
> > If you like it, can you please incorporate it in your patches.
> 
> I noticed this will break with "root_fs_names=". Sorry, will have to
> fix split_fs_names() as well. Will do.

Hi Christoph,

I fixed it. Now both split_fs_names() and list_bdev_fs_names() return
count of fstype strings it placed in the buffer. And callers now
use that count to loop (instead of relying on extra null byte at the
end of the buffer).

I tested both nodev (virtiofs, 9p) and block dev rootfs (ext4) and
it works for me. Please have a look.

Thanks
Vivek


---
 fs/filesystems.c   |    5 ++++-
 include/linux/fs.h |    2 +-
 init/do_mounts.c   |   35 +++++++++++++++++++++++------------
 3 files changed, 28 insertions(+), 14 deletions(-)

Index: redhat-linux/fs/filesystems.c
===================================================================
--- redhat-linux.orig/fs/filesystems.c	2021-07-08 08:02:09.772766786 -0400
+++ redhat-linux/fs/filesystems.c	2021-07-08 08:02:12.044860918 -0400
@@ -209,10 +209,11 @@ SYSCALL_DEFINE3(sysfs, int, option, unsi
 }
 #endif
 
-void __init list_bdev_fs_names(char *buf, size_t size)
+int __init list_bdev_fs_names(char *buf, size_t size)
 {
 	struct file_system_type *p;
 	size_t len;
+	int count = 0;
 
 	read_lock(&file_systems_lock);
 	for (p = file_systems; p; p = p->next) {
@@ -226,8 +227,10 @@ void __init list_bdev_fs_names(char *buf
 		memcpy(buf, p->name, len);
 		buf += len;
 		size -= len;
+		count++;
 	}
 	read_unlock(&file_systems_lock);
+	return count;
 }
 
 #ifdef CONFIG_PROC_FS
Index: redhat-linux/include/linux/fs.h
===================================================================
--- redhat-linux.orig/include/linux/fs.h	2021-07-08 08:02:09.774766869 -0400
+++ redhat-linux/include/linux/fs.h	2021-07-08 08:02:12.046861001 -0400
@@ -3622,7 +3622,7 @@ int proc_nr_dentry(struct ctl_table *tab
 		  void *buffer, size_t *lenp, loff_t *ppos);
 int proc_nr_inodes(struct ctl_table *table, int write,
 		   void *buffer, size_t *lenp, loff_t *ppos);
-void __init list_bdev_fs_names(char *buf, size_t size);
+int __init list_bdev_fs_names(char *buf, size_t size);
 
 #define __FMODE_EXEC		((__force int) FMODE_EXEC)
 #define __FMODE_NONOTIFY	((__force int) FMODE_NONOTIFY)
Index: redhat-linux/init/do_mounts.c
===================================================================
--- redhat-linux.orig/init/do_mounts.c	2021-07-08 08:02:09.774766869 -0400
+++ redhat-linux/init/do_mounts.c	2021-07-08 08:02:12.046861001 -0400
@@ -338,14 +338,22 @@ __setup("rootflags=", root_data_setup);
 __setup("rootfstype=", fs_names_setup);
 __setup("rootdelay=", root_delay_setup);
 
-static void __init split_fs_names(char *page, char *names)
+static int __init split_fs_names(char *page, char *names)
 {
-	strcpy(page, root_fs_names);
-	while (*page++) {
-		if (page[-1] == ',')
-			page[-1] = '\0';
+	int count = 0;
+	char *p = page;
+
+	strcpy(p, root_fs_names);
+	while (*p++) {
+		if (p[-1] == ',')
+			p[-1] = '\0';
 	}
-	*page = '\0';
+	*p = '\0';
+
+	for (p = page; *p; p += strlen(p)+1)
+		count++;
+
+	return count;
 }
 
 static int __init do_mount_root(const char *name, const char *fs,
@@ -391,15 +399,16 @@ void __init mount_block_root(char *name,
 	char *fs_names = page_address(page);
 	char *p;
 	char b[BDEVNAME_SIZE];
+	int num_fs, i;
 
 	scnprintf(b, BDEVNAME_SIZE, "unknown-block(%u,%u)",
 		  MAJOR(ROOT_DEV), MINOR(ROOT_DEV));
 	if (root_fs_names)
-		split_fs_names(fs_names, root_fs_names);
+		num_fs = split_fs_names(fs_names, root_fs_names);
 	else
-		list_bdev_fs_names(fs_names, PAGE_SIZE);
+		num_fs = list_bdev_fs_names(fs_names, PAGE_SIZE);
 retry:
-	for (p = fs_names; *p; p += strlen(p)+1) {
+	for (p = fs_names, i = 0; i < num_fs; p += strlen(p)+1, i++) {
 		int err = do_mount_root(name, p, flags, root_mount_data);
 		switch (err) {
 			case 0:
@@ -432,7 +441,7 @@ retry:
 	printk("List of all partitions:\n");
 	printk_all_partitions();
 	printk("No filesystem could mount root, tried: ");
-	for (p = fs_names; *p; p += strlen(p)+1)
+	for (p = fs_names, i = 0; i < num_fs; p += strlen(p)+1, i++)
 		printk(" %s", p);
 	printk("\n");
 	panic("VFS: Unable to mount root fs on %s", b);
@@ -533,13 +542,15 @@ static int __init mount_nodev_root(void)
 {
 	char *fs_names, *fstype;
 	int err = -EINVAL;
+	int num_fs, i;
 
 	fs_names = (void *)__get_free_page(GFP_KERNEL);
 	if (!fs_names)
 		return -EINVAL;
-	split_fs_names(fs_names, root_fs_names);
+	num_fs = split_fs_names(fs_names, root_fs_names);
 
-	for (fstype = fs_names; *fstype; fstype += strlen(fstype) + 1) {
+	for (fstype = fs_names, i = 0; i < num_fs;
+	     fstype += strlen(fstype) + 1, i++) {
 		if (!fs_is_nodev(fstype))
 			continue;
 		err = do_mount_root(root_device_name, fstype, root_mountflags,

Re: [Virtio-fs] [PATCH 3/2] fs: simplify get_filesystem_list / get_all_fs_names

[Vivek Goyal]

On Wed, Jul 07, 2021 at 05:06:36PM -0400, Vivek Goyal wrote:
> On Wed, Jul 07, 2021 at 05:04:04PM -0400, Vivek Goyal wrote:
> > On Wed, Jun 30, 2021 at 07:36:01AM +0200, Christoph Hellwig wrote:
> > > On Tue, Jun 29, 2021 at 04:50:48PM -0400, Vivek Goyal wrote:
> > > > May be we should modify mount_block_root() code so that it does not
> > > > require that extra "\0". Possibly zero initialize page and that should
> > > > make sure list_bdev_fs_names() does not have to worry about it.
> > > > 
> > > > It is possible that a page gets full from the list of filesystems, and
> > > > last byte on page is terminating null. In that case just zeroing page
> > > > will not help. We can keep track of some sort of end pointer and make
> > > > sure we are not searching beyond that for valid filesystem types.
> > > > 
> > > > end = page + PAGE_SIZE - 1;
> > > > 
> > > > mount_block_root()
> > > > {
> > > > 	for (p = fs_names; p < end && *p; p += strlen(p)+1) {
> > > > 	}
> > > > }
> > > 
> > > Maybe.  To honest I'd prefer to not even touch this unrelated code given
> > > how full of landmines it is :)
> > 
> > Hi Christoph,
> > 
> > How about following patch. This applies on top of your patches. I noticed
> > that Al had suggested to return number of filesystems from helper
> > functions. I just did that and used that to iterate in the loop.
> > 
> > I tested it with a virtual block device (root=/dev/vda1) and it works.
> > I also filled page with garbage after allocation to make sure natually
> > occurring null is not there in the middle of page to terminate string.
> > 
> > If you like it, can you please incorporate it in your patches.
> 
> I noticed this will break with "root_fs_names=". Sorry, will have to
> fix split_fs_names() as well. Will do.

Hi Christoph,

I fixed it. Now both split_fs_names() and list_bdev_fs_names() return
count of fstype strings it placed in the buffer. And callers now
use that count to loop (instead of relying on extra null byte at the
end of the buffer).

I tested both nodev (virtiofs, 9p) and block dev rootfs (ext4) and
it works for me. Please have a look.

Thanks
Vivek


---
 fs/filesystems.c   |    5 ++++-
 include/linux/fs.h |    2 +-
 init/do_mounts.c   |   35 +++++++++++++++++++++++------------
 3 files changed, 28 insertions(+), 14 deletions(-)

Index: redhat-linux/fs/filesystems.c
===================================================================
--- redhat-linux.orig/fs/filesystems.c	2021-07-08 08:02:09.772766786 -0400
+++ redhat-linux/fs/filesystems.c	2021-07-08 08:02:12.044860918 -0400
@@ -209,10 +209,11 @@ SYSCALL_DEFINE3(sysfs, int, option, unsi
 }
 #endif
 
-void __init list_bdev_fs_names(char *buf, size_t size)
+int __init list_bdev_fs_names(char *buf, size_t size)
 {
 	struct file_system_type *p;
 	size_t len;
+	int count = 0;
 
 	read_lock(&file_systems_lock);
 	for (p = file_systems; p; p = p->next) {
@@ -226,8 +227,10 @@ void __init list_bdev_fs_names(char *buf
 		memcpy(buf, p->name, len);
 		buf += len;
 		size -= len;
+		count++;
 	}
 	read_unlock(&file_systems_lock);
+	return count;
 }
 
 #ifdef CONFIG_PROC_FS
Index: redhat-linux/include/linux/fs.h
===================================================================
--- redhat-linux.orig/include/linux/fs.h	2021-07-08 08:02:09.774766869 -0400
+++ redhat-linux/include/linux/fs.h	2021-07-08 08:02:12.046861001 -0400
@@ -3622,7 +3622,7 @@ int proc_nr_dentry(struct ctl_table *tab
 		  void *buffer, size_t *lenp, loff_t *ppos);
 int proc_nr_inodes(struct ctl_table *table, int write,
 		   void *buffer, size_t *lenp, loff_t *ppos);
-void __init list_bdev_fs_names(char *buf, size_t size);
+int __init list_bdev_fs_names(char *buf, size_t size);
 
 #define __FMODE_EXEC		((__force int) FMODE_EXEC)
 #define __FMODE_NONOTIFY	((__force int) FMODE_NONOTIFY)
Index: redhat-linux/init/do_mounts.c
===================================================================
--- redhat-linux.orig/init/do_mounts.c	2021-07-08 08:02:09.774766869 -0400
+++ redhat-linux/init/do_mounts.c	2021-07-08 08:02:12.046861001 -0400
@@ -338,14 +338,22 @@ __setup("rootflags=", root_data_setup);
 __setup("rootfstype=", fs_names_setup);
 __setup("rootdelay=", root_delay_setup);
 
-static void __init split_fs_names(char *page, char *names)
+static int __init split_fs_names(char *page, char *names)
 {
-	strcpy(page, root_fs_names);
-	while (*page++) {
-		if (page[-1] == ',')
-			page[-1] = '\0';
+	int count = 0;
+	char *p = page;
+
+	strcpy(p, root_fs_names);
+	while (*p++) {
+		if (p[-1] == ',')
+			p[-1] = '\0';
 	}
-	*page = '\0';
+	*p = '\0';
+
+	for (p = page; *p; p += strlen(p)+1)
+		count++;
+
+	return count;
 }
 
 static int __init do_mount_root(const char *name, const char *fs,
@@ -391,15 +399,16 @@ void __init mount_block_root(char *name,
 	char *fs_names = page_address(page);
 	char *p;
 	char b[BDEVNAME_SIZE];
+	int num_fs, i;
 
 	scnprintf(b, BDEVNAME_SIZE, "unknown-block(%u,%u)",
 		  MAJOR(ROOT_DEV), MINOR(ROOT_DEV));
 	if (root_fs_names)
-		split_fs_names(fs_names, root_fs_names);
+		num_fs = split_fs_names(fs_names, root_fs_names);
 	else
-		list_bdev_fs_names(fs_names, PAGE_SIZE);
+		num_fs = list_bdev_fs_names(fs_names, PAGE_SIZE);
 retry:
-	for (p = fs_names; *p; p += strlen(p)+1) {
+	for (p = fs_names, i = 0; i < num_fs; p += strlen(p)+1, i++) {
 		int err = do_mount_root(name, p, flags, root_mount_data);
 		switch (err) {
 			case 0:
@@ -432,7 +441,7 @@ retry:
 	printk("List of all partitions:\n");
 	printk_all_partitions();
 	printk("No filesystem could mount root, tried: ");
-	for (p = fs_names; *p; p += strlen(p)+1)
+	for (p = fs_names, i = 0; i < num_fs; p += strlen(p)+1, i++)
 		printk(" %s", p);
 	printk("\n");
 	panic("VFS: Unable to mount root fs on %s", b);
@@ -533,13 +542,15 @@ static int __init mount_nodev_root(void)
 {
 	char *fs_names, *fstype;
 	int err = -EINVAL;
+	int num_fs, i;
 
 	fs_names = (void *)__get_free_page(GFP_KERNEL);
 	if (!fs_names)
 		return -EINVAL;
-	split_fs_names(fs_names, root_fs_names);
+	num_fs = split_fs_names(fs_names, root_fs_names);
 
-	for (fstype = fs_names; *fstype; fstype += strlen(fstype) + 1) {
+	for (fstype = fs_names, i = 0; i < num_fs;
+	     fstype += strlen(fstype) + 1, i++) {
 		if (!fs_is_nodev(fstype))
 			continue;
 		err = do_mount_root(root_device_name, fstype, root_mountflags,

Re: [Virtio-fs] [PATCH 3/2] fs: simplify get_filesystem_list / get_all_fs_names

[Vivek Goyal]

On Thu, Jul 08, 2021 at 08:59:36AM -0400, Vivek Goyal wrote:
> On Wed, Jul 07, 2021 at 05:06:36PM -0400, Vivek Goyal wrote:
> > On Wed, Jul 07, 2021 at 05:04:04PM -0400, Vivek Goyal wrote:
> > > On Wed, Jun 30, 2021 at 07:36:01AM +0200, Christoph Hellwig wrote:
> > > > On Tue, Jun 29, 2021 at 04:50:48PM -0400, Vivek Goyal wrote:
> > > > > May be we should modify mount_block_root() code so that it does not
> > > > > require that extra "\0". Possibly zero initialize page and that should
> > > > > make sure list_bdev_fs_names() does not have to worry about it.
> > > > > 
> > > > > It is possible that a page gets full from the list of filesystems, and
> > > > > last byte on page is terminating null. In that case just zeroing page
> > > > > will not help. We can keep track of some sort of end pointer and make
> > > > > sure we are not searching beyond that for valid filesystem types.
> > > > > 
> > > > > end = page + PAGE_SIZE - 1;
> > > > > 
> > > > > mount_block_root()
> > > > > {
> > > > > 	for (p = fs_names; p < end && *p; p += strlen(p)+1) {
> > > > > 	}
> > > > > }
> > > > 
> > > > Maybe.  To honest I'd prefer to not even touch this unrelated code given
> > > > how full of landmines it is :)
> > > 
> > > Hi Christoph,
> > > 
> > > How about following patch. This applies on top of your patches. I noticed
> > > that Al had suggested to return number of filesystems from helper
> > > functions. I just did that and used that to iterate in the loop.
> > > 
> > > I tested it with a virtual block device (root=/dev/vda1) and it works.
> > > I also filled page with garbage after allocation to make sure natually
> > > occurring null is not there in the middle of page to terminate string.
> > > 
> > > If you like it, can you please incorporate it in your patches.
> > 
> > I noticed this will break with "root_fs_names=". Sorry, will have to
> > fix split_fs_names() as well. Will do.
> 
> Hi Christoph,
> 
> I fixed it. Now both split_fs_names() and list_bdev_fs_names() return
> count of fstype strings it placed in the buffer. And callers now
> use that count to loop (instead of relying on extra null byte at the
> end of the buffer).
> 
> I tested both nodev (virtiofs, 9p) and block dev rootfs (ext4) and
> it works for me. Please have a look.

Hi Christoph,

In case you are finding it hard to spend some time on these patches, I 
can take those patches, merge my changes and repost them.

Vivek

> 
> 
> ---
>  fs/filesystems.c   |    5 ++++-
>  include/linux/fs.h |    2 +-
>  init/do_mounts.c   |   35 +++++++++++++++++++++++------------
>  3 files changed, 28 insertions(+), 14 deletions(-)
> 
> Index: redhat-linux/fs/filesystems.c
> ===================================================================
> --- redhat-linux.orig/fs/filesystems.c	2021-07-08 08:02:09.772766786 -0400
> +++ redhat-linux/fs/filesystems.c	2021-07-08 08:02:12.044860918 -0400
> @@ -209,10 +209,11 @@ SYSCALL_DEFINE3(sysfs, int, option, unsi
>  }
>  #endif
>  
> -void __init list_bdev_fs_names(char *buf, size_t size)
> +int __init list_bdev_fs_names(char *buf, size_t size)
>  {
>  	struct file_system_type *p;
>  	size_t len;
> +	int count = 0;
>  
>  	read_lock(&file_systems_lock);
>  	for (p = file_systems; p; p = p->next) {
> @@ -226,8 +227,10 @@ void __init list_bdev_fs_names(char *buf
>  		memcpy(buf, p->name, len);
>  		buf += len;
>  		size -= len;
> +		count++;
>  	}
>  	read_unlock(&file_systems_lock);
> +	return count;
>  }
>  
>  #ifdef CONFIG_PROC_FS
> Index: redhat-linux/include/linux/fs.h
> ===================================================================
> --- redhat-linux.orig/include/linux/fs.h	2021-07-08 08:02:09.774766869 -0400
> +++ redhat-linux/include/linux/fs.h	2021-07-08 08:02:12.046861001 -0400
> @@ -3622,7 +3622,7 @@ int proc_nr_dentry(struct ctl_table *tab
>  		  void *buffer, size_t *lenp, loff_t *ppos);
>  int proc_nr_inodes(struct ctl_table *table, int write,
>  		   void *buffer, size_t *lenp, loff_t *ppos);
> -void __init list_bdev_fs_names(char *buf, size_t size);
> +int __init list_bdev_fs_names(char *buf, size_t size);
>  
>  #define __FMODE_EXEC		((__force int) FMODE_EXEC)
>  #define __FMODE_NONOTIFY	((__force int) FMODE_NONOTIFY)
> Index: redhat-linux/init/do_mounts.c
> ===================================================================
> --- redhat-linux.orig/init/do_mounts.c	2021-07-08 08:02:09.774766869 -0400
> +++ redhat-linux/init/do_mounts.c	2021-07-08 08:02:12.046861001 -0400
> @@ -338,14 +338,22 @@ __setup("rootflags=", root_data_setup);
>  __setup("rootfstype=", fs_names_setup);
>  __setup("rootdelay=", root_delay_setup);
>  
> -static void __init split_fs_names(char *page, char *names)
> +static int __init split_fs_names(char *page, char *names)
>  {
> -	strcpy(page, root_fs_names);
> -	while (*page++) {
> -		if (page[-1] == ',')
> -			page[-1] = '\0';
> +	int count = 0;
> +	char *p = page;
> +
> +	strcpy(p, root_fs_names);
> +	while (*p++) {
> +		if (p[-1] == ',')
> +			p[-1] = '\0';
>  	}
> -	*page = '\0';
> +	*p = '\0';
> +
> +	for (p = page; *p; p += strlen(p)+1)
> +		count++;
> +
> +	return count;
>  }
>  
>  static int __init do_mount_root(const char *name, const char *fs,
> @@ -391,15 +399,16 @@ void __init mount_block_root(char *name,
>  	char *fs_names = page_address(page);
>  	char *p;
>  	char b[BDEVNAME_SIZE];
> +	int num_fs, i;
>  
>  	scnprintf(b, BDEVNAME_SIZE, "unknown-block(%u,%u)",
>  		  MAJOR(ROOT_DEV), MINOR(ROOT_DEV));
>  	if (root_fs_names)
> -		split_fs_names(fs_names, root_fs_names);
> +		num_fs = split_fs_names(fs_names, root_fs_names);
>  	else
> -		list_bdev_fs_names(fs_names, PAGE_SIZE);
> +		num_fs = list_bdev_fs_names(fs_names, PAGE_SIZE);
>  retry:
> -	for (p = fs_names; *p; p += strlen(p)+1) {
> +	for (p = fs_names, i = 0; i < num_fs; p += strlen(p)+1, i++) {
>  		int err = do_mount_root(name, p, flags, root_mount_data);
>  		switch (err) {
>  			case 0:
> @@ -432,7 +441,7 @@ retry:
>  	printk("List of all partitions:\n");
>  	printk_all_partitions();
>  	printk("No filesystem could mount root, tried: ");
> -	for (p = fs_names; *p; p += strlen(p)+1)
> +	for (p = fs_names, i = 0; i < num_fs; p += strlen(p)+1, i++)
>  		printk(" %s", p);
>  	printk("\n");
>  	panic("VFS: Unable to mount root fs on %s", b);
> @@ -533,13 +542,15 @@ static int __init mount_nodev_root(void)
>  {
>  	char *fs_names, *fstype;
>  	int err = -EINVAL;
> +	int num_fs, i;
>  
>  	fs_names = (void *)__get_free_page(GFP_KERNEL);
>  	if (!fs_names)
>  		return -EINVAL;
> -	split_fs_names(fs_names, root_fs_names);
> +	num_fs = split_fs_names(fs_names, root_fs_names);
>  
> -	for (fstype = fs_names; *fstype; fstype += strlen(fstype) + 1) {
> +	for (fstype = fs_names, i = 0; i < num_fs;
> +	     fstype += strlen(fstype) + 1, i++) {
>  		if (!fs_is_nodev(fstype))
>  			continue;
>  		err = do_mount_root(root_device_name, fstype, root_mountflags,

Re: [Virtio-fs] [PATCH 3/2] fs: simplify get_filesystem_list / get_all_fs_names

[Vivek Goyal]

On Thu, Jul 08, 2021 at 08:59:36AM -0400, Vivek Goyal wrote:
> On Wed, Jul 07, 2021 at 05:06:36PM -0400, Vivek Goyal wrote:
> > On Wed, Jul 07, 2021 at 05:04:04PM -0400, Vivek Goyal wrote:
> > > On Wed, Jun 30, 2021 at 07:36:01AM +0200, Christoph Hellwig wrote:
> > > > On Tue, Jun 29, 2021 at 04:50:48PM -0400, Vivek Goyal wrote:
> > > > > May be we should modify mount_block_root() code so that it does not
> > > > > require that extra "\0". Possibly zero initialize page and that should
> > > > > make sure list_bdev_fs_names() does not have to worry about it.
> > > > > 
> > > > > It is possible that a page gets full from the list of filesystems, and
> > > > > last byte on page is terminating null. In that case just zeroing page
> > > > > will not help. We can keep track of some sort of end pointer and make
> > > > > sure we are not searching beyond that for valid filesystem types.
> > > > > 
> > > > > end = page + PAGE_SIZE - 1;
> > > > > 
> > > > > mount_block_root()
> > > > > {
> > > > > 	for (p = fs_names; p < end && *p; p += strlen(p)+1) {
> > > > > 	}
> > > > > }
> > > > 
> > > > Maybe.  To honest I'd prefer to not even touch this unrelated code given
> > > > how full of landmines it is :)
> > > 
> > > Hi Christoph,
> > > 
> > > How about following patch. This applies on top of your patches. I noticed
> > > that Al had suggested to return number of filesystems from helper
> > > functions. I just did that and used that to iterate in the loop.
> > > 
> > > I tested it with a virtual block device (root=/dev/vda1) and it works.
> > > I also filled page with garbage after allocation to make sure natually
> > > occurring null is not there in the middle of page to terminate string.
> > > 
> > > If you like it, can you please incorporate it in your patches.
> > 
> > I noticed this will break with "root_fs_names=". Sorry, will have to
> > fix split_fs_names() as well. Will do.
> 
> Hi Christoph,
> 
> I fixed it. Now both split_fs_names() and list_bdev_fs_names() return
> count of fstype strings it placed in the buffer. And callers now
> use that count to loop (instead of relying on extra null byte at the
> end of the buffer).
> 
> I tested both nodev (virtiofs, 9p) and block dev rootfs (ext4) and
> it works for me. Please have a look.

Hi Christoph,

In case you are finding it hard to spend some time on these patches, I 
can take those patches, merge my changes and repost them.

Vivek

> 
> 
> ---
>  fs/filesystems.c   |    5 ++++-
>  include/linux/fs.h |    2 +-
>  init/do_mounts.c   |   35 +++++++++++++++++++++++------------
>  3 files changed, 28 insertions(+), 14 deletions(-)
> 
> Index: redhat-linux/fs/filesystems.c
> ===================================================================
> --- redhat-linux.orig/fs/filesystems.c	2021-07-08 08:02:09.772766786 -0400
> +++ redhat-linux/fs/filesystems.c	2021-07-08 08:02:12.044860918 -0400
> @@ -209,10 +209,11 @@ SYSCALL_DEFINE3(sysfs, int, option, unsi
>  }
>  #endif
>  
> -void __init list_bdev_fs_names(char *buf, size_t size)
> +int __init list_bdev_fs_names(char *buf, size_t size)
>  {
>  	struct file_system_type *p;
>  	size_t len;
> +	int count = 0;
>  
>  	read_lock(&file_systems_lock);
>  	for (p = file_systems; p; p = p->next) {
> @@ -226,8 +227,10 @@ void __init list_bdev_fs_names(char *buf
>  		memcpy(buf, p->name, len);
>  		buf += len;
>  		size -= len;
> +		count++;
>  	}
>  	read_unlock(&file_systems_lock);
> +	return count;
>  }
>  
>  #ifdef CONFIG_PROC_FS
> Index: redhat-linux/include/linux/fs.h
> ===================================================================
> --- redhat-linux.orig/include/linux/fs.h	2021-07-08 08:02:09.774766869 -0400
> +++ redhat-linux/include/linux/fs.h	2021-07-08 08:02:12.046861001 -0400
> @@ -3622,7 +3622,7 @@ int proc_nr_dentry(struct ctl_table *tab
>  		  void *buffer, size_t *lenp, loff_t *ppos);
>  int proc_nr_inodes(struct ctl_table *table, int write,
>  		   void *buffer, size_t *lenp, loff_t *ppos);
> -void __init list_bdev_fs_names(char *buf, size_t size);
> +int __init list_bdev_fs_names(char *buf, size_t size);
>  
>  #define __FMODE_EXEC		((__force int) FMODE_EXEC)
>  #define __FMODE_NONOTIFY	((__force int) FMODE_NONOTIFY)
> Index: redhat-linux/init/do_mounts.c
> ===================================================================
> --- redhat-linux.orig/init/do_mounts.c	2021-07-08 08:02:09.774766869 -0400
> +++ redhat-linux/init/do_mounts.c	2021-07-08 08:02:12.046861001 -0400
> @@ -338,14 +338,22 @@ __setup("rootflags=", root_data_setup);
>  __setup("rootfstype=", fs_names_setup);
>  __setup("rootdelay=", root_delay_setup);
>  
> -static void __init split_fs_names(char *page, char *names)
> +static int __init split_fs_names(char *page, char *names)
>  {
> -	strcpy(page, root_fs_names);
> -	while (*page++) {
> -		if (page[-1] == ',')
> -			page[-1] = '\0';
> +	int count = 0;
> +	char *p = page;
> +
> +	strcpy(p, root_fs_names);
> +	while (*p++) {
> +		if (p[-1] == ',')
> +			p[-1] = '\0';
>  	}
> -	*page = '\0';
> +	*p = '\0';
> +
> +	for (p = page; *p; p += strlen(p)+1)
> +		count++;
> +
> +	return count;
>  }
>  
>  static int __init do_mount_root(const char *name, const char *fs,
> @@ -391,15 +399,16 @@ void __init mount_block_root(char *name,
>  	char *fs_names = page_address(page);
>  	char *p;
>  	char b[BDEVNAME_SIZE];
> +	int num_fs, i;
>  
>  	scnprintf(b, BDEVNAME_SIZE, "unknown-block(%u,%u)",
>  		  MAJOR(ROOT_DEV), MINOR(ROOT_DEV));
>  	if (root_fs_names)
> -		split_fs_names(fs_names, root_fs_names);
> +		num_fs = split_fs_names(fs_names, root_fs_names);
>  	else
> -		list_bdev_fs_names(fs_names, PAGE_SIZE);
> +		num_fs = list_bdev_fs_names(fs_names, PAGE_SIZE);
>  retry:
> -	for (p = fs_names; *p; p += strlen(p)+1) {
> +	for (p = fs_names, i = 0; i < num_fs; p += strlen(p)+1, i++) {
>  		int err = do_mount_root(name, p, flags, root_mount_data);
>  		switch (err) {
>  			case 0:
> @@ -432,7 +441,7 @@ retry:
>  	printk("List of all partitions:\n");
>  	printk_all_partitions();
>  	printk("No filesystem could mount root, tried: ");
> -	for (p = fs_names; *p; p += strlen(p)+1)
> +	for (p = fs_names, i = 0; i < num_fs; p += strlen(p)+1, i++)
>  		printk(" %s", p);
>  	printk("\n");
>  	panic("VFS: Unable to mount root fs on %s", b);
> @@ -533,13 +542,15 @@ static int __init mount_nodev_root(void)
>  {
>  	char *fs_names, *fstype;
>  	int err = -EINVAL;
> +	int num_fs, i;
>  
>  	fs_names = (void *)__get_free_page(GFP_KERNEL);
>  	if (!fs_names)
>  		return -EINVAL;
> -	split_fs_names(fs_names, root_fs_names);
> +	num_fs = split_fs_names(fs_names, root_fs_names);
>  
> -	for (fstype = fs_names; *fstype; fstype += strlen(fstype) + 1) {
> +	for (fstype = fs_names, i = 0; i < num_fs;
> +	     fstype += strlen(fstype) + 1, i++) {
>  		if (!fs_is_nodev(fstype))
>  			continue;
>  		err = do_mount_root(root_device_name, fstype, root_mountflags,

Re: [Virtio-fs] [PATCH 3/2] fs: simplify get_filesystem_list / get_all_fs_names

[Christoph Hellwig]

On Mon, Jul 12, 2021 at 02:21:30PM -0400, Vivek Goyal wrote:
> In case you are finding it hard to spend some time on these patches, I 
> can take those patches, merge my changes and repost them.

Yes, please do.  Thanks a lot!

Re: [Virtio-fs] [PATCH 3/2] fs: simplify get_filesystem_list / get_all_fs_names

[Christoph Hellwig]

On Mon, Jul 12, 2021 at 02:21:30PM -0400, Vivek Goyal wrote:
> In case you are finding it hard to spend some time on these patches, I 
> can take those patches, merge my changes and repost them.

Yes, please do.  Thanks a lot!