[PATCH 0/3] Tracefs gid mount option fixes

[PATCH 0/3] Tracefs gid mount option fixes

[Kalesh Singh]

Hi all,

This series is based on v6.12-rc4. It fixes an issue with the  tracefs
gid mount option. Adds test case to prevent future breakages and updates
the tracefs readme to document the expected behavior of this option.

Thanks,
Kalesh

Kalesh Singh (3):
  tracing: Document tracefs gid mount option
  tracing/selftests: Add tracefs mount options test
  tracing: Fix tracefs gid mount option

 fs/tracefs/inode.c                            |  12 ++-
 kernel/trace/trace.c                          |   4 +
 .../ftrace/test.d/00basic/mount_options.tc    | 101 ++++++++++++++++++
 .../ftrace/test.d/00basic/test_ownership.tc   |  16 +--
 .../testing/selftests/ftrace/test.d/functions |  25 +++++
 5 files changed, 142 insertions(+), 16 deletions(-)
 create mode 100644 tools/testing/selftests/ftrace/test.d/00basic/mount_options.tc


base-commit: 42f7652d3eb527d03665b09edac47f85fb600924
--
2.47.0.163.g1226f6d8fa-goog

[PATCH 1/3] tracing: Document tracefs gid mount option

[Kalesh Singh]

[1] and [2] introduced a new gid mount options that could apply to a
group to all entries in tracefs.

Document this in the tracing readme.

[1] https://lore.kernel.org/r/20211208075720.4855d180@gandalf.local.home/T/#u
[2] https://lore.kernel.org/r/20211207171729.2a54e1b3@gandalf.local.home/T/#u

