[PATCH v3 0/3] selftests: livepatch: test livepatching a kprobed function

[PATCH v3 0/3] selftests: livepatch: test livepatching a kprobed function

[Michael Vetter]

This patchset adds a test for livepatching a kprobed function.

Thanks to Petr and Marcos for the reviews!

V3:
Save and restore kprobe state also when test fails, by integrating it
into setup_config() and cleanup().
Rename SYSFS variables in a more logical way.
Sort test modules in alphabetical order.
Rename module description.

V2:
Save and restore kprobe state.

Michael Vetter (3):
  selftests: livepatch: rename KLP_SYSFS_DIR to SYSFS_KLP_DIR
  selftests: livepatch: save and restore kprobe state
  selftests: livepatch: test livepatching a kprobed function

 tools/testing/selftests/livepatch/Makefile    |  3 +-
 .../testing/selftests/livepatch/functions.sh  | 13 +++-
 .../selftests/livepatch/test-kprobe.sh        | 62 +++++++++++++++++++
 .../selftests/livepatch/test_modules/Makefile |  3 +-
 .../livepatch/test_modules/test_klp_kprobe.c  | 38 ++++++++++++
 5 files changed, 114 insertions(+), 5 deletions(-)
 create mode 100755 tools/testing/selftests/livepatch/test-kprobe.sh
 create mode 100644 tools/testing/selftests/livepatch/test_modules/test_klp_kprobe.c

-- 
2.46.0

[PATCH v3 1/3] selftests: livepatch: rename KLP_SYSFS_DIR to SYSFS_KLP_DIR

[Michael Vetter]

This naming makes more sense according to the directory structure.
Especially when we later add more paths.

Signed-off-by: Michael Vetter <mvetter@suse.com>
---
 tools/testing/selftests/livepatch/functions.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh
index fc4c6a016d38..50361fceff06 100644
--- a/tools/testing/selftests/livepatch/functions.sh
+++ b/tools/testing/selftests/livepatch/functions.sh
@@ -6,7 +6,7 @@
 
 MAX_RETRIES=600
 RETRY_INTERVAL=".1"	# seconds
-KLP_SYSFS_DIR="/sys/kernel/livepatch"
+SYSFS_KLP_DIR="/sys/kernel/livepatch"
 
 # Kselftest framework requirement - SKIP code is 4
 ksft_skip=4
