On Fri, Apr 26, 2024 at 2:22 AM Jose E. Marchesi
<jose.marchesi@xxxxxxxxxx> wrote:
>
> The BPF_PROG, BPF_KPROBE and BPF_KSYSCALL macros defined in
> tools/lib/bpf/bpf_tracing.h use a clever hack in order to provide a
> convenient way to define entry points for BPF programs as if they were
> normal C functions that get typed actual arguments, instead of as
> elements in a single "context" array argument.
>
> For example, PPF_PROGS allows writing:
>
> SEC("struct_ops/cwnd_event")
> void BPF_PROG(cwnd_event, struct sock *sk, enum tcp_ca_event event)
> {
> bbr_cwnd_event(sk, event);
> dctcp_cwnd_event(sk, event);
> cubictcp_cwnd_event(sk, event);
> }
>
> That expands into a pair of functions:
>
> void ____cwnd_event (unsigned long long *ctx, struct sock *sk, enum tcp_ca_event event)
> {
> bbr_cwnd_event(sk, event);
> dctcp_cwnd_event(sk, event);
> cubictcp_cwnd_event(sk, event);
> }
>
> void cwnd_event (unsigned long long *ctx)
> {
> _Pragma("GCC diagnostic push")
> _Pragma("GCC diagnostic ignored \"-Wint-conversion\"")
> return ____cwnd_event(ctx, (void*)ctx[0], (void*)ctx[1]);
> _Pragma("GCC diagnostic pop")
> }
>
> Note how the 64-bit unsigned integers in the incoming CTX get casted
> to a void pointer, and then implicitly converted to whatever type of
> the actual argument in the wrapped function. In this case:
>
> Arg1: unsigned long long -> void * -> struct sock *
> Arg2: unsigned long long -> void * -> enum tcp_ca_event
>
> The behavior of GCC and clang when facing such conversions differ:
>
> pointer -> pointer
>
> Allowed by the C standard.
> GCC: no warning nor error.
> clang: no warning nor error.
>
> pointer -> integer type
>
> [C standard says the result of this conversion is implementation
> defined, and it may lead to unaligned pointer etc.]
>
> GCC: error: integer from pointer without a cast [-Wint-conversion]
> clang: error: incompatible pointer to integer conversion [-Wint-conversion]
>
> pointer -> enumerated type
>
> GCC: error: incompatible types in assigment (*)
> clang: error: incompatible pointer to integer conversion [-Wint-conversion]
>
> These macros work because converting pointers to pointers is allowed,
> and converting pointers to integers also works provided a suitable
> integer type even if it is implementation defined, much like casting a
> pointer to uintptr_t is guaranteed to work by the C standard. The
> conversion errors emitted by both compilers by default are silenced by
> the pragmas.
>
> However, the GCC error marked with (*) above when assigning a pointer
> to an enumerated value is not associated with the -Wint-conversion
> warning, and it is not possible to turn it off.
>
> This is preventing building the BPF kernel selftests with GCC.
>
> This patch fixes this by avoiding intermediate casts to void*,
> replaced with casts to `uintptr', which is an integer type capable of
> safely store a BPF pointer, much like the standard uintptr_t.
>
> Tested in bpf-next master.
> No regressions.
>
> Signed-off-by: Jose E. Marchesi <jose.marchesi@xxxxxxxxxx>
> Cc: Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx>
> Cc: david.faust@xxxxxxxxxx
> Cc: cupertino.miranda@xxxxxxxxxx
> ---
> tools/lib/bpf/bpf_tracing.h | 80 ++++++++++++++++++++-----------------
> 1 file changed, 43 insertions(+), 37 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf_tracing.h b/tools/lib/bpf/bpf_tracing.h
> index 1c13f8e88833..1098505a89c7 100644
> --- a/tools/lib/bpf/bpf_tracing.h
> +++ b/tools/lib/bpf/bpf_tracing.h
> @@ -4,6 +4,12 @@
>
> #include "bpf_helpers.h"
>
> +/* The following integer unsigned type must be able to hold a pointer.
> + It is used in the macros below in order to avoid eventual casts
> + from pointers to enum values, since these are rejected by GCC. */
> +
> +typedef unsigned long long uintptr;
> +
hold on, we didn't talk about adding new typedefs. This bpf_tracing.h
header is included into tons of user code, so we should avoid adding
extra global definitions and typedes. Please just use (unsigned long
long) explicitly everywhere.
Also please check CI failures ([0]).
[0] https://github.com/kernel-patches/bpf/actions/runs/8846180836/job/24291582343
pw-bot: cr
> /* Scan the ARCH passed in from ARCH env variable (see Makefile) */
> #if defined(__TARGET_ARCH_x86)
> #define bpf_target_x86
> @@ -523,9 +529,9 @@ struct pt_regs;
> #else
>
> #define BPF_KPROBE_READ_RET_IP(ip, ctx) \
> - ({ bpf_probe_read_kernel(&(ip), sizeof(ip), (void *)PT_REGS_RET(ctx)); })
> + ({ bpf_probe_read_kernel(&(ip), sizeof(ip), (uintptr)PT_REGS_RET(ctx)); })
> #define BPF_KRETPROBE_READ_RET_IP(ip, ctx) \
> - ({ bpf_probe_read_kernel(&(ip), sizeof(ip), (void *)(PT_REGS_FP(ctx) + sizeof(ip))); })
> + ({ bpf_probe_read_kernel(&(ip), sizeof(ip), (uintptr)(PT_REGS_FP(ctx) + sizeof(ip))); })
these are passing pointers, please don't just do a blind find&replace
>
> #endif
>
[...]
