NACK: [B][PATCH 2/2] unfuck sysfs_mount()

[Thadeu Lima de Souza Cascardo]

On Wed, Jun 30, 2021 at 04:00:01PM -0300, Guilherme G. Piccoli wrote:
> From: Al Viro <viro at zeniv.linux.org.uk>
> 
> BugLink: https://bugs.launchpad.net/bugs/1934175
> 
> new_sb is left uninitialized in case of early failures in kernfs_mount_ns(),
> and while IS_ERR(root) is true in all such cases, using IS_ERR(root) || !new_sb
> is not a solution - IS_ERR(root) is true in some cases when new_sb is true.
> 
> Make sure new_sb is initialized (and matches the reality) in all cases and
> fix the condition for dropping kobj reference - we want it done precisely
> in those situations where the reference has not been transferred into a new
> super_block instance.
> 
> Signed-off-by: Al Viro <viro at zeniv.linux.org.uk>
> (cherry picked from commit 7b745a4e4051e1bbce40e0b1c2cf636c70583aa4)
> Signed-off-by: Guilherme G. Piccoli <gpiccoli at canonical.com>
> ---
>  fs/sysfs/mount.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/sysfs/mount.c b/fs/sysfs/mount.c
> index fb49510c5dcf..88b388415d0e 100644
> --- a/fs/sysfs/mount.c
> +++ b/fs/sysfs/mount.c
> @@ -28,7 +28,7 @@ static struct dentry *sysfs_mount(struct file_system_type *fs_type,
>  {
>  	struct dentry *root;
>  	void *ns;
> -	bool new_sb;
> +	bool new_sb = false;
>  
>  	if (!(flags & SB_KERNMOUNT)) {
>  		if (!kobj_ns_current_may_mount(KOBJ_NS_TYPE_NET))
> @@ -38,9 +38,9 @@ static struct dentry *sysfs_mount(struct file_system_type *fs_type,
>  	ns = kobj_ns_grab_current(KOBJ_NS_TYPE_NET);
>  	root = kernfs_mount_ns(fs_type, flags, sysfs_root,
>  				SYSFS_MAGIC, &new_sb, ns);
> -	if (IS_ERR(root) || !new_sb)
> +	if (!new_sb)
>  		kobj_ns_drop(KOBJ_NS_TYPE_NET, ns);
> -	else if (new_sb)
> +	else if (!IS_ERR(root))
>  		root->d_sb->s_iflags |= SB_I_USERNS_VISIBLE;
>  
>  	return root;
> -- 
> 2.31.1

I still need to wrap my head around sget_userns (called from
kernfs_mount_ns), but it looks like we can get away with dropping the
kobj_ns reference here when this is not a new superblock, because there
will be a reference to this "old" superblock, which will prevent it from
being killed, and we only drop the kobj_ns on sysfs_kill_sb (defined below
sysfs_mount) when all superblock references are done with.

So we only need to keep the kobj_ns reference in the case of new_sb.

However, if this is a new_sb, but we return an error
(fs/kernfs/mount.c:340), with the old code, we would drop the kobj_ns
reference, as expected. With this patch, it would not drop it, leading to a
kernel leak.

This code has been totally revamped after that change, with commit
23bf1b6be9c2 ("kernfs, sysfs, cgroup, intel_rdt: Support fs_context"),
which means that whatever bug this is fixing needs to be fixed specifically
for our 4.15 kernel (and upstream 4.14.y, and v4.19.y, it seems).

Cascardo.