Re: [PATCH nftables] include: fix for musl with iptables v1.8.11

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 3364 bytes --]

Phil Sutter <phil@nwl.cc> writes:

> On Fri, Dec 20, 2024 at 01:07:56PM +0100, Alyssa Ross wrote:
>> On Fri, Dec 20, 2024 at 12:50:42PM +0100, Phil Sutter wrote:
>> > Hi Alyssa,
>> >
>> > On Fri, Dec 20, 2024 at 12:10:02AM +0100, Alyssa Ross wrote:
>> > > Since iptables commit 810f8568 (libxtables: xtoptions: Implement
>> > > XTTYPE_ETHERMACMASK), nftables failed to build for musl libc:
>> > >
>> > > 	In file included from /nix/store/bvffdqfhyxvx66bqlqqdmjmkyklkafv6-musl-1.2.5-dev/include/netinet/et…
>> > > 	                 from /nix/store/kz6fymqpgbrj6330s6wv4idcf9pwsqs4-iptables-1.8.10-de…
>> > > 	                 from src/xt.c:30:
>> > > 	/nix/store/bvffdqfhyxvx66bqlqqdmjmkyklkafv6-musl-1.2.5-dev/include/netinet/if_ether.h:115:8: error: redefinition of 'struct ethhdr'
>> > > 	  115 | struct ethhdr {
>> > > 	      |        ^~~~~~
>> > > 	In file included from ./include/linux/netfilter_bridge.h:8,
>> > > 	                 from ./include/linux/netfilter_bridge/ebtables.h:1,
>> > > 	                 from src/xt.c:27:
>> > > 	/nix/store/bvffdqfhyxvx66bqlqqdmjmkyklkafv6-musl-1.2.5-dev/include/linux/if_ether.h:173:8: note: originally defined here
>> > > 	  173 | struct ethhdr {
>> > > 	      |        ^~~~~~
>> > >
>> > > The fix is to use libc's version of if_ether.h before any kernel
>> > > headers, which takes care of conflicts with the kernel's struct ethhdr
>> > > definition by defining __UAPI_DEF_ETHHDR, which will tell the kernel's
>> > > header not to define its own version.
>> >
>> > What I don't like about this is how musl tries to force projects to not
>> > include linux/if_ether.h directly. From the project's view, this is a
>> > workaround not a fix.
>> 
>> My understanding is that it's a general principle of using any libc on
>> Linux that if there's both a libc and kernel header for the same thing,
>> the libc header should be used.  libc headers will of course include
>> other libc headers in preference to kernel headers, so if you also
>> include the kernel headers you're likely to end up with conflicts.
>> Whether conflicts occur in any particular case depends on how a
>> particular libc chooses to expose a particular kernel API.  I could be
>> misremembering, but I believe the same thing can happen with Glibc —
>> some headers under sys/ cause conflicts with their corresponding kernel
>> headers if both are included.  While this case is musl specific, I
>> think the principle applies to all libcs.
>
> While this may be true for the vast majority of user space programs,
> netfilter tools and libraries are a bit special in how close they
> interface with the kernel. Not all netfilter-related kernel API is
> exposed by glibc, for instance. Including (some) kernel headers is
> therefore unavoidable, and (as your patch shows) order of inclusion
> becomes subtly relevant in ways which won't show when compile-testing
> against glibc only.

Yeah — really it needs to be ensured in review that libc headers are
included before kernel headers, and libc headers are preferred over
kernel headers where both are available.  It's not too much to keep in
mind, but it does need to be taught/learned because the issues it causes
won't always be caught during testing in any particular environment.
The same holds for lots of things in C, though.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]