Re: [PATCH v3 1/2] selftests/livepatch: build test modules against the selected kernel

[Petr Mladek <pmladek@suse.com>]

Hi,

the commit message seems to be generated using LLM.
For me, it is a hard to read maze. But I'll try to go through it.

Let's start with Subject. What is "the selected kernel"?
How can it be selected?

I would personally say that this this patch "allows to pre-built
the test modules for the livepatch selftest into a separate directory".

My understanding is that there are several ways how to build
the test modules, some work, and some does not. I would help
me if the commit message better explains what works and what
does not work.


On Tue 2026-11-03 17:27:05, Sasha Levin wrote:
> The livepatch test modules are built against /lib/modules/$(uname -r)/build
> unless KDIR is set, and they are built unconditionally. This has three
> problems:

My understanding is that the Makefiles currently expects that
"the kernel devel files, for the running kernel, are installed
on the system " so that they could find them by
/lib/modules/$(uname -r)/build.

The KDIR allows to find the devel files in another directory?
But how do we set it?

> - When the selftests are built for a kernel in an output directory
>   (make O=...), the modules are still built against the running host
>   kernel, or fail when the host has no build directory.

What exactly is in the output directory?
What is build directory?
How they differ?

I see in Documentation/dev-tools/kselftest.rst:

  <paste>
  To build, save output files in a separate directory with O= ::

    $ make O=/tmp/kselftest TARGETS="size timers" kselftest
  </paste>

It would help if the commit message follows this naming scheme,
aka "save output files in a separate directory with O="
and mentions an usable example, e.g.

  $ make O=/tmp/kselftest TARGETS="livepatch" kselftest

explain where it is supposed to search for the build files.

My understanding is that you want to pre-built the test modules
when building a kernel. I mean to use a custom kernel source
tree instead of using devel files for the currently running kernel.


> - The top-level selftests Makefile passes O to every suite, and the
>   test_modules Makefile hands it on to the kernel build. Even an empty
>   O= on the command line overrides the KBUILD_OUTPUT set by the
>   Makefile of a kernel build directory, so the build fails against
>   output directories and distribution headers such as Debian's:
>   "make -C tools/testing/selftests TARGETS=livepatch" builds no
>   modules at all.

This is another way how to build the modules:

    make -C tools/testing/selftests TARGETS=livepatch

Again, it would be great to make it clear that it is for
per-building the test modules from a custom kernel sources.

But it mentions also KBUILD_OUTPUT which seems to be another
way, from Documentation/dev-tools/kselftest.rst:

  <paste>
  To build, save output files in a separate directory with KBUILD_OUTPUT ::

    export KBUILD_OUTPUT=/tmp/kselftest; make TARGETS="size timers" kselftest
  </paste>

It seems that "KBUILD_OUTPUT" has the same effect as "O".
Why is it mentioned in a paragraph with
"make -C tools/testing/selftests TARGETS=livepatch" example?

> - A kernel configured without CONFIG_LIVEPATCH cannot build the
>   modules, although the installed scripts would correctly skip.

This is a puzzle. If "the installed scripts would correctly skip"
then what fails?


> Select KDIR from O or KBUILD_OUTPUT when it is a prepared kernel build
> directory containing Module.symvers, then from the source tree when the
> kernel was built in place, and otherwise from the running kernel. An
> explicit KDIR still takes precedence. Pass KDIR to test_modules as an
> absolute path, also when it was given on the command line, and stop
> forwarding O and KBUILD_OUTPUT to the kernel build, including when the
> selftests are built through the kernel's kselftest targets, since KDIR
> already names the output directory.
> 
> Read CONFIG_LIVEPATCH from the selected .config and skip the modules
> only when the configuration is readable and the option is not enabled. A
> KDIR without a readable configuration still attempts the build, so that
> a directory that is not a prepared kernel tree fails the build instead
> of silently producing an installation without modules. A KDIR that does
> not exist keeps being skipped, as before.
> 
> Always clean test_modules, through the kernel source tree that contains
> them. Cleaning external modules needs neither a configuration nor
> Module.symvers, and unlike the build targets, the clean target of the
> selftests Makefile does not pass the absolute O on, so the kernel the
> modules were built against cannot always be found at that point.

The above paragraphs might be "fine" as a "More details" section.

> Have the scripts report a skip after the root check when the running
> kernel does not provide livepatch.

This patch should fix the build. This change does not fit here,
see below.

> --- a/tools/testing/selftests/livepatch/Makefile
> +++ b/tools/testing/selftests/livepatch/Makefile
> @@ -1,7 +1,26 @@
>  # SPDX-License-Identifier: GPL-2.0
>  

The code below is a maze. It would be nice to add a comment here
explaining how we try to find the kernel sources, ideally
even include examples.

   1. When using separate output directory, for example:
      make O=/tmp/kselftest TARGETS="livepatch" kselftest
      export KBUILD_OUTPUT=/tmp/kselftest; make TARGETS="livepatch" kselftest

   2. When building directly from the tools/testing/selftests/livepatch
      source directory.

   3. When using installed devel files for the running kernel.

But I might be wrong.

> +ifndef KDIR
> +LIVEPATCH_OBJTREE := $(firstword $(O) $(KBUILD_OUTPUT))

