Re: [PATCH v3 bpf-next 15/18] resolve_btfids: Tag fmod_ret candidates as targets

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 2995 bytes --]

> resolve_btfids: Tag fmod_ret candidates as targets

TL;DR: Nits only in resolve_btfids fmodret.c: the second branch in
relocation_offset() looks unreachable or wrong, and record field offsets are
hardcoded in three functions. No functional bugs reported.

> diff --git a/tools/bpf/resolve_btfids/fmodret.c b/tools/bpf/resolve_btfids/fmodret.c
> new file mode 100644
> --- /dev/null
> +++ b/tools/bpf/resolve_btfids/fmodret.c

[ ... ]

> +static bool relocation_offset(const GElf_Shdr *target, GElf_Addr offset,
> +			      size_t width, size_t *result)
> +{
> +	if (target->sh_size < width)
> +		return false;
> +	if (offset >= target->sh_addr &&
> +	    offset - target->sh_addr <= target->sh_size - width) {
> +		*result = offset - target->sh_addr;
> +		return true;
> +	}
> +	/* ET_REL relocation offsets are relative to their target section. */
> +	if (offset <= target->sh_size - width) {
> +		*result = offset;
> +		return true;
> +	}
> +	return false;
> +}

this isn't a bug, but when is the second branch in relocation_offset() taken?

For the module sections that use this helper, sh_addr is 0, so the first
test already returns the section-relative offset, and with sh_addr == 0 the
second condition cannot be true once the first has failed.

The second branch is reached only when sh_addr != 0 and offset < sh_addr,
which would be a linked image whose r_offset lies below the target section.
Treating that address as a section offset would be wrong there.

Would it be clearer to pick the interpretation from e_type, or to drop the
fallback?

[ ... ]

> +static char *format_values(const unsigned char *buf, size_t offset, size_t ptr_size,
> +			   unsigned int encoding)
> +{
> +	unsigned int nr = read_elf_value(buf + offset + 16 + ptr_size, 4, encoding);
> +	const unsigned char *p = buf + offset + FMODRET_META_HEADER_SIZE(ptr_size);

[ ... ]

> +static int choose_functions(struct elf_function_list *functions, const struct btf *btf,

[ ... ]

> +		if (!read_elf_value(buf + record + 16 + ptr_size, 4, encoding))
> +			continue;
> +		stats->with_values++;
> +		flags = read_elf_value(buf + record + 12, 4, encoding);

this isn't a bug, but the record field offsets (flags at 12, nr_values at
16 + ptr_size) are written out by hand in parse_records(), format_values()
and choose_functions().  parse_records() has already decoded flags and
nr_values, and the other two decode them again from the raw buffer at the
same offsets.

fmodret_metadata.h has macros for the magic, the version and the header
size, but none for the field offsets.

Would it be simpler to put the offsets in fmodret_metadata.h, or to have a
small helper return flags and nr_values for a record, so that the three
functions do not each repeat 12 and 16 + ptr_size?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/42981885558