Re: [PATCH v2 05/58] objtool/klp: Build klp test fixtures through the harness

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The global `orig_obj` and `patched_obj` variables are not reset by `build_pair()`, causing state leakage between sequential test segments.
- [Medium] `make_vmlinux_pair()` masks malformed test invocations as toolchain deficiencies, resulting in silent test skips.
- [Medium] `run_objtool_check` fails to invalidate the `.checksummed` cache flag after modifying object files in-place.
--

commit 19435afd8ba9b070d7a97986a8ecde2ed711708b
Author: Song Liu <song@kernel.org>
Subject: objtool/klp: Build klp test fixtures through the harness

This commit refactors the objtool livepatch testing harness to standardize
fixture compilation, abstracting build and linking routines. It extracts object
creation and checksum generation into helper functions. It also adds logic to
handle vmlinux pairs and properly sequence objtool checks.

> diff --git a/tools/objtool/tests/lib.sh b/tools/objtool/tests/lib.sh
> index 7185198253bb7..0a9da178f4dc8 100644
> --- a/tools/objtool/tests/lib.sh
> +++ b/tools/objtool/tests/lib.sh

[ ... ]

> @@ -211,17 +224,64 @@ clang_only()
>  	declared_skip "clang only${1:+: $1}"
>  }
>  
> +# build_one <fixture.c> <output object> [cflags...]
> +build_one()
> +{

[ ... ]

> +}
> +
>  # build_pair <fixture.c> [cflags...]
>  build_pair()
>  {
> -	local fixture="$FIXTURES_DIR/$1"; shift
> +	local fixture="$1"; shift
>  
> -	[ -f "$fixture" ] || fail "missing fixture $fixture"
> +	build_one "$fixture" orig.o "$@"
> +	build_one "$fixture" patched.o "$@" -DPATCHED
>  }

[Severity: Medium]
Does this code leave orig_obj pointing to stale data if run after
make_vmlinux_pair()?

If a test sequences its segments so that make_vmlinux_pair() is called first
(which sets the global orig_obj=vmlinux.o), and then build_pair() is called,
build_pair() does not reset orig_obj to orig.o. 

Would this cause a subsequent run_diff() to erroneously diff the stale
vmlinux.o instead of the newly built orig.o?

> +# run_objtool_check <objtool arguments...>
> +#
> +# Run objtool's ordinary check pass over the pair, as the kernel build does.
> +#

[ ... ]

> +run_objtool_check()
> +{
> +	local obj
> +
> +	for obj in "$orig_obj" "$patched_obj"; do
> +		"$OBJTOOL" "$@" "$workdir/$obj" ||
> +			fail "objtool $* failed on $obj"
> +	done
> +}

[Severity: Medium]
Can this function leave a stale checksum cache in place?

Other functions that create or modify objects, like build_one() and
partial_link(), remove the $workdir/.checksummed marker. 

Since run_objtool_check() executes objtool directly on the test objects, which
can append new sections like ORC metadata, should it also remove the
.checksummed file to prevent subsequent run_checksum() calls from skipping
generation and using stale checksum data?

[ ... ]

> +make_vmlinux_pair()
> +{
> +	local orig=() patched=() seen= arg
> +
> +	for arg in "$@"; do
> +		if [ "$arg" = -- ]; then seen=y; continue; fi
> +		if [ -n "$seen" ]; then patched+=( "$arg" ); else orig+=( "$arg" ); fi
> +	done
> +
> +	partial_link "$workdir/vmlinux.o" "${orig[@]}" ||
> +		probe_skip "partial link unavailable"

[Severity: Medium]
Will this silently mask test definition bugs as toolchain deficiencies?

If a test author accidentally malforms the arguments by omitting the --
delimiter, the $orig or $patched arrays will be empty. Calling partial_link
with empty input arrays would correctly fail, but capturing that failure here
and executing probe_skip masks it. 

Doesn't this contradict the commit's design principle that a fixture which will
not compile should be a failure rather than a skip?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914062514.926997-1-song@kernel.org?part=5