[PATCH 1/2] debugfs: Only clobber mode/uid/gid on remount if asked

[PATCH 1/2] debugfs: Only clobber mode/uid/gid on remount if asked

[Brian Norris]

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.

  # Don't change /sys/kernel/debug/ permissions.
  mount -t debugfs none /mnt/foo

  # Change /sys/kernel/debug/ mode and uid, but not gid.
  mount -t debugfs -o uid=bar,mode=0750 none /mnt/baz

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
I'm open to writing an LTP test case for this, if that seems like a good
idea.

 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.672.g94769d06f0-goog

[PATCH 2/2] tracefs: Only clobber mode/uid/gid on remount if asked

[Brian Norris]

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

Only clobber if the options were provided at mount time.

Note: the previous behavior was especially surprising in the presence of
automounted /sys/kernel/debug/tracing/.

  # Don't change /sys/kernel/tracing/ permissions on automount.
  umount /sys/kernel/debug/tracing/
  stat /sys/kernel/debug/tracing/.

  # Don't change /sys/kernel/tracing/ permissions.
  mount -t tracefs none /mnt/foo

  # Change /sys/kernel/tracing/ mode and uid, but not gid.
  mount -t tracefs -o uid=bar,mode=0750 none /mnt/baz

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
I'm open to writing an LTP test case for this, if that seems like a good
idea.

 fs/tracefs/inode.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
index 81d26abf486f..da85b3979195 100644
--- a/fs/tracefs/inode.c
+++ b/fs/tracefs/inode.c
@@ -141,6 +141,8 @@ struct tracefs_mount_opts {
 	kuid_t uid;
 	kgid_t gid;
 	umode_t mode;
+	/* Opt_* bitfield. */
+	unsigned int opts;
 };
 
 enum {
@@ -241,6 +243,7 @@ static int tracefs_parse_options(char *data, struct tracefs_mount_opts *opts)
 	kgid_t gid;
 	char *p;
 
+	opts->opts = 0;
 	opts->mode = TRACEFS_DEFAULT_MODE;
 
 	while ((p = strsep(&data, ",")) != NULL) {
@@ -275,24 +278,36 @@ static int tracefs_parse_options(char *data, struct tracefs_mount_opts *opts)
 		 * but traditionally tracefs has ignored all mount options
 		 */
 		}
+
+		opts->opts |= BIT(token);
 	}
 
 	return 0;
 }
 
-static int tracefs_apply_options(struct super_block *sb)
+static int tracefs_apply_options(struct super_block *sb, bool remount)
 {
 	struct tracefs_fs_info *fsi = sb->s_fs_info;
 	struct inode *inode = d_inode(sb->s_root);
 	struct tracefs_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;
+	}
 
-	inode->i_uid = opts->uid;
+	if (!remount || opts->opts & BIT(Opt_uid))
+		inode->i_uid = opts->uid;
 
-	/* Set all the group ids to the mount option */
-	set_gid(sb->s_root, opts->gid);
+	if (!remount || opts->opts & BIT(Opt_gid)) {
+		/* Set all the group ids to the mount option */
+		set_gid(sb->s_root, opts->gid);
+	}
 
 	return 0;
 }
@@ -307,7 +322,7 @@ static int tracefs_remount(struct super_block *sb, int *flags, char *data)
 	if (err)
 		goto fail;
 
-	tracefs_apply_options(sb);
+	tracefs_apply_options(sb, true);
 
 fail:
 	return err;
@@ -359,7 +374,7 @@ static int trace_fill_super(struct super_block *sb, void *data, int silent)
 
 	sb->s_op = &tracefs_super_operations;
 
-	tracefs_apply_options(sb);
+	tracefs_apply_options(sb, false);
 
 	return 0;
 
-- 
2.37.2.672.g94769d06f0-goog

Re: [PATCH 2/2] tracefs: Only clobber mode/uid/gid on remount if asked

[Steven Rostedt]

On Fri, 26 Aug 2022 17:44:17 -0700
Brian Norris <briannorris@chromium.org> wrote:

