On Mon, 18 May 2026 09:49:02 +0100 Marat Khalili <[email protected]> wrote:
> Introduce new BPF load parameters struct rte_bpf_prm_ex that can be > extended without breaking backward or forward compatibility. Introduce > new function rte_bpf_load_ex consolidating in one code path loading from > both ELF file and raw memory image, with possibility to add more options > in the future. > > Some changes in code layout and sequence: > * Both old APIs now only forwarding calls to a new single entry point. > * There is now a centralized cleanup point for all temporary resources > created during the load process. > * External symbols (xsyms) are now checked for validity just after the > load started, not after they were already used for relocation. > * File bpf_load_elf.c now only handles opening ELF file and providing > patched instruction array to the load process. These are left as two > separate functions to support other ELF sources like memory image in > the future. > * Function stubs for the case libelf is not available are moved to > bpf_load_elf.c to make keeping track of them easier (forgetting to > update stubs is a common problem). > > Signed-off-by: Marat Khalili <[email protected]> > Acked-by: Konstantin Ananyev <[email protected]> > ---Review of bundle-1895: bpf load (PATCH v3 01..10/10) Lots of AI review on this (with Claude Opus 4.7) ==================================================== Patches 01, 06, 09, and 10 had no findings and are omitted from this review. PATCH v3 02/10 -- bpf: introduce extensible load API ---------------------------------------------------- Errors: * lib/bpf/bpf_load.c, rte_bpf_load() and rte_bpf_elf_load() wrappers: These now unconditionally dereference `prm` when constructing the compound literal passed to rte_bpf_load_ex(): return rte_bpf_load_ex(&(struct rte_bpf_prm_ex){ ... .raw.ins = prm->ins, /* NULL deref if prm == NULL */ .raw.nb_ins = prm->nb_ins, .xsym = prm->xsym, .nb_xsym = prm->nb_xsym, .prog_arg = prm->prog_arg, }); Pre-patch both functions explicitly checked `prm == NULL` and returned NULL with rte_errno = EINVAL. The NULL check inside load_try() does not help: NULL is dereferenced before rte_bpf_load_ex() is even called. Add explicit `if (prm == NULL) { rte_errno = EINVAL; return NULL; }` guards at the top of both wrappers. Info: * lib/bpf/bpf_load_elf.c: the success log "successfully creates %p(jit={.func=%p,.sz=%zu})" and the error log in rte_bpf_elf_load() are removed silently. If intentional, mention in the commit message. PATCH v3 03/10 -- bpf: support up to 5 arguments ------------------------------------------------ Warnings: * lib/bpf/bpf_exec.c, rte_bpf_exec_burst(), exec_jit_burst_ex(), and rte_bpf_exec_burst_ex(): these return `uint32_t` but the patch introduces `return -EINVAL` on error: if (bpf->prm.nb_prog_arg != 1) /* Use rte_bpf_exec_burst_ex with this program. */ return -EINVAL; -22 reinterpreted as uint32_t is 0xFFFFFFEA, which the caller cannot distinguish from a valid "number of successfully processed inputs" result. The doxygen for rte_bpf_exec_burst_ex does not mention error returns. Either change the return type, document the encoding (e.g. any value > num indicates an error), or return 0 on these paths. PATCH v3 04/10 -- bpf: add cBPF origin to rte_bpf_load_ex --------------------------------------------------------- Info: * lib/bpf/bpf_convert.c: the no-libpcap stub for rte_bpf_convert() loses its `if (prog == NULL) return EINVAL` check when moved out of bpf_stub.c. Previously rte_bpf_convert(NULL) returned EINVAL; now it returns ENOTSUP. Both produce NULL so pointer-checking callers are unaffected, but rte_errno semantics changed. Worth a mention or a restore for symmetry with the libpcap-enabled path. PATCH v3 05/10 -- bpf: support rte_bpf_prm_ex with port callbacks ----------------------------------------------------------------- Errors: * lib/bpf/rte_bpf_ethdev.h: the doxygen comments for the two new functions are swapped: /** * Install callback to execute specified BPF program on given * TX port/queue. * ... * @param queue * The identifier of the TX queue on the given port */ __rte_experimental int rte_bpf_eth_rx_install(...); /** * Install callback to execute specified BPF program on given * RX port/queue. * ... * @param queue * The identifier of the RX queue on the given port */ __rte_experimental int rte_bpf_eth_tx_install(...); Swap the two doc blocks. PATCH v3 07/10 -- test/bpf: test loading cBPF directly ------------------------------------------------------ Info: * app/test/test_bpf.c, test_bpf_filter(): the failure path previously invoked test_bpf_dump(&fcode, prm) when load failed, useful for diagnosing converter regressions. The refactor drops this. Consider preserving the dump in the failure branch (or moving it into load_cbpf_program_convert on its NULL return). PATCH v3 08/10 -- test/bpf: test loading ELF file from memory ------------------------------------------------------------- Errors: * app/test/test_bpf.c, bpf_rx_test(): when rte_bpf_eth_rx_install() fails, the loaded bpf program is leaked: ret = rte_bpf_eth_rx_install(port, 0, bpf, flags); if (ret != 0) { printf(...); return ret; /* no rte_bpf_destroy(bpf) */ } bpf_tx_test() in the same patch correctly calls rte_bpf_destroy(bpf) on the equivalent error path. Per the patch 05 contract, ownership transfers to the library only on successful install. Add `rte_bpf_destroy(bpf);` before the return.
