> 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.
Ok.
> Also please check CI failures ([0]).
>
> [0] https://github.com/kernel-patches/bpf/actions/runs/8846180836/job/24291582343
How weird. This means something is going on in my local testing
environment.
> 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
>>
>
> [...]
