|SUCCESS| [v3,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/164064

_AI Code Review_

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

Commit: 20260518084912.57006-2-marat.khalili at huawei.com
Author: Marat Khalili <marat.khalili at huawei.com>
Subject: [PATCH v3 01/10] bpf: make logging prefixes more consistent

This commit introduces a new logging macro to standardize function name
prefixes in BPF library log messages, replacing the inconsistent usage
of `%s()` and `%s` for `__func__`.

> diff --git a/lib/bpf/bpf_impl.h b/lib/bpf/bpf_impl.h
> index f5fa220984..fb5ec3c4d6 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__)

Does this macro properly handle the case where no variadic arguments are
passed? The string concatenation "%s(): " fmt will function correctly,
but is this usage aligned with how RTE_LOG_LINE processes variadic
arguments in contexts where __VA_ARGS__ is empty?

[ ... ]

---

Commit: 20260518084912.57006-3-marat.khalili at huawei.com
Author: Marat Khalili <marat.khalili at huawei.com>
Subject: [PATCH v3 02/10] bpf: introduce extensible load API

This commit introduces a new extensible BPF load parameter structure
rte_bpf_prm_ex and a new loading function rte_bpf_load_ex, consolidating
multiple code paths into a single entry point.

> diff --git a/lib/bpf/bpf_load.c b/lib/bpf/bpf_load.c
> index b8a0426fe2..6501841676 100644
> --- a/lib/bpf/bpf_load.c
> +++ b/lib/bpf/bpf_load.c

[ ... ]

> @@ -80,37 +80,44 @@ bpf_check_xsym(const struct rte_bpf_xsym *xsym)
>  	return 0;
>  }
>  
> -RTE_EXPORT_SYMBOL(rte_bpf_load)
> -struct rte_bpf *
> -rte_bpf_load(const struct rte_bpf_prm *prm)
> +static int
> +bpf_check_xsyms(const struct rte_bpf_xsym *xsym, uint32_t nb_xsym)
>  {
> -	struct rte_bpf *bpf;
>  	int32_t rc;
>  	uint32_t i;
>  
> -	if (prm == NULL || prm->ins == NULL || prm->nb_ins == 0 ||
> -			(prm->nb_xsym != 0 && prm->xsym == NULL)) {
> -		rte_errno = EINVAL;
> -		return NULL;
> -	}
> +	if (nb_xsym != 0 && xsym == NULL)
> +		return -EINVAL;
>  
>  	rc = 0;
> -	for (i = 0; i != prm->nb_xsym && rc == 0; i++)
> -		rc = bpf_check_xsym(prm->xsym + i);
> +	for (i = 0; i != nb_xsym && rc == 0; i++)
> +		rc = bpf_check_xsym(xsym + i);

Does the loop need the condition rc == 0? The original code stopped
checking remaining symbols once rc became non-zero, but the current
version continues incrementing i even though rc is already non-zero.
Does this matter for correctness?

> +/*
> + * Check extensible opts for invalid size or non-zero unsupported members.
> + *
> + * This code provides forward compatibility with applications compiled against
> + * newer version of this library. `opts_sz` is the size of struct `opts` in the
> + * version used for compiling the application, read from the member `sz`;
> + * `type_sz` is the size of same struct in the version used for compiling the
> + * library.
> + *
> + * If new fields were added to the struct in the application version, `opts_sz`
> + * will be greater than `type_sz`. In this case we are making sure all bytes we
> + * don't know how to interpret are zeroes, that is any new features that are
> + * there are not being used.
> + *
> + * This function can be used to check any struct following this convention.
> + */
> +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;

Does this code overflow the opts buffer when opts_sz is smaller than
type_sz? The loop iterates from type_sz to opts_sz, so if type_sz >
opts_sz, does the loop skip and return true correctly?

[ ... ]

> diff --git a/lib/bpf/bpf_load_elf.c b/lib/bpf/bpf_load_elf.c
> index 2390823cbf..4ae7492351 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));

Does this code leak the file descriptor when close fails? The file
descriptor remains stored in load->elf_fd but is never reset, so if
cleanup is called again, does it attempt to close an invalid fd?

