Re: [PATCH v2 0/7] validate-bpf: add standalone validation debug tool

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 14 Sep 2026 15:54:10 +0100
Marat Khalili <marat.khalili@huawei.com> wrote:

> This patchset introduces a new standalone tool for pre-validating eBPF
> programs and debugging validation problems. Its should allow developers
> to trace validator state changes per instruction and understand verifier
> decisions, simplifying the debugging of rejected programs before loading
> them into a real application context.
> 
> v2:
> * for FreeBSD compatibility switched from Linux to DPDK network structs;
> * addressed small AI comments:
>   * clarified patch 1 commit message;
>   * renamed `rte_validate_bpf_logtype` to `validate_bpf_logtype`;
>   * corrected comment to an internal-use-only function to reflect that
>     next-after-last value of the program counter is no longer allowed;
> 
> Marat Khalili (7):
>   bpf/validate: fix finished status on restart
>   bpf/validate: refactor internal step function
>   bpf/validate: formalize call back requirements
>   bpf/validate: add jump notification events
>   bpf/validate: add get current event API
>   app/test: add test for bpf validate debug events
>   app/validate-bpf: add BPF validation application
> 
>  MAINTAINERS                            |    2 +
>  app/meson.build                        |    1 +
>  app/test/test_bpf_validate.c           |  341 +++++++-
>  app/validate-bpf/alloc_list.c          |   53 ++
>  app/validate-bpf/args.c                |  263 ++++++
>  app/validate-bpf/debug.c               | 1099 ++++++++++++++++++++++++
>  app/validate-bpf/debug_command.c       |  383 +++++++++
>  app/validate-bpf/debug_command.h       |   65 ++
>  app/validate-bpf/eal_init_args.c       |   57 ++
>  app/validate-bpf/internal.h            |  151 ++++
>  app/validate-bpf/main.c                |   77 ++
>  app/validate-bpf/meson.build           |   13 +
>  app/validate-bpf/parse_decl.c          |  611 +++++++++++++
>  doc/guides/rel_notes/release_26_11.rst |    5 +
>  doc/guides/tools/index.rst             |    1 +
>  doc/guides/tools/validate_bpf.rst      |   97 +++
>  lib/bpf/bpf_validate.c                 |   49 +-
>  lib/bpf/bpf_validate_debug.c           |   92 +-
>  lib/bpf/bpf_validate_debug.h           |   12 +-
>  lib/bpf/rte_bpf_validate_debug.h       |   42 +-
>  20 files changed, 3348 insertions(+), 66 deletions(-)
>  create mode 100644 app/validate-bpf/alloc_list.c
>  create mode 100644 app/validate-bpf/args.c
>  create mode 100644 app/validate-bpf/debug.c
>  create mode 100644 app/validate-bpf/debug_command.c
>  create mode 100644 app/validate-bpf/debug_command.h
>  create mode 100644 app/validate-bpf/eal_init_args.c
>  create mode 100644 app/validate-bpf/internal.h
>  create mode 100644 app/validate-bpf/main.c
>  create mode 100644 app/validate-bpf/meson.build
>  create mode 100644 app/validate-bpf/parse_decl.c
>  create mode 100644 doc/guides/tools/validate_bpf.rst
> 

Looks good to me, but AI had some nits to pick.

Re: [PATCH v2 0/7] bpf/validate: debug events and validation app

Errors
------

Patch 7/7 (app/validate-bpf: add BPF validation application)

  app/validate-bpf/parse_decl.c:

    +		arg->value.size *= array_length;

  array_length comes from take_number() on the --xsym text with no
  upper bound, and the multiply is not checked for overflow, so a
  large length silently wraps to a small size that is then reported
  to the validator as the object size.  Reproduced:

    dpdk-validate-bpf --xsym='uint64_t[2305843009213693953] v' \
        --section=.text prog.o
    Validation succeeded.

  8 * 2305843009213693953 wraps to 8, so the tool accepts the
  declaration and describes an 8-byte object.  Clamp array_length,
  or check the product, and reject with the usual text error.

