Users may have explicitly configured their debugfs permissions; we
shouldn't overwrite those just because a second mount appeared.

Only clobber if the options were provided at mount time.

Existing behavior:

  ## Pre-existing status: debugfs is 0755.
  # chmod 755 /sys/kernel/debug/
  # stat -c '%A' /sys/kernel/debug/
  drwxr-xr-x

  ## New mount sets kernel-default permissions:
  # mount -t debugfs none /mnt/foo
  # stat -c '%A' /mnt/foo
  drwx------

  ## Unexpected: the original mount changed permissions:
  # stat -c '%A' /sys/kernel/debug
  drwx------

New behavior:

  ## Pre-existing status: debugfs is 0755.
  # chmod 755 /sys/kernel/debug/
  # stat -c '%A' /sys/kernel/debug/
  drwxr-xr-x

  ## New mount inherits existing permissions:
  # mount -t debugfs none /mnt/foo
  # stat -c '%A' /mnt/foo
  drwxr-xr-x

  ## Expected: old mount is unchanged:
  # stat -c '%A' /sys/kernel/debug
  drwxr-xr-x

Full test cases are being submitted to LTP.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
Temporary LTP branch here:
https://github.com/computersforpeace/ltp/commits/debugfs

I'll put in an LTP pull request once the kernel tree actually contains
my patches.

Companion patch:
  tracefs: Only clobber mode/uid/gid on remount if asked
  https://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git/commit/?h=trace-v6.0-rc4&id=47311db8e8f33011d90dee76b39c8886120cdda4

Changes in v2:
* Update commit description
* Include LTP patch in notes
* Split from tracefs patch

 fs/debugfs/inode.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 3dcf0b8b4e93..1e36ce013631 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -82,6 +82,8 @@ struct debugfs_mount_opts {
 	kuid_t uid;
 	kgid_t gid;
 	umode_t mode;
+	/* Opt_* bitfield. */
+	unsigned int opts;
 };
 
 enum {
@@ -111,6 +113,7 @@ static int debugfs_parse_options(char *data, struct debugfs_mount_opts *opts)
 	kgid_t gid;
 	char *p;
 
+	opts->opts = 0;
 	opts->mode = DEBUGFS_DEFAULT_MODE;
 
 	while ((p = strsep(&data, ",")) != NULL) {
@@ -145,22 +148,34 @@ static int debugfs_parse_options(char *data, struct debugfs_mount_opts *opts)
 		 * but traditionally debugfs has ignored all mount options
 		 */
 		}
+
+		opts->opts |= BIT(token);
 	}
 
 	return 0;
 }
 
-static int debugfs_apply_options(struct super_block *sb)
+static int debugfs_apply_options(struct super_block *sb, bool remount)
 {
 	struct debugfs_fs_info *fsi = sb->s_fs_info;
 	struct inode *inode = d_inode(sb->s_root);
 	struct debugfs_mount_opts *opts = &fsi->mount_opts;
 
-	inode->i_mode &= ~S_IALLUGO;
-	inode->i_mode |= opts->mode;
+	/*
+	 * On remount, only reset mode/uid/gid if they were provided as mount
+	 * options.
+	 */
+
+	if (!remount || opts->opts & BIT(Opt_mode)) {
+		inode->i_mode &= ~S_IALLUGO;
+		inode->i_mode |= opts->mode;
+	}
+
+	if (!remount || opts->opts & BIT(Opt_uid))
+		inode->i_uid = opts->uid;
 
-	inode->i_uid = opts->uid;
-	inode->i_gid = opts->gid;
+	if (!remount || opts->opts & BIT(Opt_gid))
+		inode->i_gid = opts->gid;
 
 	return 0;
 }
@@ -175,7 +190,7 @@ static int debugfs_remount(struct super_block *sb, int *flags, char *data)
 	if (err)
 		goto fail;
 
-	debugfs_apply_options(sb);
+	debugfs_apply_options(sb, true);
 
 fail:
 	return err;
@@ -257,7 +272,7 @@ static int debug_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_op = &debugfs_super_operations;
 	sb->s_d_op = &debugfs_dops;
 
-	debugfs_apply_options(sb);
+	debugfs_apply_options(sb, false);
 
 	return 0;
 
-- 
2.37.2.789.g6183377224-goog

On Thu, Sep 08, 2022 at 05:13:22PM -0700, Brian Norris wrote:
> Users may have explicitly configured their debugfs permissions; we
> shouldn't overwrite those just because a second mount appeared.
> 
> Only clobber if the options were provided at mount time.
> 
> Existing behavior:
> 
>   ## Pre-existing status: debugfs is 0755.
>   # chmod 755 /sys/kernel/debug/
>   # stat -c '%A' /sys/kernel/debug/
>   drwxr-xr-x
> 
>   ## New mount sets kernel-default permissions:
>   # mount -t debugfs none /mnt/foo
>   # stat -c '%A' /mnt/foo
>   drwx------
> 
>   ## Unexpected: the original mount changed permissions:
>   # stat -c '%A' /sys/kernel/debug
>   drwx------
> 
> New behavior:
> 
>   ## Pre-existing status: debugfs is 0755.
>   # chmod 755 /sys/kernel/debug/
>   # stat -c '%A' /sys/kernel/debug/
>   drwxr-xr-x
> 
>   ## New mount inherits existing permissions:
>   # mount -t debugfs none /mnt/foo
>   # stat -c '%A' /mnt/foo
>   drwxr-xr-x
> 
>   ## Expected: old mount is unchanged:
>   # stat -c '%A' /sys/kernel/debug
>   drwxr-xr-x
> 
> Full test cases are being submitted to LTP.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> Temporary LTP branch here:
> https://github.com/computersforpeace/ltp/commits/debugfs
> 
> I'll put in an LTP pull request once the kernel tree actually contains
> my patches.
> 
> Companion patch:
>   tracefs: Only clobber mode/uid/gid on remount if asked
>   https://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git/commit/?h=trace-v6.0-rc4&id=47311db8e8f33011d90dee76b39c8886120cdda4
> 
> Changes in v2:
> * Update commit description
> * Include LTP patch in notes
> * Split from tracefs patch
> 
>  fs/debugfs/inode.c | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 3dcf0b8b4e93..1e36ce013631 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -82,6 +82,8 @@ struct debugfs_mount_opts {
>  	kuid_t uid;
>  	kgid_t gid;
>  	umode_t mode;
> +	/* Opt_* bitfield. */
> +	unsigned int opts;
>  };
>  
>  enum {
> @@ -111,6 +113,7 @@ static int debugfs_parse_options(char *data, struct debugfs_mount_opts *opts)
>  	kgid_t gid;
>  	char *p;
>  
> +	opts->opts = 0;
>  	opts->mode = DEBUGFS_DEFAULT_MODE;
>  
>  	while ((p = strsep(&data, ",")) != NULL) {
> @@ -145,22 +148,34 @@ static int debugfs_parse_options(char *data, struct debugfs_mount_opts *opts)
>  		 * but traditionally debugfs has ignored all mount options
>  		 */
>  		}
> +
> +		opts->opts |= BIT(token);
>  	}
>  
>  	return 0;
>  }
>  
> -static int debugfs_apply_options(struct super_block *sb)
> +static int debugfs_apply_options(struct super_block *sb, bool remount)

Functions with random boolean arguments are impossible to understand and
maintain over time.

Every time you run across a call to this function now, you have to go
back and look up what this specific argument means, which is a pain.

Make two functions:
	debugfs_apply_options()
	debugfs_apply_options_remount()
and then move this original function to be:
	_debugfs_apply_options()
or some such name.

Then when you call the function, you know exactly what is happening.

thanks,

greg k-h