Compilation failure in xdpdump with clang: field 'req' with variable sized type 'struct ethtool_link_settings' not at the end of a struct

[hhoffstaette]

Trying to build everything wth clang (15.0.7):

clang -Wall -pipe -O2 -march=native -I/usr/include/bpf/uapi -std=gnu11 -Wextra -Werror -DBPF_DIR_MNT=\"/sys/fs/bpf\" -DBPF_OBJECT_PATH=\"/usr/lib/bpf\" -DMAX_DISPATCHER_ACTIONS=10 -DTOOLS_VERSION=\""1.3.1"\" -DLIBBPF_VERSION=\"1.1.0\" -DRUNDIR=\"/run\" -DHAVE_LIBBPF_PERF_BUFFER__CONSUME -DHAVE_LIBBPF_BTF__LOAD_FROM_KERNEL_BY_ID -DHAVE_LIBBPF_BTF__TYPE_CNT -DHAVE_LIBBPF_BPF_OBJECT__NEXT_MAP -DHAVE_LIBBPF_BPF_OBJECT__NEXT_PROGRAM -DHAVE_LIBBPF_BPF_PROGRAM__INSN_CNT -DHAVE_LIBBPF_BPF_MAP_CREATE -DHAVE_LIBBPF_PERF_BUFFER__NEW_RAW -DHAVE_LIBBPF_BPF_XDP_ATTACH -DHAVE_LIBBPF_BPF_MAP__SET_AUTOCREATE -DHAVE_LIBBPF_BPF_PROG_TEST_RUN_OPTS -DHAVE_SECURE_GETENV -DLIBBPF_DYNAMIC -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -I../lib/../headers -I../lib/util   -Wl,-O1,--as-needed,-z,now -L../lib/libxdp -o xdpdump ../lib/util/params.o ../lib/util/logging.o ../lib/util/util.o ../lib/util/stats.o ../lib/util/xpcapng.o ../lib/util/xdp_sample.o \
 xdpdump.c  -lxdp -lpcap -lm -lbpf
xdpdump.c:195:32: error: field 'req' with variable sized type 'struct ethtool_link_settings' not at the end of a struct
      or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end]
                struct ethtool_link_settings req;
                                             ^
1 error generated.

Indeed switching lines 195<->196 around so that "req" comes last in struct ereq makes it build, however it seems to me that doing so would break the following uses with ioctls?

[tohojo]

Holger Hoffstätte ***@***.***> writes:

Trying to build everything wth clang (15.0.7): ``` clang -Wall -pipe -O2 -march=native -I/usr/include/bpf/uapi -std=gnu11 -Wextra -Werror -DBPF_DIR_MNT=\"/sys/fs/bpf\" -DBPF_OBJECT_PATH=\"/usr/lib/bpf\" -DMAX_DISPATCHER_ACTIONS=10 -DTOOLS_VERSION=\""1.3.1"\" -DLIBBPF_VERSION=\"1.1.0\" -DRUNDIR=\"/run\" -DHAVE_LIBBPF_PERF_BUFFER__CONSUME -DHAVE_LIBBPF_BTF__LOAD_FROM_KERNEL_BY_ID -DHAVE_LIBBPF_BTF__TYPE_CNT -DHAVE_LIBBPF_BPF_OBJECT__NEXT_MAP -DHAVE_LIBBPF_BPF_OBJECT__NEXT_PROGRAM -DHAVE_LIBBPF_BPF_PROGRAM__INSN_CNT -DHAVE_LIBBPF_BPF_MAP_CREATE -DHAVE_LIBBPF_PERF_BUFFER__NEW_RAW -DHAVE_LIBBPF_BPF_XDP_ATTACH -DHAVE_LIBBPF_BPF_MAP__SET_AUTOCREATE -DHAVE_LIBBPF_BPF_PROG_TEST_RUN_OPTS -DHAVE_SECURE_GETENV -DLIBBPF_DYNAMIC -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -I../lib/../headers -I../lib/util -Wl,-O1,--as-needed,-z,now -L../lib/libxdp -o xdpdump ../lib/util/params.o ../lib/util/logging.o ../lib/util/util.o ../lib/util/stats.o ../lib/util/xpcapng.o ../lib/util/xdp_sample.o \ xdpdump.c -lxdp -lpcap -lm -lbpf xdpdump.c:195:32: error: field 'req' with variable sized type 'struct ethtool_link_settings' not at the end of a struct or class is a GNU extension [-Werror,-Wgnu-variable-sized-type-not-at-end] struct ethtool_link_settings req; ^ 1 error generated. ``` Indeed switching lines 195<->196 around so that "req" comes last in `struct ereq` makes it build, however it seems to me that doing so would break the following uses with ioctls?

Yes, I believe so. The use in xdpdump.c is basically the expected usage of this struct (see the comment in ethtool.h). What version of the kernel headers are you using? Kernel 6.0 moved to a flexible-array type definition for that field; dunno if that helps?

[hhoffstaette]

What version of the kernel headers are you using? Kernel 6.0 moved to a flexible-array type definition for that field; dunno if that helps?

I'm building with linux-headers-6.2.

[tohojo]

