| Message ID | 20231221211229.13398ef3@gandalf.local.home (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v3] tracing/selftests: Add ownership modification tests for eventfs | expand |
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 <rostedt@goodmis.org> wrote: > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > 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) <rostedt@goodmis.org> > --- >
On Thu, 21 Dec 2023 21:12:29 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > 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) <mhiramat@kernel.org> Tested-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > --- > Changes since v2: https://lore.kernel.org/linux-trace-kernel/20231221194516.53e1ee43@gandalf.local.home > > - 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 > >
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 <skhan@linuxfoundation.org> 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 <rostedt@goodmis.org> 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)" <rostedt@goodmis.org> 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/20231222112831.4c7fa500@gandalf.local.home/ Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> --- 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
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