add sysfs entry "patched" for each klp_object

PATCH Superseded Song Liu 3 participants 9 messages Aug 2, 2022 – Sep 2, 2022

Subsystems (2)

Kselftest Live patching

lore

Share & tools

lore

Song Liu

author maintainer

2022-08-02 01:09 UTC #0 lore raw

v2 cover add sysfs entry "patched" for each klp_object

I was debugging an issue that a livepatch appears to be attached, but actually not. It turns out that there is a mismatch in module name (abc-xyz vs. abc_xyz), klp_find_object_module failed to find the module.

Changes v1 => v2:
1. Add selftest. (Petr Mladek)
2. Update documentation. (Petr Mladek)
3. Use sysfs_emit. (Petr Mladek)

Song Liu (2):
livepatch: add sysfs entry "patched" for each klp_object selftests/livepatch: add sysfs test

 .../ABI/testing/sysfs-kernel-livepatch        |  8 ++++
 kernel/livepatch/core.c                       | 18 +++++++++
 tools/testing/selftests/livepatch/Makefile    |  3 +-
 .../testing/selftests/livepatch/test-sysfs.sh | 40 +++++++++++++++++++
 4 files changed, 68 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/livepatch/test-sysfs.sh

--
2.30.2

Song Liu

author maintainer

2022-08-02 01:09 UTC #1 lore raw

PATCH v2 1/2 livepatch: add sysfs entry "patched" for each klp_object

Add per klp_object sysfs entry "patched". It makes it easier to debug typos in the module name.

Signed-off-by Song Liu <[email protected]>

---
I was debugging an issue that a livepatch appears to be attached, but actually not. It turns out that there is a mismatch in module name (abc-xyz vs. abc_xyz), klp_find_object_module failed to find the module.

---
 .../ABI/testing/sysfs-kernel-livepatch         |  8 ++++++++
 kernel/livepatch/core.c                        | 18 ++++++++++++++++++
 2 files changed, 26 insertions(+)

Documentation/ABI/testing/sysfs-kernel-livepatch +8 -0

index bea7bd5a1d5f..ebe39a3cea39 100644
@@ -55,6 +55,14 @@ Description:
 		The object directory contains subdirectories for each function
 		that is patched within the object.
 
+What:		/sys/kernel/livepatch/<patch>/<object>/patched
+Date:		August 2022
+KernelVersion:	6.0.0
+Contact:	[email protected]
+Description:
+		An attribute which indicates whether the object is currently
+		patched.
+
 What:		/sys/kernel/livepatch/<patch>/<object>/<function,sympos>
 Date:		Nov 2014
 KernelVersion:	3.19.0

kernel/livepatch/core.c +18 -0

