Re: [PATCH bpf-next V2] bpf: avoid casts from pointers to enums in bpf_tracing.h

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux