|SUCCESS| [10/10] doc: add load API to BPF programmer's guide

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163719

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-06

I'll review this DPDK patch series focusing on correctness bugs, coding style, API requirements, and guideline violations.

---

**Commit:** 20260506172209.6805-2-marat.khalili at huawei.com
**Author:** Marat Khalili <marat.khalili at huawei.com>
**Subject:** bpf: make logging prefixes more consistent

This commit introduces a new logging macro and standardizes function name
logging across the BPF subsystem.

```
> diff --git a/lib/bpf/bpf_impl.h b/lib/bpf/bpf_impl.h
> index f5fa22098489..fb5ec3c4d65f 100644
> --- a/lib/bpf/bpf_impl.h
> +++ b/lib/bpf/bpf_impl.h
> @@ -32,6 +32,9 @@ extern int rte_bpf_logtype;
>  #define RTE_BPF_LOG_LINE(lvl, ...) \
>  	RTE_LOG_LINE(lvl, BPF, __VA_ARGS__)
>  
> +#define RTE_BPF_LOG_FUNC_LINE(lvl, fmt, ...) \
> +	RTE_LOG_LINE(lvl, BPF, "%s(): " fmt, __func__, ##__VA_ARGS__)
```

Can this macro be enhanced for type safety? Does the string literal
concatenation here support logging format modifiers consistently across all
call sites?

---

**Commit:** 20260506172209.6805-3-marat.khalili at huawei.com
**Author:** Marat Khalili <marat.khalili at huawei.com>
**Subject:** bpf: introduce extensible load API

The commit introduces extensible API structures and reorganizes the BPF
loading code path.

```
> diff --git a/lib/bpf/bpf_load.c b/lib/bpf/bpf_load.c
> index b8a0426fe2ed..650184167609 100644
> --- a/lib/bpf/bpf_load.c
> +++ b/lib/bpf/bpf_load.c

[ ... ]

> +static int
> +bpf_load_raw(struct __rte_bpf_load *load)
> +{
> +	const struct rte_bpf_prm_ex *const prm = &load->prm;
> +	struct rte_bpf *bpf;
> +	int32_t rc;
> +
> +	RTE_ASSERT(prm->origin == RTE_BPF_ORIGIN_RAW);
> +
> +	if (prm->raw.ins == NULL || prm->raw.nb_ins == 0)
> +		return -EINVAL;
> +
> +	bpf = bpf_load(prm);
> +	if (bpf == NULL)
> +		return -ENOMEM;
```

Does bpf_load() always set rte_errno to ENOMEM when returning NULL, or
could it be other errors? If bpf_load() can fail for different reasons, can
this code mask the actual error by hardcoding -ENOMEM?

```
> +static bool
> +opts_valid(const void *opts, size_t opts_sz, size_t type_sz)
> +{
> +	if (opts == NULL)
> +		return true;
> +
> +	if (opts_sz < sizeof(opts_sz))
> +		/* Size of the struct is too small even for sz member. */
> +		return false;
> +
> +	/* Verify that all extra bytes are zeroed. */
> +	for (size_t offset = type_sz; offset < opts_sz; ++offset)
> +		if (((const char *)opts)[offset] != 0)
> +			return false;
> +
> +	return true;
> +}
```

Can this loop go out of bounds? When opts_sz is larger than type_sz, does
this code read beyond the allocated memory region pointed to by opts?

```
> +static int
> +load_try(struct __rte_bpf_load *load, const struct rte_bpf_prm_ex *app_prm)
> +{
> +	int rc;
> +
> +	if (app_prm == NULL || !opts_valid(app_prm, app_prm->sz, sizeof(load->prm)))
> +		return -EINVAL;
> +
> +	/*
> +	 * Convert extensible prm of application size to the size known to us.
> +	 *
> +	 * This code provides compatibility with applications compiled against
> +	 * different version of this library. `app_prm->sz` is the size of
> +	 * struct `rte_bpf_prm_ex` in the version used for compiling the
> +	 * application; `sizeof(load->prm)` is the size of the same struct in
> +	 * the version used for compiling the library.
> +	 *
> +	 * We are copying only the fields known to the application and leave
> +	 * the rest filled with zeroes. Any features that not known to the
> +	 * application will have backward-compatible default behaviour.
> +	 */
> +	memcpy(&load->prm, app_prm, RTE_MIN(app_prm->sz, sizeof(load->prm)));
> +	load->prm.sz = sizeof(load->prm);
```

