This series, partially coming from Chris Kuhn, enables passt to be built against musl. Tested on Alpine (x86_64) 3.17.2, musl 1.2.3-r4. See also: https://bugs.passt.top/show_bug.cgi?id=4 https://github.com/void-linux/void-packages/pull/42517 Chris Kuhn (2): conf, passt: Rename stderr to force_stderr treewide: Fix header includes to build with musl Stefano Brivio (2): netlink: Use 8 KiB * netlink message header size as response buffer util: Carry own definition of __bswap_constant{16,32} conf.c | 8 +++++--- isolation.c | 1 + netlink.c | 16 ++++++++++------ passt.c | 4 +++- passt.h | 4 ++-- tap.c | 1 + tcp.c | 1 + tcp_splice.c | 1 + udp.c | 1 + util.c | 1 + util.h | 11 +++++++++++ 11 files changed, 37 insertions(+), 12 deletions(-) -- 2.39.2
...instead of BUFSIZ. On musl, BUFSIZ is 1024, so we'll typically
truncate the response to the request we send in nl_link(). It's
usually 8192 or more with glibc.
There doesn't seem to be any macro defining the rtnetlink maximum
message size, and iproute2 just hardcodes 1024 * 1024 for the receive
buffer, but the example in netlink(7) makes somewhat sense, looking
at the kernel implementation.
It's not very clean, but we're very unlikely to hit that limit,
and if we do, we'll find out painlessly, because NLA_OK() will tell
us right away.
Reported-by: Chris Kuhn <kuhnchris+passt(a)kuhnchris.eu>
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
netlink.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/netlink.c b/netlink.c
index 8f785ca..0e0be4f 100644
--- a/netlink.c
+++ b/netlink.c
@@ -34,6 +34,8 @@
#include "log.h"
#include "netlink.h"
+#define NLBUFSIZ (8192 * sizeof(struct nlmsghdr)) /* See netlink(7) */
+
/* Socket in init, in target namespace, sequence (just needs to be monotonic) */
static int nl_sock = -1;
static int nl_sock_ns = -1;
@@ -105,7 +107,7 @@ fail:
static int nl_req(int ns, char *buf, const void *req, ssize_t len)
{
int s = ns ? nl_sock_ns : nl_sock, done = 0;
- char flush[BUFSIZ];
+ char flush[NLBUFSIZ];
ssize_t n;
while (!done && (n = recv(s, flush, sizeof(flush), MSG_DONTWAIT)) > 0) {
@@ -121,7 +123,8 @@ static int nl_req(int ns, char *buf, const void *req, ssize_t len)
}
}
- if ((send(s, req, len, 0) < len) || (len = recv(s, buf, BUFSIZ, 0)) < 0)
+ if ((send(s, req, len, 0) < len) ||
+ (len = recv(s, buf, NLBUFSIZ, 0)) < 0)
return -errno;
return len;
@@ -149,7 +152,7 @@ unsigned int nl_get_ext_if(sa_family_t af)
};
struct nlmsghdr *nh;
struct rtattr *rta;
- char buf[BUFSIZ];
+ char buf[NLBUFSIZ];
ssize_t n;
size_t na;
@@ -227,7 +230,7 @@ void nl_route(int ns, unsigned int ifi, sa_family_t af, void *gw)
struct nlmsghdr *nh;
struct rtattr *rta;
struct rtmsg *rtm;
- char buf[BUFSIZ];
+ char buf[NLBUFSIZ];
ssize_t n;
size_t na;
@@ -336,7 +339,7 @@ void nl_addr(int ns, unsigned int ifi, sa_family_t af,
struct ifaddrmsg *ifa;
struct nlmsghdr *nh;
struct rtattr *rta;
- char buf[BUFSIZ];
+ char buf[NLBUFSIZ];
ssize_t n;
size_t na;
@@ -446,7 +449,7 @@ void nl_link(int ns, unsigned int ifi, void *mac, int up, int mtu)
struct ifinfomsg *ifm;
struct nlmsghdr *nh;
struct rtattr *rta;
- char buf[BUFSIZ];
+ char buf[NLBUFSIZ];
ssize_t n;
size_t na;
--
2.39.2
On Wed, Mar 08, 2023 at 08:35:13AM +0100, Stefano Brivio wrote:...instead of BUFSIZ. On musl, BUFSIZ is 1024, so we'll typically truncate the response to the request we send in nl_link(). It's usually 8192 or more with glibc. There doesn't seem to be any macro defining the rtnetlink maximum message size, and iproute2 just hardcodes 1024 * 1024 for the receive buffer, but the example in netlink(7) makes somewhat sense, looking at the kernel implementation. It's not very clean, but we're very unlikely to hit that limit, and if we do, we'll find out painlessly, because NLA_OK() will tell us right away. Reported-by: Chris Kuhn <kuhnchris+passt(a)kuhnchris.eu> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au> I think controlling buffer sizes ourselves explicitly makes more sense, even if it weren't for the musl issues.--- netlink.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/netlink.c b/netlink.c index 8f785ca..0e0be4f 100644 --- a/netlink.c +++ b/netlink.c @@ -34,6 +34,8 @@ #include "log.h" #include "netlink.h" +#define NLBUFSIZ (8192 * sizeof(struct nlmsghdr)) /* See netlink(7) */ + /* Socket in init, in target namespace, sequence (just needs to be monotonic) */ static int nl_sock = -1; static int nl_sock_ns = -1; @@ -105,7 +107,7 @@ fail: static int nl_req(int ns, char *buf, const void *req, ssize_t len) { int s = ns ? nl_sock_ns : nl_sock, done = 0; - char flush[BUFSIZ]; + char flush[NLBUFSIZ]; ssize_t n; while (!done && (n = recv(s, flush, sizeof(flush), MSG_DONTWAIT)) > 0) { @@ -121,7 +123,8 @@ static int nl_req(int ns, char *buf, const void *req, ssize_t len) } } - if ((send(s, req, len, 0) < len) || (len = recv(s, buf, BUFSIZ, 0)) < 0) + if ((send(s, req, len, 0) < len) || + (len = recv(s, buf, NLBUFSIZ, 0)) < 0) return -errno; return len; @@ -149,7 +152,7 @@ unsigned int nl_get_ext_if(sa_family_t af) }; struct nlmsghdr *nh; struct rtattr *rta; - char buf[BUFSIZ]; + char buf[NLBUFSIZ]; ssize_t n; size_t na; @@ -227,7 +230,7 @@ void nl_route(int ns, unsigned int ifi, sa_family_t af, void *gw) struct nlmsghdr *nh; struct rtattr *rta; struct rtmsg *rtm; - char buf[BUFSIZ]; + char buf[NLBUFSIZ]; ssize_t n; size_t na; @@ -336,7 +339,7 @@ void nl_addr(int ns, unsigned int ifi, sa_family_t af, struct ifaddrmsg *ifa; struct nlmsghdr *nh; struct rtattr *rta; - char buf[BUFSIZ]; + char buf[NLBUFSIZ]; ssize_t n; size_t na; @@ -446,7 +449,7 @@ void nl_link(int ns, unsigned int ifi, void *mac, int up, int mtu) struct ifinfomsg *ifm; struct nlmsghdr *nh; struct rtattr *rta; - char buf[BUFSIZ]; + char buf[NLBUFSIZ]; ssize_t n; size_t na;-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
From: Chris Kuhn <kuhnchris+github(a)kuhnchris.eu>
While building against musl, gcc informs us that 'stderr' is a
protected keyword. This probably comes from a #define stderr (stderr)
in musl's stdio.h, to avoid a clash with extern FILE *const stderr,
but I didn't really track it down. Just rename it to force_stderr, it
makes more sense.
[sbrivio: Added commit message]
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
conf.c | 6 +++---
passt.c | 2 +-
passt.h | 4 ++--
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/conf.c b/conf.c
index 15506ec..07b0b7b 100644
--- a/conf.c
+++ b/conf.c
@@ -1356,13 +1356,13 @@ void conf(struct ctx *c, int argc, char **argv)
if (logfile)
die("Can't log to both file and stderr");
- if (c->stderr)
+ if (c->force_stderr)
die("Multiple --stderr options given");
- c->stderr = 1;
+ c->force_stderr = 1;
break;
case 'l':
- if (c->stderr)
+ if (c->force_stderr)
die("Can't log to both stderr and file");
if (logfile)
diff --git a/passt.c b/passt.c
index 5b8146e..f67213a 100644
--- a/passt.c
+++ b/passt.c
@@ -241,7 +241,7 @@ int main(int argc, char **argv)
conf(&c, argc, argv);
trace_init(c.trace);
- if (c.stderr || isatty(fileno(stdout)))
+ if (c.force_stderr || isatty(fileno(stdout)))
__openlog(log_name, LOG_PERROR, LOG_DAEMON);
quit_fd = pasta_netns_quit_init(&c);
diff --git a/passt.h b/passt.h
index b73f4ff..006d1c1 100644
--- a/passt.h
+++ b/passt.h
@@ -158,7 +158,7 @@ struct ip6_ctx {
* @trace: Enable tracing (extra debug) mode
* @quiet: Don't print informational messages
* @foreground: Run in foreground, don't log to stderr by default
- * @stderr: Force logging to stderr
+ * @force_stderr: Force logging to stderr
* @nofile: Maximum number of open files (ulimit -n)
* @sock_path: Path for UNIX domain socket
* @pcap: Path for packet capture file
@@ -207,7 +207,7 @@ struct ctx {
int trace;
int quiet;
int foreground;
- int stderr;
+ int force_stderr;
int nofile;
char sock_path[UNIX_PATH_MAX];
char pcap[PATH_MAX];
--
2.39.2
On Wed, Mar 08, 2023 at 08:35:14AM +0100, Stefano Brivio wrote:From: Chris Kuhn <kuhnchris+github(a)kuhnchris.eu> While building against musl, gcc informs us that 'stderr' is a protected keyword. This probably comes from a #define stderr (stderr) in musl's stdio.h, to avoid a clash with extern FILE *const stderr, but I didn't really track it down. Just rename it to force_stderr, it makes more sense. [sbrivio: Added commit message] Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au> Shadowing a libc global is not a great idea. Since it's a field, not a variable we're not *exactly* doing that, but it's close enough that I think this is better regardless of musl constraints. (I once had a particularly wonderful time tracking down a bizarre bug because 'index' ended up resolving to index(3) instead of the local variable I expected it to).--- conf.c | 6 +++--- passt.c | 2 +- passt.h | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/conf.c b/conf.c index 15506ec..07b0b7b 100644 --- a/conf.c +++ b/conf.c @@ -1356,13 +1356,13 @@ void conf(struct ctx *c, int argc, char **argv) if (logfile) die("Can't log to both file and stderr"); - if (c->stderr) + if (c->force_stderr) die("Multiple --stderr options given"); - c->stderr = 1; + c->force_stderr = 1; break; case 'l': - if (c->stderr) + if (c->force_stderr) die("Can't log to both stderr and file"); if (logfile) diff --git a/passt.c b/passt.c index 5b8146e..f67213a 100644 --- a/passt.c +++ b/passt.c @@ -241,7 +241,7 @@ int main(int argc, char **argv) conf(&c, argc, argv); trace_init(c.trace); - if (c.stderr || isatty(fileno(stdout))) + if (c.force_stderr || isatty(fileno(stdout))) __openlog(log_name, LOG_PERROR, LOG_DAEMON); quit_fd = pasta_netns_quit_init(&c); diff --git a/passt.h b/passt.h index b73f4ff..006d1c1 100644 --- a/passt.h +++ b/passt.h @@ -158,7 +158,7 @@ struct ip6_ctx { * @trace: Enable tracing (extra debug) mode * @quiet: Don't print informational messages * @foreground: Run in foreground, don't log to stderr by default - * @stderr: Force logging to stderr + * @force_stderr: Force logging to stderr * @nofile: Maximum number of open files (ulimit -n) * @sock_path: Path for UNIX domain socket * @pcap: Path for packet capture file @@ -207,7 +207,7 @@ struct ctx { int trace; int quiet; int foreground; - int stderr; + int force_stderr; int nofile; char sock_path[UNIX_PATH_MAX]; char pcap[PATH_MAX];-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
From: Chris Kuhn <kuhnchris+github(a)kuhnchris.eu> Roughly inspired from a patch by Chris Kuhn: fix up includes so that we can build against musl: glibc is more lenient as headers generally include a larger amount of other headers. Compared to the original patch, I only included what was needed directly in C files, instead of adding blanket includes in local header files. It's a bit more involved, but more consistent with the current (not ideal) situation. Reported-by: Chris Kuhn <kuhnchris+github(a)kuhnchris.eu> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- conf.c | 2 ++ isolation.c | 1 + netlink.c | 1 + passt.c | 2 ++ tap.c | 1 + tcp.c | 1 + tcp_splice.c | 1 + udp.c | 1 + util.c | 1 + 9 files changed, 11 insertions(+) diff --git a/conf.c b/conf.c index 07b0b7b..582c391 100644 --- a/conf.c +++ b/conf.c @@ -23,8 +23,10 @@ #include <limits.h> #include <grp.h> #include <pwd.h> +#include <signal.h> #include <stdlib.h> #include <stdint.h> +#include <stdio.h> #include <stdbool.h> #include <unistd.h> #include <syslog.h> diff --git a/isolation.c b/isolation.c index 6bae4d4..20dc879 100644 --- a/isolation.c +++ b/isolation.c @@ -65,6 +65,7 @@ #include <stdbool.h> #include <stddef.h> #include <stdlib.h> +#include <stdio.h> #include <string.h> #include <time.h> #include <unistd.h> diff --git a/netlink.c b/netlink.c index 0e0be4f..c8d39a1 100644 --- a/netlink.c +++ b/netlink.c @@ -18,6 +18,7 @@ #include <errno.h> #include <sys/types.h> #include <limits.h> +#include <signal.h> #include <stdlib.h> #include <stdbool.h> #include <stdint.h> diff --git a/passt.c b/passt.c index f67213a..dfec9d4 100644 --- a/passt.c +++ b/passt.c @@ -27,6 +27,8 @@ #include <stdlib.h> #include <unistd.h> #include <netdb.h> +#include <signal.h> +#include <stdio.h> #include <string.h> #include <errno.h> #include <time.h> diff --git a/tap.c b/tap.c index 88eed88..15fb52e 100644 --- a/tap.c +++ b/tap.c @@ -14,6 +14,7 @@ */ #include <sched.h> +#include <signal.h> #include <stdio.h> #include <errno.h> #include <limits.h> diff --git a/tcp.c b/tcp.c index 8e8d653..96ca5c7 100644 --- a/tcp.c +++ b/tcp.c @@ -267,6 +267,7 @@ #include <sched.h> #include <fcntl.h> #include <stdio.h> +#include <signal.h> #include <stdlib.h> #include <errno.h> #include <limits.h> diff --git a/tcp_splice.c b/tcp_splice.c index 67af46b..6559762 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -32,6 +32,7 @@ */ #include <sched.h> +#include <signal.h> #include <errno.h> #include <fcntl.h> #include <limits.h> diff --git a/udp.c b/udp.c index 99cfc9f..1077cde 100644 --- a/udp.c +++ b/udp.c @@ -91,6 +91,7 @@ */ #include <sched.h> +#include <signal.h> #include <stdio.h> #include <errno.h> #include <limits.h> diff --git a/util.c b/util.c index 799173f..484889b 100644 --- a/util.c +++ b/util.c @@ -13,6 +13,7 @@ */ #include <sched.h> +#include <stdio.h> #include <stdlib.h> #include <unistd.h> #include <arpa/inet.h> -- 2.39.2
On Wed, Mar 08, 2023 at 08:35:15AM +0100, Stefano Brivio wrote:From: Chris Kuhn <kuhnchris+github(a)kuhnchris.eu> Roughly inspired from a patch by Chris Kuhn: fix up includes so that we can build against musl: glibc is more lenient as headers generally include a larger amount of other headers. Compared to the original patch, I only included what was needed directly in C files, instead of adding blanket includes in local header files. It's a bit more involved, but more consistent with the current (not ideal) situation.Best I can tell, there's no ideal way to manage C includes :/.Reported-by: Chris Kuhn <kuhnchris+github(a)kuhnchris.eu> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au>--- conf.c | 2 ++ isolation.c | 1 + netlink.c | 1 + passt.c | 2 ++ tap.c | 1 + tcp.c | 1 + tcp_splice.c | 1 + udp.c | 1 + util.c | 1 + 9 files changed, 11 insertions(+) diff --git a/conf.c b/conf.c index 07b0b7b..582c391 100644 --- a/conf.c +++ b/conf.c @@ -23,8 +23,10 @@ #include <limits.h> #include <grp.h> #include <pwd.h> +#include <signal.h> #include <stdlib.h> #include <stdint.h> +#include <stdio.h> #include <stdbool.h> #include <unistd.h> #include <syslog.h> diff --git a/isolation.c b/isolation.c index 6bae4d4..20dc879 100644 --- a/isolation.c +++ b/isolation.c @@ -65,6 +65,7 @@ #include <stdbool.h> #include <stddef.h> #include <stdlib.h> +#include <stdio.h> #include <string.h> #include <time.h> #include <unistd.h> diff --git a/netlink.c b/netlink.c index 0e0be4f..c8d39a1 100644 --- a/netlink.c +++ b/netlink.c @@ -18,6 +18,7 @@ #include <errno.h> #include <sys/types.h> #include <limits.h> +#include <signal.h> #include <stdlib.h> #include <stdbool.h> #include <stdint.h> diff --git a/passt.c b/passt.c index f67213a..dfec9d4 100644 --- a/passt.c +++ b/passt.c @@ -27,6 +27,8 @@ #include <stdlib.h> #include <unistd.h> #include <netdb.h> +#include <signal.h> +#include <stdio.h> #include <string.h> #include <errno.h> #include <time.h> diff --git a/tap.c b/tap.c index 88eed88..15fb52e 100644 --- a/tap.c +++ b/tap.c @@ -14,6 +14,7 @@ */ #include <sched.h> +#include <signal.h> #include <stdio.h> #include <errno.h> #include <limits.h> diff --git a/tcp.c b/tcp.c index 8e8d653..96ca5c7 100644 --- a/tcp.c +++ b/tcp.c @@ -267,6 +267,7 @@ #include <sched.h> #include <fcntl.h> #include <stdio.h> +#include <signal.h> #include <stdlib.h> #include <errno.h> #include <limits.h> diff --git a/tcp_splice.c b/tcp_splice.c index 67af46b..6559762 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -32,6 +32,7 @@ */ #include <sched.h> +#include <signal.h> #include <errno.h> #include <fcntl.h> #include <limits.h> diff --git a/udp.c b/udp.c index 99cfc9f..1077cde 100644 --- a/udp.c +++ b/udp.c @@ -91,6 +91,7 @@ */ #include <sched.h> +#include <signal.h> #include <stdio.h> #include <errno.h> #include <limits.h> diff --git a/util.c b/util.c index 799173f..484889b 100644 --- a/util.c +++ b/util.c @@ -13,6 +13,7 @@ */ #include <sched.h> +#include <stdio.h> #include <stdlib.h> #include <unistd.h> #include <arpa/inet.h>-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
musl doesn't define those, use our own definition there. This is a
trivial implementation, similar to what's shipped by e.g. uClibc,
glibc, libiio.
Reported-by: Chris Kuhn <kuhnchris+github(a)kuhnchris.eu>
Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
util.h | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/util.h b/util.h
index 570094c..8367f51 100644
--- a/util.h
+++ b/util.h
@@ -88,6 +88,17 @@
#define MAC_ZERO ((uint8_t [ETH_ALEN]){ 0 })
#define MAC_IS_ZERO(addr) (!memcmp((addr), MAC_ZERO, ETH_ALEN))
+#ifndef __bswap_constant_16
+#define __bswap_constant_16(x) \
+ ((uint16_t) ((((x) >> 8) & 0xff) | (((x) & 0xff) << 8)))
+#endif
+
+#ifndef __bswap_constant_32
+#define __bswap_constant_32(x) \
+ ((((x) & 0xff000000) >> 24) | (((x) & 0x00ff0000) >> 8) | \
+ (((x) & 0x0000ff00) << 8) | (((x) & 0x000000ff) << 24))
+#endif
+
#if __BYTE_ORDER == __BIG_ENDIAN
#define htons_constant(x) (x)
#define htonl_constant(x) (x)
--
2.39.2
On Wed, Mar 08, 2023 at 08:35:16AM +0100, Stefano Brivio wrote:musl doesn't define those, use our own definition there. This is a trivial implementation, similar to what's shipped by e.g. uClibc, glibc, libiio. Reported-by: Chris Kuhn <kuhnchris+github(a)kuhnchris.eu> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au>--- util.h | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/util.h b/util.h index 570094c..8367f51 100644 --- a/util.h +++ b/util.h @@ -88,6 +88,17 @@ #define MAC_ZERO ((uint8_t [ETH_ALEN]){ 0 }) #define MAC_IS_ZERO(addr) (!memcmp((addr), MAC_ZERO, ETH_ALEN)) +#ifndef __bswap_constant_16 +#define __bswap_constant_16(x) \ + ((uint16_t) ((((x) >> 8) & 0xff) | (((x) & 0xff) << 8))) +#endif + +#ifndef __bswap_constant_32 +#define __bswap_constant_32(x) \ + ((((x) & 0xff000000) >> 24) | (((x) & 0x00ff0000) >> 8) | \ + (((x) & 0x0000ff00) << 8) | (((x) & 0x000000ff) << 24)) +#endif + #if __BYTE_ORDER == __BIG_ENDIAN #define htons_constant(x) (x) #define htonl_constant(x) (x)-- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson