[PATCH 0/3] use do_mount() instead of ksys_mount()

[PATCH 0/3] use do_mount() instead of ksys_mount()

[Dominik Brodowski]

This small series replaces all in-kernel calls to the
userspace-focused wrapper ksys_mount() with calls to
the kernel-centric do_mount().

For each replacement, one needs to verify that the first and
third parameter (char *dev_name, char *type) are strings
allocated in kernelspace and that the fifth parameter
(void *data) is either NULL or refers to a full page (only
occurence in init/do_mounts.c::do_mount_root()). The second
and fourth parameters (char *dir_name, unsigned long flags)
are passed by ksys_mount() to do_mount() unchanged, and
therefore do not require particular care.

---

@viro: is this something for you to take and push upstream
for the next cycle?


The same changes (on top of commit ae4b064e2a616b545acf02b8f50cc513b32c7522:

  Merge tag 'afs-fixes-20191211' of git://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs (2019-12-11 18:10:40 -0800)

are also available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/brodo/linux.git remove-ksys-mount

including all changes up to cccaa5e33525fc07f4a2ce0518e50b9ddf435e47:

  init: use do_mount() instead of ksys_mount() (2019-12-12 14:50:05 +0100)

----------------------------------------------------------------
Dominik Brodowski (3):
      devtmpfs: use do_mount() instead of ksys_mount()
      initrd: use do_mount() instead of ksys_mount()
      init: use do_mount() instead of ksys_mount()

 drivers/base/devtmpfs.c  |  6 +++---
 fs/namespace.c           | 10 ++--------
 include/linux/device.h   |  4 ++--
 include/linux/syscalls.h |  2 --
 init/do_mounts.c         | 30 +++++++++++++++++++++++-------
 init/do_mounts_initrd.c  |  6 +++---
 6 files changed, 33 insertions(+), 25 deletions(-)

Thanks,
	Dominik

-- 
2.24.1

[PATCH 1/3] devtmpfs: use do_mount() instead of ksys_mount()

[Dominik Brodowski]

In devtmpfs, do_mount() can be called directly instead of complex wrapping
by ksys_mount():
- the first and third arguments are const strings in the kernel,
  and do not need to be copied over from userspace;
- the fifth argument is NULL, and therefore no page needs to be
  copied over from userspace;
- the second and fourth argument are passed through anyway.

Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
---
 drivers/base/devtmpfs.c | 6 +++---
 include/linux/device.h  | 4 ++--
 init/do_mounts.c        | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
index 30d0523014e0..6cdbf1531238 100644
--- a/drivers/base/devtmpfs.c
+++ b/drivers/base/devtmpfs.c
@@ -359,7 +359,7 @@ static int handle_remove(const char *nodename, struct device *dev)
  * If configured, or requested by the commandline, devtmpfs will be
  * auto-mounted after the kernel mounted the root filesystem.
  */
-int devtmpfs_mount(const char *mntdir)
+int devtmpfs_mount(void)
 {
 	int err;
 
@@ -369,7 +369,7 @@ int devtmpfs_mount(const char *mntdir)
 	if (!thread)
 		return 0;
 
-	err = ksys_mount("devtmpfs", mntdir, "devtmpfs", MS_SILENT, NULL);
+	err = do_mount("devtmpfs", "dev", "devtmpfs", MS_SILENT, NULL);
 	if (err)
 		printk(KERN_INFO "devtmpfs: error mounting %i\n", err);
 	else
@@ -394,7 +394,7 @@ static int devtmpfsd(void *p)
 	*err = ksys_unshare(CLONE_NEWNS);
 	if (*err)
 		goto out;
-	*err = ksys_mount("devtmpfs", "/", "devtmpfs", MS_SILENT, NULL);
+	*err = do_mount("devtmpfs", "/", "devtmpfs", MS_SILENT, NULL);
 	if (*err)
 		goto out;
 	ksys_chdir("/.."); /* will traverse into overmounted root */
diff --git a/include/linux/device.h b/include/linux/device.h
index e226030c1df3..96ff76731e93 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1666,11 +1666,11 @@ extern bool kill_device(struct device *dev);
 #ifdef CONFIG_DEVTMPFS
 extern int devtmpfs_create_node(struct device *dev);
 extern int devtmpfs_delete_node(struct device *dev);
-extern int devtmpfs_mount(const char *mntdir);
+extern int devtmpfs_mount(void);
 #else
 static inline int devtmpfs_create_node(struct device *dev) { return 0; }
 static inline int devtmpfs_delete_node(struct device *dev) { return 0; }
-static inline int devtmpfs_mount(const char *mountpoint) { return 0; }
+static inline int devtmpfs_mount(void) { return 0; }
 #endif
 
 /* drivers/base/power/shutdown.c */
diff --git a/init/do_mounts.c b/init/do_mounts.c
index af9cda887a23..43f6d098c880 100644
--- a/init/do_mounts.c
+++ b/init/do_mounts.c
@@ -670,7 +670,7 @@ void __init prepare_namespace(void)
 
 	mount_root();
 out:
-	devtmpfs_mount("dev");
+	devtmpfs_mount();
 	ksys_mount(".", "/", NULL, MS_MOVE, NULL);
 	ksys_chroot(".");
 }
-- 
2.24.1

[PATCH 2/3] initrd: use do_mount() instead of ksys_mount()

[Dominik Brodowski]

All three calls to ksys_mount() in initrd-related kernel code can
be switched over to do_mount():
- the first and third arguments are const strings in the kernel,
  and do not need to be copied over from userspace;
- the fifth argument is NULL, and therefore no page needs to be,
  copied over from userspace;
- the second and fourth argument are passed through anyway.

Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
---
 init/do_mounts_initrd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/init/do_mounts_initrd.c b/init/do_mounts_initrd.c
index a9c6cc56f505..3bf7b74153ab 100644
--- a/init/do_mounts_initrd.c
+++ b/init/do_mounts_initrd.c
@@ -54,7 +54,7 @@ static int init_linuxrc(struct subprocess_info *info, struct cred *new)
 	ksys_dup(0);
 	/* move initrd over / and chdir/chroot in initrd root */
 	ksys_chdir("/root");
-	ksys_mount(".", "/", NULL, MS_MOVE, NULL);
+	do_mount(".", "/", NULL, MS_MOVE, NULL);
 	ksys_chroot(".");
 	ksys_setsid();
 	return 0;
@@ -89,7 +89,7 @@ static void __init handle_initrd(void)
 	current->flags &= ~PF_FREEZER_SKIP;
 
 	/* move initrd to rootfs' /old */
-	ksys_mount("..", ".", NULL, MS_MOVE, NULL);
+	do_mount("..", ".", NULL, MS_MOVE, NULL);
 	/* switch root and cwd back to / of rootfs */
 	ksys_chroot("..");
 
@@ -103,7 +103,7 @@ static void __init handle_initrd(void)
 	mount_root();
 
 	printk(KERN_NOTICE "Trying to move old root to /initrd ... ");
-	error = ksys_mount("/old", "/root/initrd", NULL, MS_MOVE, NULL);
+	error = do_mount("/old", "/root/initrd", NULL, MS_MOVE, NULL);
 	if (!error)
 		printk("okay\n");
 	else {
-- 
2.24.1

[PATCH 3/3] init: use do_mount() instead of ksys_mount()

[Dominik Brodowski]

In prepare_namespace(), do_mount() can be used instead of ksys_mount()
as the first and third argument are const strings in the kernel, the
second and fourth argument are passed through anyway, and the fifth
argument is NULL.

In do_mount_root(), ksys_mount() is called with the first and third
argument being already kernelspace strings, which do not need to be
copied over from userspace to kernelspace (again). The second and
fourth arguments are passed through to do_mount() anyway. The fifth
argument, while already residing in kernelspace, needs to be put into
a page of its own. Then, do_mount() can be used instead of
ksys_mount().

Once this is done, there are no in-kernel users to ksys_mount() left,
which can therefore be removed.

Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
---
 fs/namespace.c           | 10 ++--------
 include/linux/syscalls.h |  2 --
 init/do_mounts.c         | 28 ++++++++++++++++++++++------
 3 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 2fd0c8bcb8c1..be601d3a8008 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -3325,8 +3325,8 @@ struct dentry *mount_subtree(struct vfsmount *m, const char *name)
 }
 EXPORT_SYMBOL(mount_subtree);
 
-int ksys_mount(const char __user *dev_name, const char __user *dir_name,
-	       const char __user *type, unsigned long flags, void __user *data)
+SYSCALL_DEFINE5(mount, char __user *, dev_name, char __user *, dir_name,
+		char __user *, type, unsigned long, flags, void __user *, data)
 {
 	int ret;
 	char *kernel_type;
@@ -3359,12 +3359,6 @@ int ksys_mount(const char __user *dev_name, const char __user *dir_name,
 	return ret;
 }
 
-SYSCALL_DEFINE5(mount, char __user *, dev_name, char __user *, dir_name,
-		char __user *, type, unsigned long, flags, void __user *, data)
-{
-	return ksys_mount(dev_name, dir_name, type, flags, data);
-}
-
 /*
  * Create a kernel mount representation for a new, prepared superblock
  * (specified by fs_fd) and attach to an open_tree-like file descriptor.
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index d0391cc2dae9..5262b7a76d39 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -1231,8 +1231,6 @@ asmlinkage long sys_ni_syscall(void);
  * the ksys_xyzyyz() functions prototyped below.
  */
 
-int ksys_mount(const char __user *dev_name, const char __user *dir_name,
-	       const char __user *type, unsigned long flags, void __user *data);
 int ksys_umount(char __user *name, int flags);
 int ksys_dup(unsigned int fildes);
 int ksys_chroot(const char __user *filename);
diff --git a/init/do_mounts.c b/init/do_mounts.c
index 43f6d098c880..f55cbd9cb818 100644
--- a/init/do_mounts.c
+++ b/init/do_mounts.c
@@ -387,12 +387,25 @@ static void __init get_fs_names(char *page)
 	*s = '\0';
 }
 
-static int __init do_mount_root(char *name, char *fs, int flags, void *data)
+static int __init do_mount_root(const char *name, const char *fs,
+				 const int flags, const void *data)
 {
 	struct super_block *s;
-	int err = ksys_mount(name, "/root", fs, flags, data);
-	if (err)
-		return err;
+	char *data_page;
+	struct page *p;
+	int ret;
+
+	/* do_mount() requires a full page as fifth argument */
+	p = alloc_page(GFP_KERNEL);
+	if (!p)
+		return -ENOMEM;
+
+	data_page = page_address(p);
+	strncpy(data_page, data, PAGE_SIZE - 1);
+
+	ret = do_mount(name, "/root", fs, flags, data_page);
+	if (ret)
+		goto out;
 
 	ksys_chdir("/root");
 	s = current->fs->pwd.dentry->d_sb;
@@ -402,7 +415,10 @@ static int __init do_mount_root(char *name, char *fs, int flags, void *data)
 	       s->s_type->name,
 	       sb_rdonly(s) ? " readonly" : "",
 	       MAJOR(ROOT_DEV), MINOR(ROOT_DEV));
-	return 0;
+
+out:
+	put_page(p);
+	return ret;
 }
 
 void __init mount_block_root(char *name, int flags)
@@ -671,7 +687,7 @@ void __init prepare_namespace(void)
 	mount_root();
 out:
 	devtmpfs_mount();
-	ksys_mount(".", "/", NULL, MS_MOVE, NULL);
+	do_mount(".", "/", NULL, MS_MOVE, NULL);
 	ksys_chroot(".");
 }
 
-- 
2.24.1

Re: [PATCH 3/3] init: use do_mount() instead of ksys_mount()

[Ondřej Jirman]

Hello,

On Thu, Dec 12, 2019 at 02:57:24PM +0100, Dominik Brodowski wrote:
> In prepare_namespace(), do_mount() can be used instead of ksys_mount()
> as the first and third argument are const strings in the kernel, the
> second and fourth argument are passed through anyway, and the fifth
> argument is NULL.
> 
> In do_mount_root(), ksys_mount() is called with the first and third
> argument being already kernelspace strings, which do not need to be
> copied over from userspace to kernelspace (again). The second and
> fourth arguments are passed through to do_mount() anyway. The fifth
> argument, while already residing in kernelspace, needs to be put into
> a page of its own. Then, do_mount() can be used instead of
> ksys_mount().
> 
> Once this is done, there are no in-kernel users to ksys_mount() left,
> which can therefore be removed.
> 
> Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
> ---
>  fs/namespace.c           | 10 ++--------
>  include/linux/syscalls.h |  2 --
>  init/do_mounts.c         | 28 ++++++++++++++++++++++------
>  3 files changed, 24 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 2fd0c8bcb8c1..be601d3a8008 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -3325,8 +3325,8 @@ struct dentry *mount_subtree(struct vfsmount *m, const char *name)
>  }
>  EXPORT_SYMBOL(mount_subtree);
>  
> -int ksys_mount(const char __user *dev_name, const char __user *dir_name,
> -	       const char __user *type, unsigned long flags, void __user *data)
> +SYSCALL_DEFINE5(mount, char __user *, dev_name, char __user *, dir_name,
> +		char __user *, type, unsigned long, flags, void __user *, data)
>  {
>  	int ret;
>  	char *kernel_type;
> @@ -3359,12 +3359,6 @@ int ksys_mount(const char __user *dev_name, const char __user *dir_name,
>  	return ret;
>  }
>  
> -SYSCALL_DEFINE5(mount, char __user *, dev_name, char __user *, dir_name,
> -		char __user *, type, unsigned long, flags, void __user *, data)
> -{
> -	return ksys_mount(dev_name, dir_name, type, flags, data);
> -}
> -
>  /*
>   * Create a kernel mount representation for a new, prepared superblock
>   * (specified by fs_fd) and attach to an open_tree-like file descriptor.
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index d0391cc2dae9..5262b7a76d39 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -1231,8 +1231,6 @@ asmlinkage long sys_ni_syscall(void);
>   * the ksys_xyzyyz() functions prototyped below.
>   */
>  
> -int ksys_mount(const char __user *dev_name, const char __user *dir_name,
> -	       const char __user *type, unsigned long flags, void __user *data);
>  int ksys_umount(char __user *name, int flags);
>  int ksys_dup(unsigned int fildes);
>  int ksys_chroot(const char __user *filename);
> diff --git a/init/do_mounts.c b/init/do_mounts.c
> index 43f6d098c880..f55cbd9cb818 100644
> --- a/init/do_mounts.c
> +++ b/init/do_mounts.c
> @@ -387,12 +387,25 @@ static void __init get_fs_names(char *page)
>  	*s = '\0';
>  }
>  
> -static int __init do_mount_root(char *name, char *fs, int flags, void *data)
> +static int __init do_mount_root(const char *name, const char *fs,
> +				 const int flags, const void *data)
>  {
>  	struct super_block *s;
> -	int err = ksys_mount(name, "/root", fs, flags, data);
> -	if (err)
> -		return err;
> +	char *data_page;
> +	struct page *p;
> +	int ret;
> +
> +	/* do_mount() requires a full page as fifth argument */
> +	p = alloc_page(GFP_KERNEL);
> +	if (!p)
> +		return -ENOMEM;
> +
> +	data_page = page_address(p);
> +	strncpy(data_page, data, PAGE_SIZE - 1);

I tried 5.2-rc2 and I get kernel OOPS/panic here (do_mount_root gets inlined
into mount_block_root):

Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
Mem abort info:
  ESR = 0x96000005
  EC = 0x25: DABT (current EL), IL = 32 bits
  SET = 0, FnV = 0
  EA = 0, S1PTW = 0
Data abort info:
  ISV = 0, ISS = 0x00000005
  CM = 0, WnR = 0
[0000000000000000] user address but active_mm is swapper
Internal error: Oops: 96000005 [#1] SMP
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.5.0-rc2-00128-ge93c94cc04ae #5
Hardware name: OrangePi 3 (DT)
pstate: 80000005 (Nzcv daif -PAN -UAO)
pc : strncpy+0x10/0x30
lr : do_mount_root+0x70/0x114
sp : ffffffc01002bd20
x29: ffffffc01002bd20 x28: 0000000000000000
x27: 0000000000000000 x26: ffffffc011400468
x25: ffffffc0110d23a0 x24: ffffffff01c396c0
x23: 0000000000008000 x22: ffffff8078e5b000
x21: ffffffc0110d23a0 x20: ffffffc0110d2220
x19: ffffffff01c39700 x18: 00000000fffffffe
x17: 00000000dbaba8be x16: 0000000000000000
x15: ffffffffffffffff x14: ffffff0000000000
x13: ffffff807b490000 x12: 0000000000000000
x11: 0000000000000000 x10: ffffff807bb96120
x9 : 0000000000000000 x8 : 0000000000000000
x7 : ffffffc06a6e6000 x6 : 0000000000000000
x5 : 000000000003b820 x4 : ffffff8078e5cfff
x3 : 0000000000000201 x2 : ffffff8078e5c000
x1 : 0000000000000000 x0 : ffffff8078e5c000
Call trace:
 strncpy+0x10/0x30
 mount_block_root+0x100/0x224
 mount_root+0x10c/0x124
 prepare_namespace+0x12c/0x168
 kernel_init_freeable+0x214/0x258
 kernel_init+0x10/0xfc
 ret_from_fork+0x10/0x1c
Code: b4000142 8b020004 aa0003e2 d503201f (39400023)
---[ end trace b72d58d1ea940426 ]---
Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
SMP: stopping secondary CPUs
Kernel Offset: disabled
CPU features: 0x00002,20002000
Memory Limit: none
Rebooting in 3 seconds..

regards,
	o.

> +	ret = do_mount(name, "/root", fs, flags, data_page);
> +	if (ret)
> +		goto out;
>  
>  	ksys_chdir("/root");
>  	s = current->fs->pwd.dentry->d_sb;
> @@ -402,7 +415,10 @@ static int __init do_mount_root(char *name, char *fs, int flags, void *data)
>  	       s->s_type->name,
>  	       sb_rdonly(s) ? " readonly" : "",
>  	       MAJOR(ROOT_DEV), MINOR(ROOT_DEV));
> -	return 0;
> +
> +out:
> +	put_page(p);
> +	return ret;
>  }
>  
>  void __init mount_block_root(char *name, int flags)
> @@ -671,7 +687,7 @@ void __init prepare_namespace(void)
>  	mount_root();
>  out:
>  	devtmpfs_mount();
> -	ksys_mount(".", "/", NULL, MS_MOVE, NULL);
> +	do_mount(".", "/", NULL, MS_MOVE, NULL);
>  	ksys_chroot(".");
>  }
>  
> -- 
> 2.24.1
> 