> Users may have explicitly configured their tracefs permissions; we
> shouldn't overwrite those just because a second mount appeared.
> 
> Only clobber if the options were provided at mount time.
> 
> Note: the previous behavior was especially surprising in the presence of
> automounted /sys/kernel/debug/tracing/.
> 
>   # Don't change /sys/kernel/tracing/ permissions on automount.
>   umount /sys/kernel/debug/tracing/
>   stat /sys/kernel/debug/tracing/.
> 
>   # Don't change /sys/kernel/tracing/ permissions.
>   mount -t tracefs none /mnt/foo
> 
>   # Change /sys/kernel/tracing/ mode and uid, but not gid.
>   mount -t tracefs -o uid=bar,mode=0750 none /mnt/baz
> 

The above text doesn't make sense. Is the comments what you are doing or
what the system is doing? If it is what the system is doing, please show
the output of the stat command and how it is doing something unexpected.

Can you show the example of what is wrong, and what you expected to happen.
The above is just a bunch of commands, but does not display anything that
is incorrect.

> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> I'm open to writing an LTP test case for this, if that seems like a good
> idea.

Yes, please add a test :-)

-- Steve

> 
>  fs/tracefs/inode.c | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
> 
>

Re: [PATCH 2/2] tracefs: Only clobber mode/uid/gid on remount if asked

[Brian Norris]

Hi Steven,

On Wed, Sep 07, 2022 at 07:44:43AM -0400, Steven Rostedt wrote:
> On Fri, 26 Aug 2022 17:44:17 -0700
> Brian Norris <briannorris@chromium.org> wrote:
> 
> > Users may have explicitly configured their tracefs permissions; we
> > shouldn't overwrite those just because a second mount appeared.
> > 
> > Only clobber if the options were provided at mount time.
> > 
> > Note: the previous behavior was especially surprising in the presence of
> > automounted /sys/kernel/debug/tracing/.
> > 
> >   # Don't change /sys/kernel/tracing/ permissions on automount.
> >   umount /sys/kernel/debug/tracing/
> >   stat /sys/kernel/debug/tracing/.
> > 
> >   # Don't change /sys/kernel/tracing/ permissions.
> >   mount -t tracefs none /mnt/foo
> > 
> >   # Change /sys/kernel/tracing/ mode and uid, but not gid.
> >   mount -t tracefs -o uid=bar,mode=0750 none /mnt/baz
> > 
> 
> The above text doesn't make sense. Is the comments what you are doing or
> what the system is doing? If it is what the system is doing, please show
> the output of the stat command and how it is doing something unexpected.

Sorry, I do see how the text as-is is unclear. The intention is to
describe the new, intended behavior, and I only left the existing
behavior as implied.

> Can you show the example of what is wrong, and what you expected to happen.
> The above is just a bunch of commands, but does not display anything that
> is incorrect.

Sure, here's a narrower description of old (unexpected) and new
(expected) behavior, in case you were wanting to update the changelog on
your own:

---

Existing behavior:

  ## Pre-existing status: tracefs is 0755.
  # stat -c '%A' /sys/kernel/tracing/
  drwxr-xr-x

  ## (Re)trigger the automount.
  # umount /sys/kernel/debug/tracing
  # stat -c '%A' /sys/kernel/debug/tracing/.
  drwx------

  ## Unexpected: the automount changed mode for other mount instances.
  # stat -c '%A' /sys/kernel/tracing/
  drwx------

New behavior:

  ## Pre-existing status: tracefs is 0755.
  # stat -c '%A' /sys/kernel/tracing/
  drwxr-xr-x

  ## (Re)trigger the automount.
  # umount /sys/kernel/debug/tracing
  # stat -c '%A' /sys/kernel/debug/tracing/.
  drwxr-xr-x

  ## Expected: the automount does not change other mount instances.
  # stat -c '%A' /sys/kernel/tracing/
  drwxr-xr-x

---

There are other variations of old/new behavior (e.g., if using various
mount options), but those are not the main reason for this patch.

> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > ---
> > I'm open to writing an LTP test case for this, if that seems like a good
> > idea.
> 
> Yes, please add a test :-)

Sure, I'm dusting off my LTP VM now. But in case you'd like the patch
as-is, feel free to splice the above into the commit description.
Otherwise, I may resend as Greg requested for patch 1, after I've got a
test patch going.

Brian

Re: [PATCH 2/2] tracefs: Only clobber mode/uid/gid on remount if asked

[Steven Rostedt]

On Wed, 7 Sep 2022 13:52:15 -0700
Brian Norris <briannorris@chromium.org> wrote:

> Existing behavior:
> 
>   ## Pre-existing status: tracefs is 0755.
>   # stat -c '%A' /sys/kernel/tracing/
>   drwxr-xr-x
> 
>   ## (Re)trigger the automount.
>   # umount /sys/kernel/debug/tracing
>   # stat -c '%A' /sys/kernel/debug/tracing/.
>   drwx------
> 
>   ## Unexpected: the automount changed mode for other mount instances.
>   # stat -c '%A' /sys/kernel/tracing/
>   drwx------
> 
> New behavior:
> 
>   ## Pre-existing status: tracefs is 0755.
>   # stat -c '%A' /sys/kernel/tracing/
>   drwxr-xr-x
> 
>   ## (Re)trigger the automount.
>   # umount /sys/kernel/debug/tracing
>   # stat -c '%A' /sys/kernel/debug/tracing/.
>   drwxr-xr-x
> 
>   ## Expected: the automount does not change other mount instances.
>   # stat -c '%A' /sys/kernel/tracing/
>   drwxr-xr-x

Thanks!

Here's the new change log:

-- Steve

From: Brian Norris <briannorris@chromium.org>
Subject: [PATCH] tracefs: Only clobber mode/uid/gid on remount if asked

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

Only clobber if the options were provided at mount time.

Note: the previous behavior was especially surprising in the presence of
automounted /sys/kernel/debug/tracing/.

Existing behavior:

  ## Pre-existing status: tracefs is 0755.
  # stat -c '%A' /sys/kernel/tracing/
  drwxr-xr-x

  ## (Re)trigger the automount.
  # umount /sys/kernel/debug/tracing
  # stat -c '%A' /sys/kernel/debug/tracing/.
  drwx------

  ## Unexpected: the automount changed mode for other mount instances.
  # stat -c '%A' /sys/kernel/tracing/
  drwx------

New behavior (after this change):

  ## Pre-existing status: tracefs is 0755.
  # stat -c '%A' /sys/kernel/tracing/
  drwxr-xr-x

  ## (Re)trigger the automount.
  # umount /sys/kernel/debug/tracing
  # stat -c '%A' /sys/kernel/debug/tracing/.
  drwxr-xr-x

  ## Expected: the automount does not change other mount instances.
  # stat -c '%A' /sys/kernel/tracing/
  drwxr-xr-x


Signed-off-by: Brian Norris <briannorris@chromium.org>

Re: [PATCH 2/2] tracefs: Only clobber mode/uid/gid on remount if asked

[Steven Rostedt]

On Fri, 26 Aug 2022 17:44:17 -0700
Brian Norris <briannorris@chromium.org> wrote:

> Users may have explicitly configured their tracefs permissions; we
> shouldn't overwrite those just because a second mount appeared.
> 
> Only clobber if the options were provided at mount time.
> 
> Note: the previous behavior was especially surprising in the presence of
> automounted /sys/kernel/debug/tracing/.
> 
>   # Don't change /sys/kernel/tracing/ permissions on automount.
>   umount /sys/kernel/debug/tracing/

BTW, I noticed that the above doesn't do anything. That is,
you cannot unmount tracefs from /sys/kernel/debug/tracing.

 # ls /sys/kernel/debug/tracing/
