Message-Id: <20231222113603.96bf082a416abf8a56a26ddc@kernel.org>
Date: Fri, 22 Dec 2023 11:36:03 +0900
From: Masami Hiramatsu (Google) <mhiramat@...nel.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: LKML <linux-kernel@...r.kernel.org>, Linux Trace Kernel
 <linux-trace-kernel@...r.kernel.org>, Masami Hiramatsu
 <mhiramat@...nel.org>, Mark Rutland <mark.rutland@....com>, Mathieu
 Desnoyers <mathieu.desnoyers@...icios.com>, Linus Torvalds
 <torvalds@...ux-foundation.org>, Shuah Khan <shuah@...nel.org>, Linux
 selftests <linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH v3] tracing/selftests: Add ownership modification tests
 for eventfs

On Thu, 21 Dec 2023 21:12:29 -0500
Steven Rostedt <rostedt@...dmis.org> wrote:

> From: "Steven Rostedt (Google)" <rostedt@...dmis.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@...nel.org>
Tested-by: Masami Hiramatsu (Google) <mhiramat@...nel.org>


> Signed-off-by: Steven Rostedt (Google) <rostedt@...dmis.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
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@...nel.org>

Powered by blists - more mailing lists