linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* support booting of arbitrary non-blockdevice file systems
@ 2021-06-17 15:36 Christoph Hellwig
  2021-06-17 15:36 ` [PATCH 1/2] init: split get_fs_names Christoph Hellwig
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Christoph Hellwig @ 2021-06-17 15:36 UTC (permalink / raw)
  To: viro; +Cc: Vivek Goyal, linux-fsdevel, linux-kernel, virtio-fs

Hi all,

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/2] init: split get_fs_names
  2021-06-17 15:36 support booting of arbitrary non-blockdevice file systems Christoph Hellwig
@ 2021-06-17 15:36 ` Christoph Hellwig
  2021-06-17 15:36 ` [PATCH 2/2] init: allow mounting arbitrary non-blockdevice filesystems as root Christoph Hellwig
  2021-06-18 18:03 ` [Virtio-fs] support booting of arbitrary non-blockdevice file systems Stefan Hajnoczi
  2 siblings, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2021-06-17 15:36 UTC (permalink / raw)
  To: viro; +Cc: Vivek Goyal, linux-fsdevel, linux-kernel, virtio-fs

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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/2] init: allow mounting arbitrary non-blockdevice filesystems as root
  2021-06-17 15:36 support booting of arbitrary non-blockdevice file systems Christoph Hellwig
  2021-06-17 15:36 ` [PATCH 1/2] init: split get_fs_names Christoph Hellwig
@ 2021-06-17 15:36 ` Christoph Hellwig
  2021-06-17 16:26   ` Vivek Goyal
  2021-06-18 18:03 ` [Virtio-fs] support booting of arbitrary non-blockdevice file systems Stefan Hajnoczi
  2 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2021-06-17 15:36 UTC (permalink / raw)
  To: viro; +Cc: Vivek Goyal, linux-fsdevel, linux-kernel, virtio-fs

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 | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/init/do_mounts.c b/init/do_mounts.c
index ec32de3ad52b..64c60cb72ecb 100644
--- a/init/do_mounts.c
+++ b/init/do_mounts.c
@@ -534,6 +534,44 @@ static int __init mount_cifs_root(void)
 }
 #endif
 
+static int __init try_mount_nodev(char *fstype)
+{
+	struct file_system_type *fs = get_fs_type(fstype);
+	int err = -EINVAL;
+
+	if (!fs)
+		return -EINVAL;
+	if (!(fs->fs_flags & (FS_REQUIRES_DEV | FS_BINARY_MOUNTDATA)))
+		err = do_mount_root(root_device_name, fstype, root_mountflags,
+					root_mount_data);
+	put_filesystem(fs);
+
+	if (err != -EACCES && err != -EINVAL)
+		panic("VFS: Unable to mount root \"%s\" (%s), err=%d\n",
+			      root_device_name, fstype, err);
+	return err;
+}
+
+static int __init mount_nodev_root(void)
+{
+	char *fs_names, *p;
+	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 (p = fs_names; *p; p += strlen(p) + 1) {
+		err = try_mount_nodev(p);
+		if (!err)
+			break;
+	}
+
+	free_page((unsigned long)fs_names);
+	return err;
+}
+
 void __init mount_root(void)
 {
 #ifdef CONFIG_ROOT_NFS
@@ -550,6 +588,8 @@ void __init mount_root(void)
 		return;
 	}
 #endif
