[PATCH v2 00/13] Clean up to tap errors and epoll dispatch

older
[PATCH v2 0/4] Clean up reset of...

David Gibson

10 Aug 2023 10 Aug '23

4:33 a.m.

Getting from an epoll event to the relevant handler function is currently several levels of functions and tests. This series simplifies this to be pretty close to a single switch on a value in the epoll ref dispatching directly to the appropriate handler. Doing this requires some preliminary cleaning up of the handling of errors or disconnects on the tap device. Changes since v1: * Give listening TCP sockets their own reference type * Fold "tap reset" series into this one Changes since v2 of tap reset series: * More thorough cleanup of handling error events on the listening Unix socket. Changes since v1 of the tap reset series: * Two extra patches that further clean up the reset path David Gibson (13): tap: Clean up tap reset path tap: Clean up behaviour for errors on listening Unix socket tap: Fold reset handling into tap_handler_pasta() tap: Fold reset handling into tap_handler_passt() epoll: Generalize epoll_ref to cover things other than sockets epoll: Always use epoll_ref for the epoll data variable epoll: Fold sock_handler into general switch on epoll event fd epoll: Split handling of ICMP and ICMPv6 sockets epoll: Tiny cleanup to udp_sock_handler() epoll: Split handling of TCP timerfds into its own handler function epoll: Split handling of listening TCP sockets into their own handler epoll: Split listening Unix domain socket into its own type epoll: Use different epoll types for passt and pasta tap fds icmp.c | 118 +++++++++++++++++++++++++-------------------- icmp.h | 9 ++-- passt.c | 90 ++++++++++++++++++++-------------- passt.h | 56 +++++++++++++++++----- pasta.c | 8 +++- tap.c | 133 ++++++++++++++++++++++++++------------------------- tap.h | 7 ++- tcp.c | 84 ++++++++++++++------------------ tcp.h | 28 +++++++---- tcp_conn.h | 4 +- tcp_splice.c | 8 ++-- tcp_splice.h | 2 +- udp.c | 16 +++---- util.c | 27 ++++++++--- 14 files changed, 334 insertions(+), 256 deletions(-) -- 2.41.0

Show replies by date

David Gibson

10 Aug 10 Aug

4:33 a.m.

New subject: [PATCH v2 01/13] tap: Clean up tap reset path

In tap_handler() if we get an error on the tap device or socket, we use tap_sock_init() to re-initialise it. However, what we actually need for this reset case has remarkably little in common with the case where we're initialising for the first time: * Re-initialising the packet pools is unnecessary * The case of a passed in fd (--fd) isn't relevant * We don't even call this for pasta mode * We will never re-call tap_sock_unix_init() because we never clear fd_tap_listen In fact the only thing we do in tap_sock_init() relevant to the reset case is to remove the fd from the epoll and close it... which isn't used in the first initialisation case. So make a new tap_sock_reset() function just for this case, and simplify tap_sock_init() slightly as being used only for the first time case. Signed-off-by: David Gibson --- tap.c | 52 +++++++++++++++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 23 deletions(-) diff --git a/tap.c b/tap.c index e034f94..b4967d0 100644 --- a/tap.c +++ b/tap.c @@ -1236,19 +1236,14 @@ void tap_sock_init(struct ctx *c) tap6_l4[i].p = PACKET_INIT(pool_l4, TAP_SEQS, pkt_buf, sz); } - if (c->fd_tap != -1) { - if (c->one_off) { /* Passed as --fd */ - struct epoll_event ev = { 0 }; - - ev.data.fd = c->fd_tap; - ev.events = EPOLLIN | EPOLLET | EPOLLRDHUP; - epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap, &ev); - return; - } + if (c->fd_tap != -1) { /* Passed as --fd */ + struct epoll_event ev = { 0 }; + ASSERT(c->one_off); - epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL); - close(c->fd_tap); - c->fd_tap = -1; + ev.data.fd = c->fd_tap; + ev.events = EPOLLIN | EPOLLET | EPOLLRDHUP; + epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap, &ev); + return; } if (c->mode == MODE_PASST) { @@ -1259,6 +1254,26 @@ void tap_sock_init(struct ctx *c) } } +/** + * tap_sock_reset() - Handle closing or failure of connect AF_UNIX socket + * @c: Execution context + */ +static void tap_sock_reset(struct ctx *c) +{ + if (c->one_off) { + info("Client closed connection, exiting"); + exit(EXIT_SUCCESS); + } + + if (c->mode == MODE_PASTA) + die("Error on tap device, exiting"); + + /* Close the connected socket, wait for a new connection */ + epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL); + close(c->fd_tap); + c->fd_tap = -1; +} + /** * tap_handler() - Packet handler for AF_UNIX or tuntap file descriptor * @c: Execution context @@ -1276,15 +1291,6 @@ void tap_handler(struct ctx *c, int fd, uint32_t events, if ((c->mode == MODE_PASST && tap_handler_passt(c, now)) || (c->mode == MODE_PASTA && tap_handler_pasta(c, now)) || - (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR))) { - if (c->one_off) { - info("Client closed connection, exiting"); - exit(EXIT_SUCCESS); - } - - if (c->mode == MODE_PASTA) - die("Error on tap device, exiting"); - - tap_sock_init(c); - } + (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR))) + tap_sock_reset(c); } -- 2.41.0

David Gibson

4:33 a.m.

New subject: [PATCH v2 02/13] tap: Clean up behaviour for errors on listening Unix socket

We call tap_sock_unix_new() to handle a new connection to the qemu socket if we get an EPOLLIN event on c->fd_tap_listen. If we get any other event on the fd, we'll fall through to the "tap reset" path. But that won't do anything relevant to the listening socket, it will just close the already connected socket. Furthermore, the only other event we're subscribed to for the listening socket is EPOLLRDHUP, which doesn't apply to a non connected socket. Change the subscribed events from EPOLLRDHUP to EPOLLERR to catch general errors - not that there's any obvious case that would cause this event on a listening socket. Since we don't really expect it, and it's not obvious how we'd recover, treat it as a fatal error if we ever do get that event. Finally, fold all this handling into the tap_sock_unix_new() function, there's no real reason to split it between there and tap_handler(). Signed-off-by: David Gibson --- tap.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tap.c b/tap.c index b4967d0..c883d7e 100644 --- a/tap.c +++ b/tap.c @@ -1108,7 +1108,7 @@ static void tap_sock_unix_init(struct ctx *c) listen(fd, 0); ev.data.fd = c->fd_tap_listen = fd; - ev.events = EPOLLIN | EPOLLET | EPOLLRDHUP; + ev.events = EPOLLIN | EPOLLERR | EPOLLET; epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap_listen, &ev); info("You can now start qemu (>= 7.2, with commit 13c6be96618c):"); @@ -1121,14 +1121,18 @@ static void tap_sock_unix_init(struct ctx *c) /** * tap_sock_unix_new() - Handle new connection on listening socket * @c: Execution context + * @events: epoll events */ -static void tap_sock_unix_new(struct ctx *c) +static void tap_sock_unix_new(struct ctx *c, uint32_t events) { struct epoll_event ev = { 0 }; int v = INT_MAX / 2; struct ucred ucred; socklen_t len; + if (events != EPOLLIN) + die("Error on listening Unix socket, exiting"); + len = sizeof(ucred); /* Another client is already connected: accept and close right away. */ @@ -1284,8 +1288,8 @@ static void tap_sock_reset(struct ctx *c) void tap_handler(struct ctx *c, int fd, uint32_t events, const struct timespec *now) { - if (fd == c->fd_tap_listen && events == EPOLLIN) { - tap_sock_unix_new(c); + if (fd == c->fd_tap_listen) { + tap_sock_unix_new(c, events); return; } -- 2.41.0

David Gibson

4:33 a.m.

New subject: [PATCH v2 03/13] tap: Fold reset handling into tap_handler_pasta()

If tap_handler_pasta() fails, we reset the connection. But in the case of pasta the "reset" is just a fatal error. Fold the die() calls directly into tap_handler_pasta() for simplicity. Signed-off-by: David Gibson --- tap.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/tap.c b/tap.c index c883d7e..866ca4d 100644 --- a/tap.c +++ b/tap.c @@ -982,15 +982,18 @@ next: /** * tap_handler_pasta() - Packet handler for tuntap file descriptor * @c: Execution context + * @events: epoll events * @now: Current timestamp - * - * Return: -ECONNRESET on receive error, 0 otherwise */ -static int tap_handler_pasta(struct ctx *c, const struct timespec *now) +static void tap_handler_pasta(struct ctx *c, uint32_t events, + const struct timespec *now) { ssize_t n, len; int ret; + if (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR)) + die("Disconnect event on /dev/net/tun device, exiting"); + redo: n = 0; @@ -1037,15 +1040,12 @@ restart: tap6_handler(c, pool_tap6, now); if (len > 0 || ret == EAGAIN) - return 0; + return; if (n == TAP_BUF_BYTES) goto redo; - epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL); - close(c->fd_tap); - - return -ECONNRESET; + die("Error on tap device, exiting"); } /** @@ -1269,9 +1269,6 @@ static void tap_sock_reset(struct ctx *c) exit(EXIT_SUCCESS); } - if (c->mode == MODE_PASTA) - die("Error on tap device, exiting"); - /* Close the connected socket, wait for a new connection */ epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL); close(c->fd_tap); @@ -1293,8 +1290,11 @@ void tap_handler(struct ctx *c, int fd, uint32_t events, return; } - if ((c->mode == MODE_PASST && tap_handler_passt(c, now)) || - (c->mode == MODE_PASTA && tap_handler_pasta(c, now)) || - (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR))) - tap_sock_reset(c); + if (c->mode == MODE_PASST) { + if (tap_handler_passt(c, now) || + (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR))) + tap_sock_reset(c); + } else if (c->mode == MODE_PASTA) { + tap_handler_pasta(c, events, now); + } } -- 2.41.0

David Gibson

4:33 a.m.

New subject: [PATCH v2 04/13] tap: Fold reset handling into tap_handler_passt()