Cc: David Howells <dhowells@redhat.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
---
 kernel/trace/trace.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index a8f52b6527ca..2b64b3ec67d9 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -5501,6 +5501,10 @@ static const struct file_operations tracing_iter_fops = {
 
 static const char readme_msg[] =
 	"tracing mini-HOWTO:\n\n"
+	"By default tracefs removes all OTH file permission bits.\n"
+	"When mounting tracefs an optional group id can be specified\n"
+	"which adds the group to every directory and file in tracefs:\n\n"
+	"\t e.g. mount -t tracefs [-o [gid=<gid>]] nodev /sys/kernel/tracing\n\n"
 	"# echo 0 > tracing_on : quick way to disable tracing\n"
 	"# echo 1 > tracing_on : quick way to re-enable tracing\n\n"
 	" Important files:\n"
-- 
2.47.0.163.g1226f6d8fa-goog

Re: [PATCH 1/3] tracing: Document tracefs gid mount option

[Steven Rostedt]

On Mon, 28 Oct 2024 14:43:57 -0700
Kalesh Singh <kaleshsingh@google.com> wrote:

> [1] and [2] introduced a new gid mount options that could apply to a
> group to all entries in tracefs.
> 
> Document this in the tracing readme.
> 
> [1] https://lore.kernel.org/r/20211208075720.4855d180@gandalf.local.home/T/#u
> [2] https://lore.kernel.org/r/20211207171729.2a54e1b3@gandalf.local.home/T/#u

Your patch series is backwards. The first patch should be the fix, the
second be this patch and the last one is the test case.

You should reference commit IDs and not links.

   Commit ee7f3666995d ("tracefs: Have new files inherit the ownership of
   their parent") and commit 48b27b6b5191 ("tracefs: Set all files to the
   same group ownership as the mount option") introduced a new gid..."

-- Steve

> 
> Cc: David Howells <dhowells@redhat.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
> ---
>  kernel/trace/trace.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index a8f52b6527ca..2b64b3ec67d9 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -5501,6 +5501,10 @@ static const struct file_operations tracing_iter_fops = {
>  
>  static const char readme_msg[] =
>  	"tracing mini-HOWTO:\n\n"
> +	"By default tracefs removes all OTH file permission bits.\n"
> +	"When mounting tracefs an optional group id can be specified\n"
> +	"which adds the group to every directory and file in tracefs:\n\n"
> +	"\t e.g. mount -t tracefs [-o [gid=<gid>]] nodev /sys/kernel/tracing\n\n"
>  	"# echo 0 > tracing_on : quick way to disable tracing\n"
>  	"# echo 1 > tracing_on : quick way to re-enable tracing\n\n"
>  	" Important files:\n"

[PATCH 2/3] tracing/selftests: Add tracefs mount options test

[Kalesh Singh]

Add test to check the tracefs gid mount option is applied correctly

   ./ftracetest test.d/00basic/mount_options.tc

Use the new readme string "[gid=<gid>] as a requirement and also update
test_ownership.tc requirements to use this.

mount_options.tc will fail currently, this is fixed by the subsequent
patch in this series.

Cc: David Howells <dhowells@redhat.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
---
 .../ftrace/test.d/00basic/mount_options.tc    | 101 ++++++++++++++++++
 .../ftrace/test.d/00basic/test_ownership.tc   |  16 +--
 .../testing/selftests/ftrace/test.d/functions |  25 +++++
 3 files changed, 129 insertions(+), 13 deletions(-)
 create mode 100644 tools/testing/selftests/ftrace/test.d/00basic/mount_options.tc

diff --git a/tools/testing/selftests/ftrace/test.d/00basic/mount_options.tc b/tools/testing/selftests/ftrace/test.d/00basic/mount_options.tc
new file mode 100644
index 000000000000..b8aff85ec259
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/00basic/mount_options.tc
@@ -0,0 +1,101 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# description: Test tracefs GID mount option
+# requires: "[gid=<gid>]":README
+
+fail() {
+	local msg="$1"
+
+	echo "FAILED: $msg"
+	exit_fail
+}
+
+find_alternate_gid() {
+	local original_gid="$1"
+	tac /etc/group | grep -v ":$original_gid:" | head -1 | cut -d: -f3
+}
+
+mount_tracefs_with_options() {
+	local mount_point="$1"
+	local options="$2"
+
+	mount -t tracefs -o "$options" nodev "$mount_point"
+
+	setup
+}
+
+unmount_tracefs() {
+	local mount_point="$1"
+
+	# Need to make sure the mount isn't busy so that we can umount it
+	(cd $mount_point; finish_ftrace;)
+
+	cleanup
+}
+
+create_instance() {
+	local mount_point="$1"
+	local instance="$mount_point/instances/$(mktemp -u test-XXXXXX)"
+
+	mkdir "$instance"
+	echo "$instance"
+}
+
+remove_instance() {
+	local instance="$1"
+
+	rmdir "$instance"
+}
+
+check_gid() {
+	local mount_point="$1"
+	local expected_gid="$2"
+
+	echo "Checking permission group ..."
+
+	cd "$mount_point"
+
+	for file in "." "events" "events/sched" "events/sched/sched_switch" "events/sched/sched_switch/enable"; do
+		local gid=`stat -c "%g" $file`
+		if [ "$gid" -ne "$expected_gid" ]; then
+			cd - # Return to the previous working direcotry (tracefs root)
+			fail "$(realpath $file): Expected group $expected_gid; Got group $gid"
+		fi
+	done
+
+	cd - # Return to the previous working direcotry (tracefs root)
+}
+
+test_gid_mount_option() {
+	local mount_point=$(get_mount_point)
+	local mount_options=$(get_mnt_options "$mount_point")
+	local original_group=$(stat -c "%g" .)
+	local other_group=$(find_alternate_gid "$original_group")
+
+	# Set up mount options with new GID for testing
+	local new_options=`echo "$mount_options" | sed -e "s/gid=[0-9]*/gid=$other_group/"`
+	if [ "$new_options" = "$mount_options" ]; then
+		new_options="$mount_options,gid=$other_group"
+		mount_options="$mount_options,gid=$original_group"
+	fi
+
+	# Unmount existing tracefs instance and mount with new GID
+	unmount_tracefs "$mount_point"
+	mount_tracefs_with_options "$mount_point" "$new_options"
+
+	check_gid "$mount_point" "$other_group"
+
+	# Check that files created after the mount inherit the GID
+	local instance=$(create_instance "$mount_point")
+	check_gid "$instance" "$other_group"
+	remove_instance "$instance"
+
+	# Unmount and remount with the original GID
+	unmount_tracefs "$mount_point"
+	mount_tracefs_with_options "$mount_point" "$mount_options"
+	check_gid "$mount_point" "$original_group"
+}
+
+test_gid_mount_option
+
+exit 0
diff --git a/tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc b/tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc
index 094419e190c2..e71cc3ad0bdf 100644
--- a/tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc
+++ b/tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc
@@ -1,24 +1,14 @@
 #!/bin/sh
 # SPDX-License-Identifier: GPL-2.0
 # description: Test file and directory ownership changes for eventfs
+# requires: "[gid=<gid>]":README
 
 original_group=`stat -c "%g" .`
 original_owner=`stat -c "%u" .`
 
-mount_point=`stat -c '%m' .`
+local mount_point=$(get_mount_point)
 
-# If stat -c '%m' does not work (e.g. busybox) or failed, try to use the
-# current working directory (which should be a tracefs) as the mount point.
-if [ ! -d "$mount_point" ]; then
-	if mount | grep -qw $PWD ; then
-		mount_point=$PWD
-	else
-		# If PWD doesn't work, that is an environmental problem.
-		exit_unresolved
-	fi
-fi
-
-mount_options=`mount | grep "$mount_point" | sed -e 's/.*(\(.*\)).*/\1/'`
+mount_options=$(get_mnt_options "$mount_point")
 
 # find another owner and group that is not the original
 other_group=`tac /etc/group | grep -v ":$original_group:" | head -1 | cut -d: -f3`
diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
index 779f3e62ec90..84d6a9c7ad67 100644
--- a/tools/testing/selftests/ftrace/test.d/functions
+++ b/tools/testing/selftests/ftrace/test.d/functions
@@ -193,3 +193,28 @@ ftrace_errlog_check() { # err-prefix command-with-error-pos-by-^ command-file
     # "  Command: " and "^\n" => 13
     test $(expr 13 + $pos) -eq $N
 }
+
+# Helper to get the tracefs mount point
+get_mount_point() {
+	local mount_point=`stat -c '%m' .`
+
+	# If stat -c '%m' does not work (e.g. busybox) or failed, try to use the
+	# current working directory (which should be a tracefs) as the mount point.
+	if [ ! -d "$mount_point" ]; then
+		if mount | grep -qw "$PWD"; then
+			mount_point=$PWD
+		else
+			# If PWD doesn't work, that is an environmental problem.
+			exit_unresolved
+		fi
+	fi
+	echo "$mount_point"
+}
+
+# Helper function to retrieve mount options for a given mount point
+get_mnt_options() {
+	local mnt_point="$1"
+	local opts=$(mount | grep -m1 "$mnt_point" | sed -e 's/.*(\(.*\)).*/\1/')
+
+	echo "$opts"
+}
\ No newline at end of file
-- 
2.47.0.163.g1226f6d8fa-goog

Re: [PATCH 2/3] tracing/selftests: Add tracefs mount options test

[Steven Rostedt]

On Mon, 28 Oct 2024 14:43:58 -0700
Kalesh Singh <kaleshsingh@google.com> wrote:

> Add test to check the tracefs gid mount option is applied correctly
> 
>    ./ftracetest test.d/00basic/mount_options.tc
> 
> Use the new readme string "[gid=<gid>] as a requirement and also update
> test_ownership.tc requirements to use this.
> 
> mount_options.tc will fail currently, this is fixed by the subsequent
> patch in this series.

Test cases should never be added when they can fail. They should always
come after the fix is applied. But it appears that you check the README
to make sure that it does work and not fail.

I'll take a look at these patches in more detail later.

Thanks,

-- Steve


> 
> Cc: David Howells <dhowells@redhat.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
> ---
>  .../ftrace/test.d/00basic/mount_options.tc    | 101 ++++++++++++++++++
>  .../ftrace/test.d/00basic/test_ownership.tc   |  16 +--
>  .../testing/selftests/ftrace/test.d/functions |  25 +++++
>  3 files changed, 129 insertions(+), 13 deletions(-)
>  create mode 100644 tools/testing/selftests/ftrace/test.d/00basic/mount_options.tc
> 
> diff --git a/tools/testing/selftests/ftrace/test.d/00basic/mount_options.tc b/tools/testing/selftests/ftrace/test.d/00basic/mount_options.tc
> new file mode 100644
> index 000000000000..b8aff85ec259
> --- /dev/null
> +++ b/tools/testing/selftests/ftrace/test.d/00basic/mount_options.tc
> @@ -0,0 +1,101 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +# description: Test tracefs GID mount option
> +# requires: "[gid=<gid>]":README
> +
> +fail() {
> +	local msg="$1"
> +
> +	echo "FAILED: $msg"
> +	exit_fail
> +}
> +
> +find_alternate_gid() {
> +	local original_gid="$1"
> +	tac /etc/group | grep -v ":$original_gid:" | head -1 | cut -d: -f3
> +}
> +
> +mount_tracefs_with_options() {
> +	local mount_point="$1"
> +	local options="$2"
> +
> +	mount -t tracefs -o "$options" nodev "$mount_point"
> +
> +	setup
> +}
> +
> +unmount_tracefs() {
> +	local mount_point="$1"
> +
> +	# Need to make sure the mount isn't busy so that we can umount it
> +	(cd $mount_point; finish_ftrace;)
> +
> +	cleanup
> +}
> +
> +create_instance() {
> +	local mount_point="$1"
> +	local instance="$mount_point/instances/$(mktemp -u test-XXXXXX)"
> +
> +	mkdir "$instance"
> +	echo "$instance"
> +}
> +
> +remove_instance() {
> +	local instance="$1"
> +
> +	rmdir "$instance"
> +}
> +
> +check_gid() {
> +	local mount_point="$1"
> +	local expected_gid="$2"
> +
> +	echo "Checking permission group ..."
> +
> +	cd "$mount_point"
> +
> +	for file in "." "events" "events/sched" "events/sched/sched_switch" "events/sched/sched_switch/enable"; do
> +		local gid=`stat -c "%g" $file`
> +		if [ "$gid" -ne "$expected_gid" ]; then
> +			cd - # Return to the previous working direcotry (tracefs root)
> +			fail "$(realpath $file): Expected group $expected_gid; Got group $gid"
> +		fi
> +	done
> +
> +	cd - # Return to the previous working direcotry (tracefs root)
> +}
> +
> +test_gid_mount_option() {
> +	local mount_point=$(get_mount_point)
> +	local mount_options=$(get_mnt_options "$mount_point")
> +	local original_group=$(stat -c "%g" .)
> +	local other_group=$(find_alternate_gid "$original_group")
> +
> +	# Set up mount options with new GID for testing
> +	local new_options=`echo "$mount_options" | sed -e "s/gid=[0-9]*/gid=$other_group/"`
> +	if [ "$new_options" = "$mount_options" ]; then
> +		new_options="$mount_options,gid=$other_group"
> +		mount_options="$mount_options,gid=$original_group"
> +	fi
> +
> +	# Unmount existing tracefs instance and mount with new GID
> +	unmount_tracefs "$mount_point"
> +	mount_tracefs_with_options "$mount_point" "$new_options"
> +
> +	check_gid "$mount_point" "$other_group"
> +
> +	# Check that files created after the mount inherit the GID
> +	local instance=$(create_instance "$mount_point")
> +	check_gid "$instance" "$other_group"
> +	remove_instance "$instance"
> +
> +	# Unmount and remount with the original GID
> +	unmount_tracefs "$mount_point"
> +	mount_tracefs_with_options "$mount_point" "$mount_options"
> +	check_gid "$mount_point" "$original_group"
> +}
> +
> +test_gid_mount_option
> +
> +exit 0
> diff --git a/tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc b/tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc
> index 094419e190c2..e71cc3ad0bdf 100644
> --- a/tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc
> +++ b/tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc
> @@ -1,24 +1,14 @@
>  #!/bin/sh
>  # SPDX-License-Identifier: GPL-2.0
>  # description: Test file and directory ownership changes for eventfs
> +# requires: "[gid=<gid>]":README
>  
>  original_group=`stat -c "%g" .`
>  original_owner=`stat -c "%u" .`
>  
> -mount_point=`stat -c '%m' .`
> +local mount_point=$(get_mount_point)
>  
> -# If stat -c '%m' does not work (e.g. busybox) or failed, try to use the
> -# current working directory (which should be a tracefs) as the mount point.
> -if [ ! -d "$mount_point" ]; then
> -	if mount | grep -qw $PWD ; then
> -		mount_point=$PWD
> -	else
> -		# If PWD doesn't work, that is an environmental problem.
> -		exit_unresolved
> -	fi
> -fi
> -
> -mount_options=`mount | grep "$mount_point" | sed -e 's/.*(\(.*\)).*/\1/'`
> +mount_options=$(get_mnt_options "$mount_point")
>  
>  # find another owner and group that is not the original
>  other_group=`tac /etc/group | grep -v ":$original_group:" | head -1 | cut -d: -f3`
> diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
> index 779f3e62ec90..84d6a9c7ad67 100644
> --- a/tools/testing/selftests/ftrace/test.d/functions
> +++ b/tools/testing/selftests/ftrace/test.d/functions
> @@ -193,3 +193,28 @@ ftrace_errlog_check() { # err-prefix command-with-error-pos-by-^ command-file
>      # "  Command: " and "^\n" => 13
>      test $(expr 13 + $pos) -eq $N
>  }
> +
> +# Helper to get the tracefs mount point
> +get_mount_point() {
> +	local mount_point=`stat -c '%m' .`
> +
> +	# If stat -c '%m' does not work (e.g. busybox) or failed, try to use the
> +	# current working directory (which should be a tracefs) as the mount point.
> +	if [ ! -d "$mount_point" ]; then
> +		if mount | grep -qw "$PWD"; then
> +			mount_point=$PWD
> +		else
> +			# If PWD doesn't work, that is an environmental problem.
> +			exit_unresolved
> +		fi
> +	fi
> +	echo "$mount_point"
> +}
> +
> +# Helper function to retrieve mount options for a given mount point
> +get_mnt_options() {
> +	local mnt_point="$1"
> +	local opts=$(mount | grep -m1 "$mnt_point" | sed -e 's/.*(\(.*\)).*/\1/')
> +
> +	echo "$opts"
> +}
> \ No newline at end of file

Re: [PATCH 2/3] tracing/selftests: Add tracefs mount options test

[Kalesh Singh]

On Mon, Oct 28, 2024 at 4:14 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Mon, 28 Oct 2024 14:43:58 -0700
> Kalesh Singh <kaleshsingh@google.com> wrote:
>
> > Add test to check the tracefs gid mount option is applied correctly
> >
> >    ./ftracetest test.d/00basic/mount_options.tc
> >
> > Use the new readme string "[gid=<gid>] as a requirement and also update
> > test_ownership.tc requirements to use this.
> >
> > mount_options.tc will fail currently, this is fixed by the subsequent
> > patch in this series.
>
> Test cases should never be added when they can fail. They should always
> come after the fix is applied. But it appears that you check the README
> to make sure that it does work and not fail.
>
> I'll take a look at these patches in more detail later.

Hi Steve,

Thank you for the quick reviews, please feel free to ignore until you are back.

I'll address these comments and resend a v2.

Thanks,
Kalesh

>
> Thanks,
>
> -- Steve
>
>
> >
> > Cc: David Howells <dhowells@redhat.com>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
> > ---
> >  .../ftrace/test.d/00basic/mount_options.tc    | 101 ++++++++++++++++++
> >  .../ftrace/test.d/00basic/test_ownership.tc   |  16 +--
> >  .../testing/selftests/ftrace/test.d/functions |  25 +++++
> >  3 files changed, 129 insertions(+), 13 deletions(-)
> >  create mode 100644 tools/testing/selftests/ftrace/test.d/00basic/mount_options.tc
> >
> > diff --git a/tools/testing/selftests/ftrace/test.d/00basic/mount_options.tc b/tools/testing/selftests/ftrace/test.d/00basic/mount_options.tc
> > new file mode 100644
> > index 000000000000..b8aff85ec259
> > --- /dev/null
> > +++ b/tools/testing/selftests/ftrace/test.d/00basic/mount_options.tc
> > @@ -0,0 +1,101 @@
> > +#!/bin/sh
> > +# SPDX-License-Identifier: GPL-2.0
> > +# description: Test tracefs GID mount option
> > +# requires: "[gid=<gid>]":README
> > +
> > +fail() {
> > +     local msg="$1"
> > +
> > +     echo "FAILED: $msg"
> > +     exit_fail
> > +}
> > +
> > +find_alternate_gid() {
> > +     local original_gid="$1"
> > +     tac /etc/group | grep -v ":$original_gid:" | head -1 | cut -d: -f3
> > +}
> > +
> > +mount_tracefs_with_options() {
> > +     local mount_point="$1"
> > +     local options="$2"
> > +
> > +     mount -t tracefs -o "$options" nodev "$mount_point"
> > +
> > +     setup
> > +}
> > +
> > +unmount_tracefs() {
> > +     local mount_point="$1"
> > +
> > +     # Need to make sure the mount isn't busy so that we can umount it
> > +     (cd $mount_point; finish_ftrace;)
> > +
> > +     cleanup
> > +}
> > +
> > +create_instance() {
> > +     local mount_point="$1"
> > +     local instance="$mount_point/instances/$(mktemp -u test-XXXXXX)"
> > +
> > +     mkdir "$instance"
> > +     echo "$instance"
> > +}
> > +
> > +remove_instance() {
> > +     local instance="$1"
> > +
> > +     rmdir "$instance"
> > +}
> > +
> > +check_gid() {
> > +     local mount_point="$1"
> > +     local expected_gid="$2"
> > +
> > +     echo "Checking permission group ..."
> > +
> > +     cd "$mount_point"
> > +
> > +     for file in "." "events" "events/sched" "events/sched/sched_switch" "events/sched/sched_switch/enable"; do
> > +             local gid=`stat -c "%g" $file`
> > +             if [ "$gid" -ne "$expected_gid" ]; then
> > +                     cd - # Return to the previous working direcotry (tracefs root)
> > +                     fail "$(realpath $file): Expected group $expected_gid; Got group $gid"
> > +             fi
> > +     done
> > +
> > +     cd - # Return to the previous working direcotry (tracefs root)
> > +}
> > +
> > +test_gid_mount_option() {
> > +     local mount_point=$(get_mount_point)
> > +     local mount_options=$(get_mnt_options "$mount_point")
> > +     local original_group=$(stat -c "%g" .)
> > +     local other_group=$(find_alternate_gid "$original_group")
> > +
> > +     # Set up mount options with new GID for testing
> > +     local new_options=`echo "$mount_options" | sed -e "s/gid=[0-9]*/gid=$other_group/"`
> > +     if [ "$new_options" = "$mount_options" ]; then
> > +             new_options="$mount_options,gid=$other_group"
> > +             mount_options="$mount_options,gid=$original_group"
> > +     fi
> > +
> > +     # Unmount existing tracefs instance and mount with new GID
> > +     unmount_tracefs "$mount_point"
> > +     mount_tracefs_with_options "$mount_point" "$new_options"
> > +
> > +     check_gid "$mount_point" "$other_group"
> > +
> > +     # Check that files created after the mount inherit the GID
> > +     local instance=$(create_instance "$mount_point")
> > +     check_gid "$instance" "$other_group"
> > +     remove_instance "$instance"
> > +
> > +     # Unmount and remount with the original GID
> > +     unmount_tracefs "$mount_point"
> > +     mount_tracefs_with_options "$mount_point" "$mount_options"
> > +     check_gid "$mount_point" "$original_group"
> > +}
> > +
> > +test_gid_mount_option
> > +
> > +exit 0
> > diff --git a/tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc b/tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc
> > index 094419e190c2..e71cc3ad0bdf 100644
> > --- a/tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc
> > +++ b/tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc
> > @@ -1,24 +1,14 @@
> >  #!/bin/sh
> >  # SPDX-License-Identifier: GPL-2.0
> >  # description: Test file and directory ownership changes for eventfs
> > +# requires: "[gid=<gid>]":README
> >
> >  original_group=`stat -c "%g" .`
> >  original_owner=`stat -c "%u" .`
> >
> > -mount_point=`stat -c '%m' .`
> > +local mount_point=$(get_mount_point)
> >
> > -# If stat -c '%m' does not work (e.g. busybox) or failed, try to use the
> > -# current working directory (which should be a tracefs) as the mount point.
> > -if [ ! -d "$mount_point" ]; then
> > -     if mount | grep -qw $PWD ; then
> > -             mount_point=$PWD
> > -     else
> > -             # If PWD doesn't work, that is an environmental problem.
> > -             exit_unresolved
> > -     fi
> > -fi
> > -
> > -mount_options=`mount | grep "$mount_point" | sed -e 's/.*(\(.*\)).*/\1/'`
> > +mount_options=$(get_mnt_options "$mount_point")
> >
> >  # find another owner and group that is not the original
> >  other_group=`tac /etc/group | grep -v ":$original_group:" | head -1 | cut -d: -f3`
> > diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
> > index 779f3e62ec90..84d6a9c7ad67 100644
> > --- a/tools/testing/selftests/ftrace/test.d/functions
> > +++ b/tools/testing/selftests/ftrace/test.d/functions
> > @@ -193,3 +193,28 @@ ftrace_errlog_check() { # err-prefix command-with-error-pos-by-^ command-file
> >      # "  Command: " and "^\n" => 13
> >      test $(expr 13 + $pos) -eq $N
> >  }
> > +
> > +# Helper to get the tracefs mount point
> > +get_mount_point() {
> > +     local mount_point=`stat -c '%m' .`
> > +
> > +     # If stat -c '%m' does not work (e.g. busybox) or failed, try to use the
> > +     # current working directory (which should be a tracefs) as the mount point.
> > +     if [ ! -d "$mount_point" ]; then
> > +             if mount | grep -qw "$PWD"; then
> > +                     mount_point=$PWD
> > +             else
> > +                     # If PWD doesn't work, that is an environmental problem.
> > +                     exit_unresolved
> > +             fi
> > +     fi
> > +     echo "$mount_point"
> > +}
> > +
> > +# Helper function to retrieve mount options for a given mount point
> > +get_mnt_options() {
> > +     local mnt_point="$1"
> > +     local opts=$(mount | grep -m1 "$mnt_point" | sed -e 's/.*(\(.*\)).*/\1/')
> > +
> > +     echo "$opts"
> > +}
> > \ No newline at end of file
>

Re: [PATCH 2/3] tracing/selftests: Add tracefs mount options test

[Kalesh Singh]

On Mon, Oct 28, 2024 at 4:23 PM Kalesh Singh <kaleshsingh@google.com> wrote:
>
> On Mon, Oct 28, 2024 at 4:14 PM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On Mon, 28 Oct 2024 14:43:58 -0700
> > Kalesh Singh <kaleshsingh@google.com> wrote:
> >
> > > Add test to check the tracefs gid mount option is applied correctly
> > >
> > >    ./ftracetest test.d/00basic/mount_options.tc
> > >
> > > Use the new readme string "[gid=<gid>] as a requirement and also update
> > > test_ownership.tc requirements to use this.
> > >
> > > mount_options.tc will fail currently, this is fixed by the subsequent
> > > patch in this series.
> >
> > Test cases should never be added when they can fail. They should always
> > come after the fix is applied. But it appears that you check the README
> > to make sure that it does work and not fail.
> >
> > I'll take a look at these patches in more detail later.
>
> Hi Steve,
>
> Thank you for the quick reviews, please feel free to ignore until you are back.
>
> I'll address these comments and resend a v2.

I've posted v2 at:
https://lore.kernel.org/r/20241030171928.4168869-1-kaleshsingh@google.com/

Thanks,
Kalesh

>
> Thanks,
> Kalesh
>
> >
> > Thanks,
> >
> > -- Steve
> >
> >
> > >
> > > Cc: David Howells <dhowells@redhat.com>
> > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > > Signed-off-by: Kalesh Singh <kaleshsingh@google.com>
> > > ---
> > >  .../ftrace/test.d/00basic/mount_options.tc    | 101 ++++++++++++++++++
> > >  .../ftrace/test.d/00basic/test_ownership.tc   |  16 +--
> > >  .../testing/selftests/ftrace/test.d/functions |  25 +++++
> > >  3 files changed, 129 insertions(+), 13 deletions(-)
> > >  create mode 100644 tools/testing/selftests/ftrace/test.d/00basic/mount_options.tc
> > >
> > > diff --git a/tools/testing/selftests/ftrace/test.d/00basic/mount_options.tc b/tools/testing/selftests/ftrace/test.d/00basic/mount_options.tc
> > > new file mode 100644
> > > index 000000000000..b8aff85ec259
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/ftrace/test.d/00basic/mount_options.tc
> > > @@ -0,0 +1,101 @@
> > > +#!/bin/sh
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# description: Test tracefs GID mount option
> > > +# requires: "[gid=<gid>]":README
> > > +
> > > +fail() {
> > > +     local msg="$1"
> > > +
> > > +     echo "FAILED: $msg"
> > > +     exit_fail
> > > +}
> > > +
> > > +find_alternate_gid() {
> > > +     local original_gid="$1"
> > > +     tac /etc/group | grep -v ":$original_gid:" | head -1 | cut -d: -f3
> > > +}
> > > +
> > > +mount_tracefs_with_options() {
> > > +     local mount_point="$1"
> > > +     local options="$2"
> > > +
> > > +     mount -t tracefs -o "$options" nodev "$mount_point"
> > > +
> > > +     setup
> > > +}
> > > +
> > > +unmount_tracefs() {
> > > +     local mount_point="$1"
> > > +
> > > +     # Need to make sure the mount isn't busy so that we can umount it
> > > +     (cd $mount_point; finish_ftrace;)
> > > +
> > > +     cleanup
> > > +}
> > > +
> > > +create_instance() {
> > > +     local mount_point="$1"
> > > +     local instance="$mount_point/instances/$(mktemp -u test-XXXXXX)"
> > > +
> > > +     mkdir "$instance"
> > > +     echo "$instance"
> > > +}
> > > +
> > > +remove_instance() {
> > > +     local instance="$1"
> > > +
> > > +     rmdir "$instance"
> > > +}
> > > +
> > > +check_gid() {
> > > +     local mount_point="$1"
> > > +     local expected_gid="$2"
> > > +
> > > +     echo "Checking permission group ..."
> > > +
> > > +     cd "$mount_point"
> > > +
> > > +     for file in "." "events" "events/sched" "events/sched/sched_switch" "events/sched/sched_switch/enable"; do
> > > +             local gid=`stat -c "%g" $file`
> > > +             if [ "$gid" -ne "$expected_gid" ]; then
> > > +                     cd - # Return to the previous working direcotry (tracefs root)
> > > +                     fail "$(realpath $file): Expected group $expected_gid; Got group $gid"
> > > +             fi
> > > +     done
> > > +
> > > +     cd - # Return to the previous working direcotry (tracefs root)
> > > +}
> > > +
> > > +test_gid_mount_option() {
> > > +     local mount_point=$(get_mount_point)
> > > +     local mount_options=$(get_mnt_options "$mount_point")
> > > +     local original_group=$(stat -c "%g" .)
> > > +     local other_group=$(find_alternate_gid "$original_group")
> > > +
> > > +     # Set up mount options with new GID for testing
> > > +     local new_options=`echo "$mount_options" | sed -e "s/gid=[0-9]*/gid=$other_group/"`
> > > +     if [ "$new_options" = "$mount_options" ]; then
> > > +             new_options="$mount_options,gid=$other_group"
> > > +             mount_options="$mount_options,gid=$original_group"
> > > +     fi
> > > +
> > > +     # Unmount existing tracefs instance and mount with new GID
> > > +     unmount_tracefs "$mount_point"
> > > +     mount_tracefs_with_options "$mount_point" "$new_options"
> > > +
> > > +     check_gid "$mount_point" "$other_group"
> > > +
> > > +     # Check that files created after the mount inherit the GID
> > > +     local instance=$(create_instance "$mount_point")
> > > +     check_gid "$instance" "$other_group"
> > > +     remove_instance "$instance"
> > > +
> > > +     # Unmount and remount with the original GID
> > > +     unmount_tracefs "$mount_point"
> > > +     mount_tracefs_with_options "$mount_point" "$mount_options"
> > > +     check_gid "$mount_point" "$original_group"
> > > +}
> > > +
> > > +test_gid_mount_option
> > > +
> > > +exit 0
> > > diff --git a/tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc b/tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc
> > > index 094419e190c2..e71cc3ad0bdf 100644
> > > --- a/tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc
> > > +++ b/tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc
> > > @@ -1,24 +1,14 @@
> > >  #!/bin/sh
> > >  # SPDX-License-Identifier: GPL-2.0
> > >  # description: Test file and directory ownership changes for eventfs
> > > +# requires: "[gid=<gid>]":README
> > >
> > >  original_group=`stat -c "%g" .`
> > >  original_owner=`stat -c "%u" .`
> > >
> > > -mount_point=`stat -c '%m' .`
> > > +local mount_point=$(get_mount_point)
> > >
> > > -# If stat -c '%m' does not work (e.g. busybox) or failed, try to use the
> > > -# current working directory (which should be a tracefs) as the mount point.
> > > -if [ ! -d "$mount_point" ]; then
> > > -     if mount | grep -qw $PWD ; then
> > > -             mount_point=$PWD
> > > -     else
> > > -             # If PWD doesn't work, that is an environmental problem.
> > > -             exit_unresolved
> > > -     fi
> > > -fi
> > > -
> > > -mount_options=`mount | grep "$mount_point" | sed -e 's/.*(\(.*\)).*/\1/'`
> > > +mount_options=$(get_mnt_options "$mount_point")
> > >
> > >  # find another owner and group that is not the original
> > >  other_group=`tac /etc/group | grep -v ":$original_group:" | head -1 | cut -d: -f3`
> > > diff --git a/tools/testing/selftests/ftrace/test.d/functions b/tools/testing/selftests/ftrace/test.d/functions
> > > index 779f3e62ec90..84d6a9c7ad67 100644
> > > --- a/tools/testing/selftests/ftrace/test.d/functions
> > > +++ b/tools/testing/selftests/ftrace/test.d/functions
> > > @@ -193,3 +193,28 @@ ftrace_errlog_check() { # err-prefix command-with-error-pos-by-^ command-file
> > >      # "  Command: " and "^\n" => 13
> > >      test $(expr 13 + $pos) -eq $N
> > >  }
> > > +
> > > +# Helper to get the tracefs mount point
> > > +get_mount_point() {
> > > +     local mount_point=`stat -c '%m' .`
> > > +
> > > +     # If stat -c '%m' does not work (e.g. busybox) or failed, try to use the
> > > +     # current working directory (which should be a tracefs) as the mount point.
> > > +     if [ ! -d "$mount_point" ]; then
> > > +             if mount | grep -qw "$PWD"; then
> > > +                     mount_point=$PWD
> > > +             else
> > > +                     # If PWD doesn't work, that is an environmental problem.
> > > +                     exit_unresolved
> > > +             fi
> > > +     fi
> > > +     echo "$mount_point"
> > > +}
> > > +
> > > +# Helper function to retrieve mount options for a given mount point
> > > +get_mnt_options() {
> > > +     local mnt_point="$1"
> > > +     local opts=$(mount | grep -m1 "$mnt_point" | sed -e 's/.*(\(.*\)).*/\1/')
> > > +
> > > +     echo "$opts"
> > > +}
> > > \ No newline at end of file
> >

[PATCH 3/3] tracing: Fix tracefs gid mount option

[Kalesh Singh]

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.

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.

(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 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@redhat.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Kalesh Singh <kaleshsingh@google.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;
+
 	sync_filesystem(sb);
 	/* structure copy of new mount options to sb */
 	*sb_opts = *new_opts;
@@ -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)
-- 
2.47.0.163.g1226f6d8fa-goog

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

[Eric Sandeen]

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@suse.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@redhat.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Kalesh Singh <kaleshsingh@google.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)

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

[Kalesh Singh]

On Mon, Oct 28, 2024 at 8:00 PM Eric Sandeen <sandeen@redhat.com> wrote:
>
> 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.

Hi Eric,

You are right, they same applies to any of the mount options. I'll
update the commit text for v2.

>
> > 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?
>

I'm not sure if other filesystems are broken in the same way. The
issue happened for tracefs due to the fact that the sb root is created
before  calling mount -- from init_trace_printk_function_export(). If
there are other filesystems that have similar early initialization
(before mount) they may be broken as well.

> 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@suse.de>
> Date:   Mon Jan 17 09:07:26 2022 +1100
>
>     devtmpfs regression fix: reconfigure on each mount
>

Interesting, yes it seems like the same root cause. i.e. devtmpfs sb
root is created early on from  kernel_init() .... --> driver_init()
---> devtmpfs_init() which setups up the root inode for devtmpfs in
shmem_get_tree() ... -> vfs_get_super(); which is the case (4),
meaning that it would have ignored the options on mount for the same
reason: the superblock root indoe already exists.

I'm not very familiar with the filesystem area. If this is a common
scenario, maybe there needs to be a separate API to handle this case?


> > 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@redhat.com>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Masami Hiramatsu <mhiramat@kernel.org>
> > Signed-off-by: Kalesh Singh <kaleshsingh@google.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?

From init_trace_printk_function_export() the first time the super
block is allocated and added to file_system_type->fs_supers;
fc->s_fs_info is reset to NULL. [1]. I think that is ok since the
fs_info would have already been copied to the super block. [2]


See sget_fc():

[1] https://github.com/torvalds/linux/blob/v6.12-rc4/fs/super.c#L774
[2] https://github.com/torvalds/linux/blob/v6.12-rc4/fs/super.c#L766


--Kalesh
>
> > +
> >       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)
>