> On Thu, May 2, 2024 at 10:09 AM Jose E. Marchesi
> <jose.marchesi@xxxxxxxxxx> wrote:
>>
>> [Differences from V1:
>> - Do not introduce a global typedef, as this is a public header.
>> - Keep the void* casts in BPF_KPROBE_READ_RET_IP and
>> BPF_KRETPROBE_READ_RET_IP, as these are necessary
>> for converting to a const void* argument of
>> bpf_probe_read_kernel.]
>>
>> 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 `unsigned long long', which is an integer type
>> capable of safely store a BPF pointer, much like the standard
>> uintptr_t.
>>
>> Testing performed in bpf-next master:
>> - vmtest.sh -- ./test_verifier
>> - vmtest.sh -- ./test_progs
>> - make M=samples/bpf
>> 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 | 84 +++++++++++++++++++++----------------
>> 1 file changed, 49 insertions(+), 35 deletions(-)
>>
>
> [...]
>
>> /* If kernel doesn't have CONFIG_ARCH_HAS_SYSCALL_WRAPPER, we have to BPF_CORE_READ from pt_regs */
>> #define ___bpf_syswrap_args0() ctx
>> -#define ___bpf_syswrap_args1(x) ___bpf_syswrap_args0(), (void *)PT_REGS_PARM1_CORE_SYSCALL(regs)
>> -#define ___bpf_syswrap_args2(x, args...) ___bpf_syswrap_args1(args), (void *)PT_REGS_PARM2_CORE_SYSCALL(regs)
>> -#define ___bpf_syswrap_args3(x, args...) ___bpf_syswrap_args2(args), (void *)PT_REGS_PARM3_CORE_SYSCALL(regs)
>> -#define ___bpf_syswrap_args4(x, args...) ___bpf_syswrap_args3(args), (void *)PT_REGS_PARM4_CORE_SYSCALL(regs)
>> -#define ___bpf_syswrap_args5(x, args...) ___bpf_syswrap_args4(args), (void *)PT_REGS_PARM5_CORE_SYSCALL(regs)
>> -#define ___bpf_syswrap_args6(x, args...) ___bpf_syswrap_args5(args), (void *)PT_REGS_PARM6_CORE_SYSCALL(regs)
>> -#define ___bpf_syswrap_args7(x, args...) ___bpf_syswrap_args6(args), (void *)PT_REGS_PARM7_CORE_SYSCALL(regs)
>> +#define ___bpf_syswrap_args1(x) \
>> + ___bpf_syswrap_args0(), (unsigned long long)PT_REGS_PARM1_CORE_SYSCALL(regs)
>> +#define ___bpf_syswrap_args2(x, args...) \
>> + ___bpf_syswrap_args1(args), (unsigned long long)PT_REGS_PARM2_CORE_SYSCALL(regs)
>> +#define ___bpf_syswrap_args3(x, args...) \
>> + ___bpf_syswrap_args2(args), (unsigned long long)PT_REGS_PARM3_CORE_SYSCALL(regs)
>> +#define ___bpf_syswrap_args4(x, args...) \
>> + ___bpf_syswrap_args3(args), (unsigned long long)PT_REGS_PARM4_CORE_SYSCALL(regs)
>> +#define ___bpf_syswrap_args5(x, args...) \
>> + ___bpf_syswrap_args4(args), (unsigned long long)PT_REGS_PARM5_CORE_SYSCALL(regs)
>> +#define ___bpf_syswrap_args6(x, args...) \
>> + ___bpf_syswrap_args5(args), (unsigned long long)PT_REGS_PARM6_CORE_SYSCALL(regs)
>> +#define ___bpf_syswrap_args7(x, args...) \
>> + ___bpf_syswrap_args6(args), (unsigned long long)PT_REGS_PARM7_CORE_SYSCALL(regs)
>
> I undid all the line wrapping you did. Yes, they are even longer now,
> but at least the pattern is easy to see when all of these macros are
> single line ones.
It is much better this way. I really hated to split these lines.
> Also, I took the liberty of doing similar transformations for
> BPF_USDT() in usdt.bpf.h in the same patch, as you'll probably run
> into the same issue (not sure why you haven't caught that yet). Please
> double-check the committed patch, just to make sure I didn't screw
> anything up. Thanks. Applied to bpf-next.
LGTM.
The reason we didn't caught that with GCC is that none of the current
uses of BPF_USDT in the selftests use enumerated arguments:
progs/test_urandom_usdt.c:
int BPF_USDT(urandlib_read_with_sema, int iter_num, int iter_cnt, int buf_sz)
int BPF_USDT(urand_read_with_sema, int iter_num, int iter_cnt, int buf_sz)
int BPF_USDT(urandlib_read_without_sema, int iter_num, int iter_cnt, int buf_sz)
int BPF_USDT(urandlib_read_with_sema, int iter_num, int iter_cnt, int buf_sz)
int BPF_USDT(urand_read_without_sema, int iter_num, int iter_cnt, int buf_sz)
int BPF_USDT(urand_read_with_sema, int iter_num, int iter_cnt, int buf_sz)
int BPF_USDT(urandlib_read_without_sema, int iter_num, int iter_cnt, int buf_sz)
int BPF_USDT(urandlib_read_with_sema, int iter_num, int iter_cnt, int buf_sz)
progs/test_usdt.c:
int BPF_USDT(usdt12, int a1, int a2, long a3, long a4, unsigned a5,
long a6, __u64 a7, uintptr_t a8, int a9, short a10,
short a11, signed char a12)
progs/test_usdt_multispec.c:
int BPF_USDT(usdt_100, int x)
Thanks!
>
>> #define ___bpf_syswrap_args(args...) ___bpf_apply(___bpf_syswrap_args, ___bpf_narg(args))(args)
>>
>> /*
>> --
>> 2.30.2
>>
