Skip to contentSkip to conversation

libbpf: Fix potential uninitialized tail padding with LIBBPF_OPTS_RESET

PATCH Superseded Yonghong Song 4 participants 7 messages Nov 7, 2023

Subsystems (2)
BPF libbpf

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Yonghong Song

author reviewer
#0 lore raw

PATCH v2 1/1 libbpf: Fix potential uninitialized tail padding with LIBBPF_OPTS_RESET

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 <[email protected]>
Signed-off-by Yonghong Song <[email protected]>
---
 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/[email protected]/

tools/lib/bpf/libbpf_common.h +8 -5
index b7060f254486..ef14e99bc952 100644
@@ -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 */
Signature · 1 line
2.34.1
#1 lore raw
Quoted 123 lines · Yonghong Song

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)
Quoted 8 lines

; 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):

tools/lib/bpf/libbpf_common.h +2 -5
index b7060f254486..c89d4dbbebd8 100644
@@ -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

Quoted 70 lines

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 <[email protected]>
Signed-off-by Yonghong Song <[email protected]>
---
 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/[email protected]/

tools/lib/bpf/libbpf_common.h +8 -5
index b7060f254486..ef14e99bc952 100644
@@ -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 */
Signature · 1 line
2.34.1
author reviewer
#2 lore raw
Quoted 142 lines · Jiri Olsa
Quoted 125 lines · Yonghong Song

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);
Quoted 6 lines

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):

tools/lib/bpf/libbpf_common.h +0 -0
index b7060f254486..c89d4dbbebd8 100644
@@ -79,11 +79,8 @@
   */
  #define LIBBPF_OPTS_RESET(NAME, ...)					    \
  	do {								    \

tools/lib/bpf/libbpf_common.h line 86

Quoted 10 lines
tools/lib/bpf/libbpf_common.h +2 -5
-		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).

Quoted 73 lines

jirka

Quoted 69 lines

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 <[email protected]>
Signed-off-by Yonghong Song <[email protected]>
---
  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/[email protected]/
tools/lib/bpf/libbpf_common.h +8 -5
index b7060f254486..ef14e99bc952 100644
@@ -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 */
Signature · 1 line
2.34.1
#3 lore raw
In reply to Yonghong Song
Quoted 185 lines · Yonghong Song

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 <[email protected]>
Signed-off-by Yonghong Song <[email protected]>
---
 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/[email protected]/

tools/lib/bpf/libbpf_common.h +0 -3
index b7060f254486..ef14e99bc952 100644
@@ -79,11 +79,14 @@
  */
 #define LIBBPF_OPTS_RESET(NAME, ...)                                       \
        do {                                                                \
-               memset(&NAME, 0, sizeof(NAME));                             \
-               NAME = (typeof(NAME)) {                                     \
-                       .sz = sizeof(NAME),                                 \

tools/lib/bpf/libbpf_common.h line 89

Quoted 10 lines
tools/lib/bpf/libbpf_common.h +8 -2
-                       __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?

Quoted 6 lines
tools/lib/bpf/libbpf_common.h +0 -1
        } while (0)
 
 #endif /* __LIBBPF_LIBBPF_COMMON_H */
--

2.34.1

author reviewer
#4 lore raw
Quoted 198 lines · Andrii Nakryiko
Quoted 184 lines · Yonghong Song

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 <[email protected]>
Signed-off-by Yonghong Song <[email protected]>
---
  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/[email protected]/
tools/lib/bpf/libbpf_common.h +0 -3
index b7060f254486..ef14e99bc952 100644
@@ -79,11 +79,14 @@
   */
  #define LIBBPF_OPTS_RESET(NAME, ...)                                       \
         do {                                                                \
-               memset(&NAME, 0, sizeof(NAME));                             \
-               NAME = (typeof(NAME)) {                                     \
-                       .sz = sizeof(NAME),                                 \

tools/lib/bpf/libbpf_common.h line 89

Quoted 10 lines
tools/lib/bpf/libbpf_common.h +8 -2
-                       __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.

Quoted 2 lines

Just one minor nit: can you please simplify sizeof(typeof(NAME)) -> sizeof(NAME), it should work without typeof, right?

Right. Weill send v3 soon.

Quoted 7 lines
Quoted 7 lines
tools/lib/bpf/libbpf_common.h +0 -1
         } while (0)
 
  #endif /* __LIBBPF_LIBBPF_COMMON_H */
--

2.34.1

reviewer Tested-by on v2 1/1
#5 lore raw
In reply to Yonghong Song
Quoted 66 lines · Yonghong Song

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;
    };
Quoted 2 lines

-#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 <[email protected]>
#6 lore raw
Quoted 71 lines · Martin KaFai Lau
Quoted 66 lines · Yonghong Song

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;
    };
Quoted 2 lines

-#define bpf_netkit_opts__last_field expected_revision +#define bpf_netkit_opts__last_field flags

Quoted 3 lines

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.

Quoted 5 lines

Ran the test in a loop. It resolved the issue.

Tested-by Martin KaFai Lau <[email protected]>