Does this memcpy() handle the case where app_prm->sz is larger than
sizeof(load->prm) correctly? Can partial copies lead to inconsistent state
if the struct has pointer members pointing to data that depends on the
struct size?

```
> diff --git a/lib/bpf/bpf_load_elf.c b/lib/bpf/bpf_load_elf.c
> index 2390823cbf30..4ae7492351ae 100644
> --- a/lib/bpf/bpf_load_elf.c
> +++ b/lib/bpf/bpf_load_elf.c

[ ... ]

> +void
> +__rte_bpf_load_elf_cleanup(struct __rte_bpf_load *load)
> +{
> +	elf_end(load->elf);
> +
> +	if (load->elf_fd >= 0 && close(load->elf_fd) < 0) {
> +		const int close_errno = errno;
> +		RTE_BPF_LOG_FUNC_LINE(ERR, "error %d closing: %s",
> +			close_errno, strerror(close_errno));
> +	}
> +}
```

Can elf_end() be called with NULL safely? If load->elf can be NULL when
this function is called, does this cause undefined behavior?

---

**Commit:** 20260506172209.6805-4-marat.khalili at huawei.com
**Author:** Marat Khalili <marat.khalili at huawei.com>
**Subject:** bpf: support up to 5 arguments

This commit adds support for multi-argument BPF programs.

```
> diff --git a/lib/bpf/bpf_exec.c b/lib/bpf/bpf_exec.c
> index e4668ba10b64..d77c59991632 100644
> --- a/lib/bpf/bpf_exec.c
> +++ b/lib/bpf/bpf_exec.c

[ ... ]

> +static uint32_t
> +exec_vm_burst_ex(const struct rte_bpf *bpf, const struct rte_bpf_prog_ctx *ctx,
> +	uint64_t rc[], uint32_t num)
> +{
> +	uint32_t i;
> +	uint64_t reg[EBPF_REG_NUM];
> +	uint64_t stack[MAX_BPF_STACK_SIZE / sizeof(uint64_t)];
> +
> +	for (i = 0; i != num; i++) {
> +		const union rte_bpf_func_arg *const arg = ctx[i].arg;
> +
> +		switch (bpf->prm.nb_prog_arg) {
> +		case 5:
> +			reg[EBPF_REG_5] = arg[4].u64;
> +			/* FALLTHROUGH */
> +		case 4:
> +			reg[EBPF_REG_4] = arg[3].u64;
> +			/* FALLTHROUGH */
> +		case 3:
> +			reg[EBPF_REG_3] = arg[2].u64;
> +			/* FALLTHROUGH */
> +		case 2:
> +			reg[EBPF_REG_2] = arg[1].u64;
> +			/* FALLTHROUGH */
> +		case 1:
> +			reg[EBPF_REG_1] = arg[0].u64;
> +			/* FALLTHROUGH */
> +		case 0:
> +			break;
> +		}
> +
> +		reg[EBPF_REG_10] = (uintptr_t)(stack + RTE_DIM(stack));
> +
> +		rc[i] = bpf_exec(bpf, reg);
> +	}
> +
> +	return i;
> +}
```

Does this loop handle the case where ctx can be NULL for nullary programs?
If nb_prog_arg is 0 and ctx is NULL, can accessing ctx[i] cause a
segmentation fault?

```
> +static uint32_t
> +exec_jit_burst_ex(const struct rte_bpf *bpf, const struct rte_bpf_prog_ctx *ctx,
> +	uint64_t rc[], uint32_t num)
> +{
> +	uint32_t i;
> +	const struct rte_bpf_jit_ex jit = bpf->jit;
> +
> +	switch (bpf->prm.nb_prog_arg) {
> +	case 0:
> +		for (i = 0; i != num; i++)
> +			rc[i] = jit.func0();
> +		break;
```

For the case 0 branch, does this code check whether jit.func0 is non-NULL
before calling it? Can this dereference a NULL function pointer if JIT
compilation failed?

