Re: [PATCH bpf-next v4 15/20] libbpf: Collect .bpf_cleanup records and pass them to the kernel

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] bpf: invalid multi-line comment style in libbpf.c
--

commit 640e4ca07507c0fa24643be768ba697d779b3ae5
Author: Yonghong Song <yonghong.song@linux.dev>
Subject: libbpf: Collect .bpf_cleanup records and pass them to the kernel

This commit parses the compiler-emitted .bpf_cleanup section and extracts
the table of cleanup records. It resolves section relocations for each record,
validates alignment, and passes the per-program cleanup information to the
kernel during program load.

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 13ae9175e9686..e3e4db59fb5f4 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
[ ... ]
> @@ -4854,6 +4892,235 @@ static struct bpf_program *find_prog_by_sec_insn(const struct bpf_object *obj,
[ ... ]
> +		slot_sec[slot] = sym->st_shndx;
> +		/* The addend lives in the section data, which libelf leaves in
> +		 * the object's byte order; a non-section symbol additionally
> +		 * contributes its own value.
> +		 */
> +		slot_val[slot] = (native ? vals[slot] : bswap_32(vals[slot])) +
> +				 sym->st_value;
> +	}

[Severity: Low]
This isn't a bug, but does this multi-line comment follow the preferred BPF 
subsystem style? I believe the opening /* is typically expected to be on its 
own line.

[ ... ]
> +		for (j = 0; j < CLEANUP_REC_FIELDS; j++) {
> +			size_t idx = raw->insn_idx[j], final;
> +			struct bpf_program *p;
> +
> +			/* The end of a range is exclusive, so it may name the
> +			 * instruction just past the last one of a function,
> +			 * which belongs to the next function or to nothing at
> +			 * all. Ask about the last instruction the range covers,
> +			 * the way the kernel does.
> +			 */
> +			if (j == CLEANUP_REC_END) {

[Severity: Low]
This isn't a bug, but should this multi-line comment be formatted with the 
opening /* on a separate line to match the rest of the BPF subsystem?

[ ... ]
> +			if (owner == prog) {
> +				final = raw->insn_idx[j] - prog->sec_insn_off;
> +			} else if (prog_is_subprog(obj, owner) && owner->sub_insn_off) {
> +				/* sub_insn_off is where this subprogram was
> +				 * appended to the main program being relocated;
> +				 * zero means it is not part of it.
> +				 */
> +				final = owner->sub_insn_off +
> +					raw->insn_idx[j] - owner->sec_insn_off;

[Severity: Low]
This isn't a bug, but could we adjust this comment to have the opening /* on 
its own line for consistency as well?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921210033.1715000-1-yonghong.song@linux.dev?part=15