On Fri, Jun 03, 2022 at 11:32:42AM -0300, Marcos Paulo de Souza wrote:
> Syscalls are called a tricky way. Test that it is possible and works.
"Tricky" may be accurate, but short on details. If the full story is
too much for the commit log, maybe just handwave about the
__SYSCALL_DEFINEx preprocessor wrappers, etc. :)
>
> This new test creates one userspace process per online cpu calling getpid
> continuously and tries to livepatch the getpid function.
>
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@xxxxxxxx>
> ---
> tools/testing/selftests/livepatch/Makefile | 12 +++-
> .../selftests/livepatch/test-syscall.sh | 46 ++++++++++++++
> .../test_binaries/test_klp-call_getpid.c | 48 +++++++++++++++
> .../selftests/livepatch/test_modules/Makefile | 3 +-
> .../livepatch/test_modules/test_klp_syscall.c | 60 +++++++++++++++++++
> 5 files changed, 165 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 \
> + 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..f1d49e6ce2ee
> --- /dev/null
> +++ b/tools/testing/selftests/livepatch/test-syscall.sh
> @@ -0,0 +1,46 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (C) 2022 SUSE
> +# Author: Marcos Paulo de Souza <mpdesouza@xxxxxxxx>
> +
> +. $(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"
> +
> +declare -a pids
> +for i in $(seq 1 $(getconf _NPROCESSORS_ONLN)); do
> + ./test_klp-call_getpid &
> + pids[${#pids[*]}]="$!"
^^^^^^^^^^^
This looks fancy, would it be more approachable to use something like
this:
for i in $(seq 0 $(( $(getconf _NPROCESSORS_ONLN) - 1 )) ); do
./test_klp-call_getpid &
pids[$i]="$!"
done
or is there some strange race condition that requires using the method
in the patch?
> +done
> +
> +load_lp $MOD_SYSCALL
> +# Success, getpid syscall was livepatched
I don't think we need this comment as success is implied, otherwise
load_lp would have pulled the die() ripcord.
> +
> +for pid in ${pids[@]}; do
> + kill $pid || true
> +done
> +
> +disable_lp $MOD_SYSCALL
> +unload_lp $MOD_SYSCALL
> +
> +check_result "% insmod test_modules/$MOD_SYSCALL.ko
> +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..be9d3110687d
> --- /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@xxxxxxx>
> + */
> +
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <sys/syscall.h>
> +#include <sys/types.h>
> +#include <signal.h>
> +
> +static int run = 1;
> +static int sig_int;
> +
> +void hup_handler(int signum)
> +{
> + run = 0;
> +}
> +
> +void int_handler(int signum)
> +{
> + run = 0;
> + 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(run) {
nit: could do s/run/stop/g and flip the assignment and check here so it
matches the sig_int flag, ie, 0 by default and 1 on HUP / INT.
> + pid = syscall(SYS_getpid);
> + if (pid != orig_pid)
> + return 1;
> + count++;
> + }
> +
> + if (sig_int)
> + printf("%d iterations done\n", count);
nit: %ld for long
> +
> + return 0;
> +}
> diff --git a/tools/testing/selftests/livepatch/test_modules/Makefile b/tools/testing/selftests/livepatch/test_modules/Makefile
> index 375180bc1b16..288c65ccd080 100644
> --- a/tools/testing/selftests/livepatch/test_modules/Makefile
> +++ b/tools/testing/selftests/livepatch/test_modules/Makefile
> @@ -15,7 +15,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..e170accfb10c
> --- /dev/null
> +++ b/tools/testing/selftests/livepatch/test_modules/test_klp_syscall.c
> @@ -0,0 +1,60 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2017-2022 SUSE
> + * Authors: Libor Pechacek <lpechacek@xxxxxxx>
> + * Nicolai Stange <nstange@xxxxxxx>
> + * Marcos Paulo de Souza <mpdesouza@xxxxxxxx>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/sched.h>
> +#include <linux/livepatch.h>
> +
> +#if defined(__x86_64__)
> +#define FN_PREFIX __x64_
> +#elif defined(__s390x__)
> +#define FN_PREFIX __s390x_
> +#elif defined(__PPC64__)
> +#define FN_PREFIX __ppc64_
> +#else
> +#error "livepatch not supported"
What about 32-bit powerpc [1]? Would this cause the selftests to fail
on that hardware?
[1] https://lore.kernel.org/live-patching/cover.1640017960.git.christophe.leroy@xxxxxxxxxx/
> +#endif
> +
> +asmlinkage long lp_sys_getpid(void)
> +{
> + 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)
> +{
> + return klp_enable_patch(&patch);
> +}
> +
> +static void livepatch_exit(void)
> +{
> +}
> +
> +module_init(livepatch_init);
> +module_exit(livepatch_exit);
> +MODULE_LICENSE("GPL");
> +MODULE_INFO(livepatch, "Y");
> --
> 2.35.3
>
Hi Marcos,
A few comments and small nits found inline above.
First, the test is pretty straightforward and a reasonable one to run,
particularly putting the system under some load while patching.
What do you think about the following tweaks:
test_klp_syscall:
- only calls the new syscall behavior for a list of known PIDs
- PID list supplied via module parameter
- when calling new syscall, PID is removed from list,
PIDs-left-to-patch counter (exposed to sysfs) decremented
test-syscall.sh:
- loads test_klp_syscall with test_klp-call_getpid PIDs that it spawned
- before killing the test_klp-call_getpid PIDs, checks that
test_klp_syscall's sysfs PIDs-left-to-patch counter == 0
I think this would ensure a few things:
1. Restrict the getpid syscall change to only callers expecting new
behavior
2. Force test_klp_syscall to patch each test_klp-call_getpid
3. Force all test_klp-call_getpid to hang around long enough for (2)
Maybe this is too defensive, but I think could be handled without adding
too much complexity.
Regards,
--
Joe
