mirror of
https://github.com/torvalds/linux.git
synced 2024-12-02 09:01:34 +00:00
7f7c43693c
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.
Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Tested-by: Martin KaFai Lau <martin.lau@kernel.org>
Link: https://lore.kernel.org/bpf/20231107201511.2548645-1-yonghong.song@linux.dev
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
93 lines
3.4 KiB
C
93 lines
3.4 KiB
C
/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
|
|
|
|
/*
|
|
* Common user-facing libbpf helpers.
|
|
*
|
|
* Copyright (c) 2019 Facebook
|
|
*/
|
|
|
|
#ifndef __LIBBPF_LIBBPF_COMMON_H
|
|
#define __LIBBPF_LIBBPF_COMMON_H
|
|
|
|
#include <string.h>
|
|
#include "libbpf_version.h"
|
|
|
|
#ifndef LIBBPF_API
|
|
#define LIBBPF_API __attribute__((visibility("default")))
|
|
#endif
|
|
|
|
#define LIBBPF_DEPRECATED(msg) __attribute__((deprecated(msg)))
|
|
|
|
/* Mark a symbol as deprecated when libbpf version is >= {major}.{minor} */
|
|
#define LIBBPF_DEPRECATED_SINCE(major, minor, msg) \
|
|
__LIBBPF_MARK_DEPRECATED_ ## major ## _ ## minor \
|
|
(LIBBPF_DEPRECATED("libbpf v" # major "." # minor "+: " msg))
|
|
|
|
#define __LIBBPF_CURRENT_VERSION_GEQ(major, minor) \
|
|
(LIBBPF_MAJOR_VERSION > (major) || \
|
|
(LIBBPF_MAJOR_VERSION == (major) && LIBBPF_MINOR_VERSION >= (minor)))
|
|
|
|
/* Add checks for other versions below when planning deprecation of API symbols
|
|
* with the LIBBPF_DEPRECATED_SINCE macro.
|
|
*/
|
|
#if __LIBBPF_CURRENT_VERSION_GEQ(1, 0)
|
|
#define __LIBBPF_MARK_DEPRECATED_1_0(X) X
|
|
#else
|
|
#define __LIBBPF_MARK_DEPRECATED_1_0(X)
|
|
#endif
|
|
|
|
/* This set of internal macros allows to do "function overloading" based on
|
|
* number of arguments provided by used in backwards-compatible way during the
|
|
* transition to libbpf 1.0
|
|
* It's ugly but necessary evil that will be cleaned up when we get to 1.0.
|
|
* See bpf_prog_load() overload for example.
|
|
*/
|
|
#define ___libbpf_cat(A, B) A ## B
|
|
#define ___libbpf_select(NAME, NUM) ___libbpf_cat(NAME, NUM)
|
|
#define ___libbpf_nth(_1, _2, _3, _4, _5, _6, N, ...) N
|
|
#define ___libbpf_cnt(...) ___libbpf_nth(__VA_ARGS__, 6, 5, 4, 3, 2, 1)
|
|
#define ___libbpf_overload(NAME, ...) ___libbpf_select(NAME, ___libbpf_cnt(__VA_ARGS__))(__VA_ARGS__)
|
|
|
|
/* Helper macro to declare and initialize libbpf options struct
|
|
*
|
|
* This dance with uninitialized declaration, followed by memset to zero,
|
|
* followed by assignment using compound literal syntax is done to preserve
|
|
* ability to use a nice struct field initialization syntax and **hopefully**
|
|
* have all the padding bytes initialized to zero. It's not guaranteed though,
|
|
* when copying literal, that compiler won't copy garbage in literal's padding
|
|
* bytes, but that's the best way I've found and it seems to work in practice.
|
|
*
|
|
* Macro declares opts struct of given type and name, zero-initializes,
|
|
* including any extra padding, it with memset() and then assigns initial
|
|
* values provided by users in struct initializer-syntax as varargs.
|
|
*/
|
|
#define LIBBPF_OPTS(TYPE, NAME, ...) \
|
|
struct TYPE NAME = ({ \
|
|
memset(&NAME, 0, sizeof(struct TYPE)); \
|
|
(struct TYPE) { \
|
|
.sz = sizeof(struct TYPE), \
|
|
__VA_ARGS__ \
|
|
}; \
|
|
})
|
|
|
|
/* Helper macro to clear and optionally reinitialize libbpf options struct
|
|
*
|
|
* Small helper macro to reset all fields and to reinitialize the common
|
|
* structure size member. Values provided by users in struct initializer-
|
|
* syntax as varargs can be provided as well to reinitialize options struct
|
|
* specific members.
|
|
*/
|
|
#define LIBBPF_OPTS_RESET(NAME, ...) \
|
|
do { \
|
|
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 */
|