From: Yonghong Song <yonghong.song@linux.dev>
To: Jiri Olsa <olsajiri@gmail.com>
Cc: bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
Andrii Nakryiko <andrii@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
kernel-team@fb.com, Martin KaFai Lau <martin.lau@kernel.org>
Subject: Re: [PATCH bpf-next v2] libbpf: Fix potential uninitialized tail padding with LIBBPF_OPTS_RESET
Date: Tue, 7 Nov 2023 07:29:30 -0800 [thread overview]
Message-ID: <92ff17e2-b74d-4497-a011-8cba636b6119@linux.dev> (raw)
In-Reply-To: <ZUo2mw46o6WAylUc@krava>
On 11/7/23 5:07 AM, Jiri Olsa wrote:
> On Mon, Nov 06, 2023 at 10:29:36PM -0800, Yonghong Song wrote:
>> Martin reported that there is a libbpf complaining of non-zero-value tail
>> padding with LIBBPF_OPTS_RESET macro if struct bpf_netkit_opts is modified
>> to have a 4-byte tail padding. This only happens to clang compiler.
>> The commend line is: ./test_progs -t tc_netkit_multi_links
>> Martin and I did some investigation and found this indeed the case and
>> the following are the investigation details.
>>
>> Clang 18:
>> clang version 18.0.0
>> <I tried clang15/16/17 and they all have similar results>
>>
>> tools/lib/bpf/libbpf_common.h:
>> #define LIBBPF_OPTS_RESET(NAME, ...) \
>> do { \
>> memset(&NAME, 0, sizeof(NAME)); \
>> NAME = (typeof(NAME)) { \
>> .sz = sizeof(NAME), \
>> __VA_ARGS__ \
>> }; \
>> } while (0)
>>
>> #endif
>>
>> tools/lib/bpf/libbpf.h:
>> struct bpf_netkit_opts {
>> /* size of this struct, for forward/backward compatibility */
>> size_t sz;
>> __u32 flags;
>> __u32 relative_fd;
>> __u32 relative_id;
>> __u64 expected_revision;
>> size_t :0;
>> };
>> #define bpf_netkit_opts__last_field expected_revision
>> In the above struct bpf_netkit_opts, there is no tail padding.
>>
>> prog_tests/tc_netkit.c:
>> static void serial_test_tc_netkit_multi_links_target(int mode, int target)
>> {
>> ...
>> LIBBPF_OPTS(bpf_netkit_opts, optl);
>> ...
>> LIBBPF_OPTS_RESET(optl,
>> .flags = BPF_F_BEFORE,
>> .relative_fd = bpf_program__fd(skel->progs.tc1),
>> );
>> ...
>> }
>>
>> Let us make the following source change, note that we have a 4-byte
>> tailing padding now.
>> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
>> index 6cd9c501624f..0dd83910ae9a 100644
>> --- a/tools/lib/bpf/libbpf.h
>> +++ b/tools/lib/bpf/libbpf.h
>> @@ -803,13 +803,13 @@ bpf_program__attach_tcx(const struct bpf_program *prog, int ifindex,
>> struct bpf_netkit_opts {
>> /* size of this struct, for forward/backward compatibility */
>> size_t sz;
>> - __u32 flags;
>> __u32 relative_fd;
>> __u32 relative_id;
>> __u64 expected_revision;
>> + __u32 flags;
>> size_t :0;
>> };
>> -#define bpf_netkit_opts__last_field expected_revision
>> +#define bpf_netkit_opts__last_field flags
>>
>> The clang 18 generated asm code looks like below:
>> ; LIBBPF_OPTS_RESET(optl,
>> 55e3: 48 8d 7d 98 leaq -0x68(%rbp), %rdi
>> 55e7: 31 f6 xorl %esi, %esi
>> 55e9: ba 20 00 00 00 movl $0x20, %edx
>> 55ee: e8 00 00 00 00 callq 0x55f3 <serial_test_tc_netkit_multi_links_target+0x18d3>
>> 55f3: 48 c7 85 10 fd ff ff 20 00 00 00 movq $0x20, -0x2f0(%rbp)
>> 55fe: 48 8b 85 68 ff ff ff movq -0x98(%rbp), %rax
>> 5605: 48 8b 78 18 movq 0x18(%rax), %rdi
>> 5609: e8 00 00 00 00 callq 0x560e <serial_test_tc_netkit_multi_links_target+0x18ee>
>> 560e: 89 85 18 fd ff ff movl %eax, -0x2e8(%rbp)
>> 5614: c7 85 1c fd ff ff 00 00 00 00 movl $0x0, -0x2e4(%rbp)
>> 561e: 48 c7 85 20 fd ff ff 00 00 00 00 movq $0x0, -0x2e0(%rbp)
>> 5629: c7 85 28 fd ff ff 08 00 00 00 movl $0x8, -0x2d8(%rbp)
>> 5633: 48 8b 85 10 fd ff ff movq -0x2f0(%rbp), %rax
>> 563a: 48 89 45 98 movq %rax, -0x68(%rbp)
>> 563e: 48 8b 85 18 fd ff ff movq -0x2e8(%rbp), %rax
>> 5645: 48 89 45 a0 movq %rax, -0x60(%rbp)
>> 5649: 48 8b 85 20 fd ff ff movq -0x2e0(%rbp), %rax
>> 5650: 48 89 45 a8 movq %rax, -0x58(%rbp)
>> 5654: 48 8b 85 28 fd ff ff movq -0x2d8(%rbp), %rax
>> 565b: 48 89 45 b0 movq %rax, -0x50(%rbp)
>> ; link = bpf_program__attach_netkit(skel->progs.tc2, ifindex, &optl);
>>
>> At -O0 level, the clang compiler creates an intermediate copy.
>> We have below to store 'flags' with 4-byte store and leave another 4 byte
>> in the same 8-byte-aligned storage undefined,
>> 5629: c7 85 28 fd ff ff 08 00 00 00 movl $0x8, -0x2d8(%rbp)
>> and later we store 8-byte to the original zero'ed buffer
>> 5654: 48 8b 85 28 fd ff ff movq -0x2d8(%rbp), %rax
>> 565b: 48 89 45 b0 movq %rax, -0x50(%rbp)
>>
>> This caused a problem as the 4-byte value at [%rbp-0x2dc, %rbp-0x2e0)
>> may be garbage.
>>
>> gcc (gcc 11.4) does not have this issue as it does zeroing struct first before
>> doing assignments:
>> ; LIBBPF_OPTS_RESET(optl,
>> 50fd: 48 8d 85 40 fc ff ff leaq -0x3c0(%rbp), %rax
>> 5104: ba 20 00 00 00 movl $0x20, %edx
>> 5109: be 00 00 00 00 movl $0x0, %esi
>> 510e: 48 89 c7 movq %rax, %rdi
>> 5111: e8 00 00 00 00 callq 0x5116 <serial_test_tc_netkit_multi_links_target+0x1522>
>> 5116: 48 8b 45 f0 movq -0x10(%rbp), %rax
>> 511a: 48 8b 40 18 movq 0x18(%rax), %rax
>> 511e: 48 89 c7 movq %rax, %rdi
>> 5121: e8 00 00 00 00 callq 0x5126 <serial_test_tc_netkit_multi_links_target+0x1532>
>> 5126: 48 c7 85 40 fc ff ff 00 00 00 00 movq $0x0, -0x3c0(%rbp)
>> 5131: 48 c7 85 48 fc ff ff 00 00 00 00 movq $0x0, -0x3b8(%rbp)
>> 513c: 48 c7 85 50 fc ff ff 00 00 00 00 movq $0x0, -0x3b0(%rbp)
>> 5147: 48 c7 85 58 fc ff ff 00 00 00 00 movq $0x0, -0x3a8(%rbp)
>> 5152: 48 c7 85 40 fc ff ff 20 00 00 00 movq $0x20, -0x3c0(%rbp)
>> 515d: 89 85 48 fc ff ff movl %eax, -0x3b8(%rbp)
>> 5163: c7 85 58 fc ff ff 08 00 00 00 movl $0x8, -0x3a8(%rbp)
>> ; link = bpf_program__attach_netkit(skel->progs.tc2, ifindex, &optl);
>>
>> It is not clear how to resolve the compiler code generation as the compiler
>> generates correct code w.r.t. how to handle unnamed padding in C standard.
>> So this patch changed LIBBPF_OPTS_RESET macro to avoid uninitialized tail
>> padding. We already knows LIBBPF_OPTS macro works on both gcc and clang,
>> even with tail padding. So LIBBPF_OPTS_RESET is changed to be a
>> LIBBPF_OPTS followed by a memcpy(), thus avoiding uninitialized tail padding.
> if that's the case, could we just do (untested):
>
> diff --git a/tools/lib/bpf/libbpf_common.h b/tools/lib/bpf/libbpf_common.h
> index b7060f254486..c89d4dbbebd8 100644
> --- a/tools/lib/bpf/libbpf_common.h
> +++ b/tools/lib/bpf/libbpf_common.h
> @@ -79,11 +79,8 @@
> */
> #define LIBBPF_OPTS_RESET(NAME, ...) \
> do { \
> - memset(&NAME, 0, sizeof(NAME)); \
> - NAME = (typeof(NAME)) { \
> - .sz = sizeof(NAME), \
> - __VA_ARGS__ \
> - }; \
> + LIBBPF_OPTS(___##NAME, __VA_ARGS__); \
> + memcpy(&NAME, &___##NAME, sizeof(typeof(NAME))); \
> } while (0)
>
> #endif /* __LIBBPF_LIBBPF_COMMON_H */
Thanks Jiri. The above does not work since the first macro parameter
in LIBBPF_OPTS is a type (e.g., 'bpf_netkit_opts' for struct bpf_netkit_opts).
>
>
> jirka
>
>> The below is asm code generated with this patch and with clang compiler:
>> ; LIBBPF_OPTS_RESET(optl,
>> 55e3: 48 8d bd 10 fd ff ff leaq -0x2f0(%rbp), %rdi
>> 55ea: 31 f6 xorl %esi, %esi
>> 55ec: ba 20 00 00 00 movl $0x20, %edx
>> 55f1: e8 00 00 00 00 callq 0x55f6 <serial_test_tc_netkit_multi_links_target+0x18d6>
>> 55f6: 48 c7 85 10 fd ff ff 20 00 00 00 movq $0x20, -0x2f0(%rbp)
>> 5601: 48 8b 85 68 ff ff ff movq -0x98(%rbp), %rax
>> 5608: 48 8b 78 18 movq 0x18(%rax), %rdi
>> 560c: e8 00 00 00 00 callq 0x5611 <serial_test_tc_netkit_multi_links_target+0x18f1>
>> 5611: 89 85 18 fd ff ff movl %eax, -0x2e8(%rbp)
>> 5617: c7 85 1c fd ff ff 00 00 00 00 movl $0x0, -0x2e4(%rbp)
>> 5621: 48 c7 85 20 fd ff ff 00 00 00 00 movq $0x0, -0x2e0(%rbp)
>> 562c: c7 85 28 fd ff ff 08 00 00 00 movl $0x8, -0x2d8(%rbp)
>> 5636: 48 8b 85 10 fd ff ff movq -0x2f0(%rbp), %rax
>> 563d: 48 89 45 98 movq %rax, -0x68(%rbp)
>> 5641: 48 8b 85 18 fd ff ff movq -0x2e8(%rbp), %rax
>> 5648: 48 89 45 a0 movq %rax, -0x60(%rbp)
>> 564c: 48 8b 85 20 fd ff ff movq -0x2e0(%rbp), %rax
>> 5653: 48 89 45 a8 movq %rax, -0x58(%rbp)
>> 5657: 48 8b 85 28 fd ff ff movq -0x2d8(%rbp), %rax
>> 565e: 48 89 45 b0 movq %rax, -0x50(%rbp)
>> ; link = bpf_program__attach_netkit(skel->progs.tc2, ifindex, &optl);
>>
>> In the above code, a temporary buffer is zeroed and then has proper value assigned.
>> Finally, values in temporary buffer are copied to the original variable buffer,
>> hence tail padding is guaranteed to be 0.
>>
>> Cc: Martin KaFai Lau <martin.lau@kernel.org>
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
>> tools/lib/bpf/libbpf_common.h | 13 ++++++++-----
>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> Changelog:
>> v1 -> v2:
>> - Do not change the LIBBPF_OPTS_RESET macro definition, rather
>> re-implement to avoid potential uninitialized tail padding.
>>
>> v1 link: https://lore.kernel.org/bpf/20231105185358.1036619-1-yonghong.song@linux.dev/
>>
>> diff --git a/tools/lib/bpf/libbpf_common.h b/tools/lib/bpf/libbpf_common.h
>> index b7060f254486..ef14e99bc952 100644
>> --- a/tools/lib/bpf/libbpf_common.h
>> +++ b/tools/lib/bpf/libbpf_common.h
>> @@ -79,11 +79,14 @@
>> */
>> #define LIBBPF_OPTS_RESET(NAME, ...) \
>> do { \
>> - memset(&NAME, 0, sizeof(NAME)); \
>> - NAME = (typeof(NAME)) { \
>> - .sz = sizeof(NAME), \
>> - __VA_ARGS__ \
>> - }; \
>> + typeof(NAME) ___##NAME = ({ \
>> + memset(&___##NAME, 0, sizeof(typeof(NAME))); \
>> + (typeof(NAME)) { \
>> + .sz = sizeof(typeof(NAME)), \
>> + __VA_ARGS__ \
>> + }; \
>> + }); \
>> + memcpy(&NAME, &___##NAME, sizeof(typeof(NAME))); \
>> } while (0)
>>
>> #endif /* __LIBBPF_LIBBPF_COMMON_H */
>> --
>> 2.34.1
>>
>>
next prev parent reply other threads:[~2023-11-07 15:29 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-07 6:29 [PATCH bpf-next v2] libbpf: Fix potential uninitialized tail padding with LIBBPF_OPTS_RESET Yonghong Song
2023-11-07 13:07 ` Jiri Olsa
2023-11-07 15:29 ` Yonghong Song [this message]
2023-11-07 18:23 ` Andrii Nakryiko
2023-11-07 20:07 ` Yonghong Song
2023-11-07 18:54 ` Martin KaFai Lau
2023-11-07 19:27 ` Andrii Nakryiko
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=92ff17e2-b74d-4497-a011-8cba636b6119@linux.dev \
--to=yonghong.song@linux.dev \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kernel-team@fb.com \
--cc=martin.lau@kernel.org \
--cc=olsajiri@gmail.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