available_events            buffer_total_size_kb   error_log                 hwlat_detector   options         recursed_functions   set_event_notrace_pid   set_ftrace_pid      stack_trace         trace_clock       trace_stat           uprobe_events
available_filter_functions  current_tracer         eval_map                  instances        osnoise         saved_cmdlines       set_event_pid           set_graph_function  stack_trace_filter  trace_marker      tracing_cpumask      uprobe_profile
available_tracers           dynamic_events         events                    kprobe_events    per_cpu         saved_cmdlines_size  set_ftrace_filter       set_graph_notrace   synthetic_events    trace_marker_raw  tracing_max_latency  user_events_data
buffer_percent              dyn_ftrace_total_info  free_buffer               kprobe_profile   printk_formats  saved_tgids          set_ftrace_notrace      snapshot            timestamp_mode      trace_options     tracing_on           user_events_status
buffer_size_kb              enabled_functions      function_profile_enabled  max_graph_depth  README          set_event            set_ftrace_notrace_pid  stack_max_size      trace               trace_pipe        tracing_thresh

  # umount /sys/kernel/debug/tracing/

  # ls /sys/kernel/debug/tracing/
available_events            buffer_total_size_kb   error_log                 hwlat_detector   options         recursed_functions   set_event_notrace_pid   set_ftrace_pid      stack_trace         trace_clock       trace_stat           uprobe_events
available_filter_functions  current_tracer         eval_map                  instances        osnoise         saved_cmdlines       set_event_pid           set_graph_function  stack_trace_filter  trace_marker      tracing_cpumask      uprobe_profile
available_tracers           dynamic_events         events                    kprobe_events    per_cpu         saved_cmdlines_size  set_ftrace_filter       set_graph_notrace   synthetic_events    trace_marker_raw  tracing_max_latency  user_events_data
buffer_percent              dyn_ftrace_total_info  free_buffer               kprobe_profile   printk_formats  saved_tgids          set_ftrace_notrace      snapshot            timestamp_mode      trace_options     tracing_on           user_events_status
buffer_size_kb              enabled_functions      function_profile_enabled  max_graph_depth  README          set_event            set_ftrace_notrace_pid  stack_max_size      trace               trace_pipe        tracing_thresh


-- Steve



>   stat /sys/kernel/debug/tracing/.
> 
>   # Don't change /sys/kernel/tracing/ permissions.
>   mount -t tracefs none /mnt/foo
> 
>   # Change /sys/kernel/tracing/ mode and uid, but not gid.
>   mount -t tracefs -o uid=bar,mode=0750 none /mnt/baz
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
>

Re: [PATCH 2/2] tracefs: Only clobber mode/uid/gid on remount if asked

[Steven Rostedt]

On Wed, 7 Sep 2022 17:46:04 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> >   # Don't change /sys/kernel/tracing/ permissions on automount.
> >   umount /sys/kernel/debug/tracing/  
> 
> BTW, I noticed that the above doesn't do anything. That is,
> you cannot unmount tracefs from /sys/kernel/debug/tracing.

I just saw your new email. I guess that's just how you were triggering the
automount, by unmounting it. Right?

A lot of assumptions about what people may know ;-)

I never tried it, so I really didn't know what the result of that would be.

-- Steve

Re: [PATCH 2/2] tracefs: Only clobber mode/uid/gid on remount if asked

[Brian Norris]

On Wed, Sep 07, 2022 at 05:48:13PM -0400, Steven Rostedt wrote:
> On Wed, 7 Sep 2022 17:46:04 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > >   # Don't change /sys/kernel/tracing/ permissions on automount.
> > >   umount /sys/kernel/debug/tracing/  
> > 
> > BTW, I noticed that the above doesn't do anything. That is,
> > you cannot unmount tracefs from /sys/kernel/debug/tracing.

Actually, it does work, but it's hard to observe. See below.

> I just saw your new email. I guess that's just how you were triggering the
> automount, by unmounting it. Right?

You trimmed the important line:
  stat /sys/kernel/debug/tracing/.

It's hard to observe directly that /sys/kernel/debug/tracing is
unmounted, because traditional examination (stat(2), open(2), etc.)
methods will re-trigger the automount. The automount is *supposed* to
appear as if it is always there.

