From: "Steven Rostedt (Google)" <[email protected]>
As there were bugs found with the ownership of eventfs dynamic file
creation. Add a test to test it.
It will remount tracefs with a different gid and check the ownership of
the eventfs directory, as well as the system and event directories. It
will also check the event file directories.
It then does a chgrp on each of these as well to see if they all get
updated as expected.
Then it remounts the tracefs file system back to the original group and
makes sure that all the updated files and directories were reset back to
the original ownership.
It does the same for instances that change the ownership of he instance
directory.
Note, because the uid is not reset by a remount, it is tested for every
file by switching it to a new owner and then back again.
Signed-off-by: Steven Rostedt (Google) <[email protected]>
---
Changes since v2: https://lore.kernel.org/linux-trace-kernel/[email protected]
- Changed the instance test name from "foo-$(mktemp -u XXXXX)" to
"$(mktemp -u test-XXXXXX)" as Masami reported that busybox mktemp only
works with 6 Xs and not 5. Also changed "foo" to "test" and placed it
into the mktemp format.
.../ftrace/test.d/00basic/test_ownership.tc | 113 ++++++++++++++++++
1 file changed, 113 insertions(+)
create mode 100755 tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc
diff --git a/tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc b/tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc
new file mode 100755
index 000000000000..4c20be3a714a
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc
@@ -0,0 +1,113 @@
+#!/bin/sh
+# description: Test file and directory owership changes for eventfs
+
+original_group=`stat -c "%g" .`
+original_owner=`stat -c "%u" .`
+
+mount_point=`stat -c '%m' .`
+mount_options=`mount | grep "$mount_point" | sed -e 's/.*(\(.*\)).*/\1/'`
+
+# find another owner and group that is not the original
+other_group=`tac /etc/group | grep -v ":$original_group:" | head -1 | cut -d: -f3`
+other_owner=`tac /etc/passwd | grep -v ":$original_owner:" | head -1 | cut -d: -f3`
+
+# Remove any group ownership already
+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
+
+canary="events/timer events/timer/timer_cancel events/timer/timer_cancel/format"
+
+test() {
+ file=$1
+ test_group=$2
+
+ owner=`stat -c "%u" $file`
+ group=`stat -c "%g" $file`
+
+ echo "testing $file $owner=$original_owner and $group=$test_group"
+ if [ $owner -ne $original_owner ]; then
+ exit_fail
+ fi
+ if [ $group -ne $test_group ]; then
+ exit_fail
+ fi
+
+ # Note, the remount does not update ownership so test going to and from owner
+ echo "test owner $file to $other_owner"
+ chown $other_owner $file
+ owner=`stat -c "%u" $file`
+ if [ $owner -ne $other_owner ]; then
+ exit_fail
+ fi
+
+ chown $original_owner $file
+ owner=`stat -c "%u" $file`
+ if [ $owner -ne $original_owner ]; then
+ exit_fail
+ fi
+
+}
+
+run_tests() {
+ for d in "." "events" "events/sched" "events/sched/sched_switch" "events/sched/sched_switch/enable" $canary; do
+ test "$d" $other_group
+ done
+
+ chgrp $original_group events
+ test "events" $original_group
+ for d in "." "events/sched" "events/sched/sched_switch" "events/sched/sched_switch/enable" $canary; do
+ test "$d" $other_group
+ done
+
+ chgrp $original_group events/sched
+ test "events/sched" $original_group
+ for d in "." "events/sched/sched_switch" "events/sched/sched_switch/enable" $canary; do
+ test "$d" $other_group
+ done
+
+ chgrp $original_group events/sched/sched_switch
+ test "events/sched/sched_switch" $original_group
+ for d in "." "events/sched/sched_switch/enable" $canary; do
+ test "$d" $other_group
+ done
+
+ chgrp $original_group events/sched/sched_switch/enable
+ test "events/sched/sched_switch/enable" $original_group
+ for d in "." $canary; do
+ test "$d" $other_group
+ done
+}
+
+mount -o remount,"$new_options" .
+
+run_tests
+
+mount -o remount,"$mount_options" .
+
+for d in "." "events" "events/sched" "events/sched/sched_switch" "events/sched/sched_switch/enable" $canary; do
+ test "$d" $original_group
+done
+
+# check instances as well
+
+chgrp $other_group instances
+
+instance="$(mktemp -u test-XXXXXX)"
+
+mkdir instances/$instance
+
+cd instances/$instance
+
+run_tests
+
+cd ../..
+
+rmdir instances/$instance
+
+chgrp $original_group instances
+
+exit 0
--
2.42.0
Shuah,
This patch has no dependencies. You can take it through your tree for the
next merge window if you want. If not, I can take it.
Thanks,
-- Steve
On Thu, 21 Dec 2023 21:12:29 -0500
Steven Rostedt <[email protected]> wrote:
> From: "Steven Rostedt (Google)" <[email protected]>
>
> As there were bugs found with the ownership of eventfs dynamic file
> creation. Add a test to test it.
>
> It will remount tracefs with a different gid and check the ownership of
> the eventfs directory, as well as the system and event directories. It
> will also check the event file directories.
>
> It then does a chgrp on each of these as well to see if they all get
> updated as expected.
>
> Then it remounts the tracefs file system back to the original group and
> makes sure that all the updated files and directories were reset back to
> the original ownership.
>
> It does the same for instances that change the ownership of he instance
> directory.
>
> Note, because the uid is not reset by a remount, it is tested for every
> file by switching it to a new owner and then back again.
>
> Signed-off-by: Steven Rostedt (Google) <[email protected]>
> ---
>
On Thu, 21 Dec 2023 21:12:29 -0500
Steven Rostedt <[email protected]> wrote:
> From: "Steven Rostedt (Google)" <[email protected]>
>
> As there were bugs found with the ownership of eventfs dynamic file
> creation. Add a test to test it.
>
> It will remount tracefs with a different gid and check the ownership of
> the eventfs directory, as well as the system and event directories. It
> will also check the event file directories.
>
> It then does a chgrp on each of these as well to see if they all get
> updated as expected.
>
> Then it remounts the tracefs file system back to the original group and
> makes sure that all the updated files and directories were reset back to
> the original ownership.
>
> It does the same for instances that change the ownership of he instance
> directory.
>
> Note, because the uid is not reset by a remount, it is tested for every
> file by switching it to a new owner and then back again.
>
Thanks for update!
Acked-by: Masami Hiramatsu (Google) <[email protected]>
Tested-by: Masami Hiramatsu (Google) <[email protected]>
> Signed-off-by: Steven Rostedt (Google) <[email protected]>
> ---
> Changes since v2: https://lore.kernel.org/linux-trace-kernel/[email protected]
>
> - Changed the instance test name from "foo-$(mktemp -u XXXXX)" to
> "$(mktemp -u test-XXXXXX)" as Masami reported that busybox mktemp only
> works with 6 Xs and not 5. Also changed "foo" to "test" and placed it
> into the mktemp format.
>
> .../ftrace/test.d/00basic/test_ownership.tc | 113 ++++++++++++++++++
> 1 file changed, 113 insertions(+)
> create mode 100755 tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc
>
> diff --git a/tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc b/tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc
> new file mode 100755
> index 000000000000..4c20be3a714a
> --- /dev/null
> +++ b/tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc
> @@ -0,0 +1,113 @@
> +#!/bin/sh
> +# description: Test file and directory owership changes for eventfs
> +
> +original_group=`stat -c "%g" .`
> +original_owner=`stat -c "%u" .`
> +
> +mount_point=`stat -c '%m' .`
> +mount_options=`mount | grep "$mount_point" | sed -e 's/.*(\(.*\)).*/\1/'`
> +
> +# find another owner and group that is not the original
> +other_group=`tac /etc/group | grep -v ":$original_group:" | head -1 | cut -d: -f3`
> +other_owner=`tac /etc/passwd | grep -v ":$original_owner:" | head -1 | cut -d: -f3`
> +
> +# Remove any group ownership already
> +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
> +
> +canary="events/timer events/timer/timer_cancel events/timer/timer_cancel/format"
> +
> +test() {
> + file=$1
> + test_group=$2
> +
> + owner=`stat -c "%u" $file`
> + group=`stat -c "%g" $file`
> +
> + echo "testing $file $owner=$original_owner and $group=$test_group"
> + if [ $owner -ne $original_owner ]; then
> + exit_fail
> + fi
> + if [ $group -ne $test_group ]; then
> + exit_fail
> + fi
> +
> + # Note, the remount does not update ownership so test going to and from owner
> + echo "test owner $file to $other_owner"
> + chown $other_owner $file
> + owner=`stat -c "%u" $file`
> + if [ $owner -ne $other_owner ]; then
> + exit_fail
> + fi
> +
> + chown $original_owner $file
> + owner=`stat -c "%u" $file`
> + if [ $owner -ne $original_owner ]; then
> + exit_fail
> + fi
> +
> +}
> +
> +run_tests() {
> + for d in "." "events" "events/sched" "events/sched/sched_switch" "events/sched/sched_switch/enable" $canary; do
> + test "$d" $other_group
> + done
> +
> + chgrp $original_group events
> + test "events" $original_group
> + for d in "." "events/sched" "events/sched/sched_switch" "events/sched/sched_switch/enable" $canary; do
> + test "$d" $other_group
> + done
> +
> + chgrp $original_group events/sched
> + test "events/sched" $original_group
> + for d in "." "events/sched/sched_switch" "events/sched/sched_switch/enable" $canary; do
> + test "$d" $other_group
> + done
> +
> + chgrp $original_group events/sched/sched_switch
> + test "events/sched/sched_switch" $original_group
> + for d in "." "events/sched/sched_switch/enable" $canary; do
> + test "$d" $other_group
> + done
> +
> + chgrp $original_group events/sched/sched_switch/enable
> + test "events/sched/sched_switch/enable" $original_group
> + for d in "." $canary; do
> + test "$d" $other_group
> + done
> +}
> +
> +mount -o remount,"$new_options" .
> +
> +run_tests
> +
> +mount -o remount,"$mount_options" .
> +
> +for d in "." "events" "events/sched" "events/sched/sched_switch" "events/sched/sched_switch/enable" $canary; do
> + test "$d" $original_group
> +done
> +
> +# check instances as well
> +
> +chgrp $other_group instances
> +
> +instance="$(mktemp -u test-XXXXXX)"
> +
> +mkdir instances/$instance
> +
> +cd instances/$instance
> +
> +run_tests
> +
> +cd ../..
> +
> +rmdir instances/$instance
> +
> +chgrp $original_group instances
> +
> +exit 0
> --
> 2.42.0
>
>
--
Masami Hiramatsu (Google) <[email protected]>
On 12/21/23 19:16, Steven Rostedt wrote:
>
> Shuah,
>
> This patch has no dependencies. You can take it through your tree for the
> next merge window if you want. If not, I can take it.
>
Tried to apply this and seeing a couple of issues:
-- missing SPDX
-- this file has executable permission set unlike the existing
.tc files in the same directory
ERROR: do not set execute permissions for source files
#72: FILE: tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#72:
new file mode 100755
WARNING: Missing or malformed SPDX-License-Identifier tag in line 2
#78: FILE: tools/testing/selftests/ftrace/test.d/00basic/test_ownership.tc:2:
+# description: Test file and directory owership changes for eventfs
total: 1 errors, 2 warnings, 113 lines checked
thanks,
-- Shuah
On Fri, 22 Dec 2023 09:15:42 -0700
Shuah Khan <[email protected]> wrote:
> On 12/21/23 19:16, Steven Rostedt wrote:
> >
> > Shuah,
> >
> > This patch has no dependencies. You can take it through your tree for the
> > next merge window if you want. If not, I can take it.
> >
> Tried to apply this and seeing a couple of issues:
>
> -- missing SPDX
Hmm, I copied this from the snapshot.tc. I guess there's several test files
that are missing SPDX. That should probably be fixed.
> -- this file has executable permission set unlike the existing
> .tc files in the same directory
Oh, I forgot to disable that. When developing a new test I make it
standalone as it's easier to test the test, and then copy the file into the
directory.
I'll fix the above and send a v4.
Thanks,
-- Steve
On Fri, 22 Dec 2023 11:28:31 -0500
Steven Rostedt <[email protected]> wrote:
> > -- this file has executable permission set unlike the existing
> > .tc files in the same directory
>
> Oh, I forgot to disable that. When developing a new test I make it
> standalone as it's easier to test the test, and then copy the file into the
> directory.
>
I noticed that I did the same for trace_marker.tc that's in my for-next
queue. Instead of rebasing, as there's other commits on top of it, I'm just
going to push this to my for-next queue.
-- Steve
From 26547691107eda45b0f20ee79bad19bbe5fcbfd7 Mon Sep 17 00:00:00 2001
From: "Steven Rostedt (Google)" <[email protected]>
Date: Fri, 22 Dec 2023 11:35:22 -0500
Subject: [PATCH] tracing/selftests: Remove exec permissions from
trace_marker.tc test
The tests in the selftests should not have the exec permissions set. The
trace_marker.tc test accidentally was committed with the exec permission.
Set the permission to that file back to just read/write.
No functional nor code changes.
Link: https://lore.kernel.org/linux-trace-kernel/[email protected]/
Signed-off-by: Steven Rostedt (Google) <[email protected]>
---
tools/testing/selftests/ftrace/test.d/00basic/trace_marker.tc | 0
1 file changed, 0 insertions(+), 0 deletions(-)
mode change 100755 => 100644 tools/testing/selftests/ftrace/test.d/00basic/trace_marker.tc
diff --git a/tools/testing/selftests/ftrace/test.d/00basic/trace_marker.tc b/tools/testing/selftests/ftrace/test.d/00basic/trace_marker.tc
old mode 100755
new mode 100644
--
2.42.0