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

[Anton Protopopov <a.s.protopopov@gmail.com>]

On 26/11/09 06:13PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue to consider:
> - [Low] resolve_btfids: skipped duplicate-named functions in
>   choose_functions()
> --
> 
> commit 51f98882bd7c33a728dede77756350958f73cb62
> Author: Anton Protopopov <a.s.protopopov@gmail.com>
> 
> resolve_btfids: Tag fmod_ret candidates as targets
> 
> This commit teaches resolve_btfids to parse .BTF_fmodret_candidates
> records emitted by the bpf_scan_ir plugin and tag eligible functions in
> BTF with bpf_fmodret and bpf_fmodret_retval:<values>.
> 
> > diff --git a/tools/bpf/resolve_btfids/fmodret.c b/tools/bpf/resolve_btfids/fmodret.c
> > new file mode 100644
> > index 0000000000000..df44b93eea734
> > --- /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 advancing i = j while only inspecting functions->functions[i] skip
> candidate records on functions->functions[i + 1 .. j - 1] when multiple
> static functions share a name?
> 
> collect_elf_functions() gathers all defined STT_FUNC symbols regardless
> of whether they have an fmodret record.  If functions->functions[i] has no
> record (or zero values) while a later function with the same name in
> [i + 1, j) has a record with values, the continue statement jumps to j
> without counting the remaining functions in stats (such as
> stats->with_values and stats->ambiguous_name) or logging the
> ambiguous-name rejection when verbose > 1.

Yes, you're pointing to this the second time,
so it will be fixed in v3.

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