The above checks both $(O) $(KBUILD_OUTPUT)

> +ifneq ($(and $(LIVEPATCH_OBJTREE),$(wildcard $(LIVEPATCH_OBJTREE)/Module.symvers)),)
> +KDIR := $(LIVEPATCH_OBJTREE)

I am a bit confused. What is the KDIR when using, for example:

      make O=/tmp/kselftest TARGETS="livepatch" kselftest

Is it in "/tmp" ?

> +else ifneq ($(wildcard ../../../../Module.symvers),)
> +KDIR := ../../../..

This seems to support building by calling:

   $ cd tools/tests/selftest/livepatch
   $ make

> +else
> +KDIR := /lib/modules/$(shell uname -r)/build

This supports the build with the devel files installed for the
currently running kernel.

> +endif
> +endif
> +override KDIR := $(abspath $(KDIR))
> +export KDIR
> +MAKEOVERRIDES := $(filter-out KDIR=%,$(MAKEOVERRIDES))
> +
> +LIVEPATCH_CONFIG := $(shell if [ -r "$(KDIR)/.config" ]; then \
> +	grep -qx 'CONFIG_LIVEPATCH=y' "$(KDIR)/.config" && echo y || echo n; fi)

Why LIVEPATCH_CONFIG and not CONFIG_LIVEPATCH?
IMHO, the reverse ordering adds more harm than good.

>  TEST_GEN_FILES := test_klp-call_getpid
> +ifneq ($(LIVEPATCH_CONFIG),n)
>  TEST_GEN_MODS_DIR := test_modules
> +endif
>  TEST_PROGS_EXTENDED := functions.sh
>  TEST_PROGS := \
>  	test-livepatch.sh \
> @@ -16,3 +35,12 @@ TEST_PROGS := \
>  TEST_FILES := settings
>  
>  include ../lib.mk
> +
> +ifeq ($(LIVEPATCH_CONFIG),n)
> +clean: clean_livepatch_mods
> +
> +clean_livepatch_mods:
> +	$(Q)$(MAKE) -C test_modules clean
> +
> +.PHONY: clean_livepatch_mods

My understanding is that this will remove the test modules even
LIVEPATCH_CONFIG became disabled in the meantime. I guess
that it is related to the 3rd problem (puzzle) in the commit
message.

It makes sense. I am surprised that we need an extra target
for this.

Anyway, it would make sense to fix this in a separate patch.

> +endif
> diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh
> index 429fce1f31431..8b26b70f92d8c 100644
> --- a/tools/testing/selftests/livepatch/functions.sh
> +++ b/tools/testing/selftests/livepatch/functions.sh
> @@ -150,6 +150,7 @@ function cleanup() {
>  #		 the ftrace_enabled sysctl.
>  function setup_config() {
>  	is_root
> +	[[ -d "$SYSFS_KLP_DIR" ]] || skip "livepatch is not available"

This seems to be used only when _running_ the test. But this patch
fixes _building_. It should be in a separate patch, maybe
together with the check for CONFIG_LIVEPATCH.

>  	has_kdir
>  	push_config
>  	set_dynamic_debug
> diff --git a/tools/testing/selftests/livepatch/test_modules/Makefile b/tools/testing/selftests/livepatch/test_modules/Makefile
> index a13d398585dcb..62cb724c6b333 100644
> --- a/tools/testing/selftests/livepatch/test_modules/Makefile
> +++ b/tools/testing/selftests/livepatch/test_modules/Makefile
> @@ -16,14 +16,15 @@ obj-m += test_klp_atomic_replace.o \
>  	test_klp_state3.o \
>  	test_klp_syscall.o
>  
> +modules clean: MAKEOVERRIDES := $(filter-out O=% KBUILD_OUTPUT=%,$(MAKEOVERRIDES))
> +
>  # Ensure that KDIR exists, otherwise skip the compilation
>  modules:
>  ifneq ("$(wildcard $(KDIR))", "")
> -	$(Q)$(MAKE) -C $(KDIR) modules KBUILD_EXTMOD=$(TESTMODS_DIR)
> +	$(Q)env -u KBUILD_OUTPUT $(MAKE) -C $(KDIR) modules KBUILD_EXTMOD=$(TESTMODS_DIR)

Why is KBUILD_OUTPUT unset here?
It would deserve a comment.

>  endif
>  
> -# Ensure that KDIR exists, otherwise skip the clean target
> +# Any kernel tree can clean the test modules, so use the one containing them

Here is a comment. But what does "Any kernel tree" means?
Could we be more specific, please?

>  clean:
> -ifneq ("$(wildcard $(KDIR))", "")
> -	$(Q)$(MAKE) -C $(KDIR) clean KBUILD_EXTMOD=$(TESTMODS_DIR)
> -endif
> +	$(Q)env -u KBUILD_OUTPUT $(MAKE) -C $(TESTMODS_DIR)/../../../../.. \
> +		clean KBUILD_EXTMOD=$(TESTMODS_DIR)

Note that my workflow still works with this patch. I am testing the
running kernel, have installed the devel files, and doing:

  cd $KDIR/tools/tests/selftests/livepatch
  make run_tests

But it would be great to better document which other scenarios
are newly supported by this patch.

Best Regards,
Petr


Best Regards,
Petr
>  LocalWords:  cd