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

[Yonghong Song <yonghong.song@linux.dev>]

On 9/27/26 1:39 PM, bot+bpf-ci@kernel.org wrote:
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index 7eb15a013a83..4e9584fff264 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
> [ ... ]
>
>> @@ -4915,6 +4953,249 @@ static struct bpf_program *find_prog_by_sec_insn(const struct bpf_object *obj,
>>   	return NULL;
>>   }
>>
>> +static int bpf_object__init_cleanup_info(struct bpf_object *obj)
>> +{
>> +	Elf_Data *data = obj->efile.cleanup_data;
>> +	Elf_Data *relo = NULL;
>> +	size_t i, nrels, nslots, nrecs;
>> +	struct cleanup_raw_rec *recs;
>> +	int *slot_sec, ret = 0;
>> +	size_t *slot_val;
>> +	const __u32 *vals;
>> +	Elf64_Shdr *sh;
>> +	bool native;
>> +
>> +	if (!data || obj->efile.cleanup_shndx < 0 || !data->d_size)
>> +		return 0;
>> +
>> +	native = is_native_endianness(obj);
>> +
>> +	for (i = 0; i < obj->efile.sec_cnt; i++) {
>> +		struct elf_sec_desc *sd = &obj->efile.secs[i];
>> +
>> +		if (sd->sec_type == SEC_RELO && sd->shdr &&
>> +		    sd->shdr->sh_info == (Elf64_Word)obj->efile.cleanup_shndx) {
>> +			relo = sd->data;
>> +			break;
>> +		}
>> +	}
>> +	if (!relo) {
>> +		pr_warn("%s present without relocations\n", CLEANUP_SEC);
>> +		return -LIBBPF_ERRNO__FORMAT;
>> +	}
>> +	/*
>> +	 * LLVM leaves sh_entsize unset, so this only bites a producer that
>> +	 * declares a record size -- which is the one able to tell us it means
>> +	 * something other than three 4-byte fields.
>> +	 */
>> +	sh = elf_sec_hdr(obj, elf_sec_by_idx(obj, obj->efile.cleanup_shndx));
>> +	if (sh && sh->sh_entsize &&
>> +	    sh->sh_entsize != sizeof(struct bpf_cleanup_info)) {
>> +		pr_warn("%s record size %zu is not the expected %zu\n", CLEANUP_SEC,
>> +			(size_t)sh->sh_entsize, sizeof(struct bpf_cleanup_info));
>> +		return -LIBBPF_ERRNO__FORMAT;
>> +	}
> Should sh_entsize == 0 be explicitly treated as 12, so that wider
> records (if necessary in the future) end up being easier to reject?
>
> This design question was raised by Kumar Kartikeya Dwivedi in v5:
> https://lore.kernel.org/bpf/DLMGMYOHSGFW.24JML4GSMTV2K@gmail.com/
>
> The current code accepts sh_entsize == 0 (LLVM's default) silently, but
> treating it explicitly as 12 would allow future tools to cleanly reject
> wider records by setting sh_entsize to a different value.

Okay. This is not a bug, but rather to have better code to have
explicit 'entsize' size and then check against size of bpf_cleanup_info.

>
>> +	if (data->d_size % sizeof(struct bpf_cleanup_info)) {
>> +		pr_warn("%s size %zu is not a multiple of the record size %zu\n",
>> +			CLEANUP_SEC, data->d_size, sizeof(struct bpf_cleanup_info));
>> +		return -LIBBPF_ERRNO__FORMAT;
>> +	}
>> +
>> +	vals = data->d_buf;
>> +	nslots = data->d_size / sizeof(__u32);
>> +	nrecs = data->d_size / sizeof(struct bpf_cleanup_info);
>> +
>> +	slot_sec = calloc(nslots, sizeof(*slot_sec));
>> +	slot_val = calloc(nslots, sizeof(*slot_val));
>> +	recs = calloc(nrecs ?: 1, sizeof(*recs));
> This isn't a bug, but could the relocation loop fill recs directly
> (with sec_idx initialised to -1) so the slot_sec/slot_val scratch
> arrays and the copy loop go away?

Good point. This will make things easier.

>
> The relocation loop could write straight into recs, using sec_idx = -1
> as the 'no relocation' sentinel and checking alignment while the byte
> offset is at hand, the way bpf_object__collect_rodata_relos() checks
> (sym->st_value + addend) % BPF_INSN_SZ inline. That would leave one
> allocation instead of three and drop the copy loop.
>
> Also, is the 'nrecs ?: 1' needed? d_size has already been checked to be
> a non-zero multiple of the record size.

Will do.

>
>> +	if (!slot_sec || !slot_val || !recs) {
>> +		ret = -ENOMEM;
>> +		goto out;
>> +	}
>> +	for (i = 0; i < nslots; i++)
>> +		slot_sec[i] = -1;
>> +
>> +	/* One relocation per 4-byte field, naming the section it points into. */
>> +	nrels = relo->d_size / sizeof(Elf64_Rel);
>> +	for (i = 0; i < nrels; i++) {
> [ ... ]
>
>> +	for (i = 0; i < nslots; i++) {
>> +		struct cleanup_raw_rec *rec = &recs[i / CLEANUP_REC_FIELDS];
>> +		size_t field = i % CLEANUP_REC_FIELDS;
>> +
>> +		if (slot_sec[i] < 0) {
>> +			pr_warn("%s: field %zu has no relocation\n", CLEANUP_SEC, i);
>> +			ret = -LIBBPF_ERRNO__FORMAT;
>> +			goto out;
>> +		}
>> +		if (slot_val[i] % BPF_INSN_SZ) {
>> +			pr_warn("%s: field %zu offset %zu is not instruction aligned\n",
>> +				CLEANUP_SEC, i, slot_val[i]);
>> +			ret = -LIBBPF_ERRNO__FORMAT;
>> +			goto out;
>> +		}
>> +		rec->sec_idx[field] = slot_sec[i];
>> +		rec->insn_idx[field] = slot_val[i] / BPF_INSN_SZ;
>> +	}
>> +
>> +	obj->cleanup_recs = recs;
>> +	obj->cleanup_rec_cnt = nrecs;
>> +	recs = NULL;
>> +out:
>> +	free(recs);
>> +	free(slot_val);
>> +	free(slot_sec);
>> +	return ret;
>> +}
> [ ... ]
>
>
> ---
> 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/36346422430