Re: [PATCH v3 02/10] bpf: introduce extensible load API

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 18 May 2026 09:49:02 +0100
Marat Khalili <marat.khalili@huawei.com> 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 <marat.khalili@huawei.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> ---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.