Try these:

  umount /sys/kernel/debug/tracing/
  grep tracefs /proc/mounts
  stat /sys/kernel/debug/tracing/.
  grep tracefs /proc/mounts

The first and the second grep will give you different results.

> A lot of assumptions about what people may know ;-)

Yes, I suppose. That's also why I figured I'd write a test case, because
test cases tell you exactly what is meant.

Brian

Re: [PATCH 2/2] tracefs: Only clobber mode/uid/gid on remount if asked

[Steven Rostedt]

On Wed, 7 Sep 2022 14:53:42 -0700
Brian Norris <briannorris@chromium.org> wrote:

> Try these:
> 
>   umount /sys/kernel/debug/tracing/
>   grep tracefs /proc/mounts
>   stat /sys/kernel/debug/tracing/.
>   grep tracefs /proc/mounts
> 
> The first and the second grep will give you different results.

How about this:

  # grep tracefs /proc/mounts
tracefs /sys/kernel/tracing tracefs rw,nosuid,nodev,noexec,relatime 0 0
tracefs /sys/kernel/debug/tracing tracefs rw,nosuid,nodev,noexec,relatime 0 0

 # umount /sys/kernel/debug/tracing 
 # grep tracefs /proc/mounts
tracefs /sys/kernel/tracing tracefs rw,nosuid,nodev,noexec,relatime 0 0

 # ls /sys/kernel/debug/tracing
available_events            hwlat_detector       set_event_notrace_pid   trace_clock
available_filter_functions  instances            set_event_pid           trace_marker
available_tracers           kprobe_events        set_ftrace_filter       trace_marker_raw
buffer_percent              kprobe_profile       set_ftrace_notrace      trace_options
buffer_size_kb              max_graph_depth      set_ftrace_notrace_pid  trace_pipe
buffer_total_size_kb        options              set_ftrace_pid          trace_stat
current_tracer              osnoise              set_graph_function      tracing_cpumask
dynamic_events              per_cpu              set_graph_notrace       tracing_max_latency
dyn_ftrace_total_info       printk_formats       snapshot                tracing_on
enabled_functions           README               stack_max_size          tracing_thresh
error_log                   recursed_functions   stack_trace             uprobe_events
eval_map                    saved_cmdlines       stack_trace_filter      uprobe_profile
events                      saved_cmdlines_size  synthetic_events        user_events_data
free_buffer                 saved_tgids          timestamp_mode          user_events_status
function_profile_enabled    set_event            trace

 # grep tracefs /proc/mounts
tracefs /sys/kernel/tracing tracefs rw,nosuid,nodev,noexec,relatime 0 0
tracefs /sys/kernel/debug/tracing tracefs rw,nosuid,nodev,noexec,relatime 0 0

So it appears that accessing debugfs/tracing will automatically remount it.

Lean something new everyday! ;-)

-- Steve

Re: [PATCH 1/2] debugfs: Only clobber mode/uid/gid on remount if asked

[Greg Kroah-Hartman]

On Fri, Aug 26, 2022 at 05:44:16PM -0700, Brian Norris wrote:
> Users may have explicitly configured their debugfs permissions; we
> shouldn't overwrite those just because a second mount appeared.

What userspace mounts debugfs twice?

> Only clobber if the options were provided at mount time.
> 
>   # Don't change /sys/kernel/debug/ permissions.
>   mount -t debugfs none /mnt/foo
> 
>   # Change /sys/kernel/debug/ mode and uid, but not gid.
>   mount -t debugfs -o uid=bar,mode=0750 none /mnt/baz

So what happens today with this change?  Without it?

> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> I'm open to writing an LTP test case for this, if that seems like a good
> idea.

If it's really needed, again, why would debugfs be ever mounted more
than once?

thanks,

greg k-h

Re: [PATCH 1/2] debugfs: Only clobber mode/uid/gid on remount if asked

[Steven Rostedt]

