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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] bpf: resolve_btfids: choose_functions() skips records on
  duplicate names
--

commit 592223df16723596e1694e3167578ff1cfdd0320
Author: Anton Protopopov <a.s.protopopov@gmail.com>

resolve_btfids: Tag fmod_ret candidates as targets

This commit reads candidate records from .BTF_fmodret_candidates in
resolve_btfids, filters eligible traceable functions, and emits
bpf_fmodret and bpf_fmodret_retval: BTF declaration tags for them.

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

[ ... ]

> +/* Choose the targets among the functions and describe them in RESULT */
> +static int choose_functions(struct elf_function_list *functions, const struct btf *btf,
> +			    const struct btf_func *btf_funcs, size_t nr_btf_funcs,
> +			    const unsigned char *buf, size_t ptr_size,
> +			    unsigned int encoding, struct fmodret_retvals *result,
> +			    struct fmodret_stats *stats, int verbose)
> +{
> +	size_t i = 0, j;
> +
> +	qsort(functions->functions, functions->nr, sizeof(*functions->functions),
> +	      cmp_elf_function_name);
> +	for (i = 0; i < functions->nr; i = j) {
> +		const struct elf_function *f = &functions->functions[i];
> +		size_t record = chosen_record(f);
> +		const char *reason = NULL;
> +		unsigned int flags = 0;
> +		struct fmodret_retval *r;
> +		__u32 size;
> +		int id = 0;
> +
> +		for (j = i + 1; j < functions->nr; j++)
> +			if (strcmp(f->name, functions->functions[j].name))
> +				break;
> +
> +		if (!f->nr_strong && !f->nr_weak)
> +			continue;
> +		if (record == NO_FUNCTION) {
> +			stats->ambiguous_body++;
> +			reason = "ambiguous-body";
> +			goto rejected;
> +		}
> +		/*
> +		 * A strong definition without values has no record. If it
> +		 * replaced a weak one, only the record of the weak one points
> +		 * to its body.
> +		 */
> +		if (!f->nr_strong && !f->weak_symbol) {
> +			stats->overridden++;
> +			reason = "overridden-weak";
> +			goto rejected;
> +		}
> +		if (!read_elf_value(buf + record + 16 + ptr_size, 4, encoding))
> +			continue;
> +		stats->with_values++;
> +		flags = read_elf_value(buf + record + 12, 4, encoding);
> +		if (j - i != 1) {
> +			stats->ambiguous_name++;
> +			reason = "ambiguous-name";
> +			goto rejected;
> +		}

[Severity: Low]
Does choose_functions() skip candidate records on functions[i + 1 .. j - 1]
when multiple ELF functions share the same name?

add_record() attaches each record only to the functions at that specific
location, and choose_functions() then groups all functions with the same
name into [i, j) and advances with i = j.  If functions[i] (the lowest
address instance, such as a static helper in another translation unit) has
no record or zero values, the continue statements skip the rest of the
group before reaching the j - i != 1 check, leaving stats->with_values and
stats->ambiguous_name undercounted and omitting the verbose rejection log
for records on functions[i + 1 .. j - 1].

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106205523.1765622-1-a.s.protopopov@gmail.com?part=15