index bc475e62279d..67eb9f9168f3 100644
@@ -325,6 +325,7 @@ int klp_apply_section_relocs(struct module *pmod, Elf_Shdr *sechdrs,
  * /sys/kernel/livepatch/<patch>/transition
  * /sys/kernel/livepatch/<patch>/force
  * /sys/kernel/livepatch/<patch>/<object>
+ * /sys/kernel/livepatch/<patch>/<object>/patched
  * /sys/kernel/livepatch/<patch>/<object>/<function,sympos>
  */
 static int __klp_disable_patch(struct klp_patch *patch);

@@ -431,6 +432,22 @@ static struct attribute *klp_patch_attrs[] = {
 };
 ATTRIBUTE_GROUPS(klp_patch);
 
+static ssize_t patched_show(struct kobject *kobj,
+			    struct kobj_attribute *attr, char *buf)
+{
+	struct klp_object *obj;
+
+	obj = container_of(kobj, struct klp_object, kobj);
+	return sysfs_emit(buf, "%d\n", obj->patched);
+}
+
+static struct kobj_attribute patched_kobj_attr = __ATTR_RO(patched);
+static struct attribute *klp_object_attrs[] = {
+	&patched_kobj_attr.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(klp_object);
+
 static void klp_free_object_dynamic(struct klp_object *obj)
 {
 	kfree(obj->name);

@@ -576,6 +593,7 @@ static void klp_kobj_release_object(struct kobject *kobj)
 static struct kobj_type klp_ktype_object = {
 	.release = klp_kobj_release_object,
 	.sysfs_ops = &kobj_sysfs_ops,
+	.default_groups = klp_object_groups,
 };
 
 static void klp_kobj_release_func(struct kobject *kobj)

Signature · 1 line

2.30.2

Petr Mladek

maintainer Reviewed-by on v2 1/2

2022-09-02 11:50 UTC #2 lore raw

Re: [PATCH v2 1/2] livepatch: add sysfs entry "patched" for each klp_object

Quoted 4 lines · Song Liu

Add per klp_object sysfs entry "patched". It makes it easier to debug typos in the module name.
Signed-off-by Song Liu <[email protected]>

Looks good to me:

Reviewed-by Petr Mladek <[email protected]>

Best Regards,
Petr

Song Liu

author maintainer

2022-08-02 01:09 UTC #3 lore raw

PATCH v2 2/2 selftests/livepatch: add sysfs test

In reply to Song Liu

Add a test for livepatch sysfs entries.

Signed-off-by Song Liu <[email protected]>

---
 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

tools/testing/selftests/livepatch/Makefile +2 -1

index 1acc9e1fa3fb..02fadc9d55e0 100644
@@ -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

tools/testing/selftests/livepatch/test-sysfs.sh new file +40 -0

new file mode 100755
index 000000000000..eb4a69ba1c2c
@@ -0,0 +1,40 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2022 Song Liu <[email protected]>
+
+. $(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
+
+disable_lp $MOD_LIVEPATCH
+
+unload_lp $MOD_LIVEPATCH
+
+check_result "% modprobe $MOD_LIVEPATCH
+livepatch: enabling patch '$MOD_LIVEPATCH'
+livepatch: '$MOD_LIVEPATCH': initializing patching transition
+livepatch: '$MOD_LIVEPATCH': starting patching transition
+livepatch: '$MOD_LIVEPATCH': completing patching transition
+livepatch: '$MOD_LIVEPATCH': patching complete
+/sys/kernel/livepatch/test_klp_livepatch/enabled:1
+/sys/kernel/livepatch/test_klp_livepatch/transition:0
+/sys/kernel/livepatch/test_klp_livepatch/vmlinux/patched:1
+% echo 0 > /sys/kernel/livepatch/$MOD_LIVEPATCH/enabled
+livepatch: '$MOD_LIVEPATCH': initializing unpatching transition
+livepatch: '$MOD_LIVEPATCH': starting unpatching transition
+livepatch: '$MOD_LIVEPATCH': completing unpatching transition
+livepatch: '$MOD_LIVEPATCH': unpatching complete
+% rmmod $MOD_LIVEPATCH"
+
+exit 0

Signature · 1 line

2.30.2

Joe Lawrence

maintainer

2022-08-02 21:36 UTC #4 lore raw

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

Quoted 37 lines · Song Liu

Add a test for livepatch sysfs entries.

Signed-off-by Song Liu <[email protected]>

---
 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

tools/testing/selftests/livepatch/Makefile +2 -1

index 1acc9e1fa3fb..02fadc9d55e0 100644
@@ -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

tools/testing/selftests/livepatch/test-sysfs.sh new file +8 -0

new file mode 100755
index 000000000000..eb4a69ba1c2c
@@ -0,0 +1,40 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2022 Song Liu <[email protected]>
+
+. $(dirname $0)/functions.sh
+
+MOD_LIVEPATCH=test_klp_livepatch
+

tools/testing/selftests/livepatch/test-sysfs.sh line 18

Quoted 10 lines

tools/testing/selftests/livepatch/test-sysfs.sh new file +10 -0

+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

Clever grep. I like to use head -n100 <pattern> to dump debugging filenames and content, but this "grep anything" is nice for one liner files :)

Quoted 25 lines

tools/testing/selftests/livepatch/test-sysfs.sh new file +22 -0

+
+disable_lp $MOD_LIVEPATCH
+
+unload_lp $MOD_LIVEPATCH
+
+check_result "% modprobe $MOD_LIVEPATCH
+livepatch: enabling patch '$MOD_LIVEPATCH'
+livepatch: '$MOD_LIVEPATCH': initializing patching transition
+livepatch: '$MOD_LIVEPATCH': starting patching transition
+livepatch: '$MOD_LIVEPATCH': completing patching transition
+livepatch: '$MOD_LIVEPATCH': patching complete
+/sys/kernel/livepatch/test_klp_livepatch/enabled:1
+/sys/kernel/livepatch/test_klp_livepatch/transition:0
+/sys/kernel/livepatch/test_klp_livepatch/vmlinux/patched:1
+% echo 0 > /sys/kernel/livepatch/$MOD_LIVEPATCH/enabled
+livepatch: '$MOD_LIVEPATCH': initializing unpatching transition
+livepatch: '$MOD_LIVEPATCH': starting unpatching transition
+livepatch: '$MOD_LIVEPATCH': completing unpatching transition
+livepatch: '$MOD_LIVEPATCH': unpatching complete
+% rmmod $MOD_LIVEPATCH"
+
+exit 0

Signature · 1 line

2.30.2

The patch and test look fine. I wonder how we'll modify these type of tests if we can decouple the selftests from the kernel version. In that case we may need to individually verify files only if they exist. The upside to this version is that it will remind anyone who adds another file to update the expected check_result value.

Also, I believe Red Hat QE has a few internal tests that verify sysfs values during transitions, etc. I'll inquire about those in case we can follow up with even more sysfs verification.

--
Joe

Petr Mladek

maintainer

2022-09-02 14:06 UTC #5 lore raw

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

In reply to Song Liu

Quoted 38 lines · Song Liu

Add a test for livepatch sysfs entries.

Signed-off-by Song Liu <[email protected]>

---
 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

tools/testing/selftests/livepatch/Makefile +2 -1

index 1acc9e1fa3fb..02fadc9d55e0 100644
@@ -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

tools/testing/selftests/livepatch/test-sysfs.sh new file +9 -0

new file mode 100755
index 000000000000..eb4a69ba1c2c
@@ -0,0 +1,40 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2022 Song Liu <[email protected]>
+
+. $(dirname $0)/functions.sh
+
+MOD_LIVEPATCH=test_klp_livepatch
+
+setup_config

tools/testing/selftests/livepatch/test-sysfs.sh line 19

Quoted 10 lines

tools/testing/selftests/livepatch/test-sysfs.sh new file +10 -0

+
+# - 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

Joe Lawrence

maintainer

2022-09-02 15:52 UTC #6 lore raw

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

Quoted 58 lines · Petr Mladek

Quoted 38 lines · Song Liu
Add a test for livepatch sysfs entries.
Signed-off-by Song Liu <[email protected]>
---
 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
tools/testing/selftests/livepatch/Makefile +2 -1
index 1acc9e1fa3fb..02fadc9d55e0 100644
@@ -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
 
tools/testing/selftests/livepatch/test-sysfs.sh new file +9 -0
new file mode 100755
index 000000000000..eb4a69ba1c2c
@@ -0,0 +1,40 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2022 Song Liu <[email protected]>
+
+. $(dirname $0)/functions.sh
+
+MOD_LIVEPATCH=test_klp_livepatch
+
+setup_config
tools/testing/selftests/livepatch/test-sysfs.sh line 19
Quoted 10 lines
tools/testing/selftests/livepatch/test-sysfs.sh new file +10 -0
+
+# - 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

Quoted 6 lines

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:

I realize this was just a code sketch, but a few ideas before copy/pasting the functions :)

Quoted 1 line

# check_sysfs_exists(modname, rel_path, expected_rights) - check sysfs interface

# check_sysfs_rights(modname, rel_path, expected_rights) - check sysfs path permissions

Quoted 2 lines

# modname - livepatch module creating the sysfs interface
# rel_patch - relative patch of the sysfs interface

s/patch/path

Quoted 7 lines

# 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"

Checking for existence here might be cleaner than failing later. Unless the caller is going to do it first. Or should we just have a check_sysfs_exists() like your function comments hint at?

Quoted 1 line

	local rights=`ls -l -d $path | cut -d " " -f 1`

$(/bin/stat --format '%A' "$path") easier?

Quoted 7 lines

	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

# check_sysfs_value(modname, rel_path, expected_value) - check sysfs value

Quoted 2 lines

# modname - livepatch module creating the sysfs interface
# rel_patch - relative patch of the sysfs interface

s/patch/path

Quoted 28 lines

# 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:

Quoted 7 lines

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"

Yes, these helpers would be nice to have to better verify the sysfs structure and values.

Regards,

Signature · 1 line

Joe

Joe Lawrence

maintainer Reviewed-by on the v2 series

2022-08-02 21:37 UTC #7 lore raw

In reply to Song Liu

Quoted 20 lines · Song Liu

I was debugging an issue that a livepatch appears to be attached, but actually not. It turns out that there is a mismatch in module name (abc-xyz vs. abc_xyz), klp_find_object_module failed to find the module.
Changes v1 => v2:
1. Add selftest. (Petr Mladek)
2. Update documentation. (Petr Mladek)
3. Use sysfs_emit. (Petr Mladek)
Song Liu (2):
livepatch: add sysfs entry "patched" for each klp_object selftests/livepatch: add sysfs test
 .../ABI/testing/sysfs-kernel-livepatch        |  8 ++++
 kernel/livepatch/core.c                       | 18 +++++++++
 tools/testing/selftests/livepatch/Makefile    |  3 +-
 .../testing/selftests/livepatch/test-sysfs.sh | 40 +++++++++++++++++++
 4 files changed, 68 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/livepatch/test-sysfs.sh

Quoted 3 lines

--
2.30.2

For both,

Reviewed-by Joe Lawrence <[email protected]>

--
Joe

Song Liu

author maintainer

2022-08-30 19:09 UTC #8 lore raw

Quoted 22 lines · Joe Lawrence

Quoted 20 lines · Song Liu
I was debugging an issue that a livepatch appears to be attached, but actually not. It turns out that there is a mismatch in module name (abc-xyz vs. abc_xyz), klp_find_object_module failed to find the module.
Changes v1 => v2:
1. Add selftest. (Petr Mladek)
2. Update documentation. (Petr Mladek)
3. Use sysfs_emit. (Petr Mladek)
Song Liu (2):
livepatch: add sysfs entry "patched" for each klp_object selftests/livepatch: add sysfs test
 .../ABI/testing/sysfs-kernel-livepatch        |  8 ++++
 kernel/livepatch/core.c                       | 18 +++++++++
 tools/testing/selftests/livepatch/Makefile    |  3 +-
 .../testing/selftests/livepatch/test-sysfs.sh | 40 +++++++++++++++++++
 4 files changed, 68 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/livepatch/test-sysfs.sh

Quoted 7 lines

Quoted 3 lines
--
2.30.2
For both,
Reviewed-by Joe Lawrence <[email protected]>

Hi folks,

Do we need more work for this set?

Thanks,
Song