* [PATCH bpf-next v3] libbpf: Fix potential uninitialized tail padding with LIBBPF_OPTS_RESET
@ 2023-11-07 20:15 Yonghong Song
2023-11-08 0:40 ` patchwork-bot+netdevbpf
2023-11-08 5:38 ` Daniel Borkmann
0 siblings, 2 replies; 3+ messages in thread
From: Yonghong Song @ 2023-11-07 20:15 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:
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.
Tested-by: 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:
v2 -> v3:
- Use sizeof(NAME) instead of sizeof(typeof(NAME)).
v1 -> v2:
- Do not change the LIBBPF_OPTS_RESET macro definition, rather
re-implement to avoid potential uninitialized tail padding.
v2 link: https://lore.kernel.org/bpf/20231107062936.2537338-1-yonghong.song@linux.dev/
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..8fe248e14eb6 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(NAME)); \
+ (typeof(NAME)) { \
+ .sz = sizeof(NAME), \
+ __VA_ARGS__ \
+ }; \
+ }); \
+ memcpy(&NAME, &___##NAME, sizeof(NAME)); \
} while (0)
#endif /* __LIBBPF_LIBBPF_COMMON_H */
--
2.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next v3] libbpf: Fix potential uninitialized tail padding with LIBBPF_OPTS_RESET
2023-11-07 20:15 [PATCH bpf-next v3] libbpf: Fix potential uninitialized tail padding with LIBBPF_OPTS_RESET Yonghong Song
@ 2023-11-08 0:40 ` patchwork-bot+netdevbpf
2023-11-08 5:38 ` Daniel Borkmann
1 sibling, 0 replies; 3+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-11-08 0:40 UTC (permalink / raw)
To: Yonghong Song; +Cc: bpf, ast, andrii, daniel, kernel-team, martin.lau
Hello:
This patch was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:
On Tue, 7 Nov 2023 12:15:11 -0800 you 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.
>
> [...]
Here is the summary with links:
- [bpf-next,v3] libbpf: Fix potential uninitialized tail padding with LIBBPF_OPTS_RESET
https://git.kernel.org/bpf/bpf-next/c/9ce5a7a0c8bf
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next v3] libbpf: Fix potential uninitialized tail padding with LIBBPF_OPTS_RESET
2023-11-07 20:15 [PATCH bpf-next v3] libbpf: Fix potential uninitialized tail padding with LIBBPF_OPTS_RESET Yonghong Song
2023-11-08 0:40 ` patchwork-bot+netdevbpf
@ 2023-11-08 5:38 ` Daniel Borkmann
1 sibling, 0 replies; 3+ messages in thread
From: Daniel Borkmann @ 2023-11-08 5:38 UTC (permalink / raw)
To: Yonghong Song, bpf
Cc: Alexei Starovoitov, Andrii Nakryiko, kernel-team,
Martin KaFai Lau
On 11/7/23 9:15 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.
[...]
Too bad we need this detour, but fix lgtm, thanks!
> diff --git a/tools/lib/bpf/libbpf_common.h b/tools/lib/bpf/libbpf_common.h
> index b7060f254486..8fe248e14eb6 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(NAME)); \
> + (typeof(NAME)) { \
> + .sz = sizeof(NAME), \
> + __VA_ARGS__ \
> + }; \
> + }); \
> + memcpy(&NAME, &___##NAME, sizeof(NAME)); \
> } while (0)
>
> #endif /* __LIBBPF_LIBBPF_COMMON_H */
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-11-08 5:38 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-07 20:15 [PATCH bpf-next v3] libbpf: Fix potential uninitialized tail padding with LIBBPF_OPTS_RESET Yonghong Song
2023-11-08 0:40 ` patchwork-bot+netdevbpf
2023-11-08 5:38 ` Daniel Borkmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox