Hi Pablo,
On Thu, Jan 09, 2025 at 01:03:25AM +0100, Pablo Neira Ayuso wrote:
> On Thu, Jan 09, 2025 at 12:49:53AM +0100, Phil Sutter wrote:
> > On Thu, Jan 09, 2025 at 12:34:01AM +0100, Pablo Neira Ayuso wrote:
> > > On Fri, Dec 20, 2024 at 01:33:42PM +0100, Phil Sutter wrote:
> > > > 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.
> > > >
> > > > > > > Signed-off-by: Alyssa Ross <hi@xxxxxxxxx>
> > > > > > > ---
> > > > > > > A similar fix would solve the problem properly in iptables, which was
> > > > > > > worked around with 76fce228 ("configure: Determine if musl is used for build").
> > > > > > > The __UAPI_DEF_ETHHDR is supposed to be set by netinet/if_ether.h,
> > > > > > > rather than manually by users.
> > > > > >
> > > > > > Why does 76fce228 not work for you?
> > > > >
> > > > > It does work, but that's a fix for iptables. This is a fix for
> > > > > nftables. I could have submitted a copy of the iptables fix, but I
> > > > > don't think it's the best fix due to its reliance on internal macros
> > > > > that are not part of the public interface.
> > > >
> > > > Ah, sorry! Patch subject and description managed to confuse me.
> > > >
> > > > Pablo, what's your opinion? Maybe we should strive for the same solution
> > > > for the problem in all netfilter user space, so either take what we have
> > > > in iptables or adjust iptables to what nftables decides how things
> > > > should be?
> > >
> > > I see, you mean to use this (from iptables):
> > >
> > > commit 76fce22826f8e860b5eb5b5a2463040c17ff85da
> > > Author: Joshua Lant <joshualant@xxxxxxxxxxxxxx>
> > > Date: Wed Aug 28 13:47:31 2024 +0100
> > >
> > > configure: Determine if musl is used for build
> > >
> > > and adapt to use it from nftables (and everywhere else).
> > >
> > > Alyssa's patch is more simple, but it is mangling a cache kernel
> > > header.
> >
> > Right, so the fixing should start in kernel repo, then update the cached
> > header in nftables.
> >
> > > Is this configure.ac workaround needed everywhere in the netfilter.org
> > > trees to make musl happy?
> >
> > AIUI, only in those including linux/if_ether.h directly. ;)
>
> Hm. I am looking at include/uapi/linux/netfilter_bridge.h and...
>
> #include <linux/in.h>
> #include <linux/netfilter.h>
> #include <linux/if_ether.h>
> #include <linux/if_vlan.h>
> #include <linux/if_pppox.h>
>
> I don't understand why all those #include need to be there, this
> header file contains only defines an enumeration... git annotate takes
> me to:
>
> 607ca46e97a1 ("UAPI: (Scripted) Disintegrate include/linux")
>
> Silly question: Does it break any netfilter userspace software if
> #include <linux/if_ether.h> is moved from that kernel header file to
> to the non-uapi netfilter_bridge.h header file.
Silly counter question: Does removing an include statement break UAPI? I
was once told source files shall directly include everything they need
and not depend on includes in headers, but "now you need this extra
header" is not far away from "now you need this other header", right?
Even sillier, is Alyssa's change legal regarding UAPI? All of glibc,
musl and uClibc seem fine, but in theory there might be a libc which has
odd things in netinet/if_ether.h.
BTW: I see only a single UAPI kernel header include from netinet space
(include/uapi/linux/mptcp.h) and it's for compat reasons (06e445f740c1
("mptcp: fix conflict with <netinet/in.h>")). Speaking of which, what if
we added:
| #ifndef __KERNEL__
| #include <netinet/if_ether.h>
| #endif
early into include/uapi/linux/if_ether.h? Aside from any header caches,
this should fix all of user space at once, no?
> > Though there is such include in nftables' src/payload.c which remains
> > unchanged by Alyssa's patch. Not sure if intentional, maybe there are
> > factors which make the include harmless.
>
> This is userspace, I guess we can replace it with userspace netinet/
> header variant.
>
> > > I don't see any better option at this stage.
> >
> > According to a quick grep, there are currently six spots including
> > linux/if_ether.h (including one cached kernel header). I like about the
> > configure hack that it avoids breaking musl by accident via some
> > seemingly unrelated patch. Just one less thing to keep in mind, but this
> > is just me. I appreciate other opinions!
>
> Agreed, avoiding the configure kludge would be good.
OK! I could probably write some git hooks to support my lossy memory.
Cheers, Phil