Warnings
--------

Patch 5/7 (bpf/validate: add get current event API)

  lib/bpf/bpf_validate_debug.c:

    +		return -EINVAL;

  in rte_bpf_validate_debug_get_event(), whose return type is
  enum rte_bpf_validate_debug_event.  Every enumerator in that enum
  is non-negative, so the compatible type is unsigned and -EINVAL
  arrives at the caller as 4294967274.  It equals no enumerator,
  including RTE_BPF_VALIDATE_DEBUG_EVENT_END, so a caller cannot
  test for it.  Either return
  RTE_BPF_VALIDATE_DEBUG_EVENT_END for the NULL case and say so in
  the doc comment, or change the signature to return int with the
  event in an out-parameter.

Patch 7/7

  app/validate-bpf/parse_decl.c:

    +	void * const val = calloc(1, RTE_MAX(1u, arg.value.size));
    +	RTE_VERIFY(val != 0);

  arg.value.size is user-supplied, so an allocation failure driven by
  the command line aborts with a core dump instead of an error
  message.  Reproduced:

    dpdk-validate-bpf --xsym='uint64_t[18446744073709551615] v' \
        --section=.text prog.o
    EAL: PANIC in fill_var_xsym():
    line 427	assert "val != 0" failed

  Return an error up through the parse path like the other --xsym
  failures do.

  app/validate-bpf/main.c:

    +	RTE_VERIFY(rte_eal_init(eal_init_argc, eal_init_argv) ==
    +		eal_init_argc - 1);

  rte_eal_init() returns -1 for ordinary environmental failures such
  as a missing runtime directory or insufficient permissions.  For a
  command-line tool that should be a message and a non-zero exit, not
  rte_panic().  Same for

    +	RTE_VERIFY(rte_eal_cleanup() == 0);

  app/validate-bpf/eal_init_args.c:

    +#define RTE_EAL_INIT_ARG_SIZE_MAX sizeof("--log-level=lib.eal:warning")

  and RTE_EAL_INIT_ARGS / RTE_EAL_INIT_ARGC below it.  The RTE_
  prefix is reserved for the libraries; an application should not
  define into it.  Drop the prefix.

Info
----

Patch 5/7

  debug->current_event is set only in debug_send_event().  A
  breakpoint callback reached through debug_trigger_breakpoints()
  therefore sees whatever event fired last.  The doc comment says the
  value is undefined when no event is being processed, so this is
  consistent, but a callback shared between a breakpoint and a
  catchpoint will read a stale event rather than an obviously invalid
  one.

  rte_bpf_validate_debug_get_event() is new public experimental API
  and gets no release note.  7/7 adds a release note, but only for
  the application.

Patch 7/7

  In --debug mode the result is printed twice: debug.c prints
  "Validation succeeded." to stdout from the validation-success
  catchpoint, and main.c prints it again to stderr.

  Array declarations are written as "uint64_t[4] v", not the C
  "uint64_t v[4]".  The C spelling is rejected with

    at offset 10: expect '(' or text end

  which does not point at the real problem.  Worth either accepting
  the C form or naming the expected form in the message.  The
  documented form is in --help, so this is a diagnostics nit.

  "uint64_t[0] v" is accepted and yields value.size 0 while calloc
  still allocates one byte.  Probably worth rejecting a zero length.

Verification
------------

Applied on d55ccd4.  Each of the seven commits builds on its own with
-Dwerror=true, so bisect is safe.  bpf_validate_autotest passes 29/29
and the new bpf_validate_events_autotest passes.  valgrind is clean on
the application's normal paths, with no definite leaks.

Checked and found nothing to report: 2/7 is equivalent at all call
sites, so "No functional changes" holds; 3/7's tightening of the pc
check to ">= nb_ins" is consistent with dropping the past-the-end call
from evaluate_finish(); 4/7's nb_edge > 1 test does select exactly the
conditional jumps.

Review-Result: ERROR