@@ -322,7 +322,7 @@ function check_sysfs_rights() {
 	local rel_path="$1"; shift
 	local expected_rights="$1"; shift
 
-	local path="$KLP_SYSFS_DIR/$mod/$rel_path"
+	local path="$SYSFS_KLP_DIR/$mod/$rel_path"
 	local rights=$(/bin/stat --format '%A' "$path")
 	if test "$rights" != "$expected_rights" ; then
 		die "Unexpected access rights of $path: $expected_rights vs. $rights"
@@ -338,7 +338,7 @@ function check_sysfs_value() {
 	local rel_path="$1"; shift
 	local expected_value="$1"; shift
 
-	local path="$KLP_SYSFS_DIR/$mod/$rel_path"
+	local path="$SYSFS_KLP_DIR/$mod/$rel_path"
 	local value=`cat $path`
 	if test "$value" != "$expected_value" ; then
 		die "Unexpected value in $path: $expected_value vs. $value"
-- 
2.46.0

Re: [PATCH v3 1/3] selftests: livepatch: rename KLP_SYSFS_DIR to SYSFS_KLP_DIR

[Miroslav Benes]

On Fri, 20 Sep 2024, Michael Vetter wrote:

> This naming makes more sense according to the directory structure.
> Especially when we later add more paths.
> 
> Signed-off-by: Michael Vetter <mvetter@suse.com>

Reviewed-by: Miroslav Benes <mbenes@suse.cz>

M

[PATCH v3 2/3] selftests: livepatch: save and restore kprobe state

[Michael Vetter]

Save the state of /sys/kernel/livepatch/debug/kprobes/enabled
during setup_config() and restore it during cleanup().

This is in preparation for a future commit that will add a test
that should confirm that we cannot livepatch a kprobed function
if that kprobe has a post handler.

Signed-off-by: Michael Vetter <mvetter@suse.com>
---
 tools/testing/selftests/livepatch/functions.sh | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh
index 50361fceff06..6684c01c0567 100644
--- a/tools/testing/selftests/livepatch/functions.sh
+++ b/tools/testing/selftests/livepatch/functions.sh
@@ -6,7 +6,10 @@
 
 MAX_RETRIES=600
 RETRY_INTERVAL=".1"	# seconds
-SYSFS_KLP_DIR="/sys/kernel/livepatch"
+SYSFS_KERNEL_DIR="/sys/kernel"
+SYSFS_KLP_DIR="$SYSFS_KERNEL_DIR/livepatch"
+SYSFS_DEBUG_DIR="$SYSFS_KERNEL_DIR/debug"
+SYSFS_KPROBES_DIR="$SYSFS_DEBUG_DIR/kprobes"
 
 # Kselftest framework requirement - SKIP code is 4
 ksft_skip=4
@@ -58,6 +61,7 @@ function push_config() {
 	DYNAMIC_DEBUG=$(grep '^kernel/livepatch' /sys/kernel/debug/dynamic_debug/control | \
 			awk -F'[: ]' '{print "file " $1 " line " $2 " " $4}')
 	FTRACE_ENABLED=$(sysctl --values kernel.ftrace_enabled)
+	KPROBE_ENABLED=$(cat $SYSFS_KPROBES_DIR/enabled)
 }
 
 function pop_config() {
@@ -67,6 +71,9 @@ function pop_config() {
 	if [[ -n "$FTRACE_ENABLED" ]]; then
 		sysctl kernel.ftrace_enabled="$FTRACE_ENABLED" &> /dev/null
 	fi
+	if [[ -n "$KPROBE_ENABLED" ]]; then
+		echo "$KPROBE_ENABLED" > "$SYSFS_KPROBES_DIR/enabled"
+	fi
 }
 
 function set_dynamic_debug() {
-- 
2.46.0

Re: [PATCH v3 2/3] selftests: livepatch: save and restore kprobe state

[Miroslav Benes]

Hi,

On Fri, 20 Sep 2024, Michael Vetter wrote:

> Save the state of /sys/kernel/livepatch/debug/kprobes/enabled

I believe it is /sys/kernel/debug/kprobes/enabled.

> during setup_config() and restore it during cleanup().
> 
> This is in preparation for a future commit that will add a test
> that should confirm that we cannot livepatch a kprobed function
> if that kprobe has a post handler.
> 
> Signed-off-by: Michael Vetter <mvetter@suse.com>
> ---
>  tools/testing/selftests/livepatch/functions.sh | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh
> index 50361fceff06..6684c01c0567 100644
> --- a/tools/testing/selftests/livepatch/functions.sh
> +++ b/tools/testing/selftests/livepatch/functions.sh
> @@ -6,7 +6,10 @@
>  
>  MAX_RETRIES=600
>  RETRY_INTERVAL=".1"	# seconds
> -SYSFS_KLP_DIR="/sys/kernel/livepatch"
> +SYSFS_KERNEL_DIR="/sys/kernel"
> +SYSFS_KLP_DIR="$SYSFS_KERNEL_DIR/livepatch"
> +SYSFS_DEBUG_DIR="$SYSFS_KERNEL_DIR/debug"
> +SYSFS_KPROBES_DIR="$SYSFS_DEBUG_DIR/kprobes"

Personally I find this harder to read but it is a nit and Petr asked you 
to do it, but

>  # Kselftest framework requirement - SKIP code is 4
>  ksft_skip=4
> @@ -58,6 +61,7 @@ function push_config() {
>  	DYNAMIC_DEBUG=$(grep '^kernel/livepatch' /sys/kernel/debug/dynamic_debug/control | \

it stays opencoded here and also elsewhere which is not great. Could it be 
consistent, please?

Otherwise I would just squash the patch to the next one.

Thank you,
Miroslav

[PATCH v3 3/3] selftests: livepatch: test livepatching a kprobed function

[Michael Vetter]

The test proves that a function that is being kprobed and uses a
post_handler cannot be livepatched.

Only one ftrace_ops with FTRACE_OPS_FL_IPMODIFY set may be registered
to any given function at a time.

Signed-off-by: Michael Vetter <mvetter@suse.com>
---
 tools/testing/selftests/livepatch/Makefile    |  3 +-
 .../selftests/livepatch/test-kprobe.sh        | 62 +++++++++++++++++++
 .../selftests/livepatch/test_modules/Makefile |  3 +-
 .../livepatch/test_modules/test_klp_kprobe.c  | 38 ++++++++++++
 4 files changed, 104 insertions(+), 2 deletions(-)
 create mode 100755 tools/testing/selftests/livepatch/test-kprobe.sh
 create mode 100644 tools/testing/selftests/livepatch/test_modules/test_klp_kprobe.c

diff --git a/tools/testing/selftests/livepatch/Makefile b/tools/testing/selftests/livepatch/Makefile
index 35418a4790be..a080eb54a215 100644
--- a/tools/testing/selftests/livepatch/Makefile
+++ b/tools/testing/selftests/livepatch/Makefile
@@ -10,7 +10,8 @@ TEST_PROGS := \
 	test-state.sh \
 	test-ftrace.sh \
 	test-sysfs.sh \
-	test-syscall.sh
+	test-syscall.sh \
+	test-kprobe.sh
 
 TEST_FILES := settings
 
diff --git a/tools/testing/selftests/livepatch/test-kprobe.sh b/tools/testing/selftests/livepatch/test-kprobe.sh
new file mode 100755
index 000000000000..0c62c6b81e18
--- /dev/null
+++ b/tools/testing/selftests/livepatch/test-kprobe.sh
@@ -0,0 +1,62 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2024 SUSE
+# Author: Michael Vetter <mvetter@suse.com>
+
+. $(dirname $0)/functions.sh
+
+MOD_LIVEPATCH=test_klp_livepatch
+MOD_KPROBE=test_klp_kprobe
+
+setup_config
+
+# Kprobe a function and verify that we can't livepatch that same function
+# when it uses a post_handler since only one IPMODIFY maybe be registered
+# to any given function at a time.
+
+start_test "livepatch interaction with kprobed function with post_handler"
+
+echo 1 > /sys/kernel/debug/kprobes/enabled
+
+load_mod $MOD_KPROBE has_post_handler=true
+load_failing_mod $MOD_LIVEPATCH
+unload_mod $MOD_KPROBE
+
+check_result "% insmod test_modules/test_klp_kprobe.ko has_post_handler=true
+% insmod test_modules/$MOD_LIVEPATCH.ko
+livepatch: enabling patch '$MOD_LIVEPATCH'
+livepatch: '$MOD_LIVEPATCH': initializing patching transition
+livepatch: failed to register ftrace handler for function 'cmdline_proc_show' (-16)
+livepatch: failed to patch object 'vmlinux'
+livepatch: failed to enable patch '$MOD_LIVEPATCH'
+livepatch: '$MOD_LIVEPATCH': canceling patching transition, going to unpatch
+livepatch: '$MOD_LIVEPATCH': completing unpatching transition
+livepatch: '$MOD_LIVEPATCH': unpatching complete
+insmod: ERROR: could not insert module test_modules/$MOD_LIVEPATCH.ko: Device or resource busy
+% rmmod test_klp_kprobe"
+
+start_test "livepatch interaction with kprobed function without post_handler"
+
+load_mod $MOD_KPROBE has_post_handler=false
+load_lp $MOD_LIVEPATCH
+
+unload_mod $MOD_KPROBE
+disable_lp $MOD_LIVEPATCH
+unload_lp $MOD_LIVEPATCH
+
+check_result "% insmod test_modules/test_klp_kprobe.ko has_post_handler=false
+% insmod test_modules/$MOD_LIVEPATCH.ko
+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
+% rmmod test_klp_kprobe
+% 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
diff --git a/tools/testing/selftests/livepatch/test_modules/Makefile b/tools/testing/selftests/livepatch/test_modules/Makefile
index e6e638c4bcba..939230e571f5 100644
--- a/tools/testing/selftests/livepatch/test_modules/Makefile
+++ b/tools/testing/selftests/livepatch/test_modules/Makefile
@@ -6,11 +6,12 @@ obj-m += test_klp_atomic_replace.o \
 	test_klp_callbacks_demo.o \
 	test_klp_callbacks_demo2.o \
 	test_klp_callbacks_mod.o \
+	test_klp_kprobe.o \
 	test_klp_livepatch.o \
+	test_klp_shadow_vars.o \
 	test_klp_state.o \
 	test_klp_state2.o \
 	test_klp_state3.o \
-	test_klp_shadow_vars.o \
 	test_klp_syscall.o
 
 # Ensure that KDIR exists, otherwise skip the compilation
diff --git a/tools/testing/selftests/livepatch/test_modules/test_klp_kprobe.c b/tools/testing/selftests/livepatch/test_modules/test_klp_kprobe.c
new file mode 100644
index 000000000000..67a8d29012f6
--- /dev/null
+++ b/tools/testing/selftests/livepatch/test_modules/test_klp_kprobe.c
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2024 Marcos Paulo de Souza <mpdesouza@suse.com>
+// Copyright (C) 2024 Michael Vetter <mvetter@suse.com>
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/kprobes.h>
+
+static bool has_post_handler = true;
+module_param(has_post_handler, bool, 0444);
+
+static void __kprobes post_handler(struct kprobe *p, struct pt_regs *regs,
+				unsigned long flags)
+{
+}
+
+static struct kprobe kp = {
+	.symbol_name = "cmdline_proc_show",
+};
+
+static int __init kprobe_init(void)
+{
+	if (has_post_handler)
+		kp.post_handler = post_handler;
+
+	return register_kprobe(&kp);
+}
+
+static void __exit kprobe_exit(void)
+{
+	unregister_kprobe(&kp);
+}
+
+module_init(kprobe_init)
+module_exit(kprobe_exit)
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Michael Vetter <mvetter@suse.com>");
+MODULE_DESCRIPTION("Livepatch test: kprobe function");
-- 
2.46.0

Re: [PATCH v3 3/3] selftests: livepatch: test livepatching a kprobed function

[Miroslav Benes]

Hi,

On Fri, 20 Sep 2024, Michael Vetter wrote:

> +# Kprobe a function and verify that we can't livepatch that same function
> +# when it uses a post_handler since only one IPMODIFY maybe be registered
> +# to any given function at a time.
> +
> +start_test "livepatch interaction with kprobed function with post_handler"
> +
> +echo 1 > /sys/kernel/debug/kprobes/enabled

opencoded here again.

The rest looks good to me.

Miroslav

Re: [PATCH v3 0/3] selftests: livepatch: test livepatching a kprobed function

[Marcos Paulo de Souza]

On Fri, 2024-09-20 at 13:56 +0200, Michael Vetter wrote:
> This patchset adds a test for livepatching a kprobed function.
> 
> Thanks to Petr and Marcos for the reviews!
> 
> V3:
> Save and restore kprobe state also when test fails, by integrating it
> into setup_config() and cleanup().
> Rename SYSFS variables in a more logical way.
> Sort test modules in alphabetical order.
> Rename module description.
> 
> V2:
> Save and restore kprobe state.
> 
> Michael Vetter (3):
>   selftests: livepatch: rename KLP_SYSFS_DIR to SYSFS_KLP_DIR
>   selftests: livepatch: save and restore kprobe state
>   selftests: livepatch: test livepatching a kprobed function
> 

Thanks for the new version! LGTM, so the series is

Reviewed-by: Marcos Paulo de Souza <mpdesouza@suse.com>

>  tools/testing/selftests/livepatch/Makefile    |  3 +-
>  .../testing/selftests/livepatch/functions.sh  | 13 +++-
>  .../selftests/livepatch/test-kprobe.sh        | 62
> +++++++++++++++++++
>  .../selftests/livepatch/test_modules/Makefile |  3 +-
>  .../livepatch/test_modules/test_klp_kprobe.c  | 38 ++++++++++++
>  5 files changed, 114 insertions(+), 5 deletions(-)
>  create mode 100755 tools/testing/selftests/livepatch/test-kprobe.sh
>  create mode 100644
> tools/testing/selftests/livepatch/test_modules/test_klp_kprobe.c
> 

Re: [PATCH v3 0/3] selftests: livepatch: test livepatching a kprobed function

[Shuah Khan]

On 9/23/24 08:45, Marcos Paulo de Souza wrote:
> On Fri, 2024-09-20 at 13:56 +0200, Michael Vetter wrote:
>> This patchset adds a test for livepatching a kprobed function.
>>
>> Thanks to Petr and Marcos for the reviews!
>>
>> V3:
>> Save and restore kprobe state also when test fails, by integrating it
>> into setup_config() and cleanup().
>> Rename SYSFS variables in a more logical way.
>> Sort test modules in alphabetical order.
>> Rename module description.
>>
>> V2:
>> Save and restore kprobe state.
>>
>> Michael Vetter (3):
>>    selftests: livepatch: rename KLP_SYSFS_DIR to SYSFS_KLP_DIR
>>    selftests: livepatch: save and restore kprobe state
>>    selftests: livepatch: test livepatching a kprobed function
>>
> 
> Thanks for the new version! LGTM, so the series is
> 
> Reviewed-by: Marcos Paulo de Souza <mpdesouza@suse.com>
> 
>>   tools/testing/selftests/livepatch/Makefile    |  3 +-
>>   .../testing/selftests/livepatch/functions.sh  | 13 +++-
>>   .../selftests/livepatch/test-kprobe.sh        | 62
>> +++++++++++++++++++
>>   .../selftests/livepatch/test_modules/Makefile |  3 +-
>>   .../livepatch/test_modules/test_klp_kprobe.c  | 38 ++++++++++++
>>   5 files changed, 114 insertions(+), 5 deletions(-)
>>   create mode 100755 tools/testing/selftests/livepatch/test-kprobe.sh
>>   create mode 100644
>> tools/testing/selftests/livepatch/test_modules/test_klp_kprobe.c
>>
> 

Assuming this is going through livepatch tree:

Acked-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah

Re: [PATCH v3 0/3] selftests: livepatch: test livepatching a kprobed function

[Petr Mladek]

On Mon 2024-09-23 10:16:29, Shuah Khan wrote:
> On 9/23/24 08:45, Marcos Paulo de Souza wrote:
> > On Fri, 2024-09-20 at 13:56 +0200, Michael Vetter wrote:
> > > This patchset adds a test for livepatching a kprobed function.
> > > 
> > > Michael Vetter (3):
> > >    selftests: livepatch: rename KLP_SYSFS_DIR to SYSFS_KLP_DIR
> > >    selftests: livepatch: save and restore kprobe state
> > >    selftests: livepatch: test livepatching a kprobed function
> > > 
> > 
> 
> Assuming this is going through livepatch tree:

Yup, I am going to take it via the livepatch tree.
Though I am going to wait for more potential feedback a week or so.

> Acked-by: Shuah Khan <skhan@linuxfoundation.org>

Thanks for the review.

Best Regards,
Petr

Re: [PATCH v3 0/3] selftests: livepatch: test livepatching a kprobed function

[Petr Mladek]

On Fri 2024-09-20 13:56:28, Michael Vetter wrote:
> This patchset adds a test for livepatching a kprobed function.
> 
> Michael Vetter (3):
>   selftests: livepatch: rename KLP_SYSFS_DIR to SYSFS_KLP_DIR
>   selftests: livepatch: save and restore kprobe state
>   selftests: livepatch: test livepatching a kprobed function

Looks good. For the entire patchset:

Reviewed-by: Petr Mladek <pmladek@suse.com>
Tested-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr