From: "Jose E. Marchesi" <jose.marchesi@oracle.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: bpf@vger.kernel.org, david.faust@oracle.com,
cupertino.miranda@oracle.com
Subject: Re: [PATCH bpf-next V2] bpf: avoid casts from pointers to enums in bpf_tracing.h
Date: Fri, 03 May 2024 10:00:48 +0200 [thread overview]
Message-ID: <877cgbpbnz.fsf@oracle.com> (raw)
In-Reply-To: <CAEf4BzaWaggGZCW4mCA53Sa7xiVARNk1taONaK0gL72x6B=mtA@mail.gmail.com> (Andrii Nakryiko's message of "Thu, 2 May 2024 23:05:27 -0700")
> On Thu, May 2, 2024 at 10:09 AM Jose E. Marchesi
> <jose.marchesi@oracle.com> 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@oracle.com>
>> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
>> Cc: david.faust@oracle.com
>> Cc: cupertino.miranda@oracle.com
>> ---
>> 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
>>
next prev parent reply other threads:[~2024-05-03 8:00 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-02 17:09 [PATCH bpf-next V2] bpf: avoid casts from pointers to enums in bpf_tracing.h Jose E. Marchesi
2024-05-03 6:05 ` Andrii Nakryiko
2024-05-03 8:00 ` Jose E. Marchesi [this message]
2024-05-03 6:10 ` patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=877cgbpbnz.fsf@oracle.com \
--to=jose.marchesi@oracle.com \
--cc=andrii.nakryiko@gmail.com \
--cc=bpf@vger.kernel.org \
--cc=cupertino.miranda@oracle.com \
--cc=david.faust@oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).