| Message ID | 20231221194516.53e1ee43@gandalf.local.home (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] tracing/selftests: Add ownership modification tests for eventfs | expand |
Hi Steve, On Thu, 21 Dec 2023 19:45:16 -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. > The testcase itself is OK but is there any way to identify the system supports eventfs or not? I ran this test on v6.5.13 for checking then it failed. We may need to skip (unsupported) this test for such case. Thank you, > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > --- > Changes since v1: https://lore.kernel.org/linux-trace-kernel/20231221193551.13a0b7bd@gandalf.local.home > > - Fixed a cut and paste error of using $original_group for finding another uid > > .../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..83cbd116d06b > --- /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="foo-$(mktemp -u XXXXX)" > + > +mkdir instances/$instance > + > +cd instances/$instance > + > +run_tests > + > +cd ../.. > + > +rmdir instances/$instance > + > +chgrp $original_group instances > + > +exit 0 > -- > 2.42.0 > >
On Fri, 22 Dec 2023 10:21:48 +0900 Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > The testcase itself is OK but is there any way to identify the system > supports eventfs or not? I ran this test on v6.5.13 for checking then > it failed. We may need to skip (unsupported) this test for such case. Hmm, honestly, it should technically work on all past versions. I'll try it out to see what fails for 6.5.13. Perhaps there was another bug that the stable releases need fixing for? -- Steve
On Thu, 21 Dec 2023 20:28:13 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > On Fri, 22 Dec 2023 10:21:48 +0900 > Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > > > The testcase itself is OK but is there any way to identify the system > > supports eventfs or not? I ran this test on v6.5.13 for checking then > > it failed. We may need to skip (unsupported) this test for such case. > > Hmm, honestly, it should technically work on all past versions. Ah, sorry. It was my mistake. I need to make /etc/group,passwd files in my test environment. Let me try it again. > > I'll try it out to see what fails for 6.5.13. Perhaps there was another bug > that the stable releases need fixing for? > > -- Steve
On Thu, 21 Dec 2023 20:28:13 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > On Fri, 22 Dec 2023 10:21:48 +0900 > Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > > > The testcase itself is OK but is there any way to identify the system > > supports eventfs or not? I ran this test on v6.5.13 for checking then > > it failed. We may need to skip (unsupported) this test for such case. > > Hmm, honestly, it should technically work on all past versions. > > I'll try it out to see what fails for 6.5.13. Perhaps there was another bug > that the stable releases need fixing for? I found that the failure was my environmental issue. BTW, for busybox environment, +instance="foo-$(mktemp -u XXXXX)" This doesn't work. it needs XXXXXX (6 times X). And this is somewhat wrong usage of mktemp because it can not check there is foo-<random>. What about change it as instance="$(mktemp -u foo-XXXXXX)" ? Thanks, > > -- Steve
On Fri, 22 Dec 2023 10:48:41 +0900 Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > On Thu, 21 Dec 2023 20:28:13 -0500 > Steven Rostedt <rostedt@goodmis.org> wrote: > > > On Fri, 22 Dec 2023 10:21:48 +0900 > > Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > > > > > The testcase itself is OK but is there any way to identify the system > > > supports eventfs or not? I ran this test on v6.5.13 for checking then > > > it failed. We may need to skip (unsupported) this test for such case. > > > > Hmm, honestly, it should technically work on all past versions. > > > > I'll try it out to see what fails for 6.5.13. Perhaps there was another bug > > that the stable releases need fixing for? > > I found that the failure was my environmental issue. > BTW, for busybox environment, > > +instance="foo-$(mktemp -u XXXXX)" > > This doesn't work. it needs XXXXXX (6 times X). And this is > somewhat wrong usage of mktemp because it can not check there is > foo-<random>. > What about change it as > > instance="$(mktemp -u foo-XXXXXX)" > > ? And I confirmed that this test passed on v6.5.13 with that change. Thank you, > > Thanks, > > > > > -- Steve > > > -- > Masami Hiramatsu (Google) <mhiramat@kernel.org>
On Fri, 22 Dec 2023 10:52:00 +0900 Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > > +instance="foo-$(mktemp -u XXXXX)" > > > > This doesn't work. it needs XXXXXX (6 times X). And this is > > somewhat wrong usage of mktemp because it can not check there is > > foo-<random>. > > What about change it as > > > > instance="$(mktemp -u foo-XXXXXX)" That can work too, although I think I'll change it from "foo" to "test". > > > > ? > > And I confirmed that this test passed on v6.5.13 with that change. Thanks, I'll send out a v3 with 6 Xs. ;-) -- Steve
On Fri, 22 Dec 2023 10:52:00 +0900 Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > On Fri, 22 Dec 2023 10:48:41 +0900 > Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > And I confirmed that this test passed on v6.5.13 with that change. > I just ran it on 6.5.13 and it took *forever*! But I do have a bit of debug, and before 6.6 creating the instance and deleting it required creating and deleting thousands of inodes and dentries. -- Steve
On Thu, 21 Dec 2023 21:07:57 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > On Fri, 22 Dec 2023 10:52:00 +0900 > Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > > > On Fri, 22 Dec 2023 10:48:41 +0900 > > Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > > > And I confirmed that this test passed on v6.5.13 with that change. > > > > I just ran it on 6.5.13 and it took *forever*! > > But I do have a bit of debug, and before 6.6 creating the instance and > deleting it required creating and deleting thousands of inodes and dentries. Hmm, it may depends on the machine. I could ran it (on 64 vcpu VM) Thank you, > > -- Steve >
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..83cbd116d06b --- /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="foo-$(mktemp -u XXXXX)" + +mkdir instances/$instance + +cd instances/$instance + +run_tests + +cd ../.. + +rmdir instances/$instance + +chgrp $original_group instances + +exit 0