```
> diff --git a/lib/bpf/bpf_validate.c b/lib/bpf/bpf_validate.c
> index 5bfc59296d05..bf8a4abb5a5a 100644
> --- a/lib/bpf/bpf_validate.c
> +++ b/lib/bpf/bpf_validate.c

[ ... ]

> +static bool
> +prog_arg_is_valid(const struct rte_bpf_arg *prog_arg)
> +{
> +	/* check input argument type, don't allow mbuf ptr on 32-bit */
> +	if (prog_arg->type != RTE_BPF_ARG_RAW &&
> +			prog_arg->type != RTE_BPF_ARG_PTR &&
> +			(sizeof(uint64_t) != sizeof(uintptr_t) ||
> +			prog_arg->type != RTE_BPF_ARG_PTR_MBUF)) {
> +		RTE_BPF_LOG_FUNC_LINE(ERR, "unsupported argument type");
> +		return false;
> +	}
> +
> +	return true;
> +}
```

Can this logic handle uninitialized or out-of-range prog_arg->type values?
Does RTE_BPF_ARG_PTR_MBUF have a specific numeric value that should be
validated before use?

---

**Commit:** 20260506172209.6805-5-marat.khalili at huawei.com
**Author:** Marat Khalili <marat.khalili at huawei.com>
**Subject:** bpf: add cBPF origin to rte_bpf_load_ex

This commit adds support for loading classic BPF programs.

```
> diff --git a/lib/bpf/bpf_convert.c b/lib/bpf/bpf_convert.c
> index 953ca80670c4..c997116c691f 100644
> --- a/lib/bpf/bpf_convert.c
> +++ b/lib/bpf/bpf_convert.c

[ ... ]

> +int __rte_bpf_convert(struct __rte_bpf_load *load)
> +{
> +	struct rte_bpf_prm_ex *const prm = &load->prm;
> +	uint32_t nb_ins = 0;
> +	int ret;
> +
> +	RTE_ASSERT(prm->origin == RTE_BPF_ORIGIN_CBPF);
> +
> +	if (prm->cbpf.ins == NULL || prm->cbpf.nb_ins == 0)
> +		return -EINVAL;
> +
> +	/* 1st pass: calculate the eBPF program length */
> +	ret = bpf_convert_filter(prm->cbpf.ins, prm->cbpf.nb_ins, NULL, &nb_ins);
> +	if (ret < 0) {
> +		RTE_BPF_LOG_FUNC_LINE(ERR, "cannot get eBPF length");
> +		return ret;
> +	}
> +
> +	RTE_ASSERT(load->ins == NULL);
> +	load->ins = malloc(nb_ins * sizeof(load->ins[0]));
> +	if (load->ins == NULL)
> +		return -ENOMEM;
```

Does this malloc check for integer overflow in the size calculation? Can
nb_ins * sizeof(load->ins[0]) overflow if nb_ins is very large?

---

**Commit:** 20260506172209.6805-6-marat.khalili at huawei.com
**Author:** Marat Khalili <marat.khalili at huawei.com>
**Subject:** bpf: support rte_bpf_prm_ex with port callbacks

```
> diff --git a/lib/bpf/bpf_pkt.c b/lib/bpf/bpf_pkt.c
> index 5007f6aef57d..87065e939f31 100644
> --- a/lib/bpf/bpf_pkt.c
> +++ b/lib/bpf/bpf_pkt.c

[ ... ]

> +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_bpf_eth_rx_install, 26.11)
> +int
> +rte_bpf_eth_rx_install(uint16_t port, uint16_t queue, struct rte_bpf *bpf,
> +	uint32_t flags)
> +{
> +	int32_t rc;
> +	struct bpf_eth_cbh *cbh;
> +
> +	cbh = &rx_cbh;
> +	rte_spinlock_lock(&cbh->lock);
> +	rc = bpf_eth_elf_install(cbh, port, queue, bpf, flags);
> +	rte_spinlock_unlock(&cbh->lock);
> +
> +	return rc;
> +}
```

Does bpf_eth_elf_install() acquire any additional locks? Can this lead to
deadlock if the function tries to acquire the same lock or interacts with
code paths that do?