We call tap_sock_reset() if tap_handler_passt() fails, or if we get an error event on the socket. Fold that logic into tap_handler() passt itself which simplifies the caller. Signed-off-by: David Gibson --- tap.c | 63 ++++++++++++++++++++++++++++++----------------------------- 1 file changed, 32 insertions(+), 31 deletions(-) diff --git a/tap.c b/tap.c index 866ca4d..501af33 100644 --- a/tap.c +++ b/tap.c @@ -891,19 +891,41 @@ append: return in->count; } +/** + * tap_sock_reset() - Handle closing or failure of connect AF_UNIX socket + * @c: Execution context + */ +static void tap_sock_reset(struct ctx *c) +{ + if (c->one_off) { + info("Client closed connection, exiting"); + exit(EXIT_SUCCESS); + } + + /* Close the connected socket, wait for a new connection */ + epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL); + close(c->fd_tap); + c->fd_tap = -1; +} + /** * tap_handler_passt() - Packet handler for AF_UNIX file descriptor * @c: Execution context + * @events: epoll events * @now: Current timestamp - * - * Return: -ECONNRESET on receive error, 0 otherwise */ -static int tap_handler_passt(struct ctx *c, const struct timespec *now) +static void tap_handler_passt(struct ctx *c, uint32_t events, + const struct timespec *now) { struct ethhdr *eh; ssize_t n, rem; char *p; + if (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR)) { + tap_sock_reset(c); + return; + } + redo: p = pkt_buf; rem = 0; @@ -914,12 +936,13 @@ redo: n = recv(c->fd_tap, p, TAP_BUF_FILL, MSG_DONTWAIT); if (n < 0) { if (errno == EINTR || errno == EAGAIN || errno == EWOULDBLOCK) - return 0; + return; epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL); close(c->fd_tap); - return -ECONNRESET; + tap_sock_reset(c); + return; } while (n > (ssize_t)sizeof(uint32_t)) { @@ -934,7 +957,7 @@ redo: if (len > n) { rem = recv(c->fd_tap, p + n, len - n, 0); if ((n += rem) != len) - return 0; + return; } /* Complete the partial read above before discarding a malformed @@ -975,8 +998,6 @@ next: /* We can't use EPOLLET otherwise. */ if (rem) goto redo; - - return 0; } /** @@ -1258,23 +1279,6 @@ void tap_sock_init(struct ctx *c) } } -/** - * tap_sock_reset() - Handle closing or failure of connect AF_UNIX socket - * @c: Execution context - */ -static void tap_sock_reset(struct ctx *c) -{ - if (c->one_off) { - info("Client closed connection, exiting"); - exit(EXIT_SUCCESS); - } - - /* Close the connected socket, wait for a new connection */ - epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL); - close(c->fd_tap); - c->fd_tap = -1; -} - /** * tap_handler() - Packet handler for AF_UNIX or tuntap file descriptor * @c: Execution context @@ -1290,11 +1294,8 @@ void tap_handler(struct ctx *c, int fd, uint32_t events, return; } - if (c->mode == MODE_PASST) { - if (tap_handler_passt(c, now) || - (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR))) - tap_sock_reset(c); - } else if (c->mode == MODE_PASTA) { + if (c->mode == MODE_PASST) + tap_handler_passt(c, events, now); + else if (c->mode == MODE_PASTA) tap_handler_pasta(c, events, now); - } } -- 2.41.0

Stefano Brivio

9:49 p.m.

New subject: [PATCH v2 04/13] tap: Fold reset handling into tap_handler_passt()

On Thu, 10 Aug 2023 12:33:06 +1000 David Gibson wrote:

...

We call tap_sock_reset() if tap_handler_passt() fails, or if we get an error event on the socket. Fold that logic into tap_handler() passt itself which simplifies the caller.

Signed-off-by: David Gibson --- tap.c | 63 ++++++++++++++++++++++++++++++----------------------------- 1 file changed, 32 insertions(+), 31 deletions(-)

diff --git a/tap.c b/tap.c index 866ca4d..501af33 100644 --- a/tap.c +++ b/tap.c @@ -891,19 +891,41 @@ append: return in->count; }

+/** + * tap_sock_reset() - Handle closing or failure of connect AF_UNIX socket + * @c: Execution context + */ +static void tap_sock_reset(struct ctx *c) +{ + if (c->one_off) { + info("Client closed connection, exiting"); + exit(EXIT_SUCCESS); + } + + /* Close the connected socket, wait for a new connection */ + epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL); + close(c->fd_tap); + c->fd_tap = -1; +} + /** * tap_handler_passt() - Packet handler for AF_UNIX file descriptor * @c: Execution context + * @events: epoll events * @now: Current timestamp - * - * Return: -ECONNRESET on receive error, 0 otherwise */ -static int tap_handler_passt(struct ctx *c, const struct timespec *now) +static void tap_handler_passt(struct ctx *c, uint32_t events, + const struct timespec *now) { struct ethhdr *eh; ssize_t n, rem; char *p;

+ if (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR)) { + tap_sock_reset(c); + return; + } + redo: p = pkt_buf; rem = 0; @@ -914,12 +936,13 @@ redo: n = recv(c->fd_tap, p, TAP_BUF_FILL, MSG_DONTWAIT); if (n < 0) { if (errno == EINTR || errno == EAGAIN || errno == EWOULDBLOCK) - return 0; + return;

epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL); close(c->fd_tap);

- return -ECONNRESET; + tap_sock_reset(c);

...also reported by covscan: close() before this is redundant now. -- Stefano

David Gibson

11 Aug 11 Aug

5:07 a.m.

New subject: [PATCH v2 04/13] tap: Fold reset handling into tap_handler_passt()

On Thu, Aug 10, 2023 at 09:49:46PM +0200, Stefano Brivio wrote:

...

On Thu, 10 Aug 2023 12:33:06 +1000 David Gibson wrote:

...
We call tap_sock_reset() if tap_handler_passt() fails, or if we get an error event on the socket. Fold that logic into tap_handler() passt itself which simplifies the caller.

Signed-off-by: David Gibson --- tap.c | 63 ++++++++++++++++++++++++++++++----------------------------- 1 file changed, 32 insertions(+), 31 deletions(-)

diff --git a/tap.c b/tap.c index 866ca4d..501af33 100644 --- a/tap.c +++ b/tap.c @@ -891,19 +891,41 @@ append: return in->count; }

+/** + * tap_sock_reset() - Handle closing or failure of connect AF_UNIX socket + * @c: Execution context + */ +static void tap_sock_reset(struct ctx *c) +{ + if (c->one_off) { + info("Client closed connection, exiting"); + exit(EXIT_SUCCESS); + } + + /* Close the connected socket, wait for a new connection */ + epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL); + close(c->fd_tap); + c->fd_tap = -1; +} + /** * tap_handler_passt() - Packet handler for AF_UNIX file descriptor * @c: Execution context + * @events: epoll events * @now: Current timestamp - * - * Return: -ECONNRESET on receive error, 0 otherwise */ -static int tap_handler_passt(struct ctx *c, const struct timespec *now) +static void tap_handler_passt(struct ctx *c, uint32_t events, + const struct timespec *now) { struct ethhdr *eh; ssize_t n, rem; char *p;

+ if (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR)) { + tap_sock_reset(c); + return; + } + redo: p = pkt_buf; rem = 0; @@ -914,12 +936,13 @@ redo: n = recv(c->fd_tap, p, TAP_BUF_FILL, MSG_DONTWAIT); if (n < 0) { if (errno == EINTR || errno == EAGAIN || errno == EWOULDBLOCK) - return 0; + return;

epoll_ctl(c->epollfd, EPOLL_CTL_DEL, c->fd_tap, NULL); close(c->fd_tap);

- return -ECONNRESET; + tap_sock_reset(c);

...also reported by covscan: close() before this is redundant now.

Oops, yes. I spotted that, but then forgot to fix it. Fixed for a new spin. -- 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

David Gibson

10 Aug 10 Aug

4:33 a.m.

New subject: [PATCH v2 05/13] epoll: Generalize epoll_ref to cover things other than sockets

The epoll_ref type includes fields for the IP protocol of a socket, and the socket fd. However, we already have a few things in the epoll which aren't protocol sockets, and we may have more in future. Rename these fields to an abstract "fd type" and file descriptor for more generality. Similarly, rather than using existing IP protocol numbers for the type, introduce our own number space. For now these just correspond to the supported protocols, but we'll expand on that in future. Signed-off-by: David Gibson --- icmp.c | 6 +++--- passt.c | 25 ++++++++++++------------- passt.h | 40 +++++++++++++++++++++++++++++----------- tcp.c | 22 +++++++++++----------- tcp_conn.h | 4 ++-- tcp_splice.c | 4 ++-- udp.c | 14 +++++++------- util.c | 27 ++++++++++++++++++++------- 8 files changed, 86 insertions(+), 56 deletions(-) diff --git a/icmp.c b/icmp.c index 676fa64..a4b6a47 100644 --- a/icmp.c +++ b/icmp.c @@ -79,7 +79,7 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref, (void)events; (void)now; - n = recvfrom(ref.s, buf, sizeof(buf), 0, (struct sockaddr *)&sr, &sl); + n = recvfrom(ref.fd, buf, sizeof(buf), 0, (struct sockaddr *)&sr, &sl); if (n < 0) return; @@ -182,7 +182,7 @@ int icmp_tap_handler(const struct ctx *c, int af, const void *addr, bind_if, id, iref.u32); if (s < 0) goto fail_sock; - if (s > SOCKET_MAX) { + if (s > FD_REF_MAX) { close(s); return 1; } @@ -236,7 +236,7 @@ int icmp_tap_handler(const struct ctx *c, int af, const void *addr, bind_if, id, iref.u32); if (s < 0) goto fail_sock; - if (s > SOCKET_MAX) { + if (s > FD_REF_MAX) { close(s); return 1; } diff --git a/passt.c b/passt.c index 9123868..ae69478 100644 --- a/passt.c +++ b/passt.c @@ -55,12 +55,11 @@ char pkt_buf[PKT_BUF_BYTES] __attribute__ ((aligned(PAGE_SIZE))); -char *ip_proto_str[IPPROTO_SCTP + 1] = { - [IPPROTO_ICMP] = "ICMP", - [IPPROTO_TCP] = "TCP", - [IPPROTO_UDP] = "UDP", - [IPPROTO_ICMPV6] = "ICMPV6", - [IPPROTO_SCTP] = "SCTP", +char *epoll_type_str[EPOLL_TYPE_MAX + 1] = { + [EPOLL_TYPE_TCP] = "TCP socket", + [EPOLL_TYPE_UDP] = "UDP socket", + [EPOLL_TYPE_ICMP] = "ICMP socket", + [EPOLL_TYPE_ICMPV6] = "ICMPv6 socket", }; /** @@ -73,16 +72,16 @@ char *ip_proto_str[IPPROTO_SCTP + 1] = { static void sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events, const struct timespec *now) { - trace("%s: %s packet from socket %i (events: 0x%08x)", + trace("%s: packet from %s %i (events: 0x%08x)", c->mode == MODE_PASST ? "passt" : "pasta", - IP_PROTO_STR(ref.proto), ref.s, events); + EPOLL_TYPE_STR(ref.type), ref.fd, events); - if (!c->no_tcp && ref.proto == IPPROTO_TCP) - tcp_sock_handler( c, ref, events, now); - else if (!c->no_udp && ref.proto == IPPROTO_UDP) - udp_sock_handler( c, ref, events, now); + if (!c->no_tcp && ref.type == EPOLL_TYPE_TCP) + tcp_sock_handler(c, ref, events, now); + else if (!c->no_udp && ref.type == EPOLL_TYPE_UDP) + udp_sock_handler(c, ref, events, now); else if (!c->no_icmp && - (ref.proto == IPPROTO_ICMP || ref.proto == IPPROTO_ICMPV6)) + (ref.type == EPOLL_TYPE_ICMP || ref.type == EPOLL_TYPE_ICMPV6)) icmp_sock_handler(c, ref, events, now); } diff --git a/passt.h b/passt.h index edc4841..2110781 100644 --- a/passt.h +++ b/passt.h @@ -42,9 +42,27 @@ union epoll_ref; #include "udp.h" /** - * union epoll_ref - Breakdown of reference for epoll socket bookkeeping - * @proto: IP protocol number - * @s: Socket number (implies 2^24-1 limit on number of descriptors) + * enum epoll_type - Different types of fds we poll over + */ +enum epoll_type { + /* Special value to indicate an invalid type */ + EPOLL_TYPE_NONE = 0, + /* Sockets and timerfds for TCP handling */ + EPOLL_TYPE_TCP, + /* UDP sockets */ + EPOLL_TYPE_UDP, + /* IPv4 ICMP sockets */ + EPOLL_TYPE_ICMP, + /* ICMPv6 sockets */ + EPOLL_TYPE_ICMPV6, + + EPOLL_TYPE_MAX = EPOLL_TYPE_ICMPV6, +}; + +/** + * union epoll_ref - Breakdown of reference for epoll fd bookkeeping + * @type: Type of fd (tells us what to do with events) + * @fd: File descriptor number (implies < 2^24 total descriptors) * @tcp: TCP-specific reference part * @udp: UDP-specific reference part * @icmp: ICMP-specific reference part @@ -53,10 +71,10 @@ union epoll_ref; */ union epoll_ref { struct { - int32_t proto:8, -#define SOCKET_REF_BITS 24 -#define SOCKET_MAX MAX_FROM_BITS(SOCKET_REF_BITS) - s:SOCKET_REF_BITS; + enum epoll_type type:8; +#define FD_REF_BITS 24 +#define FD_REF_MAX MAX_FROM_BITS(FD_REF_BITS) + int32_t fd:FD_REF_BITS; union { union tcp_epoll_ref tcp; union udp_epoll_ref udp; @@ -78,10 +96,10 @@ static_assert(sizeof(union epoll_ref) <= sizeof(union epoll_data), #define PKT_BUF_BYTES MAX(TAP_BUF_BYTES, 0) extern char pkt_buf [PKT_BUF_BYTES]; -extern char *ip_proto_str[]; -#define IP_PROTO_STR(n) \ - (((uint8_t)(n) <= IPPROTO_SCTP && ip_proto_str[(n)]) ? \ - ip_proto_str[(n)] : "?") +extern char *epoll_type_str[]; +#define EPOLL_TYPE_STR(n) \ + (((uint8_t)(n) <= EPOLL_TYPE_MAX && epoll_type_str[(n)]) ? \ + epoll_type_str[(n)] : "?") #include /* For MAXNS below */ diff --git a/tcp.c b/tcp.c index d9ecee5..18b781a 100644 --- a/tcp.c +++ b/tcp.c @@ -643,7 +643,7 @@ static void conn_flag_do(const struct ctx *c, struct tcp_tap_conn *conn, static int tcp_epoll_ctl(const struct ctx *c, struct tcp_tap_conn *conn) { int m = conn->c.in_epoll ? EPOLL_CTL_MOD : EPOLL_CTL_ADD; - union epoll_ref ref = { .proto = IPPROTO_TCP, .s = conn->sock, + union epoll_ref ref = { .type = EPOLL_TYPE_TCP, .fd = conn->sock, .tcp.index = CONN_IDX(conn) }; struct epoll_event ev = { .data.u64 = ref.u64 }; @@ -663,8 +663,8 @@ static int tcp_epoll_ctl(const struct ctx *c, struct tcp_tap_conn *conn) conn->c.in_epoll = true; if (conn->timer != -1) { - union epoll_ref ref_t = { .proto = IPPROTO_TCP, - .s = conn->sock, + union epoll_ref ref_t = { .type = EPOLL_TYPE_TCP, + .fd = conn->sock, .tcp.timer = 1, .tcp.index = CONN_IDX(conn) }; struct epoll_event ev_t = { .data.u64 = ref_t.u64, @@ -692,8 +692,8 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn) return; if (conn->timer == -1) { - union epoll_ref ref = { .proto = IPPROTO_TCP, - .s = conn->sock, + union epoll_ref ref = { .type = EPOLL_TYPE_TCP, + .fd = conn->sock, .tcp.timer = 1, .tcp.index = CONN_IDX(conn) }; struct epoll_event ev = { .data.u64 = ref.u64, @@ -701,7 +701,7 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn) int fd; fd = timerfd_create(CLOCK_MONOTONIC, 0); - if (fd == -1 || fd > SOCKET_MAX) { + if (fd == -1 || fd > FD_REF_MAX) { debug("TCP: failed to get timer: %s", strerror(errno)); if (fd > -1) close(fd); @@ -1908,7 +1908,7 @@ int tcp_conn_new_sock(const struct ctx *c, sa_family_t af) s = socket(af, SOCK_STREAM | SOCK_NONBLOCK, IPPROTO_TCP); - if (s > SOCKET_MAX) { + if (s > FD_REF_MAX) { close(s); return -EIO; } @@ -2791,7 +2791,7 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref, * https://github.com/llvm/llvm-project/issues/58992 */ memset(&sa, 0, sizeof(struct sockaddr_in6)); - s = accept4(ref.s, (struct sockaddr *)&sa, &sl, SOCK_NONBLOCK); + s = accept4(ref.fd, (struct sockaddr *)&sa, &sl, SOCK_NONBLOCK); if (s < 0) return; @@ -2948,7 +2948,7 @@ void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events, conn = tc + ref.tcp.index; if (conn->c.spliced) - tcp_splice_sock_handler(c, &conn->splice, ref.s, events); + tcp_splice_sock_handler(c, &conn->splice, ref.fd, events); else tcp_tap_sock_handler(c, &conn->tap, events); } @@ -2999,7 +2999,7 @@ static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port, int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr, const char *ifname, in_port_t port) { - int r4 = SOCKET_MAX + 1, r6 = SOCKET_MAX + 1; + int r4 = FD_REF_MAX + 1, r6 = FD_REF_MAX + 1; if (af == AF_UNSPEC && c->ifi4 && c->ifi6) /* Attempt to get a dual stack socket */ @@ -3013,7 +3013,7 @@ int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr, if ((af == AF_INET6 || af == AF_UNSPEC) && c->ifi6) r6 = tcp_sock_init_af(c, AF_INET6, port, addr, ifname); - if (IN_INTERVAL(0, SOCKET_MAX, r4) || IN_INTERVAL(0, SOCKET_MAX, r6)) + if (IN_INTERVAL(0, FD_REF_MAX, r4) || IN_INTERVAL(0, FD_REF_MAX, r6)) return 0; return r4 < 0 ? r4 : r6; diff --git a/tcp_conn.h b/tcp_conn.h index 9e2b1bf..0b36940 100644 --- a/tcp_conn.h +++ b/tcp_conn.h @@ -62,7 +62,7 @@ struct tcp_tap_conn { unsigned int ws_to_tap :TCP_WS_BITS; - int sock :SOCKET_REF_BITS; + int sock :FD_REF_BITS; uint8_t events; #define CLOSED 0 @@ -80,7 +80,7 @@ struct tcp_tap_conn { (SOCK_ACCEPTED | TAP_SYN_RCVD | ESTABLISHED) - int timer :SOCKET_REF_BITS; + int timer :FD_REF_BITS; uint8_t flags; #define STALLED BIT(0) diff --git a/tcp_splice.c b/tcp_splice.c index 03e14c1..24995e2 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -173,9 +173,9 @@ static int tcp_splice_epoll_ctl(const struct ctx *c, struct tcp_splice_conn *conn) { int m = conn->c.in_epoll ? EPOLL_CTL_MOD : EPOLL_CTL_ADD; - union epoll_ref ref_a = { .proto = IPPROTO_TCP, .s = conn->a, + union epoll_ref ref_a = { .type = EPOLL_TYPE_TCP, .fd = conn->a, .tcp.index = CONN_IDX(conn) }; - union epoll_ref ref_b = { .proto = IPPROTO_TCP, .s = conn->b, + union epoll_ref ref_b = { .type = EPOLL_TYPE_TCP, .fd = conn->b, .tcp.index = CONN_IDX(conn) }; struct epoll_event ev_a = { .data.u64 = ref_a.u64 }; struct epoll_event ev_b = { .data.u64 = ref_b.u64 }; diff --git a/udp.c b/udp.c index 5a852fb..62f8360 100644 --- a/udp.c +++ b/udp.c @@ -388,9 +388,9 @@ static void udp_sock6_iov_init(const struct ctx *c) int udp_splice_new(const struct ctx *c, int v6, in_port_t src, bool ns) { struct epoll_event ev = { .events = EPOLLIN | EPOLLRDHUP | EPOLLHUP }; - union epoll_ref ref = { .proto = IPPROTO_UDP, + union epoll_ref ref = { .type = EPOLL_TYPE_UDP, .udp = { .splice = true, .ns = ns, - .v6 = v6, .port = src } + .v6 = v6, .port = src } }; struct udp_splice_port *sp; int act, s; @@ -406,7 +406,7 @@ int udp_splice_new(const struct ctx *c, int v6, in_port_t src, bool ns) s = socket(v6 ? AF_INET6 : AF_INET, SOCK_DGRAM | SOCK_NONBLOCK, IPPROTO_UDP); - if (s > SOCKET_MAX) { + if (s > FD_REF_MAX) { close(s); return -EIO; } @@ -414,7 +414,7 @@ int udp_splice_new(const struct ctx *c, int v6, in_port_t src, bool ns) if (s < 0) return s; - ref.s = s; + ref.fd = s; if (v6) { struct sockaddr_in6 addr6 = { @@ -767,7 +767,7 @@ void udp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events, udp4_localname.sin_port = htons(dstport); } - n = recvmmsg(ref.s, mmh_recv, n, 0, NULL); + n = recvmmsg(ref.fd, mmh_recv, n, 0, NULL); if (n <= 0) return; @@ -980,7 +980,7 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af, const void *addr, const char *ifname, in_port_t port) { union udp_epoll_ref uref = { .u32 = 0 }; - int s, r4 = SOCKET_MAX + 1, r6 = SOCKET_MAX + 1; + int s, r4 = FD_REF_MAX + 1, r6 = FD_REF_MAX + 1; if (ns) { uref.port = (in_port_t)(port + c->udp.fwd_out.f.delta[port]); @@ -1030,7 +1030,7 @@ int udp_sock_init(const struct ctx *c, int ns, sa_family_t af, } } - if (IN_INTERVAL(0, SOCKET_MAX, r4) || IN_INTERVAL(0, SOCKET_MAX, r6)) + if (IN_INTERVAL(0, FD_REF_MAX, r4) || IN_INTERVAL(0, FD_REF_MAX, r6)) return 0; return r4 < 0 ? r4 : r6; diff --git a/util.c b/util.c index 019c56c..2cac7ba 100644 --- a/util.c +++ b/util.c @@ -102,7 +102,7 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto, const void *bind_addr, const char *ifname, uint16_t port, uint32_t data) { - union epoll_ref ref = { .proto = proto, .data = data }; + union epoll_ref ref = { .data = data }; struct sockaddr_in addr4 = { .sin_family = AF_INET, .sin_port = htons(port), @@ -118,9 +118,22 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto, int fd, sl, y = 1, ret; struct epoll_event ev; - if (proto != IPPROTO_TCP && proto != IPPROTO_UDP && - proto != IPPROTO_ICMP && proto != IPPROTO_ICMPV6) + switch (proto) { + case IPPROTO_TCP: + ref.type = EPOLL_TYPE_TCP; + break; + case IPPROTO_UDP: + ref.type = EPOLL_TYPE_UDP; + break; + case IPPROTO_ICMP: + ref.type = EPOLL_TYPE_ICMP; + break; + case IPPROTO_ICMPV6: + ref.type = EPOLL_TYPE_ICMPV6; + break; + default: return -EPFNOSUPPORT; /* Not implemented. */ + } if (af == AF_UNSPEC) { if (!DUAL_STACK_SOCKETS || bind_addr) @@ -140,12 +153,12 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto, return ret; } - if (fd > SOCKET_MAX) { + if (fd > FD_REF_MAX) { close(fd); return -EBADF; } - ref.s = fd; + ref.fd = fd; if (af == AF_INET) { if (bind_addr) @@ -188,8 +201,8 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto, if (setsockopt(fd, SOL_SOCKET, SO_BINDTODEVICE, ifname, strlen(ifname))) { ret = -errno; - warn("Can't bind socket for %s port %u to %s, closing", - ip_proto_str[proto], port, ifname); + warn("Can't bind %s socket for port %u to %s, closing", + EPOLL_TYPE_STR(proto), port, ifname); close(fd); return ret; } -- 2.41.0

David Gibson

4:33 a.m.

New subject: [PATCH v2 06/13] epoll: Always use epoll_ref for the epoll data variable

epoll_ref contains a variety of information useful when handling epoll events on our sockets, and we place it in the epoll_event data field returned by epoll. However, for a few other things we use the 'fd' field in the standard union of types for that data field. This actually introduces a bug which is vanishingly unlikely to hit in practice, but very nasty if it ever did: theoretically if we had a very large file descriptor number for fd_tap or fd_tap_listen it could overflow into bits that overlap with the 'proto' field in epoll_ref. With some very bad luck this could mean that we mistakenly think an event on a regular socket is an event on fd_tap or fd_tap_listen. More practically, using different (but overlapping) fields of the epoll_data means we can't unify dispatch for the various different objects in the epoll. Therefore use the same epoll_ref as the data for the tap fds and the netns quit fd, adding new fd type values to describe them. Signed-off-by: David Gibson --- passt.c | 11 ++++++----- passt.h | 6 +++++- pasta.c | 8 ++++++-- tap.c | 16 ++++++++++++---- 4 files changed, 29 insertions(+), 12 deletions(-) diff --git a/passt.c b/passt.c index ae69478..45e9fbf 100644 --- a/passt.c +++ b/passt.c @@ -60,6 +60,8 @@ char *epoll_type_str[EPOLL_TYPE_MAX + 1] = { [EPOLL_TYPE_UDP] = "UDP socket", [EPOLL_TYPE_ICMP] = "ICMP socket", [EPOLL_TYPE_ICMPV6] = "ICMPv6 socket", + [EPOLL_TYPE_NSQUIT] = "namespace inotify", + [EPOLL_TYPE_TAP] = "tap device", }; /** @@ -328,12 +330,11 @@ loop: for (i = 0; i < nfds; i++) { union epoll_ref ref = *((union epoll_ref *)&events[i].data.u64); - int fd = events[i].data.fd; - if (fd == c.fd_tap || fd == c.fd_tap_listen) - tap_handler(&c, fd, events[i].events, &now); - else if (fd == quit_fd) - pasta_netns_quit_handler(&c, fd); + if (ref.type == EPOLL_TYPE_TAP) + tap_handler(&c, ref.fd, events[i].events, &now); + else if (ref.type == EPOLL_TYPE_NSQUIT) + pasta_netns_quit_handler(&c, quit_fd); else sock_handler(&c, ref, events[i].events, &now); } diff --git a/passt.h b/passt.h index 2110781..8878a11 100644 --- a/passt.h +++ b/passt.h @@ -55,8 +55,12 @@ enum epoll_type { EPOLL_TYPE_ICMP, /* ICMPv6 sockets */ EPOLL_TYPE_ICMPV6, + /* inotify fd watching for end of netns (pasta) */ + EPOLL_TYPE_NSQUIT, + /* tap char device, or qemu socket fd */ + EPOLL_TYPE_TAP, - EPOLL_TYPE_MAX = EPOLL_TYPE_ICMPV6, + EPOLL_TYPE_MAX = EPOLL_TYPE_TAP, }; /** diff --git a/pasta.c b/pasta.c index 1e84680..dbe8e8c 100644 --- a/pasta.c +++ b/pasta.c @@ -365,7 +365,10 @@ void pasta_ns_conf(struct ctx *c) int pasta_netns_quit_init(struct ctx *c) { int flags = O_NONBLOCK | O_CLOEXEC; - struct epoll_event ev = { .events = EPOLLIN }; + union epoll_ref ref = { .type = EPOLL_TYPE_NSQUIT }; + struct epoll_event ev = { + .events = EPOLLIN + }; int inotify_fd; if (c->mode != MODE_PASTA || c->no_netns_quit || !*c->netns_base) @@ -381,7 +384,8 @@ int pasta_netns_quit_init(struct ctx *c) return -1; } - ev.data.fd = inotify_fd; + ref.fd = inotify_fd; + ev.data.u64 = ref.u64; epoll_ctl(c->epollfd, EPOLL_CTL_ADD, inotify_fd, &ev); return inotify_fd; diff --git a/tap.c b/tap.c index 501af33..6c3ce48 100644 --- a/tap.c +++ b/tap.c @@ -1076,6 +1076,7 @@ restart: static void tap_sock_unix_init(struct ctx *c) { int fd = socket(AF_UNIX, SOCK_STREAM, 0); + union epoll_ref ref = { .type = EPOLL_TYPE_TAP }; struct epoll_event ev = { 0 }; struct sockaddr_un addr = { .sun_family = AF_UNIX, @@ -1128,8 +1129,9 @@ static void tap_sock_unix_init(struct ctx *c) listen(fd, 0); - ev.data.fd = c->fd_tap_listen = fd; + ref.fd = c->fd_tap_listen = fd; ev.events = EPOLLIN | EPOLLERR | EPOLLET; + ev.data.u64 = ref.u64; epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap_listen, &ev); info("You can now start qemu (>= 7.2, with commit 13c6be96618c):"); @@ -1146,6 +1148,7 @@ static void tap_sock_unix_init(struct ctx *c) */ static void tap_sock_unix_new(struct ctx *c, uint32_t events) { + union epoll_ref ref = { .type = EPOLL_TYPE_TAP }; struct epoll_event ev = { 0 }; int v = INT_MAX / 2; struct ucred ucred; @@ -1185,8 +1188,9 @@ static void tap_sock_unix_new(struct ctx *c, uint32_t events) setsockopt(c->fd_tap, SOL_SOCKET, SO_SNDBUF, &v, sizeof(v))) trace("tap: failed to set SO_SNDBUF to %i", v); - ev.data.fd = c->fd_tap; + ref.fd = c->fd_tap; ev.events = EPOLLIN | EPOLLET | EPOLLRDHUP; + ev.data.u64 = ref.u64; epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap, &ev); } @@ -1231,6 +1235,7 @@ static int tap_ns_tun(void *arg) */ static void tap_sock_tun_init(struct ctx *c) { + union epoll_ref ref = { .type = EPOLL_TYPE_TAP }; struct epoll_event ev = { 0 }; NS_CALL(tap_ns_tun, c); @@ -1239,8 +1244,9 @@ static void tap_sock_tun_init(struct ctx *c) pasta_ns_conf(c); - ev.data.fd = c->fd_tap; + ref.fd = c->fd_tap; ev.events = EPOLLIN | EPOLLRDHUP; + ev.data.u64 = ref.u64; epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap, &ev); } @@ -1262,11 +1268,13 @@ void tap_sock_init(struct ctx *c) } if (c->fd_tap != -1) { /* Passed as --fd */ + union epoll_ref ref = { .type = EPOLL_TYPE_TAP }; struct epoll_event ev = { 0 }; ASSERT(c->one_off); - ev.data.fd = c->fd_tap; + ref.fd = c->fd_tap; ev.events = EPOLLIN | EPOLLET | EPOLLRDHUP; + ev.data.u64 = ref.u64; epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap, &ev); return; } -- 2.41.0

David Gibson

4:33 a.m.

New subject: [PATCH v2 07/13] epoll: Fold sock_handler into general switch on epoll event fd

When we process events from epoll_wait(), we check for a number of special cases before calling sock_handler() which then dispatches based on the protocol type of the socket in the event. Fold these cases together into a single switch on the fd type recorded in the epoll data field. Signed-off-by: David Gibson --- passt.c | 54 +++++++++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/passt.c b/passt.c index 45e9fbf..665262b 100644 --- a/passt.c +++ b/passt.c @@ -64,29 +64,6 @@ char *epoll_type_str[EPOLL_TYPE_MAX + 1] = { [EPOLL_TYPE_TAP] = "tap device", }; -/** - * sock_handler() - Event handler for L4 sockets - * @c: Execution context - * @ref: epoll reference - * @events: epoll events - * @now: Current timestamp - */ -static void sock_handler(struct ctx *c, union epoll_ref ref, - uint32_t events, const struct timespec *now) -{ - trace("%s: packet from %s %i (events: 0x%08x)", - c->mode == MODE_PASST ? "passt" : "pasta", - EPOLL_TYPE_STR(ref.type), ref.fd, events); - - if (!c->no_tcp && ref.type == EPOLL_TYPE_TCP) - tcp_sock_handler(c, ref, events, now); - else if (!c->no_udp && ref.type == EPOLL_TYPE_UDP) - udp_sock_handler(c, ref, events, now); - else if (!c->no_icmp && - (ref.type == EPOLL_TYPE_ICMP || ref.type == EPOLL_TYPE_ICMPV6)) - icmp_sock_handler(c, ref, events, now); -} - /** * post_handler() - Run periodic and deferred tasks for L4 protocol handlers * @c: Execution context @@ -330,13 +307,36 @@ loop: for (i = 0; i < nfds; i++) { union epoll_ref ref = *((union epoll_ref *)&events[i].data.u64); + uint32_t eventmask = events[i].events; - if (ref.type == EPOLL_TYPE_TAP) + trace("%s: epoll event on %s %i (events: 0x%08x)", + c.mode == MODE_PASST ? "passt" : "pasta", + EPOLL_TYPE_STR(ref.type), ref.fd, events); + + switch (ref.type) { + case EPOLL_TYPE_TAP: tap_handler(&c, ref.fd, events[i].events, &now); - else if (ref.type == EPOLL_TYPE_NSQUIT) + break; + case EPOLL_TYPE_NSQUIT: pasta_netns_quit_handler(&c, quit_fd); - else - sock_handler(&c, ref, events[i].events, &now); + break; + case EPOLL_TYPE_TCP: + if (!c.no_tcp) + tcp_sock_handler(&c, ref, eventmask, &now); + break; + case EPOLL_TYPE_UDP: + if (!c.no_udp) + udp_sock_handler(&c, ref, eventmask, &now); + break; + case EPOLL_TYPE_ICMP: + case EPOLL_TYPE_ICMPV6: + if (!c.no_icmp) + icmp_sock_handler(&c, ref, eventmask, &now); + break; + default: + /* Can't happen */ + ASSERT(0); + } } post_handler(&c, &now); -- 2.41.0

Stefano Brivio

9:49 p.m.

New subject: [PATCH v2 07/13] epoll: Fold sock_handler into general switch on epoll event fd

On Thu, 10 Aug 2023 12:33:09 +1000 David Gibson wrote:

...

When we process events from epoll_wait(), we check for a number of special cases before calling sock_handler() which then dispatches based on the protocol type of the socket in the event.

Fold these cases together into a single switch on the fd type recorded in the epoll data field.

Signed-off-by: David Gibson --- passt.c | 54 +++++++++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/passt.c b/passt.c index 45e9fbf..665262b 100644 --- a/passt.c +++ b/passt.c @@ -64,29 +64,6 @@ char *epoll_type_str[EPOLL_TYPE_MAX + 1] = { [EPOLL_TYPE_TAP] = "tap device", };

-/** - * sock_handler() - Event handler for L4 sockets - * @c: Execution context - * @ref: epoll reference - * @events: epoll events - * @now: Current timestamp - */ -static void sock_handler(struct ctx *c, union epoll_ref ref, - uint32_t events, const struct timespec *now) -{ - trace("%s: packet from %s %i (events: 0x%08x)", - c->mode == MODE_PASST ? "passt" : "pasta", - EPOLL_TYPE_STR(ref.type), ref.fd, events); - - if (!c->no_tcp && ref.type == EPOLL_TYPE_TCP) - tcp_sock_handler(c, ref, events, now); - else if (!c->no_udp && ref.type == EPOLL_TYPE_UDP) - udp_sock_handler(c, ref, events, now); - else if (!c->no_icmp && - (ref.type == EPOLL_TYPE_ICMP || ref.type == EPOLL_TYPE_ICMPV6)) - icmp_sock_handler(c, ref, events, now); -} - /** * post_handler() - Run periodic and deferred tasks for L4 protocol handlers * @c: Execution context @@ -330,13 +307,36 @@ loop:

for (i = 0; i < nfds; i++) { union epoll_ref ref = *((union epoll_ref *)&events[i].data.u64); + uint32_t eventmask = events[i].events;

- if (ref.type == EPOLL_TYPE_TAP) + trace("%s: epoll event on %s %i (events: 0x%08x)", + c.mode == MODE_PASST ? "passt" : "pasta", + EPOLL_TYPE_STR(ref.type), ref.fd, events);

Gah, sorry I missed this earlier, but covscan reported it -- you probably wanted to pass 'eventmask' to trace(). Actually, a long time ago, I was pondering about a macro that would print constants' names ("EPOLLIN", etc.) instead, but then I thought that once you remember the values from , hex values might be a bit easier on eyes when you're digging through thousands of them... or maybe not. I don't know actually. -- Stefano

David Gibson

11 Aug 11 Aug

5:11 a.m.

New subject: [PATCH v2 07/13] epoll: Fold sock_handler into general switch on epoll event fd

On Thu, Aug 10, 2023 at 09:49:37PM +0200, Stefano Brivio wrote:

...

On Thu, 10 Aug 2023 12:33:09 +1000 David Gibson wrote:

...
When we process events from epoll_wait(), we check for a number of special cases before calling sock_handler() which then dispatches based on the protocol type of the socket in the event.

Fold these cases together into a single switch on the fd type recorded in the epoll data field.

Signed-off-by: David Gibson --- passt.c | 54 +++++++++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/passt.c b/passt.c index 45e9fbf..665262b 100644 --- a/passt.c +++ b/passt.c @@ -64,29 +64,6 @@ char *epoll_type_str[EPOLL_TYPE_MAX + 1] = { [EPOLL_TYPE_TAP] = "tap device", };

-/** - * sock_handler() - Event handler for L4 sockets - * @c: Execution context - * @ref: epoll reference - * @events: epoll events - * @now: Current timestamp - */ -static void sock_handler(struct ctx *c, union epoll_ref ref, - uint32_t events, const struct timespec *now) -{ - trace("%s: packet from %s %i (events: 0x%08x)", - c->mode == MODE_PASST ? "passt" : "pasta", - EPOLL_TYPE_STR(ref.type), ref.fd, events); - - if (!c->no_tcp && ref.type == EPOLL_TYPE_TCP) - tcp_sock_handler(c, ref, events, now); - else if (!c->no_udp && ref.type == EPOLL_TYPE_UDP) - udp_sock_handler(c, ref, events, now); - else if (!c->no_icmp && - (ref.type == EPOLL_TYPE_ICMP || ref.type == EPOLL_TYPE_ICMPV6)) - icmp_sock_handler(c, ref, events, now); -} - /** * post_handler() - Run periodic and deferred tasks for L4 protocol handlers * @c: Execution context @@ -330,13 +307,36 @@ loop:

for (i = 0; i < nfds; i++) { union epoll_ref ref = *((union epoll_ref *)&events[i].data.u64); + uint32_t eventmask = events[i].events;

- if (ref.type == EPOLL_TYPE_TAP) + trace("%s: epoll event on %s %i (events: 0x%08x)", + c.mode == MODE_PASST ? "passt" : "pasta", + EPOLL_TYPE_STR(ref.type), ref.fd, events);

Gah, sorry I missed this earlier, but covscan reported it -- you probably wanted to pass 'eventmask' to trace().

Oops, good catch. Fixed for a new spin

...

Actually, a long time ago, I was pondering about a macro that would print constants' names ("EPOLLIN", etc.) instead, but then I thought that once you remember the values from , hex values might be a bit easier on eyes when you're digging through thousands of them... or maybe not. I don't know actually.

I'm inclined to leave it as hex for simplicity. The fact that we could have multiple events means it's not really simple macro territory; we'd have to format a bunch of stuff. I don't think that's worth bothering with for a trace message unless we repeatedly find it's making our debugging more difficult, which hasn't happened so far. -- 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

David Gibson

10 Aug 10 Aug

4:33 a.m.

New subject: [PATCH v2 08/13] epoll: Split handling of ICMP and ICMPv6 sockets

We have different epoll type values for ICMP and ICMPv6 sockets, but they both call the same handler function, icmp_sock_handler(). However that function does essentially nothing in common for the two cases. So, split it into icmp_sock_handler() and icmpv6_sock_handler() and dispatch them separately from the top level. While we're there remove some parameters that the function was never using anyway. Also move the test for c->no_icmp into the functions, so that all the logic specific to ICMP is within the handler, rather than in the top level dispatch code. Signed-off-by: David Gibson --- icmp.c | 112 ++++++++++++++++++++++++++++++++------------------------ icmp.h | 9 ++--- passt.c | 5 ++- 3 files changed, 70 insertions(+), 56 deletions(-) diff --git a/icmp.c b/icmp.c index a4b6a47..b676a1a 100644 --- a/icmp.c +++ b/icmp.c @@ -60,78 +60,94 @@ static struct icmp_id_sock icmp_id_map[IP_VERSIONS][ICMP_NUM_IDS]; static uint8_t icmp_act[IP_VERSIONS][DIV_ROUND_UP(ICMP_NUM_IDS, 8)]; /** - * icmp_sock_handler() - Handle new data from socket + * icmp_sock_handler() - Handle new data from IPv4 ICMP socket * @c: Execution context * @ref: epoll reference - * @events: epoll events bitmap - * @now: Current timestamp, unused */ -void icmp_sock_handler(const struct ctx *c, union epoll_ref ref, - uint32_t events, const struct timespec *now) +void icmp_sock_handler(const struct ctx *c, union epoll_ref ref) { - union icmp_epoll_ref *iref = &ref.icmp; - struct sockaddr_storage sr; - socklen_t sl = sizeof(sr); char buf[USHRT_MAX]; + struct icmphdr *ih = (struct icmphdr *)buf; + struct sockaddr_in sr; + socklen_t sl = sizeof(sr); uint16_t seq, id; ssize_t n; - (void)events; - (void)now; + if (c->no_icmp) + return; + /* FIXME: Workaround clang-tidy not realizing that recvfrom() + * writes the socket address. See + * https://github.com/llvm/llvm-project/issues/58992 + */ + memset(&sr, 0, sizeof(sr)); n = recvfrom(ref.fd, buf, sizeof(buf), 0, (struct sockaddr *)&sr, &sl); if (n < 0) return; - if (iref->v6) { - struct sockaddr_in6 *sr6 = (struct sockaddr_in6 *)&sr; - struct icmp6hdr *ih = (struct icmp6hdr *)buf; + id = ntohs(ih->un.echo.id); + seq = ntohs(ih->un.echo.sequence); - id = ntohs(ih->icmp6_identifier); - seq = ntohs(ih->icmp6_sequence); + if (id != ref.icmp.id) + ih->un.echo.id = htons(ref.icmp.id); - /* If bind() fails e.g. because of a broken SELinux policy, this - * might happen. Fix up the identifier to match the sent one. - */ - if (id != iref->id) - ih->icmp6_identifier = htons(iref->id); + if (c->mode == MODE_PASTA) { + if (icmp_id_map[V4][id].seq == seq) + return; - /* In PASTA mode, we'll get any reply we send, discard them. */ - if (c->mode == MODE_PASTA) { - if (icmp_id_map[V6][id].seq == seq) - return; - - icmp_id_map[V6][id].seq = seq; - } + icmp_id_map[V4][id].seq = seq; + } - debug("ICMPv6: echo %s to tap, ID: %i, seq: %i", - (ih->icmp6_type == 128) ? "request" : "reply", id, seq); + debug("ICMP: echo %s to tap, ID: %i, seq: %i", + (ih->type == ICMP_ECHO) ? "request" : "reply", id, seq); - tap_icmp6_send(c, &sr6->sin6_addr, - tap_ip6_daddr(c, &sr6->sin6_addr), buf, n); - } else { - struct sockaddr_in *sr4 = (struct sockaddr_in *)&sr; - struct icmphdr *ih = (struct icmphdr *)buf; + tap_icmp4_send(c, sr.sin_addr, tap_ip4_daddr(c), buf, n); +} - id = ntohs(ih->un.echo.id); - seq = ntohs(ih->un.echo.sequence); +/** + * icmpv6_sock_handler() - Handle new data from ICMPv6 socket + * @c: Execution context + * @ref: epoll reference + */ +void icmpv6_sock_handler(const struct ctx *c, union epoll_ref ref) +{ + char buf[USHRT_MAX]; + struct icmp6hdr *ih = (struct icmp6hdr *)buf; + struct sockaddr_in6 sr; + socklen_t sl = sizeof(sr); + uint16_t seq, id; + ssize_t n; - if (id != iref->id) - ih->un.echo.id = htons(iref->id); + if (c->no_icmp) + return; - if (c->mode == MODE_PASTA) { + n = recvfrom(ref.fd, buf, sizeof(buf), 0, (struct sockaddr *)&sr, &sl); + if (n < 0) + return; - if (icmp_id_map[V4][id].seq == seq) - return; + id = ntohs(ih->icmp6_identifier); + seq = ntohs(ih->icmp6_sequence); - icmp_id_map[V4][id].seq = seq; - } + /* If bind() fails e.g. because of a broken SELinux policy, + * this might happen. Fix up the identifier to match the sent + * one. + */ + if (id != ref.icmp.id) + ih->icmp6_identifier = htons(ref.icmp.id); - debug("ICMP: echo %s to tap, ID: %i, seq: %i", - (ih->type == ICMP_ECHO) ? "request" : "reply", id, seq); + /* In PASTA mode, we'll get any reply we send, discard them. */ + if (c->mode == MODE_PASTA) { + if (icmp_id_map[V6][id].seq == seq) + return; - tap_icmp4_send(c, sr4->sin_addr, tap_ip4_daddr(c), buf, n); + icmp_id_map[V6][id].seq = seq; } + + debug("ICMPv6: echo %s to tap, ID: %i, seq: %i", + (ih->icmp6_type == 128) ? "request" : "reply", id, seq); + + tap_icmp6_send(c, &sr.sin6_addr, + tap_ip6_daddr(c, &sr.sin6_addr), buf, n); } /** @@ -150,11 +166,11 @@ int icmp_tap_handler(const struct ctx *c, int af, const void *addr, size_t plen; if (af == AF_INET) { - union icmp_epoll_ref iref = { .v6 = 0 }; struct sockaddr_in sa = { .sin_family = AF_INET, .sin_addr = { .s_addr = htonl(INADDR_ANY) }, }; + union icmp_epoll_ref iref; struct icmphdr *ih; int id, s; @@ -204,12 +220,12 @@ int icmp_tap_handler(const struct ctx *c, int af, const void *addr, id, ntohs(ih->un.echo.sequence)); } } else if (af == AF_INET6) { - union icmp_epoll_ref iref = { .v6 = 1 }; struct sockaddr_in6 sa = { .sin6_family = AF_INET6, .sin6_addr = IN6ADDR_ANY_INIT, .sin6_scope_id = c->ifi6, }; + union icmp_epoll_ref iref; struct icmp6hdr *ih; int id, s; diff --git a/icmp.h b/icmp.h index 29c7829..32f0c47 100644 --- a/icmp.h +++ b/icmp.h @@ -10,8 +10,8 @@ struct ctx; -void icmp_sock_handler(const struct ctx *c, union epoll_ref ref, - uint32_t events, const struct timespec *now); +void icmp_sock_handler(const struct ctx *c, union epoll_ref ref); +void icmpv6_sock_handler(const struct ctx *c, union epoll_ref ref); int icmp_tap_handler(const struct ctx *c, int af, const void *addr, const struct pool *p, const struct timespec *now); void icmp_timer(const struct ctx *c, const struct timespec *ts); @@ -24,10 +24,7 @@ void icmp_init(void); * @id: Associated echo identifier, needed if bind() fails */ union icmp_epoll_ref { - struct { - uint32_t v6:1, - id:16; - }; + uint16_t id; uint32_t u32; }; diff --git a/passt.c b/passt.c index 665262b..f7cd376 100644 --- a/passt.c +++ b/passt.c @@ -329,9 +329,10 @@ loop: udp_sock_handler(&c, ref, eventmask, &now); break; case EPOLL_TYPE_ICMP: + icmp_sock_handler(&c, ref); + break; case EPOLL_TYPE_ICMPV6: - if (!c.no_icmp) - icmp_sock_handler(&c, ref, eventmask, &now); + icmpv6_sock_handler(&c, ref); break; default: /* Can't happen */ -- 2.41.0

David Gibson

4:33 a.m.

New subject: [PATCH v2 09/13] epoll: Tiny cleanup to udp_sock_handler()

Move the test for c->no_udp into the function itself, rather than in the dispatching switch statement to better localize the UDP specific logic, and make for greated consistency with other handler functions. Signed-off-by: David Gibson --- passt.c | 3 +-- udp.c | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/passt.c b/passt.c index f7cd376..24bd825 100644 --- a/passt.c +++ b/passt.c @@ -325,8 +325,7 @@ loop: tcp_sock_handler(&c, ref, eventmask, &now); break; case EPOLL_TYPE_UDP: - if (!c.no_udp) - udp_sock_handler(&c, ref, eventmask, &now); + udp_sock_handler(&c, ref, eventmask, &now); break; case EPOLL_TYPE_ICMP: icmp_sock_handler(&c, ref); diff --git a/udp.c b/udp.c index 62f8360..138e7ab 100644 --- a/udp.c +++ b/udp.c @@ -756,7 +756,7 @@ void udp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events, struct mmsghdr *mmh_recv; int i, m; - if (!(events & EPOLLIN)) + if (c->no_udp || !(events & EPOLLIN)) return; if (v6) { -- 2.41.0

David Gibson

4:33 a.m.

New subject: [PATCH v2 10/13] epoll: Split handling of TCP timerfds into its own handler function

tcp_sock_handler() actually handles several different types of fd events. This includes timerfds that aren't sockets at all. The handling of these has essentially nothing in common with the other cases. So, give the TCP timers there own epoll_type value and dispatch directly to their handler. This also means we can remove the timer field from tcp_epoll_ref, the information it encoded is now implicit in the epoll_type value. Signed-off-by: David Gibson --- passt.c | 4 ++++ passt.h | 4 +++- tcp.c | 15 ++++----------- tcp.h | 3 +-- 4 files changed, 12 insertions(+), 14 deletions(-) diff --git a/passt.c b/passt.c index 24bd825..57ef767 100644 --- a/passt.c +++ b/passt.c @@ -57,6 +57,7 @@ char pkt_buf[PKT_BUF_BYTES] __attribute__ ((aligned(PAGE_SIZE))); char *epoll_type_str[EPOLL_TYPE_MAX + 1] = { [EPOLL_TYPE_TCP] = "TCP socket", + [EPOLL_TYPE_TCP_TIMER] = "TCP timer", [EPOLL_TYPE_UDP] = "UDP socket", [EPOLL_TYPE_ICMP] = "ICMP socket", [EPOLL_TYPE_ICMPV6] = "ICMPv6 socket", @@ -324,6 +325,9 @@ loop: if (!c.no_tcp) tcp_sock_handler(&c, ref, eventmask, &now); break; + case EPOLL_TYPE_TCP_TIMER: + tcp_timer_handler(&c, ref); + break; case EPOLL_TYPE_UDP: udp_sock_handler(&c, ref, eventmask, &now); break; diff --git a/passt.h b/passt.h index 8878a11..fc1efdb 100644 --- a/passt.h +++ b/passt.h @@ -47,8 +47,10 @@ union epoll_ref; enum epoll_type { /* Special value to indicate an invalid type */ EPOLL_TYPE_NONE = 0, - /* Sockets and timerfds for TCP handling */ + /* TCP sockets */ EPOLL_TYPE_TCP, + /* timerfds used for TCP timers */ + EPOLL_TYPE_TCP_TIMER, /* UDP sockets */ EPOLL_TYPE_UDP, /* IPv4 ICMP sockets */ diff --git a/tcp.c b/tcp.c index 18b781a..98761a2 100644 --- a/tcp.c +++ b/tcp.c @@ -663,9 +663,8 @@ static int tcp_epoll_ctl(const struct ctx *c, struct tcp_tap_conn *conn) conn->c.in_epoll = true; if (conn->timer != -1) { - union epoll_ref ref_t = { .type = EPOLL_TYPE_TCP, + union epoll_ref ref_t = { .type = EPOLL_TYPE_TCP_TIMER, .fd = conn->sock, - .tcp.timer = 1, .tcp.index = CONN_IDX(conn) }; struct epoll_event ev_t = { .data.u64 = ref_t.u64, .events = EPOLLIN | EPOLLET }; @@ -692,9 +691,8 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_tap_conn *conn) return; if (conn->timer == -1) { - union epoll_ref ref = { .type = EPOLL_TYPE_TCP, + union epoll_ref ref = { .type = EPOLL_TYPE_TCP_TIMER, .fd = conn->sock, - .tcp.timer = 1, .tcp.index = CONN_IDX(conn) }; struct epoll_event ev = { .data.u64 = ref.u64, .events = EPOLLIN | EPOLLET }; @@ -2813,12 +2811,12 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref, * * #syscalls timerfd_gettime */ -static void tcp_timer_handler(struct ctx *c, union epoll_ref ref) +void tcp_timer_handler(struct ctx *c, union epoll_ref ref) { struct tcp_tap_conn *conn = conn_at_idx(ref.tcp.index); struct itimerspec check_armed = { { 0 }, { 0 } }; - if (!conn) + if (c->no_tcp || !conn) return; /* We don't reset timers on ~ACK_FROM_TAP_DUE, ~ACK_TO_TAP_DUE. If the @@ -2935,11 +2933,6 @@ void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events, { union tcp_conn *conn; - if (ref.tcp.timer) { - tcp_timer_handler(c, ref); - return; - } - if (ref.tcp.listen) { tcp_conn_from_sock(c, ref, now); return; diff --git a/tcp.h b/tcp.h index 5499127..8eb7782 100644 --- a/tcp.h +++ b/tcp.h @@ -13,6 +13,7 @@ struct ctx; +void tcp_timer_handler(struct ctx *c, union epoll_ref ref); void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events, const struct timespec *now); int tcp_tap_handler(struct ctx *c, int af, const void *addr, @@ -31,7 +32,6 @@ void tcp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s, * union tcp_epoll_ref - epoll reference portion for TCP connections * @listen: Set if this file descriptor is a listening socket * @outbound: Listening socket maps to outbound, spliced connection - * @timer: Reference is a timerfd descriptor for connection * @index: Index of connection in table, or port for bound sockets * @u32: Opaque u32 value of reference */ @@ -39,7 +39,6 @@ union tcp_epoll_ref { struct { uint32_t listen:1, outbound:1, - timer:1, index:20; }; uint32_t u32; -- 2.41.0

David Gibson

4:33 a.m.

New subject: [PATCH v2 11/13] epoll: Split handling of listening TCP sockets into their own handler

tcp_sock_handler() handles both listening TCP sockets, and connected TCP sockets, but what it needs to do in those cases has essentially nothing in common. Therefore, give listening sockets their own epoll_type value and dispatch directly to their own handler from the top level. Furthermore, the two handlers need essentially entirely different information from the reference: we re-(ab)used the index field in the tcp_epoll_ref to indicate the port for the listening socket, but that's not the same meaning. So, switch listening sockets to their own reference type which we can lay out as we please. That lets us remove the listen and outbound fields from the normal (connected) tcp_epoll_ref, reducing it to just the connection table index. Signed-off-by: David Gibson --- passt.c | 8 ++++++-- passt.h | 8 ++++++-- tcp.c | 51 ++++++++++++++++++++++----------------------------- tcp.h | 25 +++++++++++++++++-------- tcp_splice.c | 4 ++-- tcp_splice.h | 2 +- util.c | 2 +- 7 files changed, 55 insertions(+), 45 deletions(-) diff --git a/passt.c b/passt.c index 57ef767..2a207b4 100644 --- a/passt.c +++ b/passt.c @@ -56,7 +56,8 @@ char pkt_buf[PKT_BUF_BYTES] __attribute__ ((aligned(PAGE_SIZE))); char *epoll_type_str[EPOLL_TYPE_MAX + 1] = { - [EPOLL_TYPE_TCP] = "TCP socket", + [EPOLL_TYPE_TCP] = "connected TCP socket", + [EPOLL_TYPE_TCP_LISTEN] = "listening TCP socket", [EPOLL_TYPE_TCP_TIMER] = "TCP timer", [EPOLL_TYPE_UDP] = "UDP socket", [EPOLL_TYPE_ICMP] = "ICMP socket", @@ -323,7 +324,10 @@ loop: break; case EPOLL_TYPE_TCP: if (!c.no_tcp) - tcp_sock_handler(&c, ref, eventmask, &now); + tcp_sock_handler(&c, ref, eventmask); + break; + case EPOLL_TYPE_TCP_LISTEN: + tcp_listen_handler(&c, ref, &now); break; case EPOLL_TYPE_TCP_TIMER: tcp_timer_handler(&c, ref); diff --git a/passt.h b/passt.h index fc1efdb..a625d48 100644 --- a/passt.h +++ b/passt.h @@ -47,8 +47,10 @@ union epoll_ref; enum epoll_type { /* Special value to indicate an invalid type */ EPOLL_TYPE_NONE = 0, - /* TCP sockets */ + /* Connected TCP sockets */ EPOLL_TYPE_TCP, + /* Listening TCP sockets */ + EPOLL_TYPE_TCP_LISTEN, /* timerfds used for TCP timers */ EPOLL_TYPE_TCP_TIMER, /* UDP sockets */ @@ -69,7 +71,8 @@ enum epoll_type { * union epoll_ref - Breakdown of reference for epoll fd bookkeeping * @type: Type of fd (tells us what to do with events) * @fd: File descriptor number (implies < 2^24 total descriptors) - * @tcp: TCP-specific reference part + * @tcp: TCP-specific reference part (connected sockets) + * @tcp_listen: TCP-specific reference part (listening sockets) * @udp: UDP-specific reference part * @icmp: ICMP-specific reference part * @data: Data handled by protocol handlers @@ -83,6 +86,7 @@ union epoll_ref { int32_t fd:FD_REF_BITS; union { union tcp_epoll_ref tcp; + union tcp_listen_epoll_ref tcp_listen; union udp_epoll_ref udp; union icmp_epoll_ref icmp; uint32_t data; diff --git a/tcp.c b/tcp.c index 98761a2..0322842 100644 --- a/tcp.c +++ b/tcp.c @@ -2735,7 +2735,8 @@ static void tcp_snat_inbound(const struct ctx *c, union inany_addr *addr) * @sa: Peer socket address (from accept()) * @now: Current timestamp */ -static void tcp_tap_conn_from_sock(struct ctx *c, union epoll_ref ref, +static void tcp_tap_conn_from_sock(struct ctx *c, + union tcp_listen_epoll_ref ref, struct tcp_tap_conn *conn, int s, struct sockaddr *sa, const struct timespec *now) @@ -2747,7 +2748,7 @@ static void tcp_tap_conn_from_sock(struct ctx *c, union epoll_ref ref, conn_event(c, conn, SOCK_ACCEPTED); inany_from_sockaddr(&conn->addr, &conn->sock_port, sa); - conn->tap_port = ref.tcp.index; + conn->tap_port = ref.port; tcp_snat_inbound(c, &conn->addr); @@ -2765,22 +2766,20 @@ static void tcp_tap_conn_from_sock(struct ctx *c, union epoll_ref ref, } /** - * tcp_conn_from_sock() - Handle new connection request from listening socket + * tcp_listen_handler() - Handle new connection request from listening socket * @c: Execution context * @ref: epoll reference of listening socket * @now: Current timestamp */ -static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref, - const struct timespec *now) +void tcp_listen_handler(struct ctx *c, union epoll_ref ref, + const struct timespec *now) { struct sockaddr_storage sa; union tcp_conn *conn; socklen_t sl; int s; - ASSERT(ref.tcp.listen); - - if (c->tcp.conn_count >= TCP_MAX_CONNS) + if (c->no_tcp || c->tcp.conn_count >= TCP_MAX_CONNS) return; sl = sizeof(sa); @@ -2796,11 +2795,11 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref, conn = tc + c->tcp.conn_count++; if (c->mode == MODE_PASTA && - tcp_splice_conn_from_sock(c, ref, &conn->splice, + tcp_splice_conn_from_sock(c, ref.tcp_listen, &conn->splice, s, (struct sockaddr *)&sa)) return; - tcp_tap_conn_from_sock(c, ref, &conn->tap, s, + tcp_tap_conn_from_sock(c, ref.tcp_listen, &conn->tap, s, (struct sockaddr *)&sa, now); } @@ -2926,19 +2925,10 @@ static void tcp_tap_sock_handler(struct ctx *c, struct tcp_tap_conn *conn, * @c: Execution context * @ref: epoll reference * @events: epoll events bitmap - * @now: Current timestamp */ -void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events, - const struct timespec *now) +void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events) { - union tcp_conn *conn; - - if (ref.tcp.listen) { - tcp_conn_from_sock(c, ref, now); - return; - } - - conn = tc + ref.tcp.index; + union tcp_conn *conn = tc + ref.tcp.index; if (conn->c.spliced) tcp_splice_sock_handler(c, &conn->splice, ref.fd, events); @@ -2959,8 +2949,9 @@ void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events, static int tcp_sock_init_af(const struct ctx *c, int af, in_port_t port, const struct in_addr *addr, const char *ifname) { - in_port_t idx = port + c->tcp.fwd_in.delta[port]; - union tcp_epoll_ref tref = { .listen = 1, .index = idx }; + union tcp_listen_epoll_ref tref = { + .port = port + c->tcp.fwd_in.delta[port], + }; int s; s = sock_l4(c, af, IPPROTO_TCP, addr, ifname, port, tref.u32); @@ -3019,9 +3010,10 @@ int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr, */ static void tcp_ns_sock_init4(const struct ctx *c, in_port_t port) { - in_port_t idx = port + c->tcp.fwd_out.delta[port]; - union tcp_epoll_ref tref = { .listen = 1, .outbound = 1, - .index = idx }; + union tcp_listen_epoll_ref tref = { + .port = port + c->tcp.fwd_out.delta[port], + .ns = true, + }; struct in_addr loopback = { htonl(INADDR_LOOPBACK) }; int s; @@ -3044,9 +3036,10 @@ static void tcp_ns_sock_init4(const struct ctx *c, in_port_t port) */ static void tcp_ns_sock_init6(const struct ctx *c, in_port_t port) { - in_port_t idx = port + c->tcp.fwd_out.delta[port]; - union tcp_epoll_ref tref = { .listen = 1, .outbound = 1, - .index = idx }; + union tcp_listen_epoll_ref tref = { + .port = port + c->tcp.fwd_out.delta[port], + .ns = true, + }; int s; ASSERT(c->mode == MODE_PASTA); diff --git a/tcp.h b/tcp.h index 8eb7782..be296ec 100644 --- a/tcp.h +++ b/tcp.h @@ -14,8 +14,9 @@ struct ctx; void tcp_timer_handler(struct ctx *c, union epoll_ref ref); -void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events, - const struct timespec *now); +void tcp_listen_handler(struct ctx *c, union epoll_ref ref, + const struct timespec *now); +void tcp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events); int tcp_tap_handler(struct ctx *c, int af, const void *addr, const struct pool *p, const struct timespec *now); int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr, @@ -30,16 +31,24 @@ void tcp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s, /** * union tcp_epoll_ref - epoll reference portion for TCP connections - * @listen: Set if this file descriptor is a listening socket - * @outbound: Listening socket maps to outbound, spliced connection - * @index: Index of connection in table, or port for bound sockets + * @index: Index of connection in table * @u32: Opaque u32 value of reference */ union tcp_epoll_ref { + uint32_t index:20; + uint32_t u32; +}; + +/** + * union tcp_listen_epoll_ref - epoll reference portion for TCP listening + * @port: Port number we're forwarding *to* (listening port plus delta) + * @ns: True if listening within the pasta namespace + * @u32: Opaque u32 value of reference + */ +union tcp_listen_epoll_ref { struct { - uint32_t listen:1, - outbound:1, - index:20; + in_port_t port; + bool ns; }; uint32_t u32; }; diff --git a/tcp_splice.c b/tcp_splice.c index 24995e2..64c1263 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -486,7 +486,7 @@ static void tcp_splice_dir(struct tcp_splice_conn *conn, int ref_sock, * Return: true if able to create a spliced connection, false otherwise * #syscalls:pasta setsockopt */ -bool tcp_splice_conn_from_sock(struct ctx *c, union epoll_ref ref, +bool tcp_splice_conn_from_sock(struct ctx *c, union tcp_listen_epoll_ref ref, struct tcp_splice_conn *conn, int s, const struct sockaddr *sa) { @@ -516,7 +516,7 @@ bool tcp_splice_conn_from_sock(struct ctx *c, union epoll_ref ref, c->tcp.splice_conn_count++; conn->a = s; - if (tcp_splice_new(c, conn, ref.tcp.index, ref.tcp.outbound)) + if (tcp_splice_new(c, conn, ref.port, ref.ns)) conn_flag(c, conn, CLOSING); return true; diff --git a/tcp_splice.h b/tcp_splice.h index 99dbac8..e7a583a 100644 --- a/tcp_splice.h +++ b/tcp_splice.h @@ -10,7 +10,7 @@ struct tcp_splice_conn; void tcp_splice_sock_handler(struct ctx *c, struct tcp_splice_conn *conn, int s, uint32_t events); -bool tcp_splice_conn_from_sock(struct ctx *c, union epoll_ref ref, +bool tcp_splice_conn_from_sock(struct ctx *c, union tcp_listen_epoll_ref ref, struct tcp_splice_conn *conn, int s, const struct sockaddr *sa); void tcp_splice_init(struct ctx *c); diff --git a/util.c b/util.c index 2cac7ba..d965f48 100644 --- a/util.c +++ b/util.c @@ -120,7 +120,7 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto, switch (proto) { case IPPROTO_TCP: - ref.type = EPOLL_TYPE_TCP; + ref.type = EPOLL_TYPE_TCP_LISTEN; break; case IPPROTO_UDP: ref.type = EPOLL_TYPE_UDP; -- 2.41.0

David Gibson

4:33 a.m.

New subject: [PATCH v2 12/13] epoll: Split listening Unix domain socket into its own type

tap_handler() actually handles events on three different types of object: the /dev/tap character device (pasta), a connected Unix domain socket (passt) or a listening Unix domain socket (passt). The last, in particular, really has no handling in common with the others, so split it into its own epoll type and directly dispatch to the relevant handler from the top level. Signed-off-by: David Gibson --- passt.c | 6 +++++- passt.h | 6 ++++-- tap.c | 15 ++++----------- tap.h | 4 ++-- 4 files changed, 15 insertions(+), 16 deletions(-) diff --git a/passt.c b/passt.c index 2a207b4..a4e6341 100644 --- a/passt.c +++ b/passt.c @@ -64,6 +64,7 @@ char *epoll_type_str[EPOLL_TYPE_MAX + 1] = { [EPOLL_TYPE_ICMPV6] = "ICMPv6 socket", [EPOLL_TYPE_NSQUIT] = "namespace inotify", [EPOLL_TYPE_TAP] = "tap device", + [EPOLL_TYPE_TAP_LISTEN] = "listening qemu socket", }; /** @@ -317,7 +318,10 @@ loop: switch (ref.type) { case EPOLL_TYPE_TAP: - tap_handler(&c, ref.fd, events[i].events, &now); + tap_handler(&c, events[i].events, &now); + break; + case EPOLL_TYPE_TAP_LISTEN: + tap_listen_handler(&c, eventmask); break; case EPOLL_TYPE_NSQUIT: pasta_netns_quit_handler(&c, quit_fd); diff --git a/passt.h b/passt.h index a625d48..dbadec2 100644 --- a/passt.h +++ b/passt.h @@ -61,10 +61,12 @@ enum epoll_type { EPOLL_TYPE_ICMPV6, /* inotify fd watching for end of netns (pasta) */ EPOLL_TYPE_NSQUIT, - /* tap char device, or qemu socket fd */ + /* tap char device, or connected qemu socket fd */ EPOLL_TYPE_TAP, + /* socket listening for qemu socket connections */ + EPOLL_TYPE_TAP_LISTEN, - EPOLL_TYPE_MAX = EPOLL_TYPE_TAP, + EPOLL_TYPE_MAX = EPOLL_TYPE_TAP_LISTEN, }; /** diff --git a/tap.c b/tap.c index 6c3ce48..ceedaaa 100644 --- a/tap.c +++ b/tap.c @@ -1076,7 +1076,7 @@ restart: static void tap_sock_unix_init(struct ctx *c) { int fd = socket(AF_UNIX, SOCK_STREAM, 0); - union epoll_ref ref = { .type = EPOLL_TYPE_TAP }; + union epoll_ref ref = { .type = EPOLL_TYPE_TAP_LISTEN }; struct epoll_event ev = { 0 }; struct sockaddr_un addr = { .sun_family = AF_UNIX, @@ -1142,11 +1142,11 @@ static void tap_sock_unix_init(struct ctx *c) } /** - * tap_sock_unix_new() - Handle new connection on listening socket + * tap_listen_handler() - Handle new connection on listening socket * @c: Execution context * @events: epoll events */ -static void tap_sock_unix_new(struct ctx *c, uint32_t events) +void tap_listen_handler(struct ctx *c, uint32_t events) { union epoll_ref ref = { .type = EPOLL_TYPE_TAP }; struct epoll_event ev = { 0 }; @@ -1290,18 +1290,11 @@ void tap_sock_init(struct ctx *c) /** * tap_handler() - Packet handler for AF_UNIX or tuntap file descriptor * @c: Execution context - * @fd: File descriptor where event occurred * @events: epoll events * @now: Current timestamp, can be NULL on EPOLLERR */ -void tap_handler(struct ctx *c, int fd, uint32_t events, - const struct timespec *now) +void tap_handler(struct ctx *c, uint32_t events, const struct timespec *now) { - if (fd == c->fd_tap_listen) { - tap_sock_unix_new(c, events); - return; - } - if (c->mode == MODE_PASST) tap_handler_passt(c, events, now); else if (c->mode == MODE_PASTA) diff --git a/tap.h b/tap.h index a1ad115..dd6bfdd 100644 --- a/tap.h +++ b/tap.h @@ -76,8 +76,8 @@ int tap_send(const struct ctx *c, const void *data, size_t len); void tap_send_frames(struct ctx *c, const struct iovec *iov, size_t n); void tap_update_mac(struct tap_hdr *taph, const unsigned char *eth_d, const unsigned char *eth_s); -void tap_handler(struct ctx *c, int fd, uint32_t events, - const struct timespec *now); +void tap_listen_handler(struct ctx *c, uint32_t events); +void tap_handler(struct ctx *c, uint32_t events, const struct timespec *now); void tap_sock_init(struct ctx *c); #endif /* TAP_H */ -- 2.41.0

David Gibson

4:33 a.m.

New subject: [PATCH v2 13/13] epoll: Use different epoll types for passt and pasta tap fds

Currently we have a single epoll event type for the "tap" fd, which could be either a handle on a /dev/net/tun device (pasta) or a connected Unix socket (passt). However for the two modes we call different handler functions. Simplify this a little by using different epoll types and dispatching directly to the correct handler function. Signed-off-by: David Gibson --- passt.c | 10 +++++++--- passt.h | 6 ++++-- tap.c | 39 +++++++++++++++------------------------ tap.h | 5 ++++- 4 files changed, 30 insertions(+), 30 deletions(-) diff --git a/passt.c b/passt.c index a4e6341..26a1dac 100644 --- a/passt.c +++ b/passt.c @@ -63,7 +63,8 @@ char *epoll_type_str[EPOLL_TYPE_MAX + 1] = { [EPOLL_TYPE_ICMP] = "ICMP socket", [EPOLL_TYPE_ICMPV6] = "ICMPv6 socket", [EPOLL_TYPE_NSQUIT] = "namespace inotify", - [EPOLL_TYPE_TAP] = "tap device", + [EPOLL_TYPE_TAP_PASTA] = "/dev/net/tun device", + [EPOLL_TYPE_TAP_PASST] = "connected qemu socket", [EPOLL_TYPE_TAP_LISTEN] = "listening qemu socket", }; @@ -317,8 +318,11 @@ loop: EPOLL_TYPE_STR(ref.type), ref.fd, events); switch (ref.type) { - case EPOLL_TYPE_TAP: - tap_handler(&c, events[i].events, &now); + case EPOLL_TYPE_TAP_PASTA: + tap_handler_pasta(&c, eventmask, &now); + break; + case EPOLL_TYPE_TAP_PASST: + tap_handler_passt(&c, eventmask, &now); break; case EPOLL_TYPE_TAP_LISTEN: tap_listen_handler(&c, eventmask); diff --git a/passt.h b/passt.h index dbadec2..0500ff0 100644 --- a/passt.h +++ b/passt.h @@ -61,8 +61,10 @@ enum epoll_type { EPOLL_TYPE_ICMPV6, /* inotify fd watching for end of netns (pasta) */ EPOLL_TYPE_NSQUIT, - /* tap char device, or connected qemu socket fd */ - EPOLL_TYPE_TAP, + /* tuntap character device */ + EPOLL_TYPE_TAP_PASTA, + /* socket connected to qemu */ + EPOLL_TYPE_TAP_PASST, /* socket listening for qemu socket connections */ EPOLL_TYPE_TAP_LISTEN, diff --git a/tap.c b/tap.c index ceedaaa..8373bc4 100644 --- a/tap.c +++ b/tap.c @@ -914,8 +914,8 @@ static void tap_sock_reset(struct ctx *c) * @events: epoll events * @now: Current timestamp */ -static void tap_handler_passt(struct ctx *c, uint32_t events, - const struct timespec *now) +void tap_handler_passt(struct ctx *c, uint32_t events, + const struct timespec *now) { struct ethhdr *eh; ssize_t n, rem; @@ -1001,13 +1001,13 @@ next: } /** - * tap_handler_pasta() - Packet handler for tuntap file descriptor + * tap_handler_pasta() - Packet handler for /dev/net/tun file descriptor * @c: Execution context * @events: epoll events * @now: Current timestamp */ -static void tap_handler_pasta(struct ctx *c, uint32_t events, - const struct timespec *now) +void tap_handler_pasta(struct ctx *c, uint32_t events, + const struct timespec *now) { ssize_t n, len; int ret; @@ -1148,7 +1148,7 @@ static void tap_sock_unix_init(struct ctx *c) */ void tap_listen_handler(struct ctx *c, uint32_t events) { - union epoll_ref ref = { .type = EPOLL_TYPE_TAP }; + union epoll_ref ref = { .type = EPOLL_TYPE_TAP_PASST }; struct epoll_event ev = { 0 }; int v = INT_MAX / 2; struct ucred ucred; @@ -1230,12 +1230,12 @@ static int tap_ns_tun(void *arg) } /** - * tap_sock_init_tun() - Set up tuntap file descriptor + * tap_sock_tun_init() - Set up /dev/net/tun file descriptor * @c: Execution context */ static void tap_sock_tun_init(struct ctx *c) { - union epoll_ref ref = { .type = EPOLL_TYPE_TAP }; + union epoll_ref ref = { .type = EPOLL_TYPE_TAP_PASTA }; struct epoll_event ev = { 0 }; NS_CALL(tap_ns_tun, c); @@ -1268,11 +1268,16 @@ void tap_sock_init(struct ctx *c) } if (c->fd_tap != -1) { /* Passed as --fd */ - union epoll_ref ref = { .type = EPOLL_TYPE_TAP }; struct epoll_event ev = { 0 }; - ASSERT(c->one_off); + union epoll_ref ref; + ASSERT(c->one_off); ref.fd = c->fd_tap; + if (c->mode == MODE_PASST) + ref.type = EPOLL_TYPE_TAP_PASST; + else + ref.type = EPOLL_TYPE_TAP_PASTA; + ev.events = EPOLLIN | EPOLLET | EPOLLRDHUP; ev.data.u64 = ref.u64; epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap, &ev); @@ -1286,17 +1291,3 @@ void tap_sock_init(struct ctx *c) tap_sock_tun_init(c); } } - -/** - * tap_handler() - Packet handler for AF_UNIX or tuntap file descriptor - * @c: Execution context - * @events: epoll events - * @now: Current timestamp, can be NULL on EPOLLERR - */ -void tap_handler(struct ctx *c, uint32_t events, const struct timespec *now) -{ - if (c->mode == MODE_PASST) - tap_handler_passt(c, events, now); - else if (c->mode == MODE_PASTA) - tap_handler_pasta(c, events, now); -} diff --git a/tap.h b/tap.h index dd6bfdd..021fb7c 100644 --- a/tap.h +++ b/tap.h @@ -77,7 +77,10 @@ void tap_send_frames(struct ctx *c, const struct iovec *iov, size_t n); void tap_update_mac(struct tap_hdr *taph, const unsigned char *eth_d, const unsigned char *eth_s); void tap_listen_handler(struct ctx *c, uint32_t events); -void tap_handler(struct ctx *c, uint32_t events, const struct timespec *now); +void tap_handler_pasta(struct ctx *c, uint32_t events, + const struct timespec *now); +void tap_handler_passt(struct ctx *c, uint32_t events, + const struct timespec *now); void tap_sock_init(struct ctx *c); #endif /* TAP_H */ -- 2.41.0

652

Age (days ago)

653

Last active (days ago)

List overview

Download

17 comments

2 participants

participants (2)

David Gibson
Stefano Brivio

[PATCH v2 00/13] Clean up to tap errors and epoll dispatch

tags

participants (2)