Re: [PATCH 3/3] init: use do_mount() instead of ksys_mount()

[Ondřej Jirman]

On Mon, Dec 16, 2019 at 02:35:36AM +0100, megi xff wrote:
> Hello,
> 
> On Thu, Dec 12, 2019 at 02:57:24PM +0100, Dominik Brodowski wrote:
> > In prepare_namespace(), do_mount() can be used instead of ksys_mount()
> > as the first and third argument are const strings in the kernel, the
> > second and fourth argument are passed through anyway, and the fifth
> > argument is NULL.
> > 
> > In do_mount_root(), ksys_mount() is called with the first and third
> > argument being already kernelspace strings, which do not need to be
> > copied over from userspace to kernelspace (again). The second and
> > fourth arguments are passed through to do_mount() anyway. The fifth
> > argument, while already residing in kernelspace, needs to be put into
> > a page of its own. Then, do_mount() can be used instead of
> > ksys_mount().
> > 
> > Once this is done, there are no in-kernel users to ksys_mount() left,
> > which can therefore be removed.
> > 
> > Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
> > ---
> >  fs/namespace.c           | 10 ++--------
> >  include/linux/syscalls.h |  2 --
> >  init/do_mounts.c         | 28 ++++++++++++++++++++++------
> >  3 files changed, 24 insertions(+), 16 deletions(-)
> > 
> > diff --git a/fs/namespace.c b/fs/namespace.c
> > index 2fd0c8bcb8c1..be601d3a8008 100644
> > --- a/fs/namespace.c
> > +++ b/fs/namespace.c
> > @@ -3325,8 +3325,8 @@ struct dentry *mount_subtree(struct vfsmount *m, const char *name)
> >  }
> >  EXPORT_SYMBOL(mount_subtree);
> >  
> > -int ksys_mount(const char __user *dev_name, const char __user *dir_name,
> > -	       const char __user *type, unsigned long flags, void __user *data)
> > +SYSCALL_DEFINE5(mount, char __user *, dev_name, char __user *, dir_name,
> > +		char __user *, type, unsigned long, flags, void __user *, data)
> >  {
> >  	int ret;
> >  	char *kernel_type;
> > @@ -3359,12 +3359,6 @@ int ksys_mount(const char __user *dev_name, const char __user *dir_name,
> >  	return ret;
> >  }
> >  
> > -SYSCALL_DEFINE5(mount, char __user *, dev_name, char __user *, dir_name,
> > -		char __user *, type, unsigned long, flags, void __user *, data)
> > -{
> > -	return ksys_mount(dev_name, dir_name, type, flags, data);
> > -}
> > -
> >  /*
> >   * Create a kernel mount representation for a new, prepared superblock
> >   * (specified by fs_fd) and attach to an open_tree-like file descriptor.
> > diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> > index d0391cc2dae9..5262b7a76d39 100644
> > --- a/include/linux/syscalls.h
> > +++ b/include/linux/syscalls.h
> > @@ -1231,8 +1231,6 @@ asmlinkage long sys_ni_syscall(void);
> >   * the ksys_xyzyyz() functions prototyped below.
> >   */
> >  
> > -int ksys_mount(const char __user *dev_name, const char __user *dir_name,
> > -	       const char __user *type, unsigned long flags, void __user *data);
> >  int ksys_umount(char __user *name, int flags);
> >  int ksys_dup(unsigned int fildes);
> >  int ksys_chroot(const char __user *filename);
> > diff --git a/init/do_mounts.c b/init/do_mounts.c
> > index 43f6d098c880..f55cbd9cb818 100644
> > --- a/init/do_mounts.c
> > +++ b/init/do_mounts.c
> > @@ -387,12 +387,25 @@ static void __init get_fs_names(char *page)
> >  	*s = '\0';
> >  }
> >  
> > -static int __init do_mount_root(char *name, char *fs, int flags, void *data)
> > +static int __init do_mount_root(const char *name, const char *fs,
> > +				 const int flags, const void *data)
> >  {
> >  	struct super_block *s;
> > -	int err = ksys_mount(name, "/root", fs, flags, data);
> > -	if (err)
> > -		return err;
> > +	char *data_page;
> > +	struct page *p;
> > +	int ret;
> > +
> > +	/* do_mount() requires a full page as fifth argument */
> > +	p = alloc_page(GFP_KERNEL);
> > +	if (!p)
> > +		return -ENOMEM;
> > +
> > +	data_page = page_address(p);
> > +	strncpy(data_page, data, PAGE_SIZE - 1);
> 
> I tried 5.2-rc2 and I get kernel OOPS/panic here (do_mount_root gets inlined
> into mount_block_root):

5.5-rc2 of course. :)

> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> Mem abort info:
>   ESR = 0x96000005
>   EC = 0x25: DABT (current EL), IL = 32 bits
>   SET = 0, FnV = 0
>   EA = 0, S1PTW = 0
> Data abort info:
>   ISV = 0, ISS = 0x00000005
>   CM = 0, WnR = 0
> [0000000000000000] user address but active_mm is swapper
> Internal error: Oops: 96000005 [#1] SMP
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.5.0-rc2-00128-ge93c94cc04ae #5
> Hardware name: OrangePi 3 (DT)
> pstate: 80000005 (Nzcv daif -PAN -UAO)
> pc : strncpy+0x10/0x30
> lr : do_mount_root+0x70/0x114
> sp : ffffffc01002bd20
> x29: ffffffc01002bd20 x28: 0000000000000000
> x27: 0000000000000000 x26: ffffffc011400468
> x25: ffffffc0110d23a0 x24: ffffffff01c396c0
> x23: 0000000000008000 x22: ffffff8078e5b000
> x21: ffffffc0110d23a0 x20: ffffffc0110d2220
> x19: ffffffff01c39700 x18: 00000000fffffffe
> x17: 00000000dbaba8be x16: 0000000000000000
> x15: ffffffffffffffff x14: ffffff0000000000
> x13: ffffff807b490000 x12: 0000000000000000
> x11: 0000000000000000 x10: ffffff807bb96120
> x9 : 0000000000000000 x8 : 0000000000000000
> x7 : ffffffc06a6e6000 x6 : 0000000000000000
> x5 : 000000000003b820 x4 : ffffff8078e5cfff
> x3 : 0000000000000201 x2 : ffffff8078e5c000
> x1 : 0000000000000000 x0 : ffffff8078e5c000
> Call trace:
>  strncpy+0x10/0x30
>  mount_block_root+0x100/0x224
>  mount_root+0x10c/0x124
>  prepare_namespace+0x12c/0x168
>  kernel_init_freeable+0x214/0x258
>  kernel_init+0x10/0xfc
>  ret_from_fork+0x10/0x1c
> Code: b4000142 8b020004 aa0003e2 d503201f (39400023)
> ---[ end trace b72d58d1ea940426 ]---
> Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
> SMP: stopping secondary CPUs
> Kernel Offset: disabled
> CPU features: 0x00002,20002000
> Memory Limit: none
> Rebooting in 3 seconds..
> 
> regards,
> 	o.
> 
> > +	ret = do_mount(name, "/root", fs, flags, data_page);
> > +	if (ret)
> > +		goto out;
> >  
> >  	ksys_chdir("/root");
> >  	s = current->fs->pwd.dentry->d_sb;
> > @@ -402,7 +415,10 @@ static int __init do_mount_root(char *name, char *fs, int flags, void *data)
> >  	       s->s_type->name,
> >  	       sb_rdonly(s) ? " readonly" : "",
> >  	       MAJOR(ROOT_DEV), MINOR(ROOT_DEV));
> > -	return 0;
> > +
> > +out:
> > +	put_page(p);
> > +	return ret;
> >  }
> >  
> >  void __init mount_block_root(char *name, int flags)
> > @@ -671,7 +687,7 @@ void __init prepare_namespace(void)
> >  	mount_root();
> >  out:
> >  	devtmpfs_mount();
> > -	ksys_mount(".", "/", NULL, MS_MOVE, NULL);
> > +	do_mount(".", "/", NULL, MS_MOVE, NULL);
> >  	ksys_chroot(".");
> >  }
> >  
> > -- 
> > 2.24.1
> > 