On Thu, 1 Sep 2022 17:58:14 +0200
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Fri, Aug 26, 2022 at 05:44:16PM -0700, Brian Norris wrote:
> > Users may have explicitly configured their debugfs permissions; we
> > shouldn't overwrite those just because a second mount appeared.  
> 
> What userspace mounts debugfs twice?
> 
> > Only clobber if the options were provided at mount time.
> > 
> >   # Don't change /sys/kernel/debug/ permissions.
> >   mount -t debugfs none /mnt/foo
> > 
> >   # Change /sys/kernel/debug/ mode and uid, but not gid.
> >   mount -t debugfs -o uid=bar,mode=0750 none /mnt/baz  
> 
> So what happens today with this change?  Without it?
> 
> > 
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > ---
> > I'm open to writing an LTP test case for this, if that seems like a good
> > idea.  
> 
> If it's really needed, again, why would debugfs be ever mounted more
> than once?
>

The real issue is with tracefs, and I think the debugfs patch is just for
consistency. But I (and others) do have debugfs mounted more that once. ;-)

  # mount |grep debugfs
  debugfs on /sys/kernel/debug type debugfs (rw,nosuid,nodev,noexec,relatime)
  debugfs on /debug type debugfs (rw,relatime)

I added /debug (and I know others that just add /d) for shortcuts to get to
the debugfs directory easier.

This patch series came about because ideally tracefs should be mounted in
/sys/kernel/tracing. But because a lot of scripts use the old path of
/sys/kernel/debug/tracing, I have tracefs mounted there automatically
when debugfs is mounted. This is so that scripts do not break.

I would love to deprecate the /sys/kernel/debug/tracing automatic mounting,
but I do not know what user space will break if that happens. libtracefs
handles finding where tracefs is, so anything that uses that is fine.

But anyway, because tracefs is mounted more than once, if someone has
tracefs mounted in the correct location "/sys/kernel/tracing" and updates
the permissions to the files, but then mounts debugfs, due to the automatic
mounting, all their changes go away.

Perhaps we only need the second patch because of the automatic mounting.
Maybe people do not care if things get reset if they manually mount debugfs
more than once. I (and others) have it in my fstab, so it's done at boot up
and any changes should affect both.

-- Steve

Re: [PATCH 1/2] debugfs: Only clobber mode/uid/gid on remount if asked

[Brian Norris]

On Thu, Sep 01, 2022 at 05:58:14PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Aug 26, 2022 at 05:44:16PM -0700, Brian Norris wrote:
> > Users may have explicitly configured their debugfs permissions; we
> > shouldn't overwrite those just because a second mount appeared.
> 
> What userspace mounts debugfs twice?

I'll admit, my particular userspace in question (Chrom{e,ium}OS) does
not. There are several debugfs mounts, but they are bind mounts, which
don't hit this problem.

But Steven hits the nail on the head for most of my reasoning; my main
motivation is for tracefs (patch 2), whose automount makes this very
surprising. I included patch 1 for consistency (tracefs essentially
copy/pasted debugfs). I could drop patch 1 if that helps somehow, but
I'd still like to consider the automount difficulties in patch 2.

> > Only clobber if the options were provided at mount time.
> > 
> >   # Don't change /sys/kernel/debug/ permissions.
> >   mount -t debugfs none /mnt/foo
> > 
> >   # Change /sys/kernel/debug/ mode and uid, but not gid.
> >   mount -t debugfs -o uid=bar,mode=0750 none /mnt/baz
> 
> So what happens today with this change?  Without it?

Sorry, this was probably a bit too implied -- the gid changes to its
default (0 if we never set it in a mount option before; or it will reset
to any previous gid= mount setting).

# ls -ld /sys/kernel/debug/.
drwxr-x---. 45 root debugfs-access 0 Dec 31  1969 /sys/kernel/debug/.
# chown root:power /sys/kernel/debug/
# ls -ld /sys/kernel/debug/.
drwxr-x---. 45 root power 0 Dec 31  1969 /sys/kernel/debug/.
# mount -t debugfs -o uid=power,mode=0750 none /tmp/mnt
# ls -ld /sys/kernel/debug/.
drwxr-x---. 45 power debugfs-access 0 Dec 31  1969 /sys/kernel/debug/.
# mount | grep '\/sys\/kernel\/debug '
debugfs on /sys/kernel/debug type debugfs (rw,nosuid,nodev,noexec,relatime,seclabel,uid=228,gid=605,mode=750)