[ ... ]

---

Commit: 20260518084912.57006-4-marat.khalili at huawei.com
Author: Marat Khalili <marat.khalili at huawei.com>
Subject: [PATCH v3 03/10] bpf: support up to 5 arguments

This commit adds support for BPF programs accepting up to 5 arguments,
introducing new execution and JIT APIs.

> diff --git a/lib/bpf/bpf_exec.c b/lib/bpf/bpf_exec.c
> index e4668ba10b..350a216ae5 100644
> --- a/lib/bpf/bpf_exec.c
> +++ b/lib/bpf/bpf_exec.c

[ ... ]

> +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;
> +
> +	/*
> +	 * Fast path: assumes application pre-validated RTE_BPF_EXEC_FLAG_JIT
> +	 * and successful JIT generation. No explicit NULL checks here.
> +	 */
> +	switch (bpf->prm.nb_prog_arg) {
> +	case 0:
> +		for (i = 0; i != num; i++)
> +			rc[i] = jit.func0();
> +		break;
> +	case 1:
> +		for (i = 0; i != num; i++) {
> +			const union rte_bpf_func_arg *const arg = ctx[i].arg;
> +			rc[i] = jit.func1(arg[0]);

Does this code safely handle the case where ctx is NULL for nullary
programs (nb_prog_arg == 0)? The case 0 branch does not access ctx, but
can a user legally pass ctx=NULL when calling with a nullary program?

[ ... ]

> diff --git a/lib/bpf/rte_bpf.h b/lib/bpf/rte_bpf.h
> index bf58a41819..0e7eaa3c18 100644
> --- a/lib/bpf/rte_bpf.h
> +++ b/lib/bpf/rte_bpf.h

[ ... ]

> +/**
> + * JIT-ed native code, member depends on number of program arguments.
> + */
> +struct rte_bpf_jit_ex {
> +	union {
> +		void *raw;
> +		rte_bpf_jit_func0_t func0;  /* nullary function */
> +		rte_bpf_jit_func1_t func1;  /* unary function */
> +		rte_bpf_jit_func2_t func2;  /* binary function */
> +		rte_bpf_jit_func3_t func3;  /* ternary function */
> +		rte_bpf_jit_func4_t func4;  /* quaternary function */
> +		rte_bpf_jit_func5_t func5;  /* quinary function */
> +	};
> +	size_t sz;
> +};

Does this struct require padding after the union to ensure correct
alignment of sz across different platforms? On some architectures, does
the compiler introduce unexpected padding that could affect
compatibility?

[ ... ]

---

Commit: 20260518084912.57006-6-marat.khalili at huawei.com
Author: Marat Khalili <marat.khalili at huawei.com>
Subject: [PATCH v3 05/10] bpf: support rte_bpf_prm_ex with port callbacks

This commit introduces new functions to install already-loaded BPF
programs as RX/TX port/queue callbacks.

> diff --git a/lib/bpf/bpf_pkt.c b/lib/bpf/bpf_pkt.c
> index 5007f6aef5..87065e939f 100644
> --- a/lib/bpf/bpf_pkt.c
> +++ b/lib/bpf/bpf_pkt.c

[ ... ]

> @@ -564,13 +559,33 @@ int
>  rte_bpf_eth_rx_elf_load(uint16_t port, uint16_t queue,
>  	const struct rte_bpf_prm *prm, const char *fname, const char *sname,
>  	uint32_t flags)
> +{
> +	struct rte_bpf *bpf;
> +	int32_t rc;
> +
> +	bpf = rte_bpf_elf_load(prm, fname, sname);
> +	if (bpf == NULL)
> +		return -rte_errno;
> +
> +	rc = rte_bpf_eth_rx_install(port, queue, bpf, flags);
> +
> +	if (rc < 0)
> +		rte_bpf_destroy(bpf);
> +
> +	return rc;
> +}

Does this code leak the bpf object when rte_bpf_eth_rx_install succeeds?
The comment in the patch mentions that on success, ownership passes to
the library, but does the caller need to know that they must not call
rte_bpf_destroy after a successful install?

[ ... ]