+	if (ROOT_DEV == 0 && mount_nodev_root() == 0)
+		return;
 #ifdef CONFIG_BLOCK
 	{
 		int err = create_dev("/dev/root", ROOT_DEV);
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] init: allow mounting arbitrary non-blockdevice filesystems as root
  2021-06-17 15:36 ` [PATCH 2/2] init: allow mounting arbitrary non-blockdevice filesystems as root Christoph Hellwig
@ 2021-06-17 16:26   ` Vivek Goyal
  2021-06-18 13:20     ` Christoph Hellwig
  0 siblings, 1 reply; 14+ messages in thread
From: Vivek Goyal @ 2021-06-17 16:26 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: viro, linux-fsdevel, linux-kernel, virtio-fs

On Thu, Jun 17, 2021 at 05:36:49PM +0200, Christoph Hellwig wrote:

[..]
> +static int __init try_mount_nodev(char *fstype)
> +{
> +	struct file_system_type *fs = get_fs_type(fstype);
> +	int err = -EINVAL;
> +
> +	if (!fs)
> +		return -EINVAL;
> +	if (!(fs->fs_flags & (FS_REQUIRES_DEV | FS_BINARY_MOUNTDATA)))

Not sure what FS_BINARY_MOUNTDATA is why fs should not have that set. nfs
seems to set it too. So that means they can't use try_mount_nodev().

> +		err = do_mount_root(root_device_name, fstype, root_mountflags,
> +					root_mount_data);
> +	put_filesystem(fs);
> +
> +	if (err != -EACCES && err != -EINVAL)

In case of success err == 0, but we still panic(). We will need to
check for success as well.
	if (err && err != -EACCES && err != -EINVAL)

> +		panic("VFS: Unable to mount root \"%s\" (%s), err=%d\n",
> +			      root_device_name, fstype, err);
> +	return err;
> +}
> +
> +static int __init mount_nodev_root(void)
> +{
> +	char *fs_names, *p;
> +	int err = -EINVAL;
> +
> +	fs_names = (void *)__get_free_page(GFP_KERNEL);
> +	if (!fs_names)
> +		return -EINVAL;
> +	split_fs_names(fs_names, root_fs_names);

root_fs_names can be NULL and it crashes with NULL pointer dereference.

Vivek

> +
> +	for (p = fs_names; *p; p += strlen(p) + 1) {
> +		err = try_mount_nodev(p);
> +		if (!err)
> +			break;
> +	}
> +
> +	free_page((unsigned long)fs_names);
> +	return err;
> +}
> +
>  void __init mount_root(void)
>  {
>  #ifdef CONFIG_ROOT_NFS
> @@ -550,6 +588,8 @@ void __init mount_root(void)
>  		return;
>  	}
>  #endif
> +	if (ROOT_DEV == 0 && mount_nodev_root() == 0)
> +		return;
>  #ifdef CONFIG_BLOCK
>  	{
>  		int err = create_dev("/dev/root", ROOT_DEV);
> -- 
> 2.30.2
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] init: allow mounting arbitrary non-blockdevice filesystems as root
  2021-06-17 16:26   ` Vivek Goyal
@ 2021-06-18 13:20     ` Christoph Hellwig
  2021-06-18 14:10       ` Vivek Goyal
  0 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2021-06-18 13:20 UTC (permalink / raw)
  To: Vivek Goyal
  Cc: Christoph Hellwig, viro, linux-fsdevel, linux-kernel, virtio-fs

On Thu, Jun 17, 2021 at 12:26:10PM -0400, Vivek Goyal wrote:
> Not sure what FS_BINARY_MOUNTDATA is why fs should not have that set. nfs
> seems to set it too. So that means they can't use try_mount_nodev().

We can't really pass actual binary mountdata using the string separation
scheme used by the rootfstype= option.  But given that NFS only uses
binary mountdata for legacy reasons and people get what they ask for
using the option I think we can drop the check.

> In case of success err == 0, but we still panic(). We will need to
> check for success as well.

Indeed.

> root_fs_names can be NULL and it crashes with NULL pointer dereference.

True.

What do you think of this version?

---
From 141caa79a619b5f5d100eeb8e94ecf8b3b1c9af7 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Fri, 18 Jun 2021 15:10:39 +0200
Subject: init: allow mounting arbitrary non-blockdevice filesystems as root

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..66c47193e9ee 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_fs_names) {
+		if (mount_nodev_root() == 0)
+			return;
+	}
 #ifdef CONFIG_BLOCK
 	{
 		int err = create_dev("/dev/root", ROOT_DEV);
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] init: allow mounting arbitrary non-blockdevice filesystems as root
  2021-06-18 13:20     ` Christoph Hellwig
@ 2021-06-18 14:10       ` Vivek Goyal
  0 siblings, 0 replies; 14+ messages in thread
