On Wed, Feb 09, 2022 at 02:02:33PM +0100, Petr Mladek wrote: > Hmm, test-ftrace.sh fails with this patch: > > localhost:/prace/kernel/linux/tools/testing/selftests/livepatch # ./test-ftrace.sh > TEST: livepatch interaction with ftrace_enabled sysctl ... SKIP: failed to set kernel.ftrace_enabled=0 > > This is the dmesg output: > > [ 436.176433] ===== TEST: livepatch interaction with ftrace_enabled sysctl ===== > [ 436.186942] livepatch: kernel.ftrace_enabled = 0 > [ 436.187854] % modprobe test_klp_livepatch > [ 436.217657] livepatch: enabling patch 'test_klp_livepatch' > [ 436.218467] livepatch: 'test_klp_livepatch': initializing patching transition > [ 436.219679] livepatch: failed to register ftrace handler for function 'cmdline_proc_show' (-16) > [ 436.221003] livepatch: failed to patch object 'vmlinux' > [ 436.221786] livepatch: failed to enable patch 'test_klp_livepatch' > [ 436.222716] livepatch: 'test_klp_livepatch': canceling patching transition, going to unpatch > [ 436.223955] livepatch: 'test_klp_livepatch': completing unpatching transition > [ 436.225476] livepatch: 'test_klp_livepatch': unpatching complete > [ 436.273295] modprobe: ERROR: could not insert 'test_klp_livepatch': Device or resource busy > [ 436.284522] livepatch: kernel.ftrace_enabled = 1 > [ 436.305353] % modprobe test_klp_livepatch > [ 436.334929] livepatch: enabling patch 'test_klp_livepatch' > [ 436.335781] livepatch: 'test_klp_livepatch': initializing patching transition > [ 436.338099] livepatch: 'test_klp_livepatch': starting patching transition > [ 438.082707] livepatch: 'test_klp_livepatch': completing patching transition > [ 438.084106] livepatch: 'test_klp_livepatch': patching complete > [ 438.199997] livepatch: sysctl: setting key "kernel.ftrace_enabled": Device or resource busy > [ 438.201249] kernel.ftrace_enabled = 1 > [ 438.201929] SKIP: failed to set kernel.ftrace_enabled=0 > > > The problem is the 2nd "set_ftrace_enabled 0" call in > tools/testing/selftests/livepatch/test-ftrace.sh > > It is done when "test_klp_livepatch" is loaded. ftrace could not be > disabled because it is used by the livepatch. It is expected behavior > and the test should succeed in this case. Ah, I missed this because in my configuration I was expecting the test to be skipped. What do we think is the preferred alternative? Perhaps we could add an optional argument to set_trace_enabled() that specifies the expected value of kernel.ftrace_enabled after invoking sysctl? What do you think of something like this: + + if [[ "$result" != "kernel.ftrace_enabled = $1" ]]; then + if [[ -z "$2" || "$2" == "$1" ]]; then + skip "failed to set kernel.ftrace_enabled=$1" + fi + elif [[ -n "$2" && "$2" != "$1" ]]; then + die "Unexpectedly set kernel.ftrace_enabled=$1" + fi test-ftrace.sh could then specify 'set_ftrace_enabled 0 1' when validating that ftrace can't be disabled when it's being used by livepatch. Not sure if it makes sense to use skip() in the first case and die() in the latter case, but being able to unexpectedly set kernel.ftrace_enabled seems more problematic and within the control of the test than not being able to set it. I could also just leave off the elif case as it's not needed for the test suite as it exists today, but I think it makes the set_ftrace_enabled() API more consistent and intuitive. Thanks, David