Re: [PATCH 3/3] init: use do_mount() instead of ksys_mount()

[Linus Torvalds]

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

On Sun, Dec 15, 2019 at 5:35 PM Ondřej Jirman <megi@xff.cz> wrote:
>
> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000

Duh. So much for the trivial obvious conversion.

It didn't take "data might be NULL" into account.

A patch like this, perhaps? Untested..

               Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1201 bytes --]

 init/do_mounts.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/init/do_mounts.c b/init/do_mounts.c
index f55cbd9cb818..d204f605dbce 100644
--- a/init/do_mounts.c
+++ b/init/do_mounts.c
@@ -391,17 +391,19 @@ static int __init do_mount_root(const char *name, const char *fs,
 				 const int flags, const void *data)
 {
 	struct super_block *s;
-	char *data_page;
-	struct page *p;
+	struct page *p = NULL;
+	char *data_page = NULL;
 	int ret;
 
-	/* do_mount() requires a full page as fifth argument */
-	p = alloc_page(GFP_KERNEL);
-	if (!p)
-		return -ENOMEM;
-
-	data_page = page_address(p);
-	strncpy(data_page, data, PAGE_SIZE - 1);
+	if (data) {
+		/* do_mount() requires a full page as fifth argument */
+		p = alloc_page(GFP_KERNEL);
+		if (!p)
+			return -ENOMEM;
+		data_page = page_address(p);
+		strncpy(data_page, data, PAGE_SIZE - 1);
+		data_page[PAGE_SIZE - 1] = '\0';
+	}
 
 	ret = do_mount(name, "/root", fs, flags, data_page);
 	if (ret)
@@ -417,7 +419,8 @@ static int __init do_mount_root(const char *name, const char *fs,
 	       MAJOR(ROOT_DEV), MINOR(ROOT_DEV));
 
 out:
-	put_page(p);
+	if (p)
+		put_page(p);
 	return ret;
 }
 