From: Vivek Goyal @ 2021-06-18 14:10 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: viro, linux-fsdevel, linux-kernel, virtio-fs, Dominique Martinet

On Fri, Jun 18, 2021 at 03:20:38PM +0200, Christoph Hellwig wrote:
> On Thu, Jun 17, 2021 at 12:26:10PM -0400, Vivek Goyal wrote:
> > Not sure what FS_BINARY_MOUNTDATA is why fs should not have that set. nfs
> > seems to set it too. So that means they can't use try_mount_nodev().
> 
> We can't really pass actual binary mountdata using the string separation
> scheme used by the rootfstype= option.  But given that NFS only uses
> binary mountdata for legacy reasons and people get what they ask for
> using the option I think we can drop the check.
> 
> > In case of success err == 0, but we still panic(). We will need to
> > check for success as well.
> 
> Indeed.
> 
> > root_fs_names can be NULL and it crashes with NULL pointer dereference.
> 
> True.
> 
> What do you think of this version?

[ Cc Dominique Martinet ]

Hi Christoph,

This one works well for me. Just one minor nit. root_device_name, can
be NULL as well if user passes following.

"rootfstype=virtiofs rw".

And currently mount_nodev_root() is assuming root_device_name is not NULL
and we crash with null pointer dereference.

May be something like this.

        if (ROOT_DEV == 0 && root_device_name && root_fs_names) {
                if (mount_nodev_root() == 0)
                        return;
        }

Strangely people are using "rootfstype=virtiofs rw" to mount virtiofs
as rootfs. They give their filesystem a tag named "/dev/root". And
currently it works and they can mount virtiofs as rootfs.

With above change, current hackish way will also continue to work and
not break existing setups.

Thanks
Vivek

> 
> ---
> From 141caa79a619b5f5d100eeb8e94ecf8b3b1c9af7 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Fri, 18 Jun 2021 15:10:39 +0200
> Subject: init: allow mounting arbitrary non-blockdevice filesystems as root
> 
> 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..66c47193e9ee 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_fs_names) {
> +		if (mount_nodev_root() == 0)
> +			return;
> +	}
>  #ifdef CONFIG_BLOCK
>  	{
>  		int err = create_dev("/dev/root", ROOT_DEV);
> -- 
> 2.30.2
> 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [Virtio-fs] support booting of arbitrary non-blockdevice file systems
  2021-06-17 15:36 support booting of arbitrary non-blockdevice file systems Christoph Hellwig
  2021-06-17 15:36 ` [PATCH 1/2] init: split get_fs_names Christoph Hellwig
  2021-06-17 15:36 ` [PATCH 2/2] init: allow mounting arbitrary non-blockdevice filesystems as root Christoph Hellwig
@ 2021-06-18 18:03 ` Stefan Hajnoczi
  2 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2021-06-18 18:03 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: viro, linux-fsdevel, virtio-fs, linux-kernel, Vivek Goyal

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

On Thu, Jun 17, 2021 at 05:36:47PM +0200, Christoph Hellwig wrote:
> this series adds support to boot off arbitrary non-blockdevice root file
> systems, based off an earlier patch from Vivek.

Cool, thanks for working on generic syntax for mounting non-blockdevice
file systems. Looks good modulo the comments Vivek had.

Stefan

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] init: split get_fs_names
  2021-06-21 15:09   ` Matthew Wilcox
@ 2021-06-21 15:22     ` Christoph Hellwig
  0 siblings, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2021-06-21 15:22 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Christoph Hellwig, viro, Vivek Goyal, linux-fsdevel,
	linux-kernel, virtio-fs

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] init: split get_fs_names
  2021-06-21  6:26 ` [PATCH 1/2] init: split get_fs_names Christoph Hellwig
  2021-06-21 14:46   ` Al Viro
@ 2021-06-21 15:09   ` Matthew Wilcox
  2021-06-21 15:22     ` Christoph Hellwig
  1 sibling, 1 reply; 14+ messages in thread
From: Matthew Wilcox @ 2021-06-21 15:09 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: viro, Vivek Goyal, linux-fsdevel, linux-kernel, virtio-fs

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.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] init: split get_fs_names
  2021-06-21 14:53     ` Christoph Hellwig
@ 2021-06-21 14:59       ` Al Viro
  0 siblings, 0 replies; 14+ messages in thread
From: Al Viro @ 2021-06-21 14:59 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Vivek Goyal, linux-fsdevel, linux-kernel, virtio-fs

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] init: split get_fs_names
  2021-06-21 14:46   ` Al Viro
  2021-06-21 14:51     ` Al Viro
@ 2021-06-21 14:53     ` Christoph Hellwig
  2021-06-21 14:59       ` Al Viro
  1 sibling, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2021-06-21 14:53 UTC (permalink / raw)
  To: Al Viro
  Cc: Christoph Hellwig, Vivek Goyal, linux-fsdevel, linux-kernel, virtio-fs

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.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] init: split get_fs_names
  2021-06-21 14:46   ` Al Viro
@ 2021-06-21 14:51     ` Al Viro
  2021-06-21 14:53     ` Christoph Hellwig
  1 sibling, 0 replies; 14+ messages in thread
From: Al Viro @ 2021-06-21 14:51 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Vivek Goyal, linux-fsdevel, linux-kernel, virtio-fs

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] init: split get_fs_names
  2021-06-21  6:26 ` [PATCH 1/2] init: split get_fs_names Christoph Hellwig
@ 2021-06-21 14:46   ` Al Viro
  2021-06-21 14:51     ` Al Viro
  2021-06-21 14:53     ` Christoph Hellwig
  2021-06-21 15:09   ` Matthew Wilcox
  1 sibling, 2 replies; 14+ messages in thread
From: Al Viro @ 2021-06-21 14:46 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Vivek Goyal, linux-fsdevel, linux-kernel, virtio-fs

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/2] init: split get_fs_names
  2021-06-21  6:26 support booting of arbitrary non-blockdevice file systems v2 Christoph Hellwig
@ 2021-06-21  6:26 ` Christoph Hellwig
  2021-06-21 14:46   ` Al Viro
  2021-06-21 15:09   ` Matthew Wilcox
  0 siblings, 2 replies; 14+ messages in thread
From: Christoph Hellwig @ 2021-06-21  6:26 UTC (permalink / raw)
  To: viro; +Cc: Vivek Goyal, linux-fsdevel, linux-kernel, virtio-fs

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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-06-21 15:22 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-17 15:36 support booting of arbitrary non-blockdevice file systems Christoph Hellwig
2021-06-17 15:36 ` [PATCH 1/2] init: split get_fs_names Christoph Hellwig
2021-06-17 15:36 ` [PATCH 2/2] init: allow mounting arbitrary non-blockdevice filesystems as root Christoph Hellwig
2021-06-17 16:26   ` Vivek Goyal
2021-06-18 13:20     ` Christoph Hellwig
2021-06-18 14:10       ` Vivek Goyal
2021-06-18 18:03 ` [Virtio-fs] support booting of arbitrary non-blockdevice file systems Stefan Hajnoczi
2021-06-21  6:26 support booting of arbitrary non-blockdevice file systems v2 Christoph Hellwig
2021-06-21  6:26 ` [PATCH 1/2] init: split get_fs_names Christoph Hellwig
2021-06-21 14:46   ` Al Viro
2021-06-21 14:51     ` Al Viro
2021-06-21 14:53     ` Christoph Hellwig
2021-06-21 14:59       ` Al Viro
2021-06-21 15:09   ` Matthew Wilcox
2021-06-21 15:22     ` Christoph Hellwig

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).