* [PATCH bpf-next v2] libbpf: Fix potential uninitialized tail padding with LIBBPF_OPTS_RESET
@ 2023-11-07 6:29 Yonghong Song
2023-11-07 13:07 ` Jiri Olsa
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Yonghong Song @ 2023-11-07 6:29 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team,
Martin KaFai Lau
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.
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v2] libbpf: Fix potential uninitialized tail padding with LIBBPF_OPTS_RESET
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
2023-11-07 18:23 ` Andrii Nakryiko
2023-11-07 18:54 ` Martin KaFai Lau
2 siblings, 1 reply; 7+ messages in thread
From: Jiri Olsa @ 2023-11-07 13:07 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
kernel-team, Martin KaFai Lau
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 */
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
>
>
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v2] libbpf: Fix potential uninitialized tail padding with LIBBPF_OPTS_RESET
2023-11-07 13:07 ` Jiri Olsa
@ 2023-11-07 15:29 ` Yonghong Song
0 siblings, 0 replies; 7+ messages in thread
From: Yonghong Song @ 2023-11-07 15:29 UTC (permalink / raw)
To: Jiri Olsa
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
kernel-team, Martin KaFai Lau
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
>>
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v2] libbpf: Fix potential uninitialized tail padding with LIBBPF_OPTS_RESET
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 18:23 ` Andrii Nakryiko
2023-11-07 20:07 ` Yonghong Song
2023-11-07 18:54 ` Martin KaFai Lau
2 siblings, 1 reply; 7+ messages in thread
From: Andrii Nakryiko @ 2023-11-07 18:23 UTC (permalink / raw)
To: Yonghong Song
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
kernel-team, Martin KaFai Lau
On Mon, Nov 6, 2023 at 10:29 PM Yonghong Song <yonghong.song@linux.dev> 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.
>
> 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))); \
ok, I think this will work in all the situations that LIBBPF_OPTS()
itself works, so looks good.
Just one minor nit: can you please simplify sizeof(typeof(NAME)) ->
sizeof(NAME), it should work without typeof, right?
> } while (0)
>
> #endif /* __LIBBPF_LIBBPF_COMMON_H */
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v2] libbpf: Fix potential uninitialized tail padding with LIBBPF_OPTS_RESET
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 18:23 ` Andrii Nakryiko
@ 2023-11-07 18:54 ` Martin KaFai Lau
2023-11-07 19:27 ` Andrii Nakryiko
2 siblings, 1 reply; 7+ messages in thread
From: Martin KaFai Lau @ 2023-11-07 18:54 UTC (permalink / raw)
To: Yonghong Song, bpf
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, kernel-team,
Martin KaFai Lau
On 11/6/23 10:29 PM, 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 bpf_netkit_ops is in the bpf tree. If avoiding a hole in bpf_netkit_opts
like above is preferred, probably the fix in this patch and the bpf_netkit_ops
change should be in the same libbpf version?
Ran the test in a loop. It resolved the issue.
Tested-by: Martin KaFai Lau <martin.lau@kernel.org>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v2] libbpf: Fix potential uninitialized tail padding with LIBBPF_OPTS_RESET
2023-11-07 18:54 ` Martin KaFai Lau
@ 2023-11-07 19:27 ` Andrii Nakryiko
0 siblings, 0 replies; 7+ messages in thread
From: Andrii Nakryiko @ 2023-11-07 19:27 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: Yonghong Song, bpf, Alexei Starovoitov, Andrii Nakryiko,
Daniel Borkmann, kernel-team, Martin KaFai Lau
On Tue, Nov 7, 2023 at 10:55 AM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 11/6/23 10:29 PM, 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 bpf_netkit_ops is in the bpf tree. If avoiding a hole in bpf_netkit_opts
> like above is preferred, probably the fix in this patch and the bpf_netkit_ops
> change should be in the same libbpf version?
Yes, they both will be in libbpf v1.3, which hopefully should be
released this week. Let's land the fix soon-ish so that it is synced
and goes into final v1.3.
>
> Ran the test in a loop. It resolved the issue.
>
> Tested-by: Martin KaFai Lau <martin.lau@kernel.org>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v2] libbpf: Fix potential uninitialized tail padding with LIBBPF_OPTS_RESET
2023-11-07 18:23 ` Andrii Nakryiko
@ 2023-11-07 20:07 ` Yonghong Song
0 siblings, 0 replies; 7+ messages in thread
From: Yonghong Song @ 2023-11-07 20:07 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
kernel-team, Martin KaFai Lau
On 11/7/23 10:23 AM, Andrii Nakryiko wrote:
> On Mon, Nov 6, 2023 at 10:29 PM Yonghong Song <yonghong.song@linux.dev> 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.
>>
>> 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))); \
> ok, I think this will work in all the situations that LIBBPF_OPTS()
> itself works, so looks good.
>
> Just one minor nit: can you please simplify sizeof(typeof(NAME)) ->
> sizeof(NAME), it should work without typeof, right?
Right. Weill send v3 soon.
>
>> } while (0)
>>
>> #endif /* __LIBBPF_LIBBPF_COMMON_H */
>> --
>> 2.34.1
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-11-07 20:07 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
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
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.