linux-kernel - Re: [PATCH 3/3] tracing: Fix tracefs gid mount option

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bbbbaa42-444f-4973-b749-7c56b937ae5f@redhat.com>
Date: Mon, 28 Oct 2024 22:00:00 -0500
From: Eric Sandeen <sandeen@...hat.com>
To: Kalesh Singh <kaleshsingh@...gle.com>, dhowells@...hat.com,
 rostedt@...dmis.org, mhiramat@...nel.org
Cc: surenb@...gle.com, jyescas@...gle.com, kernel-team@...roid.com,
 android-mm@...gle.com, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
 Shuah Khan <shuah@...nel.org>, Ali Zahraee <ahzahraee@...il.com>,
 Christian Brauner <brauner@...nel.org>, linux-kernel@...r.kernel.org,
 linux-trace-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH 3/3] tracing: Fix tracefs gid mount option

On 10/28/24 4:43 PM, Kalesh Singh wrote:
> Commit 78ff64081949 ("vfs: Convert tracefs to use the new mount API")
> tracefs to use the new mount APIs caused mounting with the gid=<gid>
> option to not take effect.

Or any other mount options. I'm sure this isn't unique to gid, right?
So, might want to fix the commit title.

> The tracefs superblock can be updated from multiple paths:
>     - on fs_initcall() to init_trace_printk_function_export()
>     - form a work queue to initialize eventfs
>       tracer_init_tracefs_work_func()
>     - fsconfig() syscall to mount of remount sysfs
> 
> The tracefs super block root inode gets created early on in
> init_trace_printk_function_export().
> 
> With the new mount API tracefs effectively uses get_tree_single() instead
> of the old API mount_single().
> 
> Previously, mount_single() ensured that the options are alway applied to
> the superblock root inode:
>     (1) If the root inode didn't exist, called fill_super() to create it
>         and apply the options.
>     (2) If the root inode exists, called reconfigure_single() which
>         effectively called tracefs_apply_options() to parse and apply
>         options to the subperblock's fs_info and inode and remount
>         eventfs (if necessary)
> 
> On the other hand, get_tree_single() effectively calls vfs_get_super()
> which:
>     (3) If the root inode doesn't exists calls fill_super() to create it
>         and apply the options.
>     (4) If the root inode already exists, updates the fs_context root
>         with the superblock's root inode.

I'm honestly a little lost here, but given the differences between mount_single()
and get_tree_single() - are other get_tree_single() users similarly broken?

Should get_tree_single() just be calling reconfigure_single() internally like
mount_single() did? The comment in reconfigure_single() confuses me.

> (4) above is always the case for tracefs mounts, since the super block's
> root inode will already be created by init_trace_printk_function_export().

this reminds me a little of 

commit a6097180d884ddab769fb25588ea8598589c218c
Author: NeilBrown <neilb@...e.de>
Date:   Mon Jan 17 09:07:26 2022 +1100

    devtmpfs regression fix: reconfigure on each mount

> This means that the gid mount option gets ignored:
>     - Since it isn't applied to the super block's root inode, it doesn't
>       get inherited by the children.
>     - Since eventfs is initialized from form a separate work queue and
>       before call to mount with the options, and it doesn't get remounted
>       for mount.
> 
> Ensure that the mount options are applied to the super block and eventfs
> is remounted to respect the new mount options.
> 
> [1] https://lore.kernel.org/r/536e99d3-345c-448b-adee-a21389d7ab4b@redhat.com/
> 
> Fixes: 78ff64081949 ("vfs: Convert tracefs to use the new mount API")
> Cc: David Howells <dhowells@...hat.com>
> Cc: Steven Rostedt <rostedt@...dmis.org>
> Cc: Masami Hiramatsu <mhiramat@...nel.org>
> Signed-off-by: Kalesh Singh <kaleshsingh@...gle.com>
> ---
>  fs/tracefs/inode.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
> index 1748dff58c3b..cfc614c638da 100644
> --- a/fs/tracefs/inode.c
> +++ b/fs/tracefs/inode.c
> @@ -392,6 +392,9 @@ static int tracefs_reconfigure(struct fs_context *fc)
>  	struct tracefs_fs_info *sb_opts = sb->s_fs_info;
>  	struct tracefs_fs_info *new_opts = fc->s_fs_info;
>  
> +	if (!new_opts)
> +		return 0;

Can this really happen?

> +
>  	sync_filesystem(sb);
>  	/* structure copy of new mount options to sb */
>  	*sb_opts = *new_opts;

FWIW doing this as a structure copy was probably a terrible choice on my part. :(

> @@ -478,14 +481,17 @@ static int tracefs_fill_super(struct super_block *sb, struct fs_context *fc)
>  	sb->s_op = &tracefs_super_operations;
>  	sb->s_d_op = &tracefs_dentry_operations;
>  
> -	tracefs_apply_options(sb, false);
> -
>  	return 0;
>  }
>  
>  static int tracefs_get_tree(struct fs_context *fc)
>  {
> -	return get_tree_single(fc, tracefs_fill_super);
> +	int err = get_tree_single(fc, tracefs_fill_super);
> +
> +	if (err)
> +		return err;
> +
> +	return tracefs_reconfigure(fc);
>  }
>  
>  static void tracefs_free_fc(struct fs_context *fc)