Holger Hoffstätte ***@***.***> writes:

> What version of the kernel headers are you using? Kernel 6.0 moved to a flexible-array type definition for that field; dunno if that helps? I'm building with linux-headers-6.2.

Right, so that should have that change, then. In which case I don't really know the know how one is supposed to get that to compile with Clang. I guess there are other applications using the ethtool APIs; what do they do?

[hhoffstaette]

I guess there are other applications using the ethtool APIs; what do they do?

All I know is that the open-ended struct cannot be inside another struct (whch it is here) unless it's at the end, because that's just how flexible arrays work since they can be allocated as part of the struct with the appropriate final size.
Let's to deeper!

holger>tar xf /var/cache/distfiles/ethtool-6.2.tar.xz 
holger>cd ethtool-6.2 
holger>./configure CC=clang
..snip..
holger>make 
make  all-am
make[1]: Entering directory '/tmp/ethtool-6.2'
depbase=`echo ethtool.o | sed 's|[^/]*$|.deps/&|;s|\.o$||'`;\
clang -DHAVE_CONFIG_H -I.  -I./uapi   -Wall -Wextra -D_POSIX_C_SOURCE=200809L -g -O2 -MT ethtool.o -MD -MP -MF $depbase.Tpo -c -o ethtool.o ethtool.c &&\
mv -f $depbase.Tpo $depbase.Po
In file included from ethtool.c:32:
./internal.h:168:31: warning: field 'base' with variable sized type 'struct ethtool_link_settings' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
        struct ethtool_link_settings base;
                                     ^
ethtool.c:1733:28: warning: field 'hdr' with variable sized type 'struct ethtool_sset_info' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
                struct ethtool_sset_info hdr;
                                         ^
ethtool.c:2680:32: warning: field 'req' with variable sized type 'struct ethtool_link_settings' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
                struct ethtool_link_settings req;
                                             ^
ethtool.c:2749:32: warning: field 'req' with variable sized type 'struct ethtool_link_settings' not at the end of a struct or class is a GNU extension [-Wgnu-variable-sized-type-not-at-end]
                struct ethtool_link_settings req;
                                             ^

😆

It just fails in xdptools due to -Werror.

[tohojo]

Holger Hoffstätte ***@***.***> writes:

> I guess there are other applications using the ethtool APIs; what do they do? All I know is that the open-ended struct cannot be inside another struct (whch it is here) unless it's at the end, because that's just how flexible arrays work since they can be allocated as part of the struct with the appropriate final size.

Yeah, but this usage is sorta expected: The flexible array is a variable length field because it can refer to different values depending on what you ask for; so you embed it into a struct where the next member is the actual value you want to access from the syscall return. I guess you could add -Wno-gnu-variable-sized-type-not-at-end to the clang build (if that's a thing?)

[hhoffstaette]

Indeed there's exactly "-Wno-gnu-variable-sized-type-not-at-end". I added it into xdp-dump/Makefile (since that's the only affected part) as CFLAGS += "-Wno-gnu-variable-sized-type-not-at-end" and it lets xdp-dump compile. It also seems to work:

$./xdpdump -D
Interface        Prio  Program name      Mode     ID   Tag               Chain actions
--------------------------------------------------------------------------------------
lo                     <No XDP program loaded!>
eth0                   xdp_dispatcher    skb      146  90f686eb86991928 
 =>              50     xdp_homeplug_drop          155  fbd415544de357c1  XDP_PASS
wlan0                  <No XDP program loaded!>

$./xdpdump -i eth0
listening on eth0, ingress XDP program ID 155 func xdp_homeplug_drop, capture mode entry, capture size 262144 bytes
1678289942.303856476: xdp_homeplug_drop()@entry: packet size 60 bytes on if_index 2, rx queue 0, id 1
1678289942.303878767: xdp_homeplug_drop()@entry: packet size 60 bytes on if_index 2, rx queue 0, id 2
<snip>
^C
18 packets captured
0 packets dropped by perf ring

Honestly this all seems dodgy and out-of-bounds-prone to me but such is life. gcc is still the default compiler in Gentoo; building with clang was just to test toolchain portability, and the rest of xdp-tools seems fine with clang.
Do you think it's worthwile to add this to the xdp-dump Makefile? Otherwise I'm not sure if there's much else we can do here.

[tohojo]

Holger Hoffstätte ***@***.***> writes:

Honestly this all seems dodgy and out-of-bounds-prone to me but such is life.

Well, yeah, it's an API design that predates all the flexible array and overflow protection stuff in C. It doesn't *actually* overflow, but the compiler can't prove that, hence the warnings :)

Do you think it's worthwile to add this to the xdp-dump Makefile? Otherwise I'm not sure if there's much else we can do here.

Hmm, on the one hand, this seems like it's better to set in any packaging that enables clang, since it's clang-only. OTOH, as long as it's harmless for GCC, I don't mind adding it in the xdpdump Makefile if you think that's helpful. Feel free to open a PR doing this, but please add a comment explaining why it's there as well, so we can remember in the future...

[hhoffstaette]

I thought about making the assignment conditional based on the value of $CC, but that will probably create more problems than solutions. Let's keep it simple.