Re: [PATCH 3/3] init: use do_mount() instead of ksys_mount()

[Eric Biggers]

On Sun, Dec 15, 2019 at 07:50:23PM -0800, Linus Torvalds wrote:
> On Sun, Dec 15, 2019 at 5:35 PM Ondřej Jirman <megi@xff.cz> wrote:
> >
> > Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> 
> Duh. So much for the trivial obvious conversion.
> 
> It didn't take "data might be NULL" into account.
> 
> A patch like this, perhaps? Untested..
> 
>                Linus

>  init/do_mounts.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/init/do_mounts.c b/init/do_mounts.c
> index f55cbd9cb818..d204f605dbce 100644
> --- a/init/do_mounts.c
> +++ b/init/do_mounts.c
> @@ -391,17 +391,19 @@ static int __init do_mount_root(const char *name, const char *fs,
>  				 const int flags, const void *data)
>  {
>  	struct super_block *s;
> -	char *data_page;
> -	struct page *p;
> +	struct page *p = NULL;
> +	char *data_page = NULL;
>  	int ret;
>  
> -	/* do_mount() requires a full page as fifth argument */
> -	p = alloc_page(GFP_KERNEL);
> -	if (!p)
> -		return -ENOMEM;
> -
> -	data_page = page_address(p);
> -	strncpy(data_page, data, PAGE_SIZE - 1);
> +	if (data) {
> +		/* do_mount() requires a full page as fifth argument */
> +		p = alloc_page(GFP_KERNEL);
> +		if (!p)
> +			return -ENOMEM;
> +		data_page = page_address(p);
> +		strncpy(data_page, data, PAGE_SIZE - 1);
> +		data_page[PAGE_SIZE - 1] = '\0';
> +	}
>  
>  	ret = do_mount(name, "/root", fs, flags, data_page);
>  	if (ret)
> @@ -417,7 +419,8 @@ static int __init do_mount_root(const char *name, const char *fs,
>  	       MAJOR(ROOT_DEV), MINOR(ROOT_DEV));
>  
>  out:
> -	put_page(p);
> +	if (p)
> +		put_page(p);
>  	return ret;

I'm seeing the boot crash too, and Linus' patch fixes it.

Note that adding the line

		data_page[PAGE_SIZE - 1] = '\0';

is not necessary since do_mount() already does it.

- Eric

Re: [PATCH 3/3] init: use do_mount() instead of ksys_mount()

[Geert Uytterhoeven]

Hi all,

On Mon, Dec 16, 2019 at 6:13 AM Eric Biggers <ebiggers@kernel.org> wrote:
> On Sun, Dec 15, 2019 at 07:50:23PM -0800, Linus Torvalds wrote:
> > On Sun, Dec 15, 2019 at 5:35 PM Ondřej Jirman <megi@xff.cz> wrote:
> > > Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> >
> > Duh. So much for the trivial obvious conversion.
> >
> > It didn't take "data might be NULL" into account.
> >
> > A patch like this, perhaps? Untested..

> > --- a/init/do_mounts.c
> > +++ b/init/do_mounts.c
> > @@ -391,17 +391,19 @@ static int __init do_mount_root(const char *name, const char *fs,
> >                                const int flags, const void *data)
> >  {
> >       struct super_block *s;
> > -     char *data_page;
> > -     struct page *p;
> > +     struct page *p = NULL;
> > +     char *data_page = NULL;
> >       int ret;
> >
> > -     /* do_mount() requires a full page as fifth argument */
> > -     p = alloc_page(GFP_KERNEL);
> > -     if (!p)
> > -             return -ENOMEM;
> > -
> > -     data_page = page_address(p);
> > -     strncpy(data_page, data, PAGE_SIZE - 1);
> > +     if (data) {
> > +             /* do_mount() requires a full page as fifth argument */
> > +             p = alloc_page(GFP_KERNEL);
> > +             if (!p)
> > +                     return -ENOMEM;
> > +             data_page = page_address(p);
> > +             strncpy(data_page, data, PAGE_SIZE - 1);
> > +             data_page[PAGE_SIZE - 1] = '\0';
> > +     }
> >
> >       ret = do_mount(name, "/root", fs, flags, data_page);
> >       if (ret)
> > @@ -417,7 +419,8 @@ static int __init do_mount_root(const char *name, const char *fs,
> >              MAJOR(ROOT_DEV), MINOR(ROOT_DEV));
> >
> >  out:
> > -     put_page(p);
> > +     if (p)
> > +             put_page(p);
> >       return ret;
>
> I'm seeing the boot crash too, and Linus' patch fixes it.

#metoo ;-)

Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>

> Note that adding the line
>
>                 data_page[PAGE_SIZE - 1] = '\0';
>
> is not necessary since do_mount() already does it.

Indeed. So the "-1" can be dropped from the strncpy() call, which brings
it even more in-line with the "full page" comment.

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/3] init: use do_mount() instead of ksys_mount()

[Guenter Roeck]

On Sun, Dec 15, 2019 at 07:50:23PM -0800, Linus Torvalds wrote:
> On Sun, Dec 15, 2019 at 5:35 PM Ondřej Jirman <megi@xff.cz> wrote:
> >
> > Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> 
> Duh. So much for the trivial obvious conversion.
> 
> It didn't take "data might be NULL" into account.
> 
> A patch like this, perhaps? Untested..
> 
>                Linus

That one must be a record.

Qemu test results:
	total: 387 pass: 93 fail: 294

For my education, what is so bad with calls like ksys_mount() that warrants
the extra code needed to remove it (and, of course, the resulting breakage) ?

Thanks,
Guenter

Re: [PATCH 3/3] init: use do_mount() instead of ksys_mount()

[Guido Günther]

Hi,
On Sun, Dec 15, 2019 at 07:50:23PM -0800, Linus Torvalds wrote:
> On Sun, Dec 15, 2019 at 5:35 PM Ondřej Jirman <megi@xff.cz> wrote:
> >
> > Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> 
> Duh. So much for the trivial obvious conversion.
> 
> It didn't take "data might be NULL" into account.
> 
> A patch like this, perhaps? Untested..
> 
>                Linus

>  init/do_mounts.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/init/do_mounts.c b/init/do_mounts.c
> index f55cbd9cb818..d204f605dbce 100644
> --- a/init/do_mounts.c
> +++ b/init/do_mounts.c
> @@ -391,17 +391,19 @@ static int __init do_mount_root(const char *name, const char *fs,
>  				 const int flags, const void *data)
>  {
>  	struct super_block *s;
> -	char *data_page;
> -	struct page *p;
> +	struct page *p = NULL;
> +	char *data_page = NULL;
n>  	int ret;
>  
> -	/* do_mount() requires a full page as fifth argument */
> -	p = alloc_page(GFP_KERNEL);
> -	if (!p)
> -		return -ENOMEM;
> -
> -	data_page = page_address(p);
> -	strncpy(data_page, data, PAGE_SIZE - 1);
> +	if (data) {
> +		/* do_mount() requires a full page as fifth argument */
> +		p = alloc_page(GFP_KERNEL);
> +		if (!p)
> +			return -ENOMEM;
> +		data_page = page_address(p);
> +		strncpy(data_page, data, PAGE_SIZE - 1);
> +		data_page[PAGE_SIZE - 1] = '\0';
> +	}
>  
>  	ret = do_mount(name, "/root", fs, flags, data_page);
>  	if (ret)
> @@ -417,7 +419,8 @@ static int __init do_mount_root(const char *name, const char *fs,
>  	       MAJOR(ROOT_DEV), MINOR(ROOT_DEV));
>  
>  out:
> -	put_page(p);
> +	if (p)
> +		put_page(p);
>  	return ret;
>  }
>  

This unbroke tftpboot on arm64 on next-20191216 for me as well:

Tested-by: Guido Günther <agx@sigxcpu.org>

Cheers,
 -- Guido

Re: [PATCH 3/3] init: use do_mount() instead of ksys_mount()

[Nick Desaulniers]

Shouldn't patches bake for a while in -next? (That way we catch regressions
before they hit mainline?)

This lit up our CI this morning.

https://travis-ci.com/ClangBuiltLinux/continuous-integration/builds

(Apologies for missing context, replying via lore.kernel.org directions.)
https://lore.kernel.org/lkml/20191212135724.331342-4-linux@dominikbrodowski.net/

Re: [PATCH 3/3] init: use do_mount() instead of ksys_mount()

[Dominik Brodowski]

On Mon, Dec 16, 2019 at 01:12:28PM -0800, Nick Desaulniers wrote:
> Shouldn't patches bake for a while in -next? (That way we catch regressions
> before they hit mainline?)
> 
> This lit up our CI this morning.
> 
> https://travis-ci.com/ClangBuiltLinux/continuous-integration/builds
> 
> (Apologies for missing context, replying via lore.kernel.org directions.)
> https://lore.kernel.org/lkml/20191212135724.331342-4-linux@dominikbrodowski.net/

A fix for this issue is already upstream, 7de7de7ca0ae .

Thanks,
	Dominik

Re: [PATCH 3/3] init: use do_mount() instead of ksys_mount()

[Nick Desaulniers]

On Mon, Dec 16, 2019 at 10:43 PM Dominik Brodowski
<linux@dominikbrodowski.net> wrote:
>
> On Mon, Dec 16, 2019 at 01:12:28PM -0800, Nick Desaulniers wrote:
> > Shouldn't patches bake for a while in -next? (That way we catch regressions
> > before they hit mainline?)
> >
> > This lit up our CI this morning.
> >
> > https://travis-ci.com/ClangBuiltLinux/continuous-integration/builds
> >
> > (Apologies for missing context, replying via lore.kernel.org directions.)
> > https://lore.kernel.org/lkml/20191212135724.331342-4-linux@dominikbrodowski.net/
>
> A fix for this issue is already upstream, 7de7de7ca0ae .

I appreciate that.  I was just surprised to have no advanced notice;
-next is our "canary in the coalmine."  Mainline is usually pretty
stable; if it goes red then we're missing testing coverage somewhere.
For mainline to break suddenly implies that either a pull was merged
from a branch that wasn't flowing into -next, or someone got [un]lucky
with the merge window, or something worse.  I saw -next failed the
same day as mainline for the same reason, so I'm going to give you the
benefit of the doubt and chalk it up to luck with timing of the merge
window.

In the future, please give patches more time to soak in -next. We
expect -next to be noisy, mainline not so much.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 0/3] use do_mount() instead of ksys_mount()

[Linus Torvalds]

On Thu, Dec 12, 2019 at 5:59 AM Dominik Brodowski
<linux@dominikbrodowski.net> wrote:
>
> This small series replaces all in-kernel calls to the
> userspace-focused wrapper ksys_mount() with calls to
> the kernel-centric do_mount().

If you fix the pointless cast I pointed out, and put it in a git
branch, I'll pull it.

In fact, just do both series in the same branch, they do conceptually
the same thing, after all.

             Linus

Re: [PATCH 0/3] use do_mount() instead of ksys_mount()

[Al Viro]

On Thu, Dec 12, 2019 at 09:11:10AM -0800, Linus Torvalds wrote:
> On Thu, Dec 12, 2019 at 5:59 AM Dominik Brodowski
> <linux@dominikbrodowski.net> wrote:
> >
> > This small series replaces all in-kernel calls to the
> > userspace-focused wrapper ksys_mount() with calls to
> > the kernel-centric do_mount().
> 
> If you fix the pointless cast I pointed out, and put it in a git
> branch, I'll pull it.
> 
> In fact, just do both series in the same branch, they do conceptually
> the same thing, after all.

IMO it's not a good idea.  Exposing the guts of fs/namespace.c to
what's essentially a userland code that happens to run in kernel thread
is asking for trouble - we'd been there and it had been hell to untangle.