[PATCH iptables 1/2] xtables: fix compilation with musl

[PATCH iptables 1/2] xtables: fix compilation with musl

[Nick Hainke]

Only include <linux/if_ether.h> if glibc is used.

Fixes errors in the form of:
In file included from /home/nick/openwrt/staging_dir/toolchain-aarch64_cortex-a53_gcc-11.2.0_musl/include/netinet/ether.h:8,
                 from xtables.c:2238:
/home/nick/openwrt/staging_dir/toolchain-aarch64_cortex-a53_gcc-11.2.0_musl/include/netinet/if_ether.h:115:8: error: redefinition of 'struct ethhdr'
  115 | struct ethhdr {
      |        ^~~~~~
In file included from xtables.c:48:
/home/nick/openwrt/build_dir/target-aarch64_cortex-a53_musl/linux-mediatek_mt7622/linux-5.15.38/user_headers/include/linux/if_ether.h:168:8: note: originally defined here
  168 | struct ethhdr {
      |        ^~~~~~
make[5]: *** [Makefile:471: libxtables_la-xtables.lo] Error 1

Signed-off-by: Nick Hainke <vincent@systemli.org>
---
 libxtables/xtables.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libxtables/xtables.c b/libxtables/xtables.c
index 96fd783a..1eb22209 100644
--- a/libxtables/xtables.c
+++ b/libxtables/xtables.c
@@ -45,7 +45,9 @@
 
 #include <xtables.h>
 #include <limits.h> /* INT_MAX in ip_tables.h/ip6_tables.h */
+#if defined(__GLIBC__)
 #include <linux/if_ether.h> /* ETH_ALEN */
+#endif
 #include <linux/netfilter_ipv4/ip_tables.h>
 #include <linux/netfilter_ipv6/ip6_tables.h>
 #include <libiptc/libxtc.h>
-- 
2.36.1

[PATCH iptables 2/2] xshared: fix compilation with musl

[Nick Hainke]

Gcc complains about missing types. Include <sys/types.h> to fix it.

Fixes errors in the form of:
In file included from xtables-legacy-multi.c:5:
xshared.h:83:56: error: unknown type name 'u_int16_t'; did you mean 'uint16_t'?
   83 | set_option(unsigned int *options, unsigned int option, u_int16_t *invflg,
      |                                                        ^~~~~~~~~
      |                                                        uint16_t
make[6]: *** [Makefile:712: xtables_legacy_multi-xtables-legacy-multi.o] Error 1

Signed-off-by: Nick Hainke <vincent@systemli.org>
---
 iptables/xshared.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/iptables/xshared.h b/iptables/xshared.h
index 14568bb0..9d2fef90 100644
--- a/iptables/xshared.h
+++ b/iptables/xshared.h
@@ -4,6 +4,7 @@
 #include <limits.h>
 #include <stdbool.h>
 #include <stdint.h>
+#include <sys/types.h>
 #include <netinet/in.h>
 #include <net/if.h>
 #include <linux/netfilter_arp/arp_tables.h>
-- 
2.36.1

Re: [PATCH iptables 1/2] xtables: fix compilation with musl

[Phil Sutter]

Hi,

On Sat, May 14, 2022 at 06:33:24PM +0200, Nick Hainke wrote:
> Only include <linux/if_ether.h> if glibc is used.

This looks like a bug in musl? OTOH explicit include of linux/if_ether.h
was added in commit c5d9a723b5159 ("fix build for missing ETH_ALEN
definition"), despite netinet/ether.h being included in line 2248 of
libxtables/xtables.c. So maybe *also* a bug in bionic?!

Cheers, Phil

Re: [PATCH iptables 2/2] xshared: fix compilation with musl

[Phil Sutter]

On Sat, May 14, 2022 at 06:33:25PM +0200, Nick Hainke wrote:
> Gcc complains about missing types. Include <sys/types.h> to fix it.
> 
> Fixes errors in the form of:
> In file included from xtables-legacy-multi.c:5:
> xshared.h:83:56: error: unknown type name 'u_int16_t'; did you mean 'uint16_t'?
>    83 | set_option(unsigned int *options, unsigned int option, u_int16_t *invflg,
>       |                                                        ^~~~~~~~~
>       |                                                        uint16_t
> make[6]: *** [Makefile:712: xtables_legacy_multi-xtables-legacy-multi.o] Error 1

Does it work if you change the type to uint16_t instead? This looks like
fixing for a typo in f647f61f273a1 ("xtables: Make invflags 16bit wide")
which I didn't notice.

Thanks, Phil

Re: [PATCH iptables 1/2] xtables: fix compilation with musl

[Maciej Żenczykowski]

On Sat, May 14, 2022 at 10:04 AM Phil Sutter <phil@nwl.cc> wrote:
> On Sat, May 14, 2022 at 06:33:24PM +0200, Nick Hainke wrote:
> > Only include <linux/if_ether.h> if glibc is used.
>
> This looks like a bug in musl? OTOH explicit include of linux/if_ether.h
> was added in commit c5d9a723b5159 ("fix build for missing ETH_ALEN
> definition"), despite netinet/ether.h being included in line 2248 of
> libxtables/xtables.c. So maybe *also* a bug in bionic?!

You stripped the email you're replying to, and while I'm on lkml and
netdev - with my personal account - I'm not (apparently) subscribed to
netfilter-devel (or I'm not subscribed from work account).
Either way, if my search-fu is correct you're replying to
https://marc.info/?l=netfilter-devel&m=165254651011397&w=2

+#if defined(__GLIBC__)
 #include <linux/if_ether.h> /* ETH_ALEN */
+#endif

and you're presumably CC'ing me due to

https://git.netfilter.org/iptables/commit/libxtables/xtables.c?id=c5d9a723b5159a28f547b577711787295a14fd84

which added the include in the first place...:

fix build for missing ETH_ALEN definition
(this is needed at least with bionic)

+#include <linux/if_ether.h> /* ETH_ALEN */

Based on the above, clearly adding an 'if defined GLIBC' wrapper will
break bionic...
and presumably glibc doesn't care whether the #include is done one way
or the other?

Obviously it could be '#if !defined MUSL' instead...

As for the fix?  And whether glibc or musl or bionic are wrong or not...
Utterly uncertain...

Though, I will point out #include's 2000 lines into a .c file are kind of funky.

Ultimately I find
https://android.git.corp.google.com/platform/external/iptables/+/7608e136bd495fe734ad18a6897dd4425e1a633b%5E%21/

+#ifdef __BIONIC__
+#include <linux/if_ether.h> /* ETH_ALEN */
+#endif

which is in aosp to this day... see:
https://android.git.corp.google.com/platform/external/iptables/+/refs/heads/master/libxtables/xtables.c#48

If I revert that (ie. remove the above 3 lines), then aosp compilation fails:

external/iptables/libxtables/xtables.c:2144:45: error: use of
undeclared identifier 'ETH_ALEN'
static const unsigned char mac_type_unicast[ETH_ALEN] =   {};
                                            ^
...etc...

which suggests the "#include <netinet/ether.h>" immediately before
that isn't sufficient.

I think that should include

https://cs.android.com/android/platform/superproject/+/master:bionic/libc/include/netinet/ether.h

which should #include <netinet/if_ether.h> which is

https://cs.android.com/android/platform/superproject/+/master:bionic/libc/include/netinet/if_ether.h

which should #include <linux/if_ether.h>

but... only #if defined(__USE_BSD)

is the problem perhaps the lack of __USE_BSD?

And indeed defining __USE_BSD just before the include:

+#define __USE_BSD 1
 #include <netinet/ether.h>

fixes the build on aosp (with the 3 lines still removed).

But I have no idea if we should or should not #define __USE_BSD ...
And who / what is actually wrong...

Re: [PATCH iptables 1/2] xtables: fix compilation with musl

[Phil Sutter]

Hi Maciej,

On Sat, May 14, 2022 at 12:14:27PM -0700, Maciej Żenczykowski wrote:
> On Sat, May 14, 2022 at 10:04 AM Phil Sutter <phil@nwl.cc> wrote:
> > On Sat, May 14, 2022 at 06:33:24PM +0200, Nick Hainke wrote:
> > > Only include <linux/if_ether.h> if glibc is used.
> >
> > This looks like a bug in musl? OTOH explicit include of linux/if_ether.h
> > was added in commit c5d9a723b5159 ("fix build for missing ETH_ALEN
> > definition"), despite netinet/ether.h being included in line 2248 of
> > libxtables/xtables.c. So maybe *also* a bug in bionic?!
> 
> You stripped the email you're replying to, and while I'm on lkml and
> netdev - with my personal account - I'm not (apparently) subscribed to
> netfilter-devel (or I'm not subscribed from work account).

Oh, sorry for the caused inconvenience.

> Either way, if my search-fu is correct you're replying to
> https://marc.info/?l=netfilter-devel&m=165254651011397&w=2
> 
> +#if defined(__GLIBC__)
>  #include <linux/if_ether.h> /* ETH_ALEN */
> +#endif
> 
> and you're presumably CC'ing me due to
> 
> https://git.netfilter.org/iptables/commit/libxtables/xtables.c?id=c5d9a723b5159a28f547b577711787295a14fd84
> 
> which added the include in the first place...:

That's correct. I assumed that you added the include for a reason and
it's breaking Nick's use-case, the two of you want to have a word with
each other. :)

> fix build for missing ETH_ALEN definition
> (this is needed at least with bionic)
> 
> +#include <linux/if_ether.h> /* ETH_ALEN */
> 
> Based on the above, clearly adding an 'if defined GLIBC' wrapper will
> break bionic...
> and presumably glibc doesn't care whether the #include is done one way
> or the other?

With glibc, netinet/ether.h includes netinet/if_ether.h which in turn
includes linux/if_ether.h where finally ETH_ALEN is defined.

In xtables.c we definitely need netinet/ether.h for ether_aton()
declaration.

> Obviously it could be '#if !defined MUSL' instead...

Could ...

> As for the fix?  And whether glibc or musl or bionic are wrong or not...
> Utterly uncertain...
> 
> Though, I will point out #include's 2000 lines into a .c file are kind of funky.

ACK!

> Ultimately I find
> https://android.git.corp.google.com/platform/external/iptables/+/7608e136bd495fe734ad18a6897dd4425e1a633b%5E%21/
> 
> +#ifdef __BIONIC__
> +#include <linux/if_ether.h> /* ETH_ALEN */
> +#endif

While I think musl not catching the "double" include is a bug, I'd
prefer the ifdef __BIONIC__ solution since it started the "but my libc
needs this" game.

Nick, if the above change fixes musl builds for you, would you mind
submitting it formally along with a move of the netinet/ether.h include
from mid-file to top?

Thanks, Phil

Re: [PATCH iptables 1/2] xtables: fix compilation with musl

[Maciej Żenczykowski]

On Sun, May 15, 2022 at 5:05 AM Phil Sutter <phil@nwl.cc> wrote:
>
> Hi Maciej,
>
> On Sat, May 14, 2022 at 12:14:27PM -0700, Maciej Żenczykowski wrote:
> > On Sat, May 14, 2022 at 10:04 AM Phil Sutter <phil@nwl.cc> wrote:
> > > On Sat, May 14, 2022 at 06:33:24PM +0200, Nick Hainke wrote:
> > > > Only include <linux/if_ether.h> if glibc is used.
> > >
> > > This looks like a bug in musl? OTOH explicit include of linux/if_ether.h
> > > was added in commit c5d9a723b5159 ("fix build for missing ETH_ALEN
> > > definition"), despite netinet/ether.h being included in line 2248 of
> > > libxtables/xtables.c. So maybe *also* a bug in bionic?!
> >
> > You stripped the email you're replying to, and while I'm on lkml and
> > netdev - with my personal account - I'm not (apparently) subscribed to
> > netfilter-devel (or I'm not subscribed from work account).
>
> Oh, sorry for the caused inconvenience.
>
> > Either way, if my search-fu is correct you're replying to
> > https://marc.info/?l=netfilter-devel&m=165254651011397&w=2
> >
> > +#if defined(__GLIBC__)
> >  #include <linux/if_ether.h> /* ETH_ALEN */
> > +#endif
> >
> > and you're presumably CC'ing me due to
> >
> > https://git.netfilter.org/iptables/commit/libxtables/xtables.c?id=c5d9a723b5159a28f547b577711787295a14fd84
> >
> > which added the include in the first place...:
>
> That's correct. I assumed that you added the include for a reason and
> it's breaking Nick's use-case, the two of you want to have a word with
> each other. :)
>
> > fix build for missing ETH_ALEN definition
> > (this is needed at least with bionic)
> >
> > +#include <linux/if_ether.h> /* ETH_ALEN */
> >
> > Based on the above, clearly adding an 'if defined GLIBC' wrapper will
> > break bionic...
> > and presumably glibc doesn't care whether the #include is done one way
> > or the other?
>
> With glibc, netinet/ether.h includes netinet/if_ether.h which in turn
> includes linux/if_ether.h where finally ETH_ALEN is defined.
>
> In xtables.c we definitely need netinet/ether.h for ether_aton()
> declaration.
>
> > Obviously it could be '#if !defined MUSL' instead...
>
> Could ...
>
> > As for the fix?  And whether glibc or musl or bionic are wrong or not...
> > Utterly uncertain...
> >
> > Though, I will point out #include's 2000 lines into a .c file are kind of funky.
>
> ACK!
>
> > Ultimately I find
> > https://android.git.corp.google.com/platform/external/iptables/+/7608e136bd495fe734ad18a6897dd4425e1a633b%5E%21/
> >
> > +#ifdef __BIONIC__
> > +#include <linux/if_ether.h> /* ETH_ALEN */
> > +#endif
>
> While I think musl not catching the "double" include is a bug, I'd
> prefer the ifdef __BIONIC__ solution since it started the "but my libc
> needs this" game.
>
> Nick, if the above change fixes musl builds for you, would you mind
> submitting it formally along with a move of the netinet/ether.h include
> from mid-file to top?
>
> Thanks, Phil

Any thoughts about the rest of my email - wrt. #define __USE_BSD
- do you know how that is supposed to work?

Re: [PATCH iptables 1/2] xtables: fix compilation with musl

[Florian Westphal]

Phil Sutter <phil@nwl.cc> wrote:
> > fix build for missing ETH_ALEN definition
> > (this is needed at least with bionic)
> > 
> > +#include <linux/if_ether.h> /* ETH_ALEN */
> > 
> > Based on the above, clearly adding an 'if defined GLIBC' wrapper will
> > break bionic...
> > and presumably glibc doesn't care whether the #include is done one way
> > or the other?
> 
> With glibc, netinet/ether.h includes netinet/if_ether.h which in turn
> includes linux/if_ether.h where finally ETH_ALEN is defined.
> 
> In xtables.c we definitely need netinet/ether.h for ether_aton()
> declaration.

Or we hand-roll a xt_ether_aton and add XT_ETH_ALEN to avoid
this include.

Probably easier to maintain than to add all these ifdefs?

Re: [PATCH iptables 1/2] xtables: fix compilation with musl

[Maciej Żenczykowski]

On Sun, May 15, 2022 at 7:09 AM Florian Westphal <fw@strlen.de> wrote:
>
> Phil Sutter <phil@nwl.cc> wrote:
> > > fix build for missing ETH_ALEN definition
> > > (this is needed at least with bionic)
> > >
> > > +#include <linux/if_ether.h> /* ETH_ALEN */
> > >
> > > Based on the above, clearly adding an 'if defined GLIBC' wrapper will
> > > break bionic...
> > > and presumably glibc doesn't care whether the #include is done one way
> > > or the other?
> >
> > With glibc, netinet/ether.h includes netinet/if_ether.h which in turn
> > includes linux/if_ether.h where finally ETH_ALEN is defined.
> >
> > In xtables.c we definitely need netinet/ether.h for ether_aton()
> > declaration.
>
> Or we hand-roll a xt_ether_aton and add XT_ETH_ALEN to avoid
> this include.
>
> Probably easier to maintain than to add all these ifdefs?

or even simply replace both the #include's with
#ifndef ETH_ALEN
#define ETH_ALEN 6
#endif

[PATCH] treewide: use uint* instead of u_int*

[vincent]

From: Nick Hainke <vincent@systemli.org>

Gcc complains about missing types. Two commits introduced u_int* instead
of uint*. Use uint treewide.

Fixes errors in the form of:
In file included from xtables-legacy-multi.c:5:
xshared.h:83:56: error: unknown type name 'u_int16_t'; did you mean 'uint16_t'?
    83 | set_option(unsigned int *options, unsigned int option, u_int16_t *invflg,
        |                                                        ^~~~~~~~~
        |                                                        uint16_t
make[6]: *** [Makefile:712: xtables_legacy_multi-xtables-legacy-multi.o] Error 1

Fixes: c8f28cc8b841 ("extensions: libxt_conntrack: add support for
                      specifying port ranges")
Fixes: f647f61f273a ("xtables: Make invflags 16bit wide")

Signed-off-by: Nick Hainke <vincent@systemli.org>
---
 extensions/libxt_conntrack.c | 2 +-
 iptables/xshared.c           | 2 +-
 iptables/xshared.h           | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/extensions/libxt_conntrack.c b/extensions/libxt_conntrack.c
index 64018ce1..234085c5 100644
--- a/extensions/libxt_conntrack.c
+++ b/extensions/libxt_conntrack.c
@@ -778,7 +778,7 @@ matchinfo_print(const void *ip, const struct xt_entry_match *match, int numeric,
 
 static void
 conntrack_dump_ports(const char *prefix, const char *opt,
-		     u_int16_t port_low, u_int16_t port_high)
+		     uint16_t port_low, uint16_t port_high)
 {
 	if (port_high == 0 || port_low == port_high)
 		printf(" %s%s %u", prefix, opt, port_low);
diff --git a/iptables/xshared.c b/iptables/xshared.c
index a8512d38..9b5e5b5b 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -1025,7 +1025,7 @@ static const int inverse_for_options[NUMBER_OF_OPT] =
 };
 
 void
-set_option(unsigned int *options, unsigned int option, u_int16_t *invflg,
+set_option(unsigned int *options, unsigned int option, uint16_t *invflg,
 	   bool invert)
 {
 	if (*options & option)
diff --git a/iptables/xshared.h b/iptables/xshared.h
index 14568bb0..f8212988 100644
--- a/iptables/xshared.h
+++ b/iptables/xshared.h
@@ -80,7 +80,7 @@ struct xtables_target;
 #define IPT_INV_ARPHRD		0x0800
 
 void
-set_option(unsigned int *options, unsigned int option, u_int16_t *invflg,
+set_option(unsigned int *options, unsigned int option, uint16_t *invflg,
 	   bool invert);
 
 /**
-- 
2.36.1

Re: [PATCH iptables 1/2] xtables: fix compilation with musl

[Nick]

>> Ultimately I find
>> https://android.git.corp.google.com/platform/external/iptables/+/7608e136bd495fe734ad18a6897dd4425e1a633b%5E%21/
>>
>> +#ifdef __BIONIC__
>> +#include <linux/if_ether.h> /* ETH_ALEN */
>> +#endif
> While I think musl not catching the "double" include is a bug, I'd
> prefer the ifdef __BIONIC__ solution since it started the "but my libc
> needs this" game.
>
> Nick, if the above change fixes musl builds for you, would you mind
> submitting it formally along with a move of the netinet/ether.h include
> from mid-file to top?
I will test again. :) However, I can not open the 
"android.git.corp.google.com"? Can you maybe link (also for reference) 
to a freely available source?

Bests
Nick

Re: [PATCH iptables 1/2] xtables: fix compilation with musl

[Maciej Żenczykowski]

On Sun, May 15, 2022 at 11:53 PM Nick <vincent@systemli.org> wrote:
>
>
> >> Ultimately I find
> >> https://android.git.corp.google.com/platform/external/iptables/+/7608e136bd495fe734ad18a6897dd4425e1a633b%5E%21/
> >>
> >> +#ifdef __BIONIC__
> >> +#include <linux/if_ether.h> /* ETH_ALEN */
> >> +#endif
> > While I think musl not catching the "double" include is a bug, I'd
> > prefer the ifdef __BIONIC__ solution since it started the "but my libc
> > needs this" game.
> >
> > Nick, if the above change fixes musl builds for you, would you mind
> > submitting it formally along with a move of the netinet/ether.h include
> > from mid-file to top?
> I will test again. :) However, I can not open the
> "android.git.corp.google.com"? Can you maybe link (also for reference)
> to a freely available source?

Try https://cs.android.com/android/_/android/platform/external/iptables/+/7608e136bd495fe734ad18a6897dd4425e1a633b

Re: [PATCH] treewide: use uint* instead of u_int*

[Jan Engelhardt]

On Monday 2022-05-16 08:47, vincent@systemli.org wrote:

>From: Nick Hainke <vincent@systemli.org>
>
>Gcc complains about missing types. Two commits introduced u_int* instead
>of uint*. Use uint treewide.

I approve of this.

There are, however, a few more instances of u_int* in the source tree, they
could be fixed up in the same go.
> extensions/libxt_conntrack.c | 2 +-
> iptables/xshared.c           | 2 +-
> iptables/xshared.h           | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)

[PATCH] treewide: use uint* instead of u_int*

[vincent]

From: Nick Hainke <vincent@systemli.org>

Gcc complains about missing types. Some commits introduced u_int* instead
of uint*. Use uint treewide.

Fixes errors in the form of:
In file included from xtables-legacy-multi.c:5:
xshared.h:83:56: error: unknown type name 'u_int16_t'; did you mean 'uint16_t'?
    83 | set_option(unsigned int *options, unsigned int option, u_int16_t *invflg,
        |                                                        ^~~~~~~~~
        |                                                        uint16_t
make[6]: *** [Makefile:712: xtables_legacy_multi-xtables-legacy-multi.o] Error 1

Signed-off-by: Nick Hainke <vincent@systemli.org>
---
 extensions/libxt_conntrack.c              | 2 +-
 include/libipq/libipq.h                   | 6 +++---
 include/libiptc/libxtc.h                  | 2 +-
 include/linux/netfilter_arp/arpt_mangle.h | 2 +-
 iptables/xshared.c                        | 2 +-
 iptables/xshared.h                        | 2 +-
 libipq/ipq_create_handle.3                | 2 +-
 libipq/ipq_set_mode.3                     | 2 +-
 8 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/extensions/libxt_conntrack.c b/extensions/libxt_conntrack.c
index 64018ce1..234085c5 100644
--- a/extensions/libxt_conntrack.c
+++ b/extensions/libxt_conntrack.c
@@ -778,7 +778,7 @@ matchinfo_print(const void *ip, const struct xt_entry_match *match, int numeric,
 
 static void
 conntrack_dump_ports(const char *prefix, const char *opt,
-		     u_int16_t port_low, u_int16_t port_high)
+		     uint16_t port_low, uint16_t port_high)
 {
 	if (port_high == 0 || port_low == port_high)
 		printf(" %s%s %u", prefix, opt, port_low);
diff --git a/include/libipq/libipq.h b/include/libipq/libipq.h
index 3cd13292..48c368f5 100644
--- a/include/libipq/libipq.h
+++ b/include/libipq/libipq.h
@@ -48,19 +48,19 @@ typedef unsigned long ipq_id_t;
 struct ipq_handle
 {
 	int fd;
-	u_int8_t blocking;
+	uint8_t blocking;
 	struct sockaddr_nl local;
 	struct sockaddr_nl peer;
 };
 
-struct ipq_handle *ipq_create_handle(u_int32_t flags, u_int32_t protocol);
+struct ipq_handle *ipq_create_handle(uint32_t flags, uint32_t protocol);
 
 int ipq_destroy_handle(struct ipq_handle *h);
 
 ssize_t ipq_read(const struct ipq_handle *h,
                 unsigned char *buf, size_t len, int timeout);
 
-int ipq_set_mode(const struct ipq_handle *h, u_int8_t mode, size_t len);
+int ipq_set_mode(const struct ipq_handle *h, uint8_t mode, size_t len);
 
 ipq_packet_msg_t *ipq_get_packet(const unsigned char *buf);
 
diff --git a/include/libiptc/libxtc.h b/include/libiptc/libxtc.h
index 37010188..a1d16ef9 100644
--- a/include/libiptc/libxtc.h
+++ b/include/libiptc/libxtc.h
@@ -10,7 +10,7 @@ extern "C" {
 #endif
 
 #ifndef XT_MIN_ALIGN
-/* xt_entry has pointers and u_int64_t's in it, so if you align to
+/* xt_entry has pointers and uint64_t's in it, so if you align to
    it, you'll also align to any crazy matches and targets someone
    might write */
 #define XT_MIN_ALIGN (__alignof__(struct xt_entry))
diff --git a/include/linux/netfilter_arp/arpt_mangle.h b/include/linux/netfilter_arp/arpt_mangle.h
index 250f5029..f83ad10a 100644
--- a/include/linux/netfilter_arp/arpt_mangle.h
+++ b/include/linux/netfilter_arp/arpt_mangle.h
@@ -13,7 +13,7 @@ struct arpt_mangle
 	union {
 		struct in_addr tgt_ip;
 	} u_t;
-	u_int8_t flags;
+	uint8_t flags;
 	int target;
 };
 
diff --git a/iptables/xshared.c b/iptables/xshared.c
index a8512d38..9b5e5b5b 100644
--- a/iptables/xshared.c
+++ b/iptables/xshared.c
@@ -1025,7 +1025,7 @@ static const int inverse_for_options[NUMBER_OF_OPT] =
 };
 
 void
-set_option(unsigned int *options, unsigned int option, u_int16_t *invflg,
+set_option(unsigned int *options, unsigned int option, uint16_t *invflg,
 	   bool invert)
 {
 	if (*options & option)
diff --git a/iptables/xshared.h b/iptables/xshared.h
index 14568bb0..f8212988 100644
--- a/iptables/xshared.h
+++ b/iptables/xshared.h
@@ -80,7 +80,7 @@ struct xtables_target;
 #define IPT_INV_ARPHRD		0x0800
 
 void
-set_option(unsigned int *options, unsigned int option, u_int16_t *invflg,
+set_option(unsigned int *options, unsigned int option, uint16_t *invflg,
 	   bool invert);
 
 /**
diff --git a/libipq/ipq_create_handle.3 b/libipq/ipq_create_handle.3
index 11ef95c4..ebe46daa 100644
--- a/libipq/ipq_create_handle.3
+++ b/libipq/ipq_create_handle.3
@@ -24,7 +24,7 @@ ipq_create_handle, ipq_destroy_handle \(em create and destroy libipq handles.
 .br
 .B #include <libipq.h>
 .sp
-.BI "struct ipq_handle *ipq_create_handle(u_int32_t " flags ", u_int32_t " protocol ");"
+.BI "struct ipq_handle *ipq_create_handle(uint32_t " flags ", uint32_t " protocol ");"
 .br
 .BI "int ipq_destroy_handle(struct ipq_handle *" h );
 .SH DESCRIPTION
diff --git a/libipq/ipq_set_mode.3 b/libipq/ipq_set_mode.3
index 0edd3c00..e206886c 100644
--- a/libipq/ipq_set_mode.3
+++ b/libipq/ipq_set_mode.3
@@ -24,7 +24,7 @@ ipq_set_mode \(em set the ip_queue queuing mode
 .br
 .B #include <libipq.h>
 .sp
-.BI "int ipq_set_mode(const struct ipq_handle *" h ", u_int8_t " mode ", size_t " range );
+.BI "int ipq_set_mode(const struct ipq_handle *" h ", uint8_t " mode ", size_t " range );
 .SH DESCRIPTION
 The
 .B ipq_set_mode
-- 
2.36.1

[PATCH] xtables: fix compilation with musl

[vincent]

From: Nick Hainke <vincent@systemli.org>

Only include <linux/if_ether.h> if bionic is used.

Fixes errors in the form of:
In file included from /home/nick/openwrt/staging_dir/toolchain-aarch64_cortex-a53_gcc-11.2.0_musl/include/netinet/ether.h:8,
                 from xtables.c:2238:
/home/nick/openwrt/staging_dir/toolchain-aarch64_cortex-a53_gcc-11.2.0_musl/include/netinet/if_ether.h:115:8: error: redefinition of 'struct ethhdr'
  115 | struct ethhdr {
      |        ^~~~~~
In file included from xtables.c:48:
/home/nick/openwrt/build_dir/target-aarch64_cortex-a53_musl/linux-mediatek_mt7622/linux-5.15.38/user_headers/include/linux/if_ether.h:168:8: note: originally defined here
  168 | struct ethhdr {
      |        ^~~~~~
make[5]: *** [Makefile:471: libxtables_la-xtables.lo] Error 1

Signed-off-by: Nick Hainke <vincent@systemli.org>
---
 libxtables/xtables.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libxtables/xtables.c b/libxtables/xtables.c
index 96fd783a..7dd7729d 100644
--- a/libxtables/xtables.c
+++ b/libxtables/xtables.c
@@ -45,7 +45,9 @@
 
 #include <xtables.h>
 #include <limits.h> /* INT_MAX in ip_tables.h/ip6_tables.h */
+#ifdef __BIONIC__
 #include <linux/if_ether.h> /* ETH_ALEN */
+#endif
 #include <linux/netfilter_ipv4/ip_tables.h>
 #include <linux/netfilter_ipv6/ip6_tables.h>
 #include <libiptc/libxtc.h>
-- 
2.36.1

Re: [PATCH] treewide: use uint* instead of u_int*

[Phil Sutter]

On Mon, May 16, 2022 at 06:16:41PM +0200, vincent@systemli.org wrote:
[...]
> diff --git a/include/libipq/libipq.h b/include/libipq/libipq.h
> index 3cd13292..48c368f5 100644
> --- a/include/libipq/libipq.h
> +++ b/include/libipq/libipq.h
> @@ -48,19 +48,19 @@ typedef unsigned long ipq_id_t;
>  struct ipq_handle
>  {
>  	int fd;
> -	u_int8_t blocking;
> +	uint8_t blocking;
>  	struct sockaddr_nl local;
>  	struct sockaddr_nl peer;
>  };
>  
> -struct ipq_handle *ipq_create_handle(u_int32_t flags, u_int32_t protocol);
> +struct ipq_handle *ipq_create_handle(uint32_t flags, uint32_t protocol);

Might this break API compatibility? ABI won't change, but I suppose
users would have to include stdint.h prior to this header. Are we safe
if we change the include from sys/types.h to stdint.h in line 27 of that
file?

[...]
> diff --git a/include/linux/netfilter_arp/arpt_mangle.h b/include/linux/netfilter_arp/arpt_mangle.h
> index 250f5029..f83ad10a 100644
> --- a/include/linux/netfilter_arp/arpt_mangle.h
> +++ b/include/linux/netfilter_arp/arpt_mangle.h
> @@ -13,7 +13,7 @@ struct arpt_mangle
>  	union {
>  		struct in_addr tgt_ip;
>  	} u_t;
> -	u_int8_t flags;
> +	uint8_t flags;
>  	int target;
>  };

This is a kernel-header. The type was changed to __u8 in kernel repo, so
we should use that instead.

Thanks, Phil

Re: [PATCH] treewide: use uint* instead of u_int*

[Jan Engelhardt]

On Tuesday 2022-05-17 10:10, Phil Sutter wrote:
>> +++ b/include/libipq/libipq.h

>> -	u_int8_t blocking;
>> +	uint8_t blocking;
>
>Might this break API compatibility? ABI won't change, but I suppose
>users would have to include stdint.h prior to this header. Are we safe
>if we change the include from sys/types.h to stdint.h in line 27 of that
>file?

Always include what you use, so yeah, libipq.h should include stdint.h.

Re: [PATCH iptables 1/2] xtables: fix compilation with musl

[Phil Sutter]

On Sun, May 15, 2022 at 07:13:27AM -0700, Maciej Żenczykowski wrote:
> On Sun, May 15, 2022 at 7:09 AM Florian Westphal <fw@strlen.de> wrote:
> > Phil Sutter <phil@nwl.cc> wrote:
> > > > fix build for missing ETH_ALEN definition
> > > > (this is needed at least with bionic)
> > > >
> > > > +#include <linux/if_ether.h> /* ETH_ALEN */
> > > >
> > > > Based on the above, clearly adding an 'if defined GLIBC' wrapper will
> > > > break bionic...
> > > > and presumably glibc doesn't care whether the #include is done one way
> > > > or the other?
> > >
> > > With glibc, netinet/ether.h includes netinet/if_ether.h which in turn
> > > includes linux/if_ether.h where finally ETH_ALEN is defined.
> > >
> > > In xtables.c we definitely need netinet/ether.h for ether_aton()
> > > declaration.
> >
> > Or we hand-roll a xt_ether_aton and add XT_ETH_ALEN to avoid
> > this include.
> >
> > Probably easier to maintain than to add all these ifdefs?
> 
> or even simply replace both the #include's with
> #ifndef ETH_ALEN
> #define ETH_ALEN 6
> #endif

If that's sufficient for both musl and bionic, probably the easiest
solution with least potential for surprises.

Cheers, Phil

Re: [PATCH iptables 1/2] xtables: fix compilation with musl

[Phil Sutter]

On Sun, May 15, 2022 at 06:40:51AM -0700, Maciej Żenczykowski wrote:
> On Sun, May 15, 2022 at 5:05 AM Phil Sutter <phil@nwl.cc> wrote:
> >
> > Hi Maciej,
> >
> > On Sat, May 14, 2022 at 12:14:27PM -0700, Maciej Żenczykowski wrote:
> > > On Sat, May 14, 2022 at 10:04 AM Phil Sutter <phil@nwl.cc> wrote:
> > > > On Sat, May 14, 2022 at 06:33:24PM +0200, Nick Hainke wrote:
> > > > > Only include <linux/if_ether.h> if glibc is used.
> > > >
> > > > This looks like a bug in musl? OTOH explicit include of linux/if_ether.h
> > > > was added in commit c5d9a723b5159 ("fix build for missing ETH_ALEN
> > > > definition"), despite netinet/ether.h being included in line 2248 of
> > > > libxtables/xtables.c. So maybe *also* a bug in bionic?!
> > >
> > > You stripped the email you're replying to, and while I'm on lkml and
> > > netdev - with my personal account - I'm not (apparently) subscribed to
> > > netfilter-devel (or I'm not subscribed from work account).
> >
> > Oh, sorry for the caused inconvenience.
> >
> > > Either way, if my search-fu is correct you're replying to
> > > https://marc.info/?l=netfilter-devel&m=165254651011397&w=2
> > >
> > > +#if defined(__GLIBC__)
> > >  #include <linux/if_ether.h> /* ETH_ALEN */
> > > +#endif
> > >
> > > and you're presumably CC'ing me due to
> > >
> > > https://git.netfilter.org/iptables/commit/libxtables/xtables.c?id=c5d9a723b5159a28f547b577711787295a14fd84
> > >
> > > which added the include in the first place...:
> >
> > That's correct. I assumed that you added the include for a reason and
> > it's breaking Nick's use-case, the two of you want to have a word with
> > each other. :)
> >
> > > fix build for missing ETH_ALEN definition
> > > (this is needed at least with bionic)
> > >
> > > +#include <linux/if_ether.h> /* ETH_ALEN */
> > >
> > > Based on the above, clearly adding an 'if defined GLIBC' wrapper will
> > > break bionic...
> > > and presumably glibc doesn't care whether the #include is done one way
> > > or the other?
> >
> > With glibc, netinet/ether.h includes netinet/if_ether.h which in turn
> > includes linux/if_ether.h where finally ETH_ALEN is defined.
> >
> > In xtables.c we definitely need netinet/ether.h for ether_aton()
> > declaration.
> >
> > > Obviously it could be '#if !defined MUSL' instead...
> >
> > Could ...
> >
> > > As for the fix?  And whether glibc or musl or bionic are wrong or not...
> > > Utterly uncertain...
> > >
> > > Though, I will point out #include's 2000 lines into a .c file are kind of funky.
> >
> > ACK!
> >
> > > Ultimately I find
> > > https://android.git.corp.google.com/platform/external/iptables/+/7608e136bd495fe734ad18a6897dd4425e1a633b%5E%21/
> > >
> > > +#ifdef __BIONIC__
> > > +#include <linux/if_ether.h> /* ETH_ALEN */
> > > +#endif
> >
> > While I think musl not catching the "double" include is a bug, I'd
> > prefer the ifdef __BIONIC__ solution since it started the "but my libc
> > needs this" game.
> >
> > Nick, if the above change fixes musl builds for you, would you mind
> > submitting it formally along with a move of the netinet/ether.h include
> > from mid-file to top?
> >
> > Thanks, Phil
> 
> Any thoughts about the rest of my email - wrt. #define __USE_BSD
> - do you know how that is supposed to work?

No, but isn't this a detail of bionic header layout?

Cheers, Phil

Re: [PATCH iptables 1/2] xtables: fix compilation with musl

[Maciej Żenczykowski]

On Tue, May 17, 2022 at 1:17 AM Phil Sutter <phil@nwl.cc> wrote:
>
> On Sun, May 15, 2022 at 06:40:51AM -0700, Maciej Żenczykowski wrote:
> > On Sun, May 15, 2022 at 5:05 AM Phil Sutter <phil@nwl.cc> wrote:
> > >
> > > Hi Maciej,
> > >
> > > On Sat, May 14, 2022 at 12:14:27PM -0700, Maciej Żenczykowski wrote:
> > > > On Sat, May 14, 2022 at 10:04 AM Phil Sutter <phil@nwl.cc> wrote:
> > > > > On Sat, May 14, 2022 at 06:33:24PM +0200, Nick Hainke wrote:
> > > > > > Only include <linux/if_ether.h> if glibc is used.
> > > > >
> > > > > This looks like a bug in musl? OTOH explicit include of linux/if_ether.h
> > > > > was added in commit c5d9a723b5159 ("fix build for missing ETH_ALEN
> > > > > definition"), despite netinet/ether.h being included in line 2248 of
> > > > > libxtables/xtables.c. So maybe *also* a bug in bionic?!
> > > >
> > > > You stripped the email you're replying to, and while I'm on lkml and
> > > > netdev - with my personal account - I'm not (apparently) subscribed to
> > > > netfilter-devel (or I'm not subscribed from work account).
> > >
> > > Oh, sorry for the caused inconvenience.
> > >
> > > > Either way, if my search-fu is correct you're replying to
> > > > https://marc.info/?l=netfilter-devel&m=165254651011397&w=2
> > > >
> > > > +#if defined(__GLIBC__)
> > > >  #include <linux/if_ether.h> /* ETH_ALEN */
> > > > +#endif
> > > >
> > > > and you're presumably CC'ing me due to
> > > >
> > > > https://git.netfilter.org/iptables/commit/libxtables/xtables.c?id=c5d9a723b5159a28f547b577711787295a14fd84
> > > >
> > > > which added the include in the first place...:
> > >
> > > That's correct. I assumed that you added the include for a reason and
> > > it's breaking Nick's use-case, the two of you want to have a word with
> > > each other. :)
> > >
> > > > fix build for missing ETH_ALEN definition
> > > > (this is needed at least with bionic)
> > > >
> > > > +#include <linux/if_ether.h> /* ETH_ALEN */
> > > >
> > > > Based on the above, clearly adding an 'if defined GLIBC' wrapper will
> > > > break bionic...
> > > > and presumably glibc doesn't care whether the #include is done one way
> > > > or the other?
> > >
> > > With glibc, netinet/ether.h includes netinet/if_ether.h which in turn
> > > includes linux/if_ether.h where finally ETH_ALEN is defined.
> > >
> > > In xtables.c we definitely need netinet/ether.h for ether_aton()
> > > declaration.
> > >
> > > > Obviously it could be '#if !defined MUSL' instead...
> > >
> > > Could ...
> > >
> > > > As for the fix?  And whether glibc or musl or bionic are wrong or not...
> > > > Utterly uncertain...
> > > >
> > > > Though, I will point out #include's 2000 lines into a .c file are kind of funky.
> > >
> > > ACK!
> > >
> > > > Ultimately I find
> > > > https://android.git.corp.google.com/platform/external/iptables/+/7608e136bd495fe734ad18a6897dd4425e1a633b%5E%21/
> > > >
> > > > +#ifdef __BIONIC__
> > > > +#include <linux/if_ether.h> /* ETH_ALEN */
> > > > +#endif
> > >
> > > While I think musl not catching the "double" include is a bug, I'd
> > > prefer the ifdef __BIONIC__ solution since it started the "but my libc
> > > needs this" game.
> > >
> > > Nick, if the above change fixes musl builds for you, would you mind
> > > submitting it formally along with a move of the netinet/ether.h include
> > > from mid-file to top?
> > >
> > > Thanks, Phil
> >
> > Any thoughts about the rest of my email - wrt. #define __USE_BSD
> > - do you know how that is supposed to work?
>
> No, but isn't this a detail of bionic header layout?
>
> Cheers, Phil

No idea... is there actually a standard's document somewhere that
actually describes which header file should declare what (and under
what #define conditions)?

Without it, I have absolutely no idea...
glibc also has tons of headers that require #define SOMETHING before
including them in order to get them to declare certain things.

I have no idea why the #ifdef BSD guard is there in bionic... maybe
it's wrong, maybe it's compatible with some other libc header files...
It doesn't really feel like the sort of thing that would be added by
mistake (ie. it feels very intentional)...

But changing bionic (for example to remove the #ifdef), feels scarier,
because it affects all things using bionic (including android app
native code I think), as opposed to just a single file in iptables...

Re: [PATCH] treewide: use uint* instead of u_int*

[Phil Sutter]

On Tue, May 17, 2022 at 10:14:10AM +0200, Jan Engelhardt wrote:
> On Tuesday 2022-05-17 10:10, Phil Sutter wrote:
> >> +++ b/include/libipq/libipq.h
> 
> >> -	u_int8_t blocking;
> >> +	uint8_t blocking;
> >
> >Might this break API compatibility? ABI won't change, but I suppose
> >users would have to include stdint.h prior to this header. Are we safe
> >if we change the include from sys/types.h to stdint.h in line 27 of that
> >file?
> 
> Always include what you use, so yeah, libipq.h should include stdint.h.

Thanks. Patch pushed with the two changes I suggested.

Thanks, Phil

Re: [PATCH] treewide: use uint* instead of u_int*

[Nick]

Thanks for pushing. Sorry, for being absent.

On 5/18/22 15:21, Phil Sutter wrote:
> On Tue, May 17, 2022 at 10:14:10AM +0200, Jan Engelhardt wrote:
>> On Tuesday 2022-05-17 10:10, Phil Sutter wrote:
>>>> +++ b/include/libipq/libipq.h
>>>> -	u_int8_t blocking;
>>>> +	uint8_t blocking;
>>> Might this break API compatibility? ABI won't change, but I suppose
>>> users would have to include stdint.h prior to this header. Are we safe
>>> if we change the include from sys/types.h to stdint.h in line 27 of that
>>> file?
>> Always include what you use, so yeah, libipq.h should include stdint.h.
> Thanks. Patch pushed with the two changes I suggested.
>
> Thanks, Phil