I can include more before/after examples in the commit message if you
want. Honestly, that's kind of why I offered to write test cases; test
cases show what's happening better than narrative descriptions.

> > 
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > ---
> > I'm open to writing an LTP test case for this, if that seems like a good
> > idea.
> 
> If it's really needed, again, why would debugfs be ever mounted more
> than once?

Steven gave examples. Again, my particular use is more for patch 2. But
I think the current behavior is pretty surprising, if anybody ever
*does* try to mount more than once.

Brian

Re: [PATCH 1/2] debugfs: Only clobber mode/uid/gid on remount if asked

[Greg Kroah-Hartman]

On Thu, Sep 01, 2022 at 03:32:18PM -0700, Brian Norris wrote:
> On Thu, Sep 01, 2022 at 05:58:14PM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Aug 26, 2022 at 05:44:16PM -0700, Brian Norris wrote:
> > > Users may have explicitly configured their debugfs permissions; we
> > > shouldn't overwrite those just because a second mount appeared.
> > 
> > What userspace mounts debugfs twice?
> 
> I'll admit, my particular userspace in question (Chrom{e,ium}OS) does
> not. There are several debugfs mounts, but they are bind mounts, which
> don't hit this problem.
> 
> But Steven hits the nail on the head for most of my reasoning; my main
> motivation is for tracefs (patch 2), whose automount makes this very
> surprising. I included patch 1 for consistency (tracefs essentially
> copy/pasted debugfs). I could drop patch 1 if that helps somehow, but
> I'd still like to consider the automount difficulties in patch 2.
> 
> > > Only clobber if the options were provided at mount time.
> > > 
> > >   # Don't change /sys/kernel/debug/ permissions.
> > >   mount -t debugfs none /mnt/foo
> > > 
> > >   # Change /sys/kernel/debug/ mode and uid, but not gid.
> > >   mount -t debugfs -o uid=bar,mode=0750 none /mnt/baz
> > 
> > So what happens today with this change?  Without it?
> 
> Sorry, this was probably a bit too implied -- the gid changes to its
> default (0 if we never set it in a mount option before; or it will reset
> to any previous gid= mount setting).
> 
> # ls -ld /sys/kernel/debug/.
> drwxr-x---. 45 root debugfs-access 0 Dec 31  1969 /sys/kernel/debug/.
> # chown root:power /sys/kernel/debug/
> # ls -ld /sys/kernel/debug/.
> drwxr-x---. 45 root power 0 Dec 31  1969 /sys/kernel/debug/.
> # mount -t debugfs -o uid=power,mode=0750 none /tmp/mnt
> # ls -ld /sys/kernel/debug/.
> drwxr-x---. 45 power debugfs-access 0 Dec 31  1969 /sys/kernel/debug/.
> # mount | grep '\/sys\/kernel\/debug '
> debugfs on /sys/kernel/debug type debugfs (rw,nosuid,nodev,noexec,relatime,seclabel,uid=228,gid=605,mode=750)
> 
> I can include more before/after examples in the commit message if you
> want. Honestly, that's kind of why I offered to write test cases; test
> cases show what's happening better than narrative descriptions.

before/after in the changelog comments are very good, thanks.  And yes,
tests are also good, I'd gladly take that too if you do a v2 of this
patch.

And make it independant of the tracefs change please, these would go
through two different trees as they have different maintainers.

thanks,

greg k-h

Re: [PATCH 1/2] debugfs: Only clobber mode/uid/gid on remount if asked

[Brian Norris]

On Fri, Sep 02, 2022 at 07:45:30AM +0200, Greg Kroah-Hartman wrote:
> before/after in the changelog comments are very good, thanks.  And yes,
> tests are also good, I'd gladly take that too if you do a v2 of this
> patch.
> 
> And make it independant of the tracefs change please, these would go
> through two different trees as they have different maintainers.

