| Message ID | 20220630141226.2802-3-mpdesouza@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | livepatch: Move tests from lib/livepatch to selftests/livepatch | expand |
On Thu, Jun 30, 2022 at 11:12:26AM -0300, Marcos Paulo de Souza wrote: > Syscalls are called a tricky way. Some architectures add a prefix to the > syscall name (SYSCALL_WRAPPER). > > This new test creates one userspace process per online cpu calling getpid > continuously and tries to livepatch the getpid function. Add the correct > function prefix for all archs that select HAS_SYSCALL_WRAPPER. > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> Hi Marcos, Thanks for working on this and sorry for the delayed reply. I gave it a spin yesterday and have a few comments below... > --- > tools/testing/selftests/livepatch/Makefile | 12 +- > .../selftests/livepatch/test-syscall.sh | 52 ++++++ > .../test_binaries/test_klp-call_getpid.c | 48 ++++++ > .../selftests/livepatch/test_modules/Makefile | 3 +- > .../livepatch/test_modules/test_klp_syscall.c | 150 ++++++++++++++++++ > 5 files changed, 261 insertions(+), 4 deletions(-) > create mode 100755 tools/testing/selftests/livepatch/test-syscall.sh > create mode 100644 tools/testing/selftests/livepatch/test_binaries/test_klp-call_getpid.c > create mode 100644 tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c > > diff --git a/tools/testing/selftests/livepatch/Makefile b/tools/testing/selftests/livepatch/Makefile > index 5ef492b87bb1..35014197184e 100644 > --- a/tools/testing/selftests/livepatch/Makefile > +++ b/tools/testing/selftests/livepatch/Makefile > @@ -1,10 +1,14 @@ > # SPDX-License-Identifier: GPL-2.0 > +include ../../../build/Build.include > +include ../../../scripts/Makefile.arch > +include ../../../scripts/Makefile.include > > TEST_FILES := settings \ > test_modules > > # We need the test_modules dir in order to make gen_tar and install to work > -TEST_GEN_PROGS_EXTENDED := test_modules/test_klp_atomic_replace.ko \ > +TEST_GEN_PROGS_EXTENDED := test_binaries/test_klp-call_getpid \ Before I commit this to muscle memory, would "test_programs" better describe this directory? When I see "binaries", I think of source vs. build and not kernel vs. userspace. > + test_modules/test_klp_atomic_replace.ko \ > test_modules/test_klp_callbacks_busy.ko \ > test_modules/test_klp_callbacks_demo.ko \ > test_modules/test_klp_callbacks_demo2.ko \ > @@ -13,7 +17,8 @@ TEST_GEN_PROGS_EXTENDED := test_modules/test_klp_atomic_replace.ko \ > test_modules/test_klp_state.ko \ > test_modules/test_klp_state2.ko \ > test_modules/test_klp_state3.ko \ > - test_modules/test_klp_shadow_vars.ko > + test_modules/test_klp_shadow_vars.ko \ > + test_modules/test_klp_syscall.ko > > TEST_PROGS_EXTENDED := functions.sh > TEST_PROGS := \ > @@ -21,7 +26,8 @@ TEST_PROGS := \ > test-callbacks.sh \ > test-shadow-vars.sh \ > test-state.sh \ > - test-ftrace.sh > + test-ftrace.sh \ > + test-syscall.sh > > # override lib.mk's default rules > OVERRIDE_TARGETS := 1 > diff --git a/tools/testing/selftests/livepatch/test-syscall.sh b/tools/testing/selftests/livepatch/test-syscall.sh > new file mode 100755 > index 000000000000..fb3270de7f1f > --- /dev/null > +++ b/tools/testing/selftests/livepatch/test-syscall.sh > @@ -0,0 +1,52 @@ > +#!/bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (C) 2022 SUSE > +# Author: Marcos Paulo de Souza <mpdesouza@suse.com> > + > +. $(dirname $0)/functions.sh > + > +MOD_SYSCALL=test_klp_syscall > + > +setup_config > + > +# - Start _NRPROC processes calling getpid and load a livepatch to patch the > +# getpid syscall > + > +start_test "patch getpid syscall while being heavily hammered" > + > +for i in $(seq 0 $(( $(getconf _NPROCESSORS_ONLN) -1)) ); do > + ./test_binaries/test_klp-call_getpid & > + pids[$i]="$!" > +done On second thought, bash should only iterate through the elements that are defined, so your earlier version is probably better: for i in $(seq 1 $(getconf _NPROCESSORS_ONLN)); do > + > +pid_list=$(echo ${pids[@]} | tr ' ' ',') > +load_lp $MOD_SYSCALL klp_pids=$pid_list > + We could wait for all test_klp-call_getpid instances to execute the livepatch code: loop_until 'grep -q '^0$' /sys/kernel/test_klp_syscall/npids' This has a built in timeout, so it's not infinitely looping. You could add some die "reason" logic here, or just let the existing code cleanup after the failed test. > +pending_pids=$(cat /sys/kernel/test_klp_syscall/npids) > + > +for pid in ${pids[@]}; do > + kill $pid || true > +done > + > +disable_lp $MOD_SYSCALL > +unload_lp $MOD_SYSCALL > + > +if [[ "$pending_pids" != "0" ]]; then > + echo -e "FAIL\n\n" > + die "processes not livepatched: $pending_pids" > +fi > + > +check_result "% insmod test_modules/$MOD_SYSCALL.ko klp_pids=$pid_list > +livepatch: enabling patch '$MOD_SYSCALL' > +livepatch: '$MOD_SYSCALL': initializing patching transition > +livepatch: '$MOD_SYSCALL': starting patching transition > +livepatch: '$MOD_SYSCALL': completing patching transition > +livepatch: '$MOD_SYSCALL': patching complete > +% echo 0 > /sys/kernel/livepatch/$MOD_SYSCALL/enabled > +livepatch: '$MOD_SYSCALL': initializing unpatching transition > +livepatch: '$MOD_SYSCALL': starting unpatching transition > +livepatch: '$MOD_SYSCALL': completing unpatching transition > +livepatch: '$MOD_SYSCALL': unpatching complete > +% rmmod $MOD_SYSCALL" > + > +exit 0 > diff --git a/tools/testing/selftests/livepatch/test_binaries/test_klp-call_getpid.c b/tools/testing/selftests/livepatch/test_binaries/test_klp-call_getpid.c > new file mode 100644 > index 000000000000..ec470660dee6 > --- /dev/null > +++ b/tools/testing/selftests/livepatch/test_binaries/test_klp-call_getpid.c > @@ -0,0 +1,48 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2017 SUSE > + * Author: Libor Pechacek <lpechacek@suse.cz> > + */ > + > +#include <stdio.h> > +#include <unistd.h> > +#include <sys/syscall.h> > +#include <sys/types.h> > +#include <signal.h> > + > +static int stop = 0; nit: no need to init global to 0 > +static int sig_int; > + > +void hup_handler(int signum) > +{ > + stop = 1; > +} > + > +void int_handler(int signum) > +{ > + stop = 1; > + sig_int = 1; > +} > + > +int main(int argc, char *argv[]) > +{ > + pid_t orig_pid, pid; > + long count = 0; > + > + signal(SIGHUP, &hup_handler); > + signal(SIGINT, &int_handler); > + > + orig_pid = syscall(SYS_getpid); > + > + while (!stop) { > + pid = syscall(SYS_getpid); > + if (pid != orig_pid) > + return 1; This test doesn't care about the user program return code, but I wonder if the status should be flipped -- this is the desired code path, not the one at the end of main(), right? > + count++; > + } > + > + if (sig_int) > + printf("%ld iterations done\n", count); > + > + return 0; > +} > diff --git a/tools/testing/selftests/livepatch/test_modules/Makefile b/tools/testing/selftests/livepatch/test_modules/Makefile > index 1eab43b741cd..ebb754accf46 100644 > --- a/tools/testing/selftests/livepatch/test_modules/Makefile > +++ b/tools/testing/selftests/livepatch/test_modules/Makefile > @@ -10,7 +10,8 @@ obj-m += test_klp_atomic_replace.o \ > test_klp_state.o \ > test_klp_state2.o \ > test_klp_state3.o \ > - test_klp_shadow_vars.o > + test_klp_shadow_vars.o \ > + test_klp_syscall.o > > %.ko: > make -C $(KDIR) M=$(TESTMODS_DIR) $@ > diff --git a/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c b/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c > new file mode 100644 > index 000000000000..e90f4ac8e7a4 > --- /dev/null > +++ b/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c > @@ -0,0 +1,150 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2017-2022 SUSE > + * Authors: Libor Pechacek <lpechacek@suse.cz> > + * Nicolai Stange <nstange@suse.de> > + * Marcos Paulo de Souza <mpdesouza@suse.com> > + */ > + > +#include <linux/module.h> > +#include <linux/kernel.h> > +#include <linux/sched.h> > +#include <linux/slab.h> > +#include <linux/livepatch.h> > + > +#if defined(__x86_64__) > +#define FN_PREFIX __x64_ > +#elif defined(__s390x__) > +#define FN_PREFIX __s390x_ > +#elif defined(__aarch64__) > +#define FN_PREFIX __arm64_ > +#else > +/* powerpc does not select ARCH_HAS_SYSCALL_WRAPPER */ > +#define FN_PREFIX > +#endif > + > +struct klp_pid_t { > + pid_t pid; > + struct list_head list; > +}; > +static LIST_HEAD(klp_pid_list); > + > +/* Protects klp_pid_list */ > +static DEFINE_MUTEX(kpid_mutex); > + > +static int klp_pids[NR_CPUS]; > +static unsigned int npids; > +module_param_array(klp_pids, int, &npids, 0); > + > +static ssize_t npids_show(struct kobject *kobj, struct kobj_attribute *attr, > + char *buf) > +{ > + return sprintf(buf, "%u\n", npids); > +} > + > +static struct kobj_attribute klp_attr = __ATTR_RO(npids); > +static struct kobject *klp_kobj; > + > +static void free_klp_pid_list(void) > +{ > + struct klp_pid_t *kpid, *temp; > + > + mutex_lock(&kpid_mutex); > + list_for_each_entry_safe(kpid, temp, &klp_pid_list, list) { > + list_del(&kpid->list); > + kfree(kpid); > + } > + mutex_unlock(&kpid_mutex); > +} > + > +asmlinkage long lp_sys_getpid(void) > +{ > + struct klp_pid_t *kpid, *temp; > + > + /* > + * For each thread calling getpid, check if the pid exists in > + * klp_pid_list. If yes, decrement the npids variable and remove the pid > + * from the list. npids will be later used to ensure that all pids > + * transitioned to the liveaptched state. I go back and forth with this comment: 1) the code is pretty self-explanatory and 2) it explains what test-syscall.sh is doing with npids Describing the behavioral difference I suggest below would be helpful (if you agree w/the change). Something about removing the current pid from the list, okay, too. Then as far as describing npids' purpose, I'd put that up at the top of the file where npids is defined. It can be as generic as describing what it's counting. > + */ > + mutex_lock(&kpid_mutex); > + list_for_each_entry_safe(kpid, temp, &klp_pid_list, list) { > + if (current->pid == kpid->pid) { > + list_del(&kpid->list); > + kfree(kpid); > + npids--; > + break; I think it would be safer to return task_tgid_vnr() here, but ... > + } > + } > + mutex_unlock(&kpid_mutex); > + > + return task_tgid_vnr(current); task_pid_vnr() here. That way we're only changing behavior for the processes in the list and not all programs across the system. > +} > + > +static struct klp_func vmlinux_funcs[] = { > + { > + .old_name = __stringify(FN_PREFIX) "sys_getpid", > + .new_func = lp_sys_getpid, > + }, {} > +}; > + > +static struct klp_object objs[] = { > + { > + /* name being NULL means vmlinux */ > + .funcs = vmlinux_funcs, > + }, {} > +}; > + > +static struct klp_patch patch = { > + .mod = THIS_MODULE, > + .objs = objs, > +}; > + > +static int livepatch_init(void) > +{ > + int ret; > + struct klp_pid_t *kpid; > + > + if (npids > 0) { > + int i; > + > + for (i = 0; i < npids; i++) { > + kpid = kmalloc(sizeof(struct klp_pid_t), GFP_KERNEL); > + if (!kpid) > + goto err_mem; > + > + kpid->pid = klp_pids[i]; > + list_add(&kpid->list, &klp_pid_list); > + } > + } > + > + klp_kobj = kobject_create_and_add("test_klp_syscall", kernel_kobj); > + if (!klp_kobj) > + goto err_mem; > + > + ret = sysfs_create_file(klp_kobj, &klp_attr.attr); > + if (ret) { > + kobject_put(klp_kobj); > + goto out_klp_pid_list; > + } > + > + return klp_enable_patch(&patch); > + > +err_mem: > + ret = -ENOMEM; > +out_klp_pid_list: > + free_klp_pid_list(); > + > + return ret; > +} > + > +static void livepatch_exit(void) > +{ > + free_klp_pid_list(); > + kobject_put(klp_kobj); > +} > + > +module_init(livepatch_init); > +module_exit(livepatch_exit); > +MODULE_LICENSE("GPL"); > +MODULE_INFO(livepatch, "Y"); The linked list code looks like it should work and cleans up properly, but if the current test is the extent of what we need, it would be simpler to directly use klp_pids[] to track progress: since we're never going to livepatch pid 0, we could just search klp_pids[] for our pid and then zero it out once we've executed the livepatch code. This would let us drop out the kmalloc and list management code (looks to be nearly 25% of the file at this point). I think the kpid_mutex still needs to stay though. Iterating through the fixed array shouldn't be too much more expensive than through the (shrinking) klp_pid_list. On the other hand, if you think that we'll need to adapt this module to handle a dynamically changing list of klp_pids[], then perhaps a separate list is the way to go. Final note, kbuild is probably my weakest area, so I'm of limited help review-wise there. In trying out the code, I wasn't sure exactly how to run things. I did: $ make -C tools/testing/selftests/livepatch/ run_tests and that built everything and ran the tests (good). I changed a few .c files and tried again, but they didn't rebuild. Then from tools/testing/selftests/livepatch/test_modules/ I ran `make` and it only ran the clean target (not good, I think). What is the typical workflow supposed to be in this combined setup? Thanks, -- Joe
On Tue 2022-07-12 10:56:11, Joe Lawrence wrote: > On Thu, Jun 30, 2022 at 11:12:26AM -0300, Marcos Paulo de Souza wrote: > > Syscalls are called a tricky way. Some architectures add a prefix to the > > syscall name (SYSCALL_WRAPPER). > > > > This new test creates one userspace process per online cpu calling getpid > > continuously and tries to livepatch the getpid function. Add the correct > > function prefix for all archs that select HAS_SYSCALL_WRAPPER. > > > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> > > Hi Marcos, > > Thanks for working on this and sorry for the delayed reply. I gave it a > spin yesterday and have a few comments below... > > > diff --git a/tools/testing/selftests/livepatch/Makefile b/tools/testing/selftests/livepatch/Makefile > > index 5ef492b87bb1..35014197184e 100644 > > --- a/tools/testing/selftests/livepatch/Makefile > > +++ b/tools/testing/selftests/livepatch/Makefile > > @@ -1,10 +1,14 @@ > > # SPDX-License-Identifier: GPL-2.0 > > +include ../../../build/Build.include > > +include ../../../scripts/Makefile.arch > > +include ../../../scripts/Makefile.include This looks bad. It is outside of tools/tests/selftests/. It would mean that the tests are not self-contained any longer. The tests should support all scenarios described in Documentation/dev-tools/kselftest.rst. Note that it allows to install them and run them from the installed copy. I guess that this is useful if you want to package the selftests. > Final note, kbuild is probably my weakest area, so I'm of limited help > review-wise there. In trying out the code, I wasn't sure exactly how to > run things. I did: > > $ make -C tools/testing/selftests/livepatch/ run_tests > > and that built everything and ran the tests (good). I changed a few .c > files and tried again, but they didn't rebuild. Then from > tools/testing/selftests/livepatch/test_modules/ I ran `make` and it only > ran the clean target (not good, I think). > > What is the typical workflow supposed to be in this combined setup? I personally test the mainline kernels the following way: workstation> make binrpm-pkg test-system-qemu# sshfs workstation:/path/to/packages /mnt test-system-qemu# rpm -ivh /mnt/RPMS/x86_64/kernel-xxxx.rpm test-system-qemu# reboot test-system-qemu# sshfs workstation:/path/to/sources /mnt test-system-qemu# cd /mnt/linux/tools/tests/selftests/livepatch test-system-qemu# make run_tests With this patchset I get: test-system-qemu:/mnt/kernel/linux/tools/testing/selftests/livepatch # make run_tests make -C test_modules test_klp_atomic_replace.ko make[1]: Entering directory '/mnt/kernel/linux/tools/testing/selftests/livepatch/test_modules' make -C /lib/modules/5.19.0-rc8-default+/build M=/mnt/kernel/linux/tools/testing/selftests/livepatch/test_modules test_klp_atomic_replace.ko make[2]: Entering directory '/mnt/kernel/linux/tools/testing/selftests/livepatch/test_modules' make[2]: *** /lib/modules/5.19.0-rc8-default+/build: No such file or directory. Stop. make[2]: Leaving directory '/mnt/kernel/linux/tools/testing/selftests/livepatch/test_modules' make[1]: *** [Makefile:17: test_klp_atomic_replace.ko] Error 2 make[1]: Leaving directory '/mnt/kernel/linux/tools/testing/selftests/livepatch/test_modules' make: *** [Makefile:44: /mnt/kernel/linux/tools/testing/selftests/livepatch/test_modules/test_klp_atomic_replace.ko] Error 2 I get the same when I try it from the top level directory: test-system-qemu:/mnt/kernel/linux # make -C tools/testing/selftests TARGETS=livepatch run_tests make: Entering directory '/mnt/kernel/linux/tools/testing/selftests' make --no-builtin-rules ARCH=x86 -C ../../.. headers_install make[1]: Entering directory '/mnt/kernel/linux' INSTALL ./usr/include make[1]: Leaving directory '/mnt/kernel/linux' make[1]: Entering directory '/mnt/kernel/linux/tools/testing/selftests/livepatch' make -C test_modules test_klp_atomic_replace.ko make[2]: Entering directory '/mnt/kernel/linux/tools/testing/selftests/livepatch/test_modules' make -C /lib/modules/5.19.0-rc8-default+/build M=/mnt/kernel/linux/tools/testing/selftests/livepatch/test_modules test_klp_atomic_replace.ko make[3]: Entering directory '/mnt/kernel/linux/tools/testing/selftests/livepatch/test_modules' make[3]: *** /lib/modules/5.19.0-rc8-default+/build: No such file or directory. Stop. make[3]: Leaving directory '/mnt/kernel/linux/tools/testing/selftests/livepatch/test_modules' make[2]: *** [Makefile:17: test_klp_atomic_replace.ko] Error 2 make[2]: Leaving directory '/mnt/kernel/linux/tools/testing/selftests/livepatch/test_modules' make[1]: *** [Makefile:44: /mnt/kernel/linux/tools/testing/selftests/livepatch/test_modules/test_klp_atomic_replace.ko] Error 2 make[1]: Leaving directory '/mnt/kernel/linux/tools/testing/selftests/livepatch' make: *** [Makefile:178: all] Error 2 make: Leaving directory '/mnt/kernel/linux/tools/testing/selftests' I am not sure if this is exactly the process described in Documentation/dev-tools/kselftest.rst. But it looks to me that it is the way how to test "any" kernel with selftests from an up-to-date kernel sources. Best Regards, Petr
On Tue, Jul 12, 2022 at 10:56:11AM -0400, Joe Lawrence wrote: > On Thu, Jun 30, 2022 at 11:12:26AM -0300, Marcos Paulo de Souza wrote: > > Syscalls are called a tricky way. Some architectures add a prefix to the > > syscall name (SYSCALL_WRAPPER). > > > > This new test creates one userspace process per online cpu calling getpid > > continuously and tries to livepatch the getpid function. Add the correct > > function prefix for all archs that select HAS_SYSCALL_WRAPPER. > > > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> > > Hi Marcos, > > Thanks for working on this and sorry for the delayed reply. I gave it a > spin yesterday and have a few comments below... It has been sometime since the last version, thanks for the comments. I have some questions bellow. > > > --- > > tools/testing/selftests/livepatch/Makefile | 12 +- > > .../selftests/livepatch/test-syscall.sh | 52 ++++++ > > .../test_binaries/test_klp-call_getpid.c | 48 ++++++ > > .../selftests/livepatch/test_modules/Makefile | 3 +- > > .../livepatch/test_modules/test_klp_syscall.c | 150 ++++++++++++++++++ > > 5 files changed, 261 insertions(+), 4 deletions(-) > > create mode 100755 tools/testing/selftests/livepatch/test-syscall.sh > > create mode 100644 tools/testing/selftests/livepatch/test_binaries/test_klp-call_getpid.c > > create mode 100644 tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c > > > > diff --git a/tools/testing/selftests/livepatch/Makefile b/tools/testing/selftests/livepatch/Makefile > > index 5ef492b87bb1..35014197184e 100644 > > --- a/tools/testing/selftests/livepatch/Makefile > > +++ b/tools/testing/selftests/livepatch/Makefile > > @@ -1,10 +1,14 @@ > > # SPDX-License-Identifier: GPL-2.0 > > +include ../../../build/Build.include > > +include ../../../scripts/Makefile.arch > > +include ../../../scripts/Makefile.include > > > > TEST_FILES := settings \ > > test_modules > > > > # We need the test_modules dir in order to make gen_tar and install to work > > -TEST_GEN_PROGS_EXTENDED := test_modules/test_klp_atomic_replace.ko \ > > +TEST_GEN_PROGS_EXTENDED := test_binaries/test_klp-call_getpid \ > > Before I commit this to muscle memory, would "test_programs" better > describe this directory? When I see "binaries", I think of source vs. > build and not kernel vs. userspace. Done locally. > > > + test_modules/test_klp_atomic_replace.ko \ > > test_modules/test_klp_callbacks_busy.ko \ > > test_modules/test_klp_callbacks_demo.ko \ > > test_modules/test_klp_callbacks_demo2.ko \ > > @@ -13,7 +17,8 @@ TEST_GEN_PROGS_EXTENDED := test_modules/test_klp_atomic_replace.ko \ > > test_modules/test_klp_state.ko \ > > test_modules/test_klp_state2.ko \ > > test_modules/test_klp_state3.ko \ > > - test_modules/test_klp_shadow_vars.ko > > + test_modules/test_klp_shadow_vars.ko \ > > + test_modules/test_klp_syscall.ko > > > > TEST_PROGS_EXTENDED := functions.sh > > TEST_PROGS := \ > > @@ -21,7 +26,8 @@ TEST_PROGS := \ > > test-callbacks.sh \ > > test-shadow-vars.sh \ > > test-state.sh \ > > - test-ftrace.sh > > + test-ftrace.sh \ > > + test-syscall.sh > > > > # override lib.mk's default rules > > OVERRIDE_TARGETS := 1 > > diff --git a/tools/testing/selftests/livepatch/test-syscall.sh b/tools/testing/selftests/livepatch/test-syscall.sh > > new file mode 100755 > > index 000000000000..fb3270de7f1f > > --- /dev/null > > +++ b/tools/testing/selftests/livepatch/test-syscall.sh > > @@ -0,0 +1,52 @@ > > +#!/bin/bash > > +# SPDX-License-Identifier: GPL-2.0 > > +# Copyright (C) 2022 SUSE > > +# Author: Marcos Paulo de Souza <mpdesouza@suse.com> > > + > > +. $(dirname $0)/functions.sh > > + > > +MOD_SYSCALL=test_klp_syscall > > + > > +setup_config > > + > > +# - Start _NRPROC processes calling getpid and load a livepatch to patch the > > +# getpid syscall > > + > > +start_test "patch getpid syscall while being heavily hammered" > > + > > +for i in $(seq 0 $(( $(getconf _NPROCESSORS_ONLN) -1)) ); do > > + ./test_binaries/test_klp-call_getpid & > > + pids[$i]="$!" > > +done > > On second thought, bash should only iterate through the elements that > are defined, so your earlier version is probably better: > > for i in $(seq 1 $(getconf _NPROCESSORS_ONLN)); do > > > + > > +pid_list=$(echo ${pids[@]} | tr ' ' ',') > > +load_lp $MOD_SYSCALL klp_pids=$pid_list > > + > > We could wait for all test_klp-call_getpid instances to execute the > livepatch code: > > loop_until 'grep -q '^0$' /sys/kernel/test_klp_syscall/npids' > > This has a built in timeout, so it's not infinitely looping. You > could add some die "reason" logic here, or just let the existing code > cleanup after the failed test. Makes sense. Done locally. > > > +pending_pids=$(cat /sys/kernel/test_klp_syscall/npids) > > + > > +for pid in ${pids[@]}; do > > + kill $pid || true > > +done > > + > > +disable_lp $MOD_SYSCALL > > +unload_lp $MOD_SYSCALL > > + > > +if [[ "$pending_pids" != "0" ]]; then > > + echo -e "FAIL\n\n" > > + die "processes not livepatched: $pending_pids" > > +fi > > + > > +check_result "% insmod test_modules/$MOD_SYSCALL.ko klp_pids=$pid_list > > +livepatch: enabling patch '$MOD_SYSCALL' > > +livepatch: '$MOD_SYSCALL': initializing patching transition > > +livepatch: '$MOD_SYSCALL': starting patching transition > > +livepatch: '$MOD_SYSCALL': completing patching transition > > +livepatch: '$MOD_SYSCALL': patching complete > > +% echo 0 > /sys/kernel/livepatch/$MOD_SYSCALL/enabled > > +livepatch: '$MOD_SYSCALL': initializing unpatching transition > > +livepatch: '$MOD_SYSCALL': starting unpatching transition > > +livepatch: '$MOD_SYSCALL': completing unpatching transition > > +livepatch: '$MOD_SYSCALL': unpatching complete > > +% rmmod $MOD_SYSCALL" > > + > > +exit 0 > > diff --git a/tools/testing/selftests/livepatch/test_binaries/test_klp-call_getpid.c b/tools/testing/selftests/livepatch/test_binaries/test_klp-call_getpid.c > > new file mode 100644 > > index 000000000000..ec470660dee6 > > --- /dev/null > > +++ b/tools/testing/selftests/livepatch/test_binaries/test_klp-call_getpid.c > > @@ -0,0 +1,48 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (C) 2017 SUSE > > + * Author: Libor Pechacek <lpechacek@suse.cz> > > + */ > > + > > +#include <stdio.h> > > +#include <unistd.h> > > +#include <sys/syscall.h> > > +#include <sys/types.h> > > +#include <signal.h> > > + > > +static int stop = 0; > > nit: no need to init global to 0 Indeed. Fixed. > > > +static int sig_int; > > + > > +void hup_handler(int signum) > > +{ > > + stop = 1; > > +} > > + > > +void int_handler(int signum) > > +{ > > + stop = 1; > > + sig_int = 1; > > +} > > + > > +int main(int argc, char *argv[]) > > +{ > > + pid_t orig_pid, pid; > > + long count = 0; > > + > > + signal(SIGHUP, &hup_handler); > > + signal(SIGINT, &int_handler); > > + > > + orig_pid = syscall(SYS_getpid); > > + > > + while (!stop) { > > + pid = syscall(SYS_getpid); > > + if (pid != orig_pid) > > + return 1; > > This test doesn't care about the user program return code, but I wonder > if the status should be flipped -- this is the desired code path, not > the one at the end of main(), right? I got the code from our tests. IIUC, syscall with SYS_getpid cannot fail. I'll double check before sending v3. > > > + count++; > > + } > > + > > + if (sig_int) > > + printf("%ld iterations done\n", count); > > + > > + return 0; > > +} > > diff --git a/tools/testing/selftests/livepatch/test_modules/Makefile b/tools/testing/selftests/livepatch/test_modules/Makefile > > index 1eab43b741cd..ebb754accf46 100644 > > --- a/tools/testing/selftests/livepatch/test_modules/Makefile > > +++ b/tools/testing/selftests/livepatch/test_modules/Makefile > > @@ -10,7 +10,8 @@ obj-m += test_klp_atomic_replace.o \ > > test_klp_state.o \ > > test_klp_state2.o \ > > test_klp_state3.o \ > > - test_klp_shadow_vars.o > > + test_klp_shadow_vars.o \ > > + test_klp_syscall.o > > > > %.ko: > > make -C $(KDIR) M=$(TESTMODS_DIR) $@ > > diff --git a/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c b/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c > > new file mode 100644 > > index 000000000000..e90f4ac8e7a4 > > --- /dev/null > > +++ b/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c > > @@ -0,0 +1,150 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (C) 2017-2022 SUSE > > + * Authors: Libor Pechacek <lpechacek@suse.cz> > > + * Nicolai Stange <nstange@suse.de> > > + * Marcos Paulo de Souza <mpdesouza@suse.com> > > + */ > > + > > +#include <linux/module.h> > > +#include <linux/kernel.h> > > +#include <linux/sched.h> > > +#include <linux/slab.h> > > +#include <linux/livepatch.h> > > + > > +#if defined(__x86_64__) > > +#define FN_PREFIX __x64_ > > +#elif defined(__s390x__) > > +#define FN_PREFIX __s390x_ > > +#elif defined(__aarch64__) > > +#define FN_PREFIX __arm64_ > > +#else > > +/* powerpc does not select ARCH_HAS_SYSCALL_WRAPPER */ > > +#define FN_PREFIX > > +#endif > > + > > +struct klp_pid_t { > > + pid_t pid; > > + struct list_head list; > > +}; > > +static LIST_HEAD(klp_pid_list); > > + > > +/* Protects klp_pid_list */ > > +static DEFINE_MUTEX(kpid_mutex); > > + > > +static int klp_pids[NR_CPUS]; > > +static unsigned int npids; > > +module_param_array(klp_pids, int, &npids, 0); > > + > > +static ssize_t npids_show(struct kobject *kobj, struct kobj_attribute *attr, > > + char *buf) > > +{ > > + return sprintf(buf, "%u\n", npids); > > +} > > + > > +static struct kobj_attribute klp_attr = __ATTR_RO(npids); > > +static struct kobject *klp_kobj; > > + > > +static void free_klp_pid_list(void) > > +{ > > + struct klp_pid_t *kpid, *temp; > > + > > + mutex_lock(&kpid_mutex); > > + list_for_each_entry_safe(kpid, temp, &klp_pid_list, list) { > > + list_del(&kpid->list); > > + kfree(kpid); > > + } > > + mutex_unlock(&kpid_mutex); > > +} > > + > > +asmlinkage long lp_sys_getpid(void) > > +{ > > + struct klp_pid_t *kpid, *temp; > > + > > + /* > > + * For each thread calling getpid, check if the pid exists in > > + * klp_pid_list. If yes, decrement the npids variable and remove the pid > > + * from the list. npids will be later used to ensure that all pids > > + * transitioned to the liveaptched state. > > I go back and forth with this comment: 1) the code is pretty > self-explanatory and 2) it explains what test-syscall.sh is doing with > npids > > Describing the behavioral difference I suggest below would be helpful > (if you agree w/the change). Something about removing the current pid > from the list, okay, too. Makes sense. I'll move the comment to the test. > > Then as far as describing npids' purpose, I'd put that up at the top of > the file where npids is defined. It can be as generic as describing > what it's counting. Thanks for the suggestion. A short description will be added when defining the module argument. > > > + */ > > + mutex_lock(&kpid_mutex); > > + list_for_each_entry_safe(kpid, temp, &klp_pid_list, list) { > > + if (current->pid == kpid->pid) { > > + list_del(&kpid->list); > > + kfree(kpid); > > + npids--; > > + break; > > I think it would be safer to return task_tgid_vnr() here, but ... > > > + } > > + } > > + mutex_unlock(&kpid_mutex); > > + > > + return task_tgid_vnr(current); > > task_pid_vnr() here. That way we're only changing behavior for the > processes in the list and not all programs across the system. So, looking at getpid syscall definition, it calls task_tgid_vnr: SYSCALL_DEFINE0(getpid) { return task_tgid_vnr(current); } The function task_pid_vnr is called by gettid. So, IIUC, the test should call task_tgid_vnr, since getpid is being livepatched. Am I missing something? > > > +} > > + > > +static struct klp_func vmlinux_funcs[] = { > > + { > > + .old_name = __stringify(FN_PREFIX) "sys_getpid", > > + .new_func = lp_sys_getpid, > > + }, {} > > +}; > > + > > +static struct klp_object objs[] = { > > + { > > + /* name being NULL means vmlinux */ > > + .funcs = vmlinux_funcs, > > + }, {} > > +}; > > + > > +static struct klp_patch patch = { > > + .mod = THIS_MODULE, > > + .objs = objs, > > +}; > > + > > +static int livepatch_init(void) > > +{ > > + int ret; > > + struct klp_pid_t *kpid; > > + > > + if (npids > 0) { > > + int i; > > + > > + for (i = 0; i < npids; i++) { > > + kpid = kmalloc(sizeof(struct klp_pid_t), GFP_KERNEL); > > + if (!kpid) > > + goto err_mem; > > + > > + kpid->pid = klp_pids[i]; > > + list_add(&kpid->list, &klp_pid_list); > > + } > > + } > > + > > + klp_kobj = kobject_create_and_add("test_klp_syscall", kernel_kobj); > > + if (!klp_kobj) > > + goto err_mem; > > + > > + ret = sysfs_create_file(klp_kobj, &klp_attr.attr); > > + if (ret) { > > + kobject_put(klp_kobj); > > + goto out_klp_pid_list; > > + } > > + > > + return klp_enable_patch(&patch); > > + > > +err_mem: > > + ret = -ENOMEM; > > +out_klp_pid_list: > > + free_klp_pid_list(); > > + > > + return ret; > > +} > > + > > +static void livepatch_exit(void) > > +{ > > + free_klp_pid_list(); > > + kobject_put(klp_kobj); > > +} > > + > > +module_init(livepatch_init); > > +module_exit(livepatch_exit); > > +MODULE_LICENSE("GPL"); > > +MODULE_INFO(livepatch, "Y"); > > The linked list code looks like it should work and cleans up properly, > but if the current test is the extent of what we need, it would be > simpler to directly use klp_pids[] to track progress: since we're never > going to livepatch pid 0, we could just search klp_pids[] for our pid > and then zero it out once we've executed the livepatch code. > > This would let us drop out the kmalloc and list management code (looks > to be nearly 25% of the file at this point). I think the kpid_mutex > still needs to stay though. Iterating through the fixed array shouldn't > be too much more expensive than through the (shrinking) klp_pid_list. > > On the other hand, if you think that we'll need to adapt this module to > handle a dynamically changing list of klp_pids[], then perhaps a > separate list is the way to go. My first idea was to use klp_pids indexed by NR_CPUS, but I thought about bigger machines with more CPUs, making the array too big for just a couple of pids to be tracked... Thinking again, I don't think it's a problem. Also, I don't believe that we would need to dynamically add pids, so this code can go. In the next version I'll just access klp_pids directly. Thanks for the suggestion! > > Final note, kbuild is probably my weakest area, so I'm of limited help > review-wise there. In trying out the code, I wasn't sure exactly how to > run things. I did: > > $ make -C tools/testing/selftests/livepatch/ run_tests > > and that built everything and ran the tests (good). I changed a few .c > files and tried again, but they didn't rebuild. Then from > tools/testing/selftests/livepatch/test_modules/ I ran `make` and it only > ran the clean target (not good, I think). I need to test it and make sure the code is rebuilt. Thanks for noticing this problem. > > What is the typical workflow supposed to be in this combined setup? The plan was to support all forms of running the selftests: by running it in place and by running the "installed" kselftests. I'll test it again to make sure that the code is rebuilt when it should. > > Thanks, > > -- > Joe >
On Wed, Nov 23, 2022 at 10:35:27AM -0300, Marcos Paulo de Souza wrote: > On Tue, Jul 12, 2022 at 10:56:11AM -0400, Joe Lawrence wrote: > > On Thu, Jun 30, 2022 at 11:12:26AM -0300, Marcos Paulo de Souza wrote: > > > Syscalls are called a tricky way. Some architectures add a prefix to the > > > syscall name (SYSCALL_WRAPPER). > > > > > > This new test creates one userspace process per online cpu calling getpid > > > continuously and tries to livepatch the getpid function. Add the correct > > > function prefix for all archs that select HAS_SYSCALL_WRAPPER. > > > > > > Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> > > > > Hi Marcos, > > > > Thanks for working on this and sorry for the delayed reply. I gave it a > > spin yesterday and have a few comments below... > > It has been sometime since the last version, thanks for the comments. I have > some questions bellow. > > > > > > --- > > > tools/testing/selftests/livepatch/Makefile | 12 +- > > > .../selftests/livepatch/test-syscall.sh | 52 ++++++ > > > .../test_binaries/test_klp-call_getpid.c | 48 ++++++ > > > .../selftests/livepatch/test_modules/Makefile | 3 +- > > > .../livepatch/test_modules/test_klp_syscall.c | 150 ++++++++++++++++++ > > > 5 files changed, 261 insertions(+), 4 deletions(-) > > > create mode 100755 tools/testing/selftests/livepatch/test-syscall.sh > > > create mode 100644 tools/testing/selftests/livepatch/test_binaries/test_klp-call_getpid.c > > > create mode 100644 tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c > > > > > > diff --git a/tools/testing/selftests/livepatch/Makefile b/tools/testing/selftests/livepatch/Makefile > > > index 5ef492b87bb1..35014197184e 100644 > > > --- a/tools/testing/selftests/livepatch/Makefile > > > +++ b/tools/testing/selftests/livepatch/Makefile > > > @@ -1,10 +1,14 @@ > > > # SPDX-License-Identifier: GPL-2.0 > > > +include ../../../build/Build.include > > > +include ../../../scripts/Makefile.arch > > > +include ../../../scripts/Makefile.include > > > > > > TEST_FILES := settings \ > > > test_modules > > > > > > # We need the test_modules dir in order to make gen_tar and install to work > > > -TEST_GEN_PROGS_EXTENDED := test_modules/test_klp_atomic_replace.ko \ > > > +TEST_GEN_PROGS_EXTENDED := test_binaries/test_klp-call_getpid \ > > > > Before I commit this to muscle memory, would "test_programs" better > > describe this directory? When I see "binaries", I think of source vs. > > build and not kernel vs. userspace. > > Done locally. > > > > > > + test_modules/test_klp_atomic_replace.ko \ > > > test_modules/test_klp_callbacks_busy.ko \ > > > test_modules/test_klp_callbacks_demo.ko \ > > > test_modules/test_klp_callbacks_demo2.ko \ > > > @@ -13,7 +17,8 @@ TEST_GEN_PROGS_EXTENDED := test_modules/test_klp_atomic_replace.ko \ > > > test_modules/test_klp_state.ko \ > > > test_modules/test_klp_state2.ko \ > > > test_modules/test_klp_state3.ko \ > > > - test_modules/test_klp_shadow_vars.ko > > > + test_modules/test_klp_shadow_vars.ko \ > > > + test_modules/test_klp_syscall.ko > > > > > > TEST_PROGS_EXTENDED := functions.sh > > > TEST_PROGS := \ > > > @@ -21,7 +26,8 @@ TEST_PROGS := \ > > > test-callbacks.sh \ > > > test-shadow-vars.sh \ > > > test-state.sh \ > > > - test-ftrace.sh > > > + test-ftrace.sh \ > > > + test-syscall.sh > > > > > > # override lib.mk's default rules > > > OVERRIDE_TARGETS := 1 > > > diff --git a/tools/testing/selftests/livepatch/test-syscall.sh b/tools/testing/selftests/livepatch/test-syscall.sh > > > new file mode 100755 > > > index 000000000000..fb3270de7f1f > > > --- /dev/null > > > +++ b/tools/testing/selftests/livepatch/test-syscall.sh > > > @@ -0,0 +1,52 @@ > > > +#!/bin/bash > > > +# SPDX-License-Identifier: GPL-2.0 > > > +# Copyright (C) 2022 SUSE > > > +# Author: Marcos Paulo de Souza <mpdesouza@suse.com> > > > + > > > +. $(dirname $0)/functions.sh > > > + > > > +MOD_SYSCALL=test_klp_syscall > > > + > > > +setup_config > > > + > > > +# - Start _NRPROC processes calling getpid and load a livepatch to patch the > > > +# getpid syscall > > > + > > > +start_test "patch getpid syscall while being heavily hammered" > > > + > > > +for i in $(seq 0 $(( $(getconf _NPROCESSORS_ONLN) -1)) ); do > > > + ./test_binaries/test_klp-call_getpid & > > > + pids[$i]="$!" > > > +done > > > > On second thought, bash should only iterate through the elements that > > are defined, so your earlier version is probably better: > > > > for i in $(seq 1 $(getconf _NPROCESSORS_ONLN)); do > > > > > + > > > +pid_list=$(echo ${pids[@]} | tr ' ' ',') > > > +load_lp $MOD_SYSCALL klp_pids=$pid_list > > > + > > > > We could wait for all test_klp-call_getpid instances to execute the > > livepatch code: > > > > loop_until 'grep -q '^0$' /sys/kernel/test_klp_syscall/npids' > > > > This has a built in timeout, so it's not infinitely looping. You > > could add some die "reason" logic here, or just let the existing code > > cleanup after the failed test. > > Makes sense. Done locally. > > > > > > +pending_pids=$(cat /sys/kernel/test_klp_syscall/npids) > > > + > > > +for pid in ${pids[@]}; do > > > + kill $pid || true > > > +done > > > + > > > +disable_lp $MOD_SYSCALL > > > +unload_lp $MOD_SYSCALL > > > + > > > +if [[ "$pending_pids" != "0" ]]; then > > > + echo -e "FAIL\n\n" > > > + die "processes not livepatched: $pending_pids" > > > +fi > > > + > > > +check_result "% insmod test_modules/$MOD_SYSCALL.ko klp_pids=$pid_list > > > +livepatch: enabling patch '$MOD_SYSCALL' > > > +livepatch: '$MOD_SYSCALL': initializing patching transition > > > +livepatch: '$MOD_SYSCALL': starting patching transition > > > +livepatch: '$MOD_SYSCALL': completing patching transition > > > +livepatch: '$MOD_SYSCALL': patching complete > > > +% echo 0 > /sys/kernel/livepatch/$MOD_SYSCALL/enabled > > > +livepatch: '$MOD_SYSCALL': initializing unpatching transition > > > +livepatch: '$MOD_SYSCALL': starting unpatching transition > > > +livepatch: '$MOD_SYSCALL': completing unpatching transition > > > +livepatch: '$MOD_SYSCALL': unpatching complete > > > +% rmmod $MOD_SYSCALL" > > > + > > > +exit 0 > > > diff --git a/tools/testing/selftests/livepatch/test_binaries/test_klp-call_getpid.c b/tools/testing/selftests/livepatch/test_binaries/test_klp-call_getpid.c > > > new file mode 100644 > > > index 000000000000..ec470660dee6 > > > --- /dev/null > > > +++ b/tools/testing/selftests/livepatch/test_binaries/test_klp-call_getpid.c > > > @@ -0,0 +1,48 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * Copyright (C) 2017 SUSE > > > + * Author: Libor Pechacek <lpechacek@suse.cz> > > > + */ > > > + > > > +#include <stdio.h> > > > +#include <unistd.h> > > > +#include <sys/syscall.h> > > > +#include <sys/types.h> > > > +#include <signal.h> > > > + > > > +static int stop = 0; > > > > nit: no need to init global to 0 > > Indeed. Fixed. > > > > > > +static int sig_int; > > > + > > > +void hup_handler(int signum) > > > +{ > > > + stop = 1; > > > +} > > > + > > > +void int_handler(int signum) > > > +{ > > > + stop = 1; > > > + sig_int = 1; > > > +} > > > + > > > +int main(int argc, char *argv[]) > > > +{ > > > + pid_t orig_pid, pid; > > > + long count = 0; > > > + > > > + signal(SIGHUP, &hup_handler); > > > + signal(SIGINT, &int_handler); > > > + > > > + orig_pid = syscall(SYS_getpid); > > > + > > > + while (!stop) { > > > + pid = syscall(SYS_getpid); > > > + if (pid != orig_pid) > > > + return 1; > > > > This test doesn't care about the user program return code, but I wonder > > if the status should be flipped -- this is the desired code path, not > > the one at the end of main(), right? > > I got the code from our tests. IIUC, syscall with SYS_getpid cannot fail. I'll > double check before sending v3. > > > > > > + count++; > > > + } > > > + > > > + if (sig_int) > > > + printf("%ld iterations done\n", count); > > > + > > > + return 0; > > > +} > > > diff --git a/tools/testing/selftests/livepatch/test_modules/Makefile b/tools/testing/selftests/livepatch/test_modules/Makefile > > > index 1eab43b741cd..ebb754accf46 100644 > > > --- a/tools/testing/selftests/livepatch/test_modules/Makefile > > > +++ b/tools/testing/selftests/livepatch/test_modules/Makefile > > > @@ -10,7 +10,8 @@ obj-m += test_klp_atomic_replace.o \ > > > test_klp_state.o \ > > > test_klp_state2.o \ > > > test_klp_state3.o \ > > > - test_klp_shadow_vars.o > > > + test_klp_shadow_vars.o \ > > > + test_klp_syscall.o > > > > > > %.ko: > > > make -C $(KDIR) M=$(TESTMODS_DIR) $@ > > > diff --git a/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c b/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c > > > new file mode 100644 > > > index 000000000000..e90f4ac8e7a4 > > > --- /dev/null > > > +++ b/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c > > > @@ -0,0 +1,150 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * Copyright (C) 2017-2022 SUSE > > > + * Authors: Libor Pechacek <lpechacek@suse.cz> > > > + * Nicolai Stange <nstange@suse.de> > > > + * Marcos Paulo de Souza <mpdesouza@suse.com> > > > + */ > > > + > > > +#include <linux/module.h> > > > +#include <linux/kernel.h> > > > +#include <linux/sched.h> > > > +#include <linux/slab.h> > > > +#include <linux/livepatch.h> > > > + > > > +#if defined(__x86_64__) > > > +#define FN_PREFIX __x64_ > > > +#elif defined(__s390x__) > > > +#define FN_PREFIX __s390x_ > > > +#elif defined(__aarch64__) > > > +#define FN_PREFIX __arm64_ > > > +#else > > > +/* powerpc does not select ARCH_HAS_SYSCALL_WRAPPER */ > > > +#define FN_PREFIX > > > +#endif > > > + > > > +struct klp_pid_t { > > > + pid_t pid; > > > + struct list_head list; > > > +}; > > > +static LIST_HEAD(klp_pid_list); > > > + > > > +/* Protects klp_pid_list */ > > > +static DEFINE_MUTEX(kpid_mutex); > > > + > > > +static int klp_pids[NR_CPUS]; > > > +static unsigned int npids; > > > +module_param_array(klp_pids, int, &npids, 0); > > > + > > > +static ssize_t npids_show(struct kobject *kobj, struct kobj_attribute *attr, > > > + char *buf) > > > +{ > > > + return sprintf(buf, "%u\n", npids); > > > +} > > > + > > > +static struct kobj_attribute klp_attr = __ATTR_RO(npids); > > > +static struct kobject *klp_kobj; > > > + > > > +static void free_klp_pid_list(void) > > > +{ > > > + struct klp_pid_t *kpid, *temp; > > > + > > > + mutex_lock(&kpid_mutex); > > > + list_for_each_entry_safe(kpid, temp, &klp_pid_list, list) { > > > + list_del(&kpid->list); > > > + kfree(kpid); > > > + } > > > + mutex_unlock(&kpid_mutex); > > > +} > > > + > > > +asmlinkage long lp_sys_getpid(void) > > > +{ > > > + struct klp_pid_t *kpid, *temp; > > > + > > > + /* > > > + * For each thread calling getpid, check if the pid exists in > > > + * klp_pid_list. If yes, decrement the npids variable and remove the pid > > > + * from the list. npids will be later used to ensure that all pids > > > + * transitioned to the liveaptched state. > > > > I go back and forth with this comment: 1) the code is pretty > > self-explanatory and 2) it explains what test-syscall.sh is doing with > > npids > > > > Describing the behavioral difference I suggest below would be helpful > > (if you agree w/the change). Something about removing the current pid > > from the list, okay, too. > > Makes sense. I'll move the comment to the test. > > > > > Then as far as describing npids' purpose, I'd put that up at the top of > > the file where npids is defined. It can be as generic as describing > > what it's counting. > > Thanks for the suggestion. A short description will be added when defining the > module argument. > > > > > > + */ > > > + mutex_lock(&kpid_mutex); > > > + list_for_each_entry_safe(kpid, temp, &klp_pid_list, list) { > > > + if (current->pid == kpid->pid) { > > > + list_del(&kpid->list); > > > + kfree(kpid); > > > + npids--; > > > + break; > > > > I think it would be safer to return task_tgid_vnr() here, but ... > > > > > + } > > > + } > > > + mutex_unlock(&kpid_mutex); > > > + > > > + return task_tgid_vnr(current); > > > > task_pid_vnr() here. That way we're only changing behavior for the > > processes in the list and not all programs across the system. > > So, looking at getpid syscall definition, it calls task_tgid_vnr: > > SYSCALL_DEFINE0(getpid) > { > return task_tgid_vnr(current); > } > > The function task_pid_vnr is called by gettid. So, IIUC, the test should call > task_tgid_vnr, since getpid is being livepatched. > > Am I missing something? > > > > > > +} > > > + > > > +static struct klp_func vmlinux_funcs[] = { > > > + { > > > + .old_name = __stringify(FN_PREFIX) "sys_getpid", > > > + .new_func = lp_sys_getpid, > > > + }, {} > > > +}; > > > + > > > +static struct klp_object objs[] = { > > > + { > > > + /* name being NULL means vmlinux */ > > > + .funcs = vmlinux_funcs, > > > + }, {} > > > +}; > > > + > > > +static struct klp_patch patch = { > > > + .mod = THIS_MODULE, > > > + .objs = objs, > > > +}; > > > + > > > +static int livepatch_init(void) > > > +{ > > > + int ret; > > > + struct klp_pid_t *kpid; > > > + > > > + if (npids > 0) { > > > + int i; > > > + > > > + for (i = 0; i < npids; i++) { > > > + kpid = kmalloc(sizeof(struct klp_pid_t), GFP_KERNEL); > > > + if (!kpid) > > > + goto err_mem; > > > + > > > + kpid->pid = klp_pids[i]; > > > + list_add(&kpid->list, &klp_pid_list); > > > + } > > > + } > > > + > > > + klp_kobj = kobject_create_and_add("test_klp_syscall", kernel_kobj); > > > + if (!klp_kobj) > > > + goto err_mem; > > > + > > > + ret = sysfs_create_file(klp_kobj, &klp_attr.attr); > > > + if (ret) { > > > + kobject_put(klp_kobj); > > > + goto out_klp_pid_list; > > > + } > > > + > > > + return klp_enable_patch(&patch); > > > + > > > +err_mem: > > > + ret = -ENOMEM; > > > +out_klp_pid_list: > > > + free_klp_pid_list(); > > > + > > > + return ret; > > > +} > > > + > > > +static void livepatch_exit(void) > > > +{ > > > + free_klp_pid_list(); > > > + kobject_put(klp_kobj); > > > +} > > > + > > > +module_init(livepatch_init); > > > +module_exit(livepatch_exit); > > > +MODULE_LICENSE("GPL"); > > > +MODULE_INFO(livepatch, "Y"); > > > > The linked list code looks like it should work and cleans up properly, > > but if the current test is the extent of what we need, it would be > > simpler to directly use klp_pids[] to track progress: since we're never > > going to livepatch pid 0, we could just search klp_pids[] for our pid > > and then zero it out once we've executed the livepatch code. > > > > This would let us drop out the kmalloc and list management code (looks > > to be nearly 25% of the file at this point). I think the kpid_mutex > > still needs to stay though. Iterating through the fixed array shouldn't > > be too much more expensive than through the (shrinking) klp_pid_list. > > > > On the other hand, if you think that we'll need to adapt this module to > > handle a dynamically changing list of klp_pids[], then perhaps a > > separate list is the way to go. > > My first idea was to use klp_pids indexed by NR_CPUS, but I thought about bigger > machines with more CPUs, making the array too big for just a couple of pids to > be tracked... So, I already have klp_pids being defines using NR_CPUS, so forget what I said earlier. I'm changing the code locally to address klp_pids without using a linked list. > > Thinking again, I don't think it's a problem. Also, I don't believe that we > would need to dynamically add pids, so this code can go. In the next version > I'll just access klp_pids directly. Thanks for the suggestion! > > > > > Final note, kbuild is probably my weakest area, so I'm of limited help > > review-wise there. In trying out the code, I wasn't sure exactly how to > > run things. I did: > > > > $ make -C tools/testing/selftests/livepatch/ run_tests > > > > and that built everything and ran the tests (good). I changed a few .c > > files and tried again, but they didn't rebuild. Then from > > tools/testing/selftests/livepatch/test_modules/ I ran `make` and it only > > ran the clean target (not good, I think). > > I need to test it and make sure the code is rebuilt. Thanks for noticing this > problem. > > > > > What is the typical workflow supposed to be in this combined setup? > > The plan was to support all forms of running the selftests: by running it in > place and by running the "installed" kselftests. I'll test it again to make sure > that the code is rebuilt when it should. > > > > > Thanks, > > > > -- > > Joe > >
On Tue, Jul 12, 2022 at 10:56:11AM -0400, Joe Lawrence wrote: > On Thu, Jun 30, 2022 at 11:12:26AM -0300, Marcos Paulo de Souza wrote: ... > nit: no need to init global to 0 > > > +static int sig_int; > > + > > +void hup_handler(int signum) > > +{ > > + stop = 1; > > +} > > + > > +void int_handler(int signum) > > +{ > > + stop = 1; > > + sig_int = 1; > > +} > > + > > +int main(int argc, char *argv[]) > > +{ > > + pid_t orig_pid, pid; > > + long count = 0; > > + > > + signal(SIGHUP, &hup_handler); > > + signal(SIGINT, &int_handler); > > + > > + orig_pid = syscall(SYS_getpid); > > + > > + while (!stop) { > > + pid = syscall(SYS_getpid); > > + if (pid != orig_pid) > > + return 1; > > This test doesn't care about the user program return code, but I wonder > if the status should be flipped -- this is the desired code path, not > the one at the end of main(), right? > ... > > + */ > > + mutex_lock(&kpid_mutex); > > + list_for_each_entry_safe(kpid, temp, &klp_pid_list, list) { > > + if (current->pid == kpid->pid) { > > + list_del(&kpid->list); > > + kfree(kpid); > > + npids--; > > + break; > > I think it would be safer to return task_tgid_vnr() here, but ... > > > + } > > + } > > + mutex_unlock(&kpid_mutex); > > + > > + return task_tgid_vnr(current); > > task_pid_vnr() here. That way we're only changing behavior for the > processes in the list and not all programs across the system. I believe that these two suggestions can be linked per your answer. First of all, I didn't write the original test program, but I agree that we can make it better. My intent by upstreaming the test was to ensure that test programs would keep working even when livepatching getpid while having processes calling getpid nonstop. For the purpose of the test, the test module livepatches getpid, but keeping the same behavior as before. The only change is to keep track of the test programs that need to transition to livepatched state. Per your comment on the test program it seems that we expected to receive a different value from getpid, but it's not the case here. I believe the chec on test program is confusing and doesn't bring any benefit, so maybe it's better to remove it and keep the test even simpler: --- a/tools/testing/selftests/livepatch/test_programs/test_klp-call_getpid.c +++ b/tools/testing/selftests/livepatch/test_programs/test_klp-call_getpid.c @@ -26,18 +26,13 @@ void int_handler(int signum) int main(int argc, char *argv[]) { - pid_t orig_pid, pid; long count = 0; signal(SIGHUP, &hup_handler); signal(SIGINT, &int_handler); - orig_pid = syscall(SYS_getpid); - while (!stop) { - pid = syscall(SYS_getpid); - if (pid != orig_pid) - return 1; + (void)syscall(SYS_getpid); count++; } As as only care about the processes transitioning to the livepatched state, we really don't care about the getpid return value (as it should be the same from patches and unpatched state). I believe this resolves both issues. What do you think?
On 11/24/22 8:05 AM, Marcos Paulo de Souza wrote: > On Tue, Jul 12, 2022 at 10:56:11AM -0400, Joe Lawrence wrote: >> On Thu, Jun 30, 2022 at 11:12:26AM -0300, Marcos Paulo de Souza wrote: > ... >> nit: no need to init global to 0 >> >>> +static int sig_int; >>> + >>> +void hup_handler(int signum) >>> +{ >>> + stop = 1; >>> +} >>> + >>> +void int_handler(int signum) >>> +{ >>> + stop = 1; >>> + sig_int = 1; >>> +} >>> + >>> +int main(int argc, char *argv[]) >>> +{ >>> + pid_t orig_pid, pid; >>> + long count = 0; >>> + >>> + signal(SIGHUP, &hup_handler); >>> + signal(SIGINT, &int_handler); >>> + >>> + orig_pid = syscall(SYS_getpid); >>> + >>> + while (!stop) { >>> + pid = syscall(SYS_getpid); >>> + if (pid != orig_pid) >>> + return 1; >> >> This test doesn't care about the user program return code, but I wonder >> if the status should be flipped -- this is the desired code path, not >> the one at the end of main(), right? >> > ... Ah, nevermind ... >>> + */ >>> + mutex_lock(&kpid_mutex); >>> + list_for_each_entry_safe(kpid, temp, &klp_pid_list, list) { >>> + if (current->pid == kpid->pid) { >>> + list_del(&kpid->list); >>> + kfree(kpid); >>> + npids--; >>> + break; >> >> I think it would be safer to return task_tgid_vnr() here, but ... >> >>> + } >>> + } >>> + mutex_unlock(&kpid_mutex); >>> + >>> + return task_tgid_vnr(current); >> >> task_pid_vnr() here. That way we're only changing behavior for the >> processes in the list and not all programs across the system. > > I believe that these two suggestions can be linked per your answer. First of > all, I didn't write the original test program, but I agree that we can make it > better. > > My intent by upstreaming the test was to ensure that test programs > would keep working even when livepatching getpid while having processes calling > getpid nonstop. For the purpose of the test, the test module livepatches getpid, > but keeping the same behavior as before. The only change is to keep track of the > test programs that need to transition to livepatched state. > > Per your comment on the test program it seems that we expected to receive a > different value from getpid, but it's not the case here. I believe the chec on > test program is confusing and doesn't bring any benefit, so maybe it's better to > remove it and keep the test even simpler: > Now that I re-read it months later, I see that I mistakenly thought that the livepatch was supposed to modify the return value. So disregard my earlier suggestions / questions about that. > --- a/tools/testing/selftests/livepatch/test_programs/test_klp-call_getpid.c > +++ b/tools/testing/selftests/livepatch/test_programs/test_klp-call_getpid.c > @@ -26,18 +26,13 @@ void int_handler(int signum) > > int main(int argc, char *argv[]) > { > - pid_t orig_pid, pid; > long count = 0; > > signal(SIGHUP, &hup_handler); > signal(SIGINT, &int_handler); > > - orig_pid = syscall(SYS_getpid); > - > while (!stop) { > - pid = syscall(SYS_getpid); > - if (pid != orig_pid) > - return 1; > + (void)syscall(SYS_getpid); > count++; > } > > As as only care about the processes transitioning to the livepatched state, we > really don't care about the getpid return value (as it should be the same from patches > and unpatched state). > > I believe this resolves both issues. What do you think? > Verifying orig_pid against the current pid return value would verify that it was cleanly patched... though that seems incidental to the purpose of the test. The simplification above is ok with me either way.
diff --git a/tools/testing/selftests/livepatch/Makefile b/tools/testing/selftests/livepatch/Makefile index 5ef492b87bb1..35014197184e 100644 --- a/tools/testing/selftests/livepatch/Makefile +++ b/tools/testing/selftests/livepatch/Makefile @@ -1,10 +1,14 @@ # SPDX-License-Identifier: GPL-2.0 +include ../../../build/Build.include +include ../../../scripts/Makefile.arch +include ../../../scripts/Makefile.include TEST_FILES := settings \ test_modules # We need the test_modules dir in order to make gen_tar and install to work -TEST_GEN_PROGS_EXTENDED := test_modules/test_klp_atomic_replace.ko \ +TEST_GEN_PROGS_EXTENDED := test_binaries/test_klp-call_getpid \ + test_modules/test_klp_atomic_replace.ko \ test_modules/test_klp_callbacks_busy.ko \ test_modules/test_klp_callbacks_demo.ko \ test_modules/test_klp_callbacks_demo2.ko \ @@ -13,7 +17,8 @@ TEST_GEN_PROGS_EXTENDED := test_modules/test_klp_atomic_replace.ko \ test_modules/test_klp_state.ko \ test_modules/test_klp_state2.ko \ test_modules/test_klp_state3.ko \ - test_modules/test_klp_shadow_vars.ko + test_modules/test_klp_shadow_vars.ko \ + test_modules/test_klp_syscall.ko TEST_PROGS_EXTENDED := functions.sh TEST_PROGS := \ @@ -21,7 +26,8 @@ TEST_PROGS := \ test-callbacks.sh \ test-shadow-vars.sh \ test-state.sh \ - test-ftrace.sh + test-ftrace.sh \ + test-syscall.sh # override lib.mk's default rules OVERRIDE_TARGETS := 1 diff --git a/tools/testing/selftests/livepatch/test-syscall.sh b/tools/testing/selftests/livepatch/test-syscall.sh new file mode 100755 index 000000000000..fb3270de7f1f --- /dev/null +++ b/tools/testing/selftests/livepatch/test-syscall.sh @@ -0,0 +1,52 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (C) 2022 SUSE +# Author: Marcos Paulo de Souza <mpdesouza@suse.com> + +. $(dirname $0)/functions.sh + +MOD_SYSCALL=test_klp_syscall + +setup_config + +# - Start _NRPROC processes calling getpid and load a livepatch to patch the +# getpid syscall + +start_test "patch getpid syscall while being heavily hammered" + +for i in $(seq 0 $(( $(getconf _NPROCESSORS_ONLN) -1)) ); do + ./test_binaries/test_klp-call_getpid & + pids[$i]="$!" +done + +pid_list=$(echo ${pids[@]} | tr ' ' ',') +load_lp $MOD_SYSCALL klp_pids=$pid_list + +pending_pids=$(cat /sys/kernel/test_klp_syscall/npids) + +for pid in ${pids[@]}; do + kill $pid || true +done + +disable_lp $MOD_SYSCALL +unload_lp $MOD_SYSCALL + +if [[ "$pending_pids" != "0" ]]; then + echo -e "FAIL\n\n" + die "processes not livepatched: $pending_pids" +fi + +check_result "% insmod test_modules/$MOD_SYSCALL.ko klp_pids=$pid_list +livepatch: enabling patch '$MOD_SYSCALL' +livepatch: '$MOD_SYSCALL': initializing patching transition +livepatch: '$MOD_SYSCALL': starting patching transition +livepatch: '$MOD_SYSCALL': completing patching transition +livepatch: '$MOD_SYSCALL': patching complete +% echo 0 > /sys/kernel/livepatch/$MOD_SYSCALL/enabled +livepatch: '$MOD_SYSCALL': initializing unpatching transition +livepatch: '$MOD_SYSCALL': starting unpatching transition +livepatch: '$MOD_SYSCALL': completing unpatching transition +livepatch: '$MOD_SYSCALL': unpatching complete +% rmmod $MOD_SYSCALL" + +exit 0 diff --git a/tools/testing/selftests/livepatch/test_binaries/test_klp-call_getpid.c b/tools/testing/selftests/livepatch/test_binaries/test_klp-call_getpid.c new file mode 100644 index 000000000000..ec470660dee6 --- /dev/null +++ b/tools/testing/selftests/livepatch/test_binaries/test_klp-call_getpid.c @@ -0,0 +1,48 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2017 SUSE + * Author: Libor Pechacek <lpechacek@suse.cz> + */ + +#include <stdio.h> +#include <unistd.h> +#include <sys/syscall.h> +#include <sys/types.h> +#include <signal.h> + +static int stop = 0; +static int sig_int; + +void hup_handler(int signum) +{ + stop = 1; +} + +void int_handler(int signum) +{ + stop = 1; + sig_int = 1; +} + +int main(int argc, char *argv[]) +{ + pid_t orig_pid, pid; + long count = 0; + + signal(SIGHUP, &hup_handler); + signal(SIGINT, &int_handler); + + orig_pid = syscall(SYS_getpid); + + while (!stop) { + pid = syscall(SYS_getpid); + if (pid != orig_pid) + return 1; + count++; + } + + if (sig_int) + printf("%ld iterations done\n", count); + + return 0; +} diff --git a/tools/testing/selftests/livepatch/test_modules/Makefile b/tools/testing/selftests/livepatch/test_modules/Makefile index 1eab43b741cd..ebb754accf46 100644 --- a/tools/testing/selftests/livepatch/test_modules/Makefile +++ b/tools/testing/selftests/livepatch/test_modules/Makefile @@ -10,7 +10,8 @@ obj-m += test_klp_atomic_replace.o \ test_klp_state.o \ test_klp_state2.o \ test_klp_state3.o \ - test_klp_shadow_vars.o + test_klp_shadow_vars.o \ + test_klp_syscall.o %.ko: make -C $(KDIR) M=$(TESTMODS_DIR) $@ diff --git a/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c b/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c new file mode 100644 index 000000000000..e90f4ac8e7a4 --- /dev/null +++ b/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c @@ -0,0 +1,150 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2017-2022 SUSE + * Authors: Libor Pechacek <lpechacek@suse.cz> + * Nicolai Stange <nstange@suse.de> + * Marcos Paulo de Souza <mpdesouza@suse.com> + */ + +#include <linux/module.h> +#include <linux/kernel.h> +#include <linux/sched.h> +#include <linux/slab.h> +#include <linux/livepatch.h> + +#if defined(__x86_64__) +#define FN_PREFIX __x64_ +#elif defined(__s390x__) +#define FN_PREFIX __s390x_ +#elif defined(__aarch64__) +#define FN_PREFIX __arm64_ +#else +/* powerpc does not select ARCH_HAS_SYSCALL_WRAPPER */ +#define FN_PREFIX +#endif + +struct klp_pid_t { + pid_t pid; + struct list_head list; +}; +static LIST_HEAD(klp_pid_list); + +/* Protects klp_pid_list */ +static DEFINE_MUTEX(kpid_mutex); + +static int klp_pids[NR_CPUS]; +static unsigned int npids; +module_param_array(klp_pids, int, &npids, 0); + +static ssize_t npids_show(struct kobject *kobj, struct kobj_attribute *attr, + char *buf) +{ + return sprintf(buf, "%u\n", npids); +} + +static struct kobj_attribute klp_attr = __ATTR_RO(npids); +static struct kobject *klp_kobj; + +static void free_klp_pid_list(void) +{ + struct klp_pid_t *kpid, *temp; + + mutex_lock(&kpid_mutex); + list_for_each_entry_safe(kpid, temp, &klp_pid_list, list) { + list_del(&kpid->list); + kfree(kpid); + } + mutex_unlock(&kpid_mutex); +} + +asmlinkage long lp_sys_getpid(void) +{ + struct klp_pid_t *kpid, *temp; + + /* + * For each thread calling getpid, check if the pid exists in + * klp_pid_list. If yes, decrement the npids variable and remove the pid + * from the list. npids will be later used to ensure that all pids + * transitioned to the liveaptched state. + */ + mutex_lock(&kpid_mutex); + list_for_each_entry_safe(kpid, temp, &klp_pid_list, list) { + if (current->pid == kpid->pid) { + list_del(&kpid->list); + kfree(kpid); + npids--; + break; + } + } + mutex_unlock(&kpid_mutex); + + return task_tgid_vnr(current); +} + +static struct klp_func vmlinux_funcs[] = { + { + .old_name = __stringify(FN_PREFIX) "sys_getpid", + .new_func = lp_sys_getpid, + }, {} +}; + +static struct klp_object objs[] = { + { + /* name being NULL means vmlinux */ + .funcs = vmlinux_funcs, + }, {} +}; + +static struct klp_patch patch = { + .mod = THIS_MODULE, + .objs = objs, +}; + +static int livepatch_init(void) +{ + int ret; + struct klp_pid_t *kpid; + + if (npids > 0) { + int i; + + for (i = 0; i < npids; i++) { + kpid = kmalloc(sizeof(struct klp_pid_t), GFP_KERNEL); + if (!kpid) + goto err_mem; + + kpid->pid = klp_pids[i]; + list_add(&kpid->list, &klp_pid_list); + } + } + + klp_kobj = kobject_create_and_add("test_klp_syscall", kernel_kobj); + if (!klp_kobj) + goto err_mem; + + ret = sysfs_create_file(klp_kobj, &klp_attr.attr); + if (ret) { + kobject_put(klp_kobj); + goto out_klp_pid_list; + } + + return klp_enable_patch(&patch); + +err_mem: + ret = -ENOMEM; +out_klp_pid_list: + free_klp_pid_list(); + + return ret; +} + +static void livepatch_exit(void) +{ + free_klp_pid_list(); + kobject_put(klp_kobj); +} + +module_init(livepatch_init); +module_exit(livepatch_exit); +MODULE_LICENSE("GPL"); +MODULE_INFO(livepatch, "Y");
Syscalls are called a tricky way. Some architectures add a prefix to the syscall name (SYSCALL_WRAPPER). This new test creates one userspace process per online cpu calling getpid continuously and tries to livepatch the getpid function. Add the correct function prefix for all archs that select HAS_SYSCALL_WRAPPER. Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com> --- tools/testing/selftests/livepatch/Makefile | 12 +- .../selftests/livepatch/test-syscall.sh | 52 ++++++ .../test_binaries/test_klp-call_getpid.c | 48 ++++++ .../selftests/livepatch/test_modules/Makefile | 3 +- .../livepatch/test_modules/test_klp_syscall.c | 150 ++++++++++++++++++ 5 files changed, 261 insertions(+), 4 deletions(-) create mode 100755 tools/testing/selftests/livepatch/test-syscall.sh create mode 100644 tools/testing/selftests/livepatch/test_binaries/test_klp-call_getpid.c create mode 100644 tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c