Re: [PATCH v5 36/58] objtool/klp: Add test for position-independent checksums

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] objtool/klp: test script comments mismatch C fixture implementation
--

commit 7722f94b411608f4277261adb44a750295859754
Author: Song Liu <song@kernel.org>
Date:   Wed Sep 16 18:43:29 2026 +0000

    objtool/klp: Add test for position-independent checksums
    
    A function that only moves has not changed, and its checksum must not move
    with it. Otherwise every patch reports as changed everything that shifted
    because something ahead of it grew.

> diff --git a/tools/objtool/tests/generic/test-checksum-position.sh b/tools/objtool/tests/generic/test-checksum-position.sh
> new file mode 100755
> index 0000000000000..d05e28014e1e6
> --- /dev/null
> +++ b/tools/objtool/tests/generic/test-checksum-position.sh
> @@ -0,0 +1,50 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0

[ ... ]

> +# A jump or call without a relocation encodes its target as an offset from the
> +# instruction.  Hashing those bytes makes the checksum change whenever anything
> +# ahead of the function changes size -- so an unrelated edit elsewhere in the
> +# file reports this function as changed too, and the patch grows to include it
> +# and everything it references.  Nothing fails; the livepatch is just larger and
> +# riskier than the patch it came from.
> +#
> +# Here the "patch" adds a function ahead of target() and changes nothing else.

[Severity: Low]
Is this comment accurate given that the C fixture uses __attribute__((aligned(64)))
instead of adding a new function?

It appears the C code in checksum_position.c explicitly states that PATCHED
changes the distance by aligning the functions rather than by inserting a
function between them.

> +
> +. "$(dirname "$0")/../lib.sh"
> +
> +setup
> +
> +# -fno-function-sections, or each function is at offset 0 of its own section
> +# and target() never moves.
> +build_pair checksum_position.c -fno-function-sections
> +
> +assert_input_symbol target
> +
> +# The fixture is only meaningful if target() actually moved.
> +# What has to change is the displacement target's calls encode, and that is
> +# the distance to callee() -- not target's own offset.  A compiler which put
> +# the added function ahead of both would move target and leave the distance

[Severity: Low]
Does this explanation still apply?

Like the comment above, this seems to continue explaining the mechanism based
on an obsolete added function approach rather than the alignment approach
documented in the C fixture.

> +# alone, and then the bytes are identical and the checksum matches for the
> +# uninteresting reason.  Ask about the distance.
> +sym_off()   # $1 object, $2 symbol
> +{
> +	in_symbols "$1" | awk -v n="$2" '$NF == n { print $2; exit }'
> +}
> +
> +orig_t="$(sym_off orig.o target)";    orig_c="$(sym_off orig.o callee)"
> +new_t="$(sym_off patched.o target)";  new_c="$(sym_off patched.o callee)"
> +
> +[ -n "$orig_t" ] && [ -n "$orig_c" ] && [ -n "$new_t" ] && [ -n "$new_c" ] ||
> +	fail "target or callee missing from one of the objects"
> +
> +orig_gap=$(( 16#$orig_t - 16#$orig_c ))
> +new_gap=$((  16#$new_t  - 16#$new_c ))
> +[ "$orig_gap" != "$new_gap" ] ||
> +	probe_skip "this compiler moved target() and callee() together;" \

[Severity: Low]
Should this skip message be updated as well?

It implies the functions were moved together by compiler reordering, as in the
added function approach, rather than reacting to the alignment directive used
in the C fixture.

> +		   "the call displacement did not change"
> +
> +assert_checksum_matches target
> +
> +pass "checksum unchanged when the function only moves"

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