I think I've covered all that in v2. The patch contents haven't changed
-- just the metadata, and a link to a draft LTP test. As noted in v2, I
plan to submit a PR for that once the kernel changes are in.

https://lore.kernel.org/lkml/20220908171319.v2.1.Icbd40fce59f55ad74b80e5d435ea233579348a78@changeid/

Brian

Re: [PATCH 1/2] debugfs: Only clobber mode/uid/gid on remount if asked

[Steven Rostedt]

On Thu, 8 Sep 2022 17:16:17 -0700
Brian Norris <briannorris@chromium.org> wrote:

> I think I've covered all that in v2. The patch contents haven't changed
> -- just the metadata, and a link to a draft LTP test. As noted in v2, I
> plan to submit a PR for that once the kernel changes are in.

Instead of doing an LTP test, could you just write a script that could test
it in the kernel selftests?

See tools/testing/selftests/...

in the kernel repository.

Not exactly sure which directory it would go. Perhaps the "mounts"
directory?

If you create a tracefs one, it could go into the ftrace directory.

-- Steve

Re: [PATCH 1/2] debugfs: Only clobber mode/uid/gid on remount if asked

[Brian Norris]

On Thu, Sep 08, 2022 at 08:43:32PM -0400, Steven Rostedt wrote:
> On Thu, 8 Sep 2022 17:16:17 -0700
> Instead of doing an LTP test, could you just write a script that could test
> it in the kernel selftests?
> 
> See tools/testing/selftests/...
> 
> in the kernel repository.

I honestly don't know why both LTP and kselftests exist. But I *did*
specifically ask about LTP and got a "yes" from both you and Greg.

I suppose I can go back and remove all the LTP niceties that I just
added to my bare script (setup, cleanup, clean handling of individual
test cases, unified reporting stats; does selftests have any of
that?)... But that'll have to be next week, if I can find the time at
all.

> If you create a tracefs one, it could go into the ftrace directory.

Since the tests cases are so similar, my current script tests both
debugfs and tracefs. So I probably won't create two separate buckets for
this.

Brian

Re: [PATCH 1/2] debugfs: Only clobber mode/uid/gid on remount if asked

[Steven Rostedt]

On Thu, 8 Sep 2022 17:57:11 -0700
Brian Norris <briannorris@chromium.org> wrote:

> On Thu, Sep 08, 2022 at 08:43:32PM -0400, Steven Rostedt wrote:
> > On Thu, 8 Sep 2022 17:16:17 -0700
> > Instead of doing an LTP test, could you just write a script that could test
> > it in the kernel selftests?
> > 
> > See tools/testing/selftests/...
> > 
> > in the kernel repository.  
> 
> I honestly don't know why both LTP and kselftests exist. But I *did*

Well, LTP started off great, but then became overwhelming, and would
fail on a lot that I didn't care about. Then we started the kernel
selftests which is suppose to be small unit tests where tests get added
along with new features. And because the tests are in the kernel tree,
they get added together. That was one of the issues with LTP, it was
harder to coordinate what tests went with which kernel.

I have not run LTP in years, but run selftests weekly.

> specifically ask about LTP and got a "yes" from both you and Greg.

I was just saying "yes" to testing, I must have overlooked that you
mentioned LTP. All I saw was "testing" and thought "Yes!" ;-)

> 
> I suppose I can go back and remove all the LTP niceties that I just
> added to my bare script (setup, cleanup, clean handling of individual
> test cases, unified reporting stats; does selftests have any of
> that?)... But that'll have to be next week, if I can find the time at
> all.

Nah, you did the work, and it doesn't hurt to have it in LTP too. I may
no longer use LTP, but I'm sure there are a lot of others that do.

> 
> > If you create a tracefs one, it could go into the ftrace directory.  
> 
> Since the tests cases are so similar, my current script tests both
> debugfs and tracefs. So I probably won't create two separate buckets for
> this.

If you have a script that tests both, I'm happy to add it in the ftrace
selftests.

Sorry for the confusion.

-- Steve