Re: [PATCH v2 2/2] selftests/livepatch: add sysfs test

[Petr Mladek <pmladek@suse.com>]

On Mon 2022-08-01 18:08:57, Song Liu wrote:
> Add a test for livepatch sysfs entries.
> 
> Signed-off-by: Song Liu <song@kernel.org>
> ---
>  tools/testing/selftests/livepatch/Makefile    |  3 +-
>  .../testing/selftests/livepatch/test-sysfs.sh | 40 +++++++++++++++++++
>  2 files changed, 42 insertions(+), 1 deletion(-)
>  create mode 100755 tools/testing/selftests/livepatch/test-sysfs.sh
> 
> diff --git a/tools/testing/selftests/livepatch/Makefile b/tools/testing/selftests/livepatch/Makefile
> index 1acc9e1fa3fb..02fadc9d55e0 100644
> --- a/tools/testing/selftests/livepatch/Makefile
> +++ b/tools/testing/selftests/livepatch/Makefile
> @@ -6,7 +6,8 @@ TEST_PROGS := \
>  	test-callbacks.sh \
>  	test-shadow-vars.sh \
>  	test-state.sh \
> -	test-ftrace.sh
> +	test-ftrace.sh \
> +	test-sysfs.sh
>  
>  TEST_FILES := settings
>  
> diff --git a/tools/testing/selftests/livepatch/test-sysfs.sh b/tools/testing/selftests/livepatch/test-sysfs.sh
> new file mode 100755
> index 000000000000..eb4a69ba1c2c
> --- /dev/null
> +++ b/tools/testing/selftests/livepatch/test-sysfs.sh
> @@ -0,0 +1,40 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (C) 2022 Song Liu <song@kernel.org>
> +
> +. $(dirname $0)/functions.sh
> +
> +MOD_LIVEPATCH=test_klp_livepatch
> +
> +setup_config
> +
> +# - load a livepatch and verifies the sysfs entries work as expected
> +
> +start_test "sysfs test"
> +
> +load_lp $MOD_LIVEPATCH
> +
> +grep . "/sys/kernel/livepatch/$MOD_LIVEPATCH"/* > /dev/kmsg
> +grep . "/sys/kernel/livepatch/$MOD_LIVEPATCH"/*/* > /dev/kmsg
> +

I see the following when I run the test:

host:kernel/linux/tools/testing/selftests/livepatch # ./test-sysfs.sh 
TEST: sysfs test ... grep: /sys/kernel/livepatch/test_klp_livepatch/force: Permission denied
grep: /sys/kernel/livepatch/test_klp_livepatch/vmlinux: Is a directory
grep: /sys/kernel/livepatch/test_klp_livepatch/vmlinux/cmdline_proc_show,1: Is a directory
ok

The errors are expected. A solution would be to redirect 2>&1 or
2>/dev/null. Both looks a bit ugly to me.

Instead, I would suggest to create some helper scripts in functions.sh,
for example:

# check_sysfs_exists(modname, rel_path, expected_rights) - check sysfs interface
#	modname - livepatch module creating the sysfs interface
#	rel_patch - relative patch of the sysfs interface
#	expected_rights - expected access rights
function check_sysfs_rights() {
	local mod="$1"; shift
	local rel_path="$1"; shift
	local expected_rights="$1"; shift

	local path="$KLP_SYSFS_DIR/$mod/$rel_path"
	local rights=`ls -l -d $path | cut -d " " -f 1`

	if test "$rights" != "$expected_rights" ; then
	    die "Unexpected access rights of $path: $expected_rights vs. $rights"
	fi
}

# check_sysfs_exists(modname, rel_path, expected_value) - check sysfs interface
#	modname - livepatch module creating the sysfs interface
#	rel_patch - relative patch of the sysfs interface
#	expected_value - expected value read from the file
function check_sysfs_value() {
	local mod="$1"; shift
	local rel_path="$1"; shift
	local expected_value="$1"; shift

	local path="$KLP_SYSFS_DIR/$mod/$rel_path"
	local value=`cat $path`

	if test "$value" != "$expected_value" ; then
	    die "Unexpected value in $path: $expected_value vs. $value"
	fi
}

It would allow to create a fine tuned tests, for example:

check_sysfs_rights "$MOD_LIVEPATCH" "" "drwxr-xr-x"
check_sysfs_rights "$MOD_LIVEPATCH" "enabled" "-rw-r--r--"

check_sysfs_value  "$MOD_LIVEPATCH" "enabled" "1"



Also it would be great to test that the "patched" value is "0"
when the object is not patched. I would require to create
a test module that might be livepatched. I mean something like:


check_sysfs_value  $MOD_LIVEPATCH "$TEST_MODULE/patched" "0"
load_mod $TEST_MODULE
check_sysfs_value  $MOD_LIVEPATCH "$TEST_MODULE/patched" "1"
unload_mod $TEST_MODULE
check_sysfs_value  $MOD_LIVEPATCH "$TEST_MODULE/patched" "0"


Best Regards,
Petr