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

[Marat Khalili <marat.khalili@huawei.com>]

Thanks a lot for looking, please see the answers inline below.

> 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.

Addressed in v3.

> 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.

Behavior is intentionally not defined and caller is intentionally not required
to check return value from this API for errors to keep its life easier.
The return value is always valid with the catchpoint callback.

> 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.

Addressed in v3.

> 
>   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);

Addressed in v3.

>   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.

Addressed in v3.

> 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.

There is currently no use case for this, and undefined behaviour allows
implementing this in the future.

>   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.

Technically true, but practically I suspect there are no users of this API
yet, so noise in release notes is kept to the minimum.

> 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.

There is a prompt between them, so the result looks ok.

>   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.

Yes, this is a documented current limitation, patches are welcome.

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

Decisions on validity of external symbols are left to the BPF loader.

> 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.

Addressed in v3.

> Build fails on FreeBSD
// snip
> ../app/validate-bpf/parse_decl.c:392:33: error: 'array_length' may be used uninitialized [-
> Werror=maybe-uninitialized]

Hopefully addressed in v3.