Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] If `btf_compact_strings()` fails, `btf__permute()` returns an error but leaves the BTF object in an irreversibly truncated and permuted state without updating the caller's `id_map`. - [Low] LIBBPF-001 API rule violation: `btf_add_local_str` incorrectly uses `libbpf_err()` as an internal function, and `btf__add_str` fails to use it on its error return. Pre-existing issues: - [High] `btf__permute` completely corrupts base BTF permutations due to incorrect handling of `start_offs`, resulting in heap out-of-bounds accesses and total deletion of the VOID type. -- commit 9de40ca1824ca5dbac1f65a763fe3a6beedba81f Author: Alan Maguire <[email protected]> libbpf: Support moving permuted BTF types into split BTF This commit extends btf__permute() with an optional transfer mode to move specific permuted BTF types into a newly-created split BTF. It handles remapping type IDs, migrating referenced strings, and compacting string tables so that strings exclusive to the moved types reside only in the split BTF. > diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c > index 9449f6f50f18f..30f8c426d1453 100644 > --- a/tools/lib/bpf/btf.c > +++ b/tools/lib/bpf/btf.c > @@ -2147,16 +2147,9 @@ int btf__find_str(struct btf *btf, const char *s) > * - > 0 offset into string section, on success; > * - < 0, on error. > */ > -int btf__add_str(struct btf *btf, const char *s) > +static int btf_add_local_str(struct btf *btf, const char *s) > { > - int off; > - int err; > - > - if (btf->base_btf) { > - off = btf__find_str(btf->base_btf, s); > - if (off != -ENOENT) > - return off; > - } > + int off, err; > > err = btf_ensure_modifiable(btf); > if (err) > @@ -2167,10 +2160,24 @@ int btf__add_str(struct btf *btf, const char *s) > return libbpf_err(off); ^^^^^^^^^^^^^^^^^^^^^^ [Severity: Low] Does the internal static function btf_add_local_str() incorrectly use the libbpf_err() wrapper here, while the public API btf__add_str() below fails to wrap its own error return? > > btf->hdr.str_len = strset__data_size(btf->strs_set); > + btf->strs_deduped = false; > > return btf->start_str_off + off; > } > > +int btf__add_str(struct btf *btf, const char *s) > +{ > + int off; > + > + if (btf->base_btf) { > + off = btf__find_str(btf->base_btf, s); > + if (off != -ENOENT) > + return off; > + } > + > + return btf_add_local_str(btf, s); > +} [ ... ] > @@ -6678,33 +6759,82 @@ int btf__permute(struct btf *btf, __u32 *id_map, > __u32 id_map_cnt, > if (err) > goto done; > > + /* > + * Build the inverse map: for each requested destination ID, record the > + * original type ID that will occupy it. order_map is indexed by the > + * destination ID relative to the first ID represented by id_map. The > + * caller id_map maps old IDs to requested destinations, but BTF types > + * must be emitted in destination-ID order; the inverse map supplies the > + * old type to copy for each output slot without repeatedly searching > + * id_map. > + */ > for (i = start_offs; i < id_map_cnt; i++) { > - id = id_map[i]; > - if (id < btf->start_id || id >= btf__type_cnt(btf)) { > + __u32 requested_id = new_id_map[i]; > + __u32 order_idx; > + > + if (btf_permute_id_is_transfer(requested_id)) { > + if (!transfer_btfp) { > + err = -EINVAL; > + goto done; > + } > + requested_id &= ~BTF_PERMUTE_ID_TRANSFER; > + nr_transfer++; > + } > + if (requested_id < btf->start_id || requested_id >= > btf__type_cnt(btf)) { > err = -EINVAL; > goto done; > } > - id -= btf->start_id - start_offs; > + order_idx = requested_id - btf->start_id + start_offs; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] This is a pre-existing issue, but does the order_idx calculation go out of bounds for base BTFs? For a base BTF, btf->start_id is 0 and start_offs is 1. If requested_id reaches id_map_cnt - 1 (the maximum valid ID), order_idx will become id_map_cnt. This would cause an out-of-bounds access when writing to order_map[order_idx] below, since order_map is allocated with size id_map_cnt. > /* cannot be mapped to the same ID */ > - if (order_map[id]) { > + if (order_map[order_idx]) { > err = -EINVAL; > goto done; > } > - order_map[id] = i + btf->start_id - start_offs; > + order_map[order_idx] = i + btf->start_id - start_offs; > + } [ ... ] > @@ -6729,18 +6916,58 @@ int btf__permute(struct btf *btf, __u32 *id_map, > __u32 id_map_cnt, > goto done; > } > > - for (nt = new_types, i = 0; i < id_map_cnt - start_offs; i++) { > + /* Install the compacted parent types and rebuild their index. */ > + for (nt = new_types, i = 0; i < nr_base_types; i++) { > btf->type_offs[i] = nt - new_types; > nt += btf_type_size(btf, nt); > } > - > - free(order_map); > free(btf->types_data); > btf->types_data = new_types; > + committed = true; > + btf_hdr_update_type_len(btf, nt - new_types); > + btf->nr_types = nr_base_types; > + > + /* > + * An important goal is to also transfer strings to the transfer > + * BTF where they do not have duplicates in BTF. This is important > + * for cases like inline BTF where inline function names could > + * comprise a significant portion of the string table. The approach > + * is to first compact the set of strings in the base now transfer > + * is complete; this will remove unreferenced strings. Then > + * local references in the transfer BTF have to be moved downwards > + * based upon that compaction. Finally also compact transfer BTF > + * references so that we replace duplicate references in split with > + * base references where present. > + * > + * Once all this is done, we end up with transfer-only strings in > + * transfer split BTF and shared strings in BTF. The result is > + * a transfer BTF with a lot of transfer-only strings will not > + * pollute the string table of the non-transfer BTF. > + */ > + if (transfer_btf) { > + err = btf_compact_strings(btf, btf_ext); > + if (err) > + goto done; ^^^^^^^^^ [Severity: High] If btf_compact_strings() fails here, we return an error but the btf->types_data replacement has already been committed. Does this leave the BTF object in an irreversibly corrupted state from the caller's perspective, since id_map is not updated on the error path and the transferred types are lost? > + err = btf_permute_rebase_split_strs(transfer_btf, old_str_off); > + if (err) > + goto done; > + err = btf_compact_strings(transfer_btf, NULL); > + if (err) > + goto done; > + *transfer_btfp = transfer_btf; > + } > + > + /* On success return the final old-to-new type-ID map to the caller. */ > + memcpy(id_map, new_id_map, id_map_cnt * sizeof(*id_map)); > + free(new_id_map); > + free(order_map); > return 0; > > done: > + free(new_id_map); > free(order_map); > - free(new_types); > + if (!committed) > + free(new_types); > + btf__free(transfer_btf); > return libbpf_err(err); > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
