- dev - passt

Re: [PATCH v5 15/19] icmp: Use flowsides as the source of truth wherever possible
by Stefano Brivio 18 May '24

18 May '24

On Fri, 17 May 2024 16:58:38 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote: > On Thu, May 16, 2024 at 10:53:50PM +0200, Stefano Brivio wrote: > > On Tue, 14 May 2024 11:03:33 +1000 > > David Gibson <david(a)gibson.dropbear.id.au> wrote: > > > > > icmp_sock_handler() obtains the guest address from it's most recently > > > observed IP, and the ICMP id from the epoll reference. Both of these > > > can be obtained readily from the flow. > > > > > > icmp_tap_handler() builds its socket address for sendto() directly > > > from the destination address supplied by the incoming tap packet. > > > This can instead be generated from the flow. > > > > > > struct icmp_ping_flow contains a field for the ICMP id of the ping, but > > > this is now redundant, since the id is also stored as the "port" in the > > > common flowsides. > > > > > > Using the flowsides as the common source of truth here prepares us for > > > allowing more flexible NAT and forwarding by properly initialising > > > that flowside information. > > > > > > Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> > > > --- > > > icmp.c | 37 ++++++++++++++++++++++--------------- > > > icmp_flow.h | 1 - > > > tap.c | 11 ----------- > > > tap.h | 1 - > > > 4 files changed, 22 insertions(+), 28 deletions(-) > > > > > > diff --git a/icmp.c b/icmp.c > > > index 37a3586..1e9a05e 100644 > > > --- a/icmp.c > > > +++ b/icmp.c > > > @@ -58,6 +58,7 @@ static struct icmp_ping_flow *icmp_id_map[IP_VERSIONS][ICMP_NUM_IDS]; > > > void icmp_sock_handler(const struct ctx *c, union epoll_ref ref) > > > { > > > struct icmp_ping_flow *pingf = PINGF(ref.flowside.flow); > > > + const struct flowside *ini = &pingf->f.side[INISIDE]; > > > union sockaddr_inany sr; > > > socklen_t sl = sizeof(sr); > > > char buf[USHRT_MAX]; > > > @@ -83,7 +84,7 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref) > > > goto unexpected; > > > > > > /* Adjust packet back to guest-side ID */ > > > - ih4->un.echo.id = htons(pingf->id); > > > + ih4->un.echo.id = htons(ini->eport); > > > seq = ntohs(ih4->un.echo.sequence); > > > } else if (pingf->f.type == FLOW_PING6) { > > > struct icmp6hdr *ih6 = (struct icmp6hdr *)buf; > > > @@ -93,7 +94,7 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref) > > > goto unexpected; > > > > > > /* Adjust packet back to guest-side ID */ > > > - ih6->icmp6_identifier = htons(pingf->id); > > > + ih6->icmp6_identifier = htons(ini->eport); > > > seq = ntohs(ih6->icmp6_sequence); > > > } else { > > > ASSERT(0); > > > @@ -108,13 +109,20 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref) > > > } > > > > > > flow_dbg(pingf, "echo reply to tap, ID: %"PRIu16", seq: %"PRIu16, > > > - pingf->id, seq); > > > + ini->eport, seq); > > > > > > - if (pingf->f.type == FLOW_PING4) > > > - tap_icmp4_send(c, sr.sa4.sin_addr, tap_ip4_daddr(c), buf, n); > > > - else if (pingf->f.type == FLOW_PING6) > > > - tap_icmp6_send(c, &sr.sa6.sin6_addr, > > > - tap_ip6_daddr(c, &sr.sa6.sin6_addr), buf, n); > > > + if (pingf->f.type == FLOW_PING4) { > > > + const struct in_addr *saddr = inany_v4(&ini->faddr); > > > + const struct in_addr *daddr = inany_v4(&ini->eaddr); > > > + > > > + ASSERT(saddr && daddr); /* Must have IPv4 addresses */ > > > > ...are those somehow special compared to IPv6 ones? > > Well, we're about to call a function that's specific to IPv4 ICMP and > will require IPv4 addresses for both ends. Yes, I understand that part, but I was wondering why "Must have IPv4 addresses" and not IPv6 ones below. On the other hand, due to how the inany thing works, we'll always have IPv6 addresses "set" there. > > [snip] > > > diff --git a/icmp_flow.h b/icmp_flow.h > > > index 5a2eed9..f053211 100644 > > > --- a/icmp_flow.h > > > +++ b/icmp_flow.h > > > @@ -22,7 +22,6 @@ struct icmp_ping_flow { > > > int seq; > > > int sock; > > > time_t ts; > > > - uint16_t id; > > > > Nit: drop 'id' from struct comment. > > Fixed, thanks. > -- Stefano

2 1

Re: [PATCH v5 06/19] flow: Populate address information for initiating side
by Stefano Brivio 18 May '24

18 May '24

On Fri, 17 May 2024 14:27:46 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote: > On Thu, May 16, 2024 at 08:23:37PM +0200, Stefano Brivio wrote: > > On Tue, 14 May 2024 11:03:24 +1000 > > David Gibson <david(a)gibson.dropbear.id.au> wrote: > > > > > This requires the address and port information for the initiating side be > > > populated when a flow enters INI state. Implement that for TCP and ICMP. > > > > > > For now this leaves some information redundantly recorded in both generic > > > and type specific fields. We'll fix that in later patches. > > > > > > Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> > > > --- > > > flow.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++--- > > > flow_table.h | 8 ++++- > > > icmp.c | 10 ++++-- > > > tcp.c | 4 +-- > > > 4 files changed, 103 insertions(+), 11 deletions(-) > > > > > > diff --git a/flow.c b/flow.c > > > index aee2736..3d5b3a5 100644 > > > --- a/flow.c > > > +++ b/flow.c > > > @@ -108,6 +108,31 @@ static const union flow *flow_new_entry; /* = NULL */ > > > /* Last time the flow timers ran */ > > > static struct timespec flow_timer_run; > > > > > > +/** flowside_from_af() - Initialise flowside from addresses > > > + * @fside: flowside to initialise > > > + * @af: Address family (AF_INET or AF_INET6) > > > + * @eaddr: Endpoint address (pointer to in_addr or in6_addr) > > > + * @eport: Endpoint port > > > + * @faddr: Forwarding address (pointer to in_addr or in6_addr) > > > + * @fport: Forwarding port > > > > This starts showing a mild inconsistency: > > > > - initiating / forwarding sides > > So I was trying to go with "initiating" and "forwarded" sides here, > distinct from "forwarding" used for addresses. But the potential for > confusion between "forwarded" and "forwarding" is way too high, I > agree. > > I considered "accepting side", but I don't like that because a) in the > case of UDP "accepting" isn't really a thing and b) there's no > guarantee that the peer on that side will actually "accept" the > "connection". > > "receiving side" has all those problems and more. > > > - endpoint / forwarding addresses > > > > The initiating side can correspond to the endpoint address ("ours") or > > to the forwarding address ("theirs"). > > Not sure what you mean by that - both initiating side and forwarded > side each have both an endpoint and a forwarding address. > > > The forwarding side doesn't necessarily correspond to the forwarding > > address. > > Right :/ > > > But I have no better ideas than "initiating" and "forwarding" for > > sides. Should we consider something different for addresses and ports, > > such as "ours" and "theirs"? > > I dislike those because I think it's not always clear if "us" is just > pasta, or includes the guest. Plus the distinction here is between > the side that initiates the flow and the.. other one.. that doesn't > correspond to "ours" or "theirs" in any consistent way I can see. > > > Local/remote might be misleading as well. > > Peer, passt and pasta all share the same initial. > > "initiating side" / "target side" maybe? Or just "primary side" / > "secondary side" or "A side" / "B side"? > > Actually, I think I like "target" - I feel like "flow_target()" > conveys what "flow_forward()" does pretty well. I'll go with that > pending any further suggestions you have. Yes, "target" sounds pretty good to me as well. So we would have "endpoint" and "forwarding" addresses, but "initiating" and "target" side, right? An alternative could be "client" / "server", it should be always correct, but it would sound a bit strange for ICMP and UDP. > [snip] > > > @@ -150,18 +177,28 @@ static void flow_set_state(struct flow_common *f, enum flow_state state) > > > FLOW_STATE(f)); > > > > > > if (MAX(state, oldstate) >= FLOW_STATE_FWD) > > > - flow_log_(f, LOG_DEBUG, "%s => %s", pif_name(f->pif[INISIDE]), > > > - pif_name(f->pif[FWDSIDE])); > > > + flow_log_(f, LOG_DEBUG, "%s [%s]:%hu -> [%s]:%hu => %s", > > > + pif_name(f->pif[INISIDE]), > > > + inany_ntop(&ini->eaddr, estr, sizeof(estr)), > > > + ini->eport, > > > + inany_ntop(&ini->faddr, fstr, sizeof(fstr)), > > > + ini->fport, > > > + pif_name(f->pif[FWDSIDE])); > > > else if (MAX(state, oldstate) >= FLOW_STATE_INI) > > > - flow_log_(f, LOG_DEBUG, "%s => ?", pif_name(f->pif[INISIDE])); > > > + flow_log_(f, LOG_DEBUG, "%s [%s]:%hu -> [%s]:%hu => ?", > > > + pif_name(f->pif[INISIDE]), > > > + inany_ntop(&ini->eaddr, estr, sizeof(estr)), > > > + ini->eport, > > > + inany_ntop(&ini->faddr, fstr, sizeof(fstr)), > > > + ini->fport); > > > > If I review v2 of "util, tcp: Add helper to display socket addresses", > > can I skip reviewing this? :) > > Alas, no - this isn't based on a socket address. However, I have been > looking at introducing a flowside_ntop() function which would help in > a similar way here. Right, I realised only later that your patch only took care of socket addresses... anyway, reviewed, this hunk also looks correct to me. > > > } > > > > > > /** > > > - * flow_initiate() - Move flow to INI state, setting INISIDE details > > > + * flow_initiate_() - Move flow to INI state, setting pif[INISIDE] > > > * @flow: Flow to change state > > > * @pif: pif of the initiating side > > > */ > > > -void flow_initiate(union flow *flow, uint8_t pif) > > > +static void flow_initiate_(union flow *flow, uint8_t pif) > > > > I don't feel like the underscore here is really necessary. > > Yeah, I'm trying to convey that it's not safe to call this directly - > it won't perform a correct state transition without the extra logic in > flow_initiate_af() and flow_initiate_sa(). Ah, I see now, okay. -- Stefano

2 1

Re: [PATCH v5 01/19] flow: Clarify and enforce flow state transitions
by Stefano Brivio 18 May '24

18 May '24

On Fri, 17 May 2024 13:57:58 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote: > On Thu, May 16, 2024 at 11:30:58AM +0200, Stefano Brivio wrote: > > On Tue, 14 May 2024 11:03:19 +1000 > > David Gibson <david(a)gibson.dropbear.id.au> wrote: > > > > [...] > > > > > /** > > > * struct flow_common - Common fields for packet flows > > > + * @state: State of the flow table entry > > > * @type: Type of packet flow > > > */ > > > struct flow_common { > > > + uint8_t state; > > > > In this case, I would typically do > > (https://seitan.rocks/seitan/tree/common/gluten.h?id=5a9302bab9c9bb3d1577f04…): > > > > #ifdef __GNUC__ > > enum flow_state state:8; > > #else > > uint8_t state; > > #endif > > I don't object to that, but I have two questions > > - What's the advantage to using the explicit enum? Is that for the > benefit of static checkers and/or compiler diagnostics? Yes: if we assign a value that's not in the enum, I expect static checkers to complain. But also for humans: even with that ifdef, a reader would know right away what values that might have. > AFAIK C > itself doesn't really treat enums any differently to integer > types. Right. > - What's the need for GNUC? Are enum bitfields a gnu extension? They're rather permitted by gcc's interpretation of the standard: https://gcc.gnu.org/onlinedocs/gcc-14.1.0/gcc/Structures-unions-enumeration… Allowable bit-field types other than _Bool, signed int, and unsigned int (C99 and C11 6.7.2.1). Other integer types, such as long int, and enumerated types are permitted even in strictly conforming mode. but C11 says (6.7.2.1): A bit-field shall have a type that is a qualified or unqualified version of _Bool, signed int, unsigned int, or some other implementation-defined type. It is implementation-defined whether atomic types are permitted. Is an enum a "version" of those? Maybe not. That was at least the interpretation adopted by older gcc versions, up to 4.7.4: https://gcc.gnu.org/onlinedocs/gcc-4.7.4/gcc/Structures-unions-enumerations… Allowable bit-field types other than _Bool, signed int, and unsigned int (C99 6.7.2.1). No other types are permitted in strictly conforming mode. Did that change from C99? Not really: A bit-field shall have a type that is a qualified or unqualified version of _Bool, signed int, unsigned int, or some other implementation-defined type. > Even versus C11, which we already require? Yes, I would say it doesn't change things. Only C23 would improve that: https://open-std.org/JTC1/SC22/WG14/www/docs/n3030.htm#design-constant.type by allowing us to define the underlying type. > > ...and in any case we need to make sure to assign single values in the > > enum above: there are no guarantees that FLOW_STATE_ACTIVE is 3 > > otherwise (except for that static_assert(), but that's not its purpose). > > I'm not clear how this comment relates to the one before. It's unrelated, but: > AFAIK > nothing in here (or the rest of the series) relies on the specific > numeric values of the flow state values (although we do rely on them > being ordered as written in some places). while we rely on the fact that no value is bigger than 255, I realised that the standards already guarantee that values start from 0 and every subsequent constant is defined as one more than the previous one, all the way from C90 to C11, so this would actually be fine. Sorry, I don't know exactly why I thought that wouldn't be the case, I was pretty sure of the opposite until I checked. -- Stefano

2 1

[PATCH v2 0/4] mbuto: Assorted fixes and simplifications
by David Gibson 18 May '24

18 May '24

While debugging a problem Laurent Vivier was having with the passt test suite, I discovered mbuto's archivemount support is entirely broken: if archivemount is on the system, mbuto won't work. While fixing that, I discovered some slightly related simplfications that can be made. Here they all are. Changes since v1: * Updated commit message in 2/4 to clarify why the desired advantages of using archivemount can't be used in practice * Improve accuracy of comment on compress_select in 3/4 * Clarify removal of update existing archive functionality. David Gibson (4): ${wd} is always set, no need to test for it Remove stale archivemount support Split "auto" compression mode into its own path Remove unnecessary cpio_init function mbuto | 128 +++++++++++++++------------------------------------------- 1 file changed, 33 insertions(+), 95 deletions(-) -- 2.44.0

2 6

Re: [PATCH v2] util, tcp: Add helper to display socket addresses
by Stefano Brivio 17 May '24

17 May '24

On Fri, 17 May 2024 13:30:17 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote: > When reporting errors, we sometimes want to show a relevant socket address. > Doing so by extracting the various relevant fields can be pretty awkward, > so introduce a sockaddr_ntop() helper to make it simpler. For now we just > have one user in tcp.c, but I have further upcoming patches which can make > use of it. > > Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> > --- > tcp.c | 23 ++++++-------- > util.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > util.h | 13 ++++++++ > 3 files changed, 116 insertions(+), 14 deletions(-) > > Changes since v1: > * More careful and documented reasoning about the size of the strings > needed for output. > * Some more complex logic to calling inet_ntop() into a temporary > buffer then copying it That helper looks neat, and perhaps we'll reuse it somewhere, but... do we really need it? I mean, maybe it's not so elegant, but something like this (lightly "tested" with the main() below) should also do the trick and it's much less code: -- #include <arpa/inet.h> #include <limits.h> #include <stdint.h> #include <stdio.h> #include <string.h> #include <sys/random.h> const char *sockaddr_ntop(const void *sa, char *dst, ssize_t size) { sa_family_t family = ((const struct sockaddr *)sa)->sa_family; const struct sockaddr_in6 *a6 = sa; const struct sockaddr_in *a4 = sa; char *p = dst; switch (family) { case AF_INET: if ((size -= sizeof(":9")) < 0) return NULL; if (!inet_ntop(AF_INET, &a4->sin_addr, p, size)) return NULL; break; case AF_INET6: if ((size -= sizeof("[")) < 0) return NULL; if ((size -= snprintf(p++, size, "[")) < (int)sizeof("::]:9")) return NULL; if (!inet_ntop(AF_INET6, &a6->sin6_addr, p, size)) return NULL; } size -= strlen(p); p += strlen(p); if (snprintf(p, size, family == AF_INET6 ? "]:%hu" : ":%hu", family == AF_INET6 ? ntohs(a6->sin6_port) : ntohs(a4->sin_port)) >= size) return NULL; return dst; } int main() { struct sockaddr_in6 test6 = { .sin6_family = AF_INET6 }; struct sockaddr_in test4 = { .sin_family = AF_INET }; struct { struct in6_addr a6; struct in_addr a4; uint16_t p; uint8_t len; } r; char buf[BUFSIZ]; int i; for (i = 0; i < (1 << 18); i++) { getrandom(&r, sizeof(r), 0); test6.sin6_addr = r.a6; test4.sin_addr = r.a4; test4.sin_port = test6.sin6_port = r.p; fprintf(stdout, "%s\n", sockaddr_ntop(&test4, buf, r.len)); fprintf(stdout, "%s\n", sockaddr_ntop(&test6, buf, r.len)); } return 0; } -- if you want to go ahead with the iprintf() helper, however, it's also fine by me. > > diff --git a/tcp.c b/tcp.c > index 21d0af0..efbbc1c 100644 > --- a/tcp.c > +++ b/tcp.c > @@ -2758,6 +2758,7 @@ static void tcp_tap_conn_from_sock(struct ctx *c, in_port_t dstport, > void tcp_listen_handler(struct ctx *c, union epoll_ref ref, > const struct timespec *now) > { > + char sastr[SOCKADDR_STRLEN]; > union sockaddr_inany sa; > socklen_t sl = sizeof(sa); > union flow *flow; > @@ -2776,25 +2777,15 @@ void tcp_listen_handler(struct ctx *c, union epoll_ref ref, > > if (IN4_IS_ADDR_UNSPECIFIED(addr) || > IN4_IS_ADDR_BROADCAST(addr) || > - IN4_IS_ADDR_MULTICAST(addr) || port == 0) { > - char str[INET_ADDRSTRLEN]; > - > - err("Invalid endpoint from TCP accept(): %s:%hu", > - inet_ntop(AF_INET, addr, str, sizeof(str)), port); > - goto cancel; > - } > + IN4_IS_ADDR_MULTICAST(addr) || port == 0) > + goto bad_endpoint; > } else if (sa.sa_family == AF_INET6) { > const struct in6_addr *addr = &sa.sa6.sin6_addr; > in_port_t port = sa.sa6.sin6_port; > > if (IN6_IS_ADDR_UNSPECIFIED(addr) || > - IN6_IS_ADDR_MULTICAST(addr) || port == 0) { > - char str[INET6_ADDRSTRLEN]; > - > - err("Invalid endpoint from TCP accept(): %s:%hu", > - inet_ntop(AF_INET6, addr, str, sizeof(str)), port); > - goto cancel; > - } > + IN6_IS_ADDR_MULTICAST(addr) || port == 0) > + goto bad_endpoint; > } > > if (tcp_splice_conn_from_sock(c, ref.tcp_listen.pif, > @@ -2804,6 +2795,10 @@ void tcp_listen_handler(struct ctx *c, union epoll_ref ref, > tcp_tap_conn_from_sock(c, ref.tcp_listen.port, flow, s, &sa, now); > return; > > +bad_endpoint: > + err("Invalid endpoint from TCP accept(): %s", > + sockaddr_ntop(&sa, sastr, sizeof(sastr))); > + > cancel: > flow_alloc_cancel(flow); > } > diff --git a/util.c b/util.c > index 849fa7f..74c5070 100644 > --- a/util.c > +++ b/util.c > @@ -553,3 +553,97 @@ int write_remainder(int fd, const struct iovec *iov, int iovcnt, size_t skip) > > return 0; > } > + > +/** iprintf() - Incremental printf helper > + * @buf: Buffer into which to format string > + * @end: Pointer to one past @buf's allocated space > + * @fmt: Format string > + * @...: Format arguments > + * > + * Formats a string into the buffer at @buf, never writing past @end (including > + * terminating \0). It returns a pointer to the \0 at the end of the formatted > + * string, allowing iprintf() calls to be chained, building up a long string in > + * pieces. > + * > + * Return: On success, a pointer to the \0 at the end of the formatted string. > + * NULL if the formatted string would be truncated or on other errors > + */ > +__attribute__((format(printf, 3, 4))) > +static char *iprintf(char *buf, const char *const end, const char *fmt, ...) > +{ > + char *next = NULL; > + va_list ap; > + int n; > + > + va_start(ap, fmt); > + n = vsnprintf(buf, end - buf, fmt, ap); > + if (n < 0) { > + /* Output error */ > + errno = EIO; > + goto out; > + } > + > + if (n > (end - buf)) { > + /* Truncated */ > + errno = ENOSPC; > + goto out; > + } > + > + next = buf + n; > + > +out: > + va_end(ap); > + return next; > +} > + > +/** sockaddr_ntop() - Convert a socket address to text format > + * @sa: Socket address > + * @dst: output buffer, minimum SOCKADDR_STRLEN bytes > + * @size: size of buffer at @dst > + * > + * Return: On success, a non-null pointer to @dst, NULL on failure > + */ > +const char *sockaddr_ntop(const void *sa, char *dst, socklen_t size) > +{ > + sa_family_t family = ((const struct sockaddr *)sa)->sa_family; > + const char *const end = dst + size; > + char *p = dst; > + > + switch (family) { > + case AF_INET: { > + const struct sockaddr_in *sa4 = sa; > + > + if (!inet_ntop(AF_INET, &sa4->sin_addr, p, end - p)) > + return NULL; > + p += strlen(p); > + > + if (!iprintf(p, end, ":%hu", ntohs(sa4->sin_port))) > + return NULL; > + > + break; > + } > + > + case AF_INET6: { > + const struct sockaddr_in6 *sa6 = sa; > + > + if (!(p = iprintf(p, end, "["))) > + return NULL; > + > + if (!inet_ntop(AF_INET6, &sa6->sin6_addr, p, end - p)) > + return NULL; > + p += strlen(p); > + > + if (iprintf(p, end, "]:%hu", ntohs(sa6->sin6_port))) > + return NULL; > + > + break; > + } > + > + /* FIXME: Implement AF_UNIX */ > + default: > + errno = EAFNOSUPPORT; > + return NULL; > + } > + > + return dst; > +} > diff --git a/util.h b/util.h > index 264423b..a637d31 100644 > --- a/util.h > +++ b/util.h > @@ -180,6 +180,19 @@ static inline const char *af_name(sa_family_t af) > } > } > > +/* Maximum required string length to format into decimal */ > +#define U16_STRLEN 6 /* "65535\0" */ > + > +/* inet address (- '\0') + port (u16) (- '\0') + ':' + '\0' */ > +#define SOCKADDR_INET_STRLEN (INET_ADDRSTRLEN-1 + U16_STRLEN-1 + 2) > + > +/* inet6 address (- '\0') + port (u16) (- '\0') + '[' + ']' + ':' + '\0' */ > +#define SOCKADDR_INET6_STRLEN (INET6_ADDRSTRLEN-1 + U16_STRLEN-1 + 4) Isn't it actually less error-prone and more readable to do something like the stuff I dropped in d32edee60a93 ("passt: Use INET{,6}_ADDRSTRLEN instead of open coded sizeof"), say: #define SOCKADDR_INET6_STRLEN \ sizeof("[0123:4567:89ab:cdef:0123:4567:89ab:cdef]:65535") ? > + > +#define SOCKADDR_STRLEN MAX(SOCKADDR_INET_STRLEN, SOCKADDR_INET6_STRLEN) > + > +const char *sockaddr_ntop(const void *sa, char *dst, socklen_t size); > + > /** > * mod_sub() - Modular arithmetic subtraction > * @a: Minued, unsigned value < @m -- Stefano

1 0

[PATCH v6 0/3] Support for SO_PEEK_OFF socket option
by Jon Maloy 17 May '24

17 May '24

Only patch #3 updated from v5. Jon Maloy (3): tcp: move seq_to_tap update to when frame is queued tcp: leverage support of SO_PEEK_OFF socket option when available tcp: allow retransmit when peer receive window is zero tcp.c | 146 ++++++++++++++++++++++++++++++++++++++++------------- tcp_conn.h | 2 + 2 files changed, 112 insertions(+), 36 deletions(-) -- 2.42.0

1 3

Re: [PATCH v5 3/3] tcp: allow retransmit when peer receive window is zero
by Stefano Brivio 17 May '24

17 May '24

On Thu, 16 May 2024 22:39:53 -0400 Jon Maloy <jmaloy(a)redhat.com> wrote: > On 2024-05-16 18:21, Stefano Brivio wrote: > > On Thu, 16 May 2024 17:33:28 -0400 > > Jon Maloy <jmaloy(a)redhat.com> wrote: > > > >> A bug in kernel TCP may lead to a deadlock where a zero window is sent > >> from the peer, while it is unable to send out window updates even after > >> reads have freed up enough buffer space to permit a larger window. > >> In this situation, new window advertisemnts from the peer can only be > >> triggered by packets arriving from this side. > >> > >> However, such packets are never sent, because the zero-window condition > >> currently prevents this side from sending out any packets whatsoever > >> to the peer. > >> > >> We notice that the above bug is triggered *only* after the peer has > >> dropped an arriving packet because of severe memory squeeze, and that we > >> hence always enter a retransmission situation when this occurs. This > >> also means that it goes against the RFC 9293 recommendation that a > >> previously advertised window never should shrink. > >> > >> RFC 9293 gives the solution to this situation. In chapter 3.6.1 we find > >> the following statement: > >> "A TCP receiver SHOULD NOT shrink the window, i.e., move the right > >> window edge to the left (SHLD-14). However, a sending TCP peer MUST > >> be robust against window shrinking, which may cause the > >> "usable window" (see Section 3.8.6.2.1) to become negative (MUST-34). > >> > >> If this happens, the sender SHOULD NOT send new data (SHLD-15), but > >> SHOULD retransmit normally the old unacknowledged data between SND.UNA > >> and SND.UNA+SND.WND (SHLD-16). The sender MAY also retransmit old data > >> beyond SND.UNA+SND.WND (MAY-7)" > >> > >> We never see the window become negative, but we interpret this as a > >> recommendation to use the previously available window during > >> retransmission even when the currently advertised window is zero. > >> > >> We use the above mechanism only for timer-induced retransmits, while > >> the fast-retransmit mechanism won't trigger on this condition. > >> > >> It should be noted that although this solves the problem we have at > >> hand, it is not a genuine solution to the kernel bug. There may well > >> be TCP stacks around in other OS-es which don't do this, nor have > >> keep-alive probing as an alternatve way to solve the situation. > >> > >> Signed-off-by: Jon Maloy <jmaloy(a)redhat.com> > >> > >> --- > >> v2: - Using previously advertised window during retransmission, instead > >> highest send sequencece number in the cycle. > >> v3: - Rebased to newest code > >> - Changes based on feedback from PASST team > >> - Sending out empty probe message at timer expiration when > >> we are not in retransmit situation. > >> v4: - Some small changes based on feedback from PASST team. > >> - Replaced fast retransmit with a one-time 'fast probe' when > >> window is zero. > >> v5: - Gave up on 'fast probing' for now. When I got the sequence > >> numbers right in the flag message (after having emptied the tap > >> queue), it turns out an empty message does *not* force a new peer > >> window update as was my previous understanding of the code. > >> - Added cppcheck suppression line (which I was unable to verify) > > ...hmm, why? All it takes is a 'make cppcheck'. Or is your version of > > cppcheck not showing it? > Exactly. My version is too recent, I think. > > > >> as suggested by S. Brivio. > >> - Removed sending of empty probe when window from tap side is zero. > >> It looks pointless at the moment, at least for solving the above > >> described situation. > > So, as far as I understand, this patch now doesn't introduce any > > functional change, except that if an existing timer happened to be > > pending (this patch never schedules it), > > and the window _we_ are > > advertising (wnd_to_tap) is zero (I'm not sure why that's relevant), > > then we'll send an ACK segment. > The change is that we really will do a retransmit from the timer handler, > ignoring that current window is zero. ...but how does the window *we* advertise matter here? You're checking conn->wnd_to_tap now, not conn->wnd_from_tap. Anyway, if you're now abandoning this approach, it doesn't matter. > This solves the deadlock problem > I have been describing, but it is not a good solution. > > One problem is that new incoming data will cause an EPOLLIN event, > which will cause a call to tcp_data_from_sock(), which will also ignore > the zero-window. Right now it doesn't ignore it. Do you plan to use that function to check if the usable window is now negative (because the window value is zero but we have unacknowledged data), and re-send something in that case? > This is easy to fix, I realize: > > if (events & EPOLLIN && conn->wnd_from_tap) > tcp_data_from_sock(c, conn); > > The next is that I would like to start a short timer when I receive > the zero-window and seq_to_tap > seq_ack_from_tap (same condition > as in previous versions, but without sending the flag packet). > As far as I can see there is no simple way to schedule the timer to > the value I want. I am open to suggestions here. You could reuse/abuse ACK_TO_TAP_DUE (after all, you want to send a keep-alive in a while), and schedule a different timeout in tcp_timer_ctl() if the peer window is zero. Then, in the timer handler, depending on the peer window, you would check if you want to use ACK_IF_NEEDED (no keepalive), or ACK (force a keepalive in any case). -- Stefano

2 1

[PATCH] util, tcp: Add helper to display socket addresses
by David Gibson 16 May '24

16 May '24

When reporting errors, we sometimes want to show a relevant socket address. Doing so by extracting the various relevant fields can be pretty awkward, so introduce a sockaddr_ntop() helper to make it simpler. For now we just have one user in tcp.c, but I have further upcoming patches which can make use of it. Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- tcp.c | 23 +++++++++-------------- util.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ util.h | 4 ++++ 3 files changed, 57 insertions(+), 14 deletions(-) diff --git a/tcp.c b/tcp.c index 21d0af0..efbbc1c 100644 --- a/tcp.c +++ b/tcp.c @@ -2758,6 +2758,7 @@ static void tcp_tap_conn_from_sock(struct ctx *c, in_port_t dstport, void tcp_listen_handler(struct ctx *c, union epoll_ref ref, const struct timespec *now) { + char sastr[SOCKADDR_STRLEN]; union sockaddr_inany sa; socklen_t sl = sizeof(sa); union flow *flow; @@ -2776,25 +2777,15 @@ void tcp_listen_handler(struct ctx *c, union epoll_ref ref, if (IN4_IS_ADDR_UNSPECIFIED(addr) || IN4_IS_ADDR_BROADCAST(addr) || - IN4_IS_ADDR_MULTICAST(addr) || port == 0) { - char str[INET_ADDRSTRLEN]; - - err("Invalid endpoint from TCP accept(): %s:%hu", - inet_ntop(AF_INET, addr, str, sizeof(str)), port); - goto cancel; - } + IN4_IS_ADDR_MULTICAST(addr) || port == 0) + goto bad_endpoint; } else if (sa.sa_family == AF_INET6) { const struct in6_addr *addr = &sa.sa6.sin6_addr; in_port_t port = sa.sa6.sin6_port; if (IN6_IS_ADDR_UNSPECIFIED(addr) || - IN6_IS_ADDR_MULTICAST(addr) || port == 0) { - char str[INET6_ADDRSTRLEN]; - - err("Invalid endpoint from TCP accept(): %s:%hu", - inet_ntop(AF_INET6, addr, str, sizeof(str)), port); - goto cancel; - } + IN6_IS_ADDR_MULTICAST(addr) || port == 0) + goto bad_endpoint; } if (tcp_splice_conn_from_sock(c, ref.tcp_listen.pif, @@ -2804,6 +2795,10 @@ void tcp_listen_handler(struct ctx *c, union epoll_ref ref, tcp_tap_conn_from_sock(c, ref.tcp_listen.port, flow, s, &sa, now); return; +bad_endpoint: + err("Invalid endpoint from TCP accept(): %s", + sockaddr_ntop(&sa, sastr, sizeof(sastr))); + cancel: flow_alloc_cancel(flow); } diff --git a/util.c b/util.c index 849fa7f..c78a5f3 100644 --- a/util.c +++ b/util.c @@ -553,3 +553,47 @@ int write_remainder(int fd, const struct iovec *iov, int iovcnt, size_t skip) return 0; } + + +/** sockaddr_ntop - Convert a socket address to text format + * @src: Socket address + * @dst: output buffer, minimum SOCKADDR_STRLEN bytes + * @size: size of buffer at @dst + * + * Return: On success, a non-null pointer to @dst, NULL on failure + */ +const char *sockaddr_ntop(const void *sa, char *dst, socklen_t size) +{ + sa_family_t family = ((const struct sockaddr *)sa)->sa_family; + + switch (family) { + case AF_INET: { + const struct sockaddr_in *sa4 = sa; + char addr[INET_ADDRSTRLEN]; + + if (!inet_ntop(AF_INET, &sa4->sin_addr, addr, sizeof(addr))) + break; + + snprintf(dst, size, "%s:%hu", addr, ntohs(sa4->sin_port)); + return dst; + } + + case AF_INET6: { + const struct sockaddr_in6 *sa6 = sa; + char addr[INET6_ADDRSTRLEN]; + + if (!inet_ntop(AF_INET6, &sa6->sin6_addr, addr, sizeof(addr))) + break; + + snprintf(dst, size, "[%s]:%hu", addr, ntohs(sa6->sin6_port)); + return dst; + } + + /* FIXME: Implement AF_UNIX */ + default: + errno = EAFNOSUPPORT; + } + + /* Failure */ + return NULL; +} diff --git a/util.h b/util.h index 264423b..2c7154e 100644 --- a/util.h +++ b/util.h @@ -180,6 +180,10 @@ static inline const char *af_name(sa_family_t af) } } +#define SOCKADDR_STRLEN MAX(INET_ADDRSTRLEN + 6, \ + INET6_ADDRSTRLEN + 8) +const char *sockaddr_ntop(const void *sa, char *dst, socklen_t size); + /** * mod_sub() - Modular arithmetic subtraction * @a: Minued, unsigned value < @m -- 2.45.0

2 2

[PATCH v4] tcp: move seq_to_tap update to when frame is queued
by Jon Maloy 15 May '24

15 May '24

commit a469fc393fa1 ("tcp, tap: Don't increase tap-side sequence counter for dropped frames") delayed update of conn->seq_to_tap until the moment the corresponding frame has been successfully pushed out. This has the advantage that we immediately can make a new attempt to transmit a frame after a failed trasnmit, rather than waiting for the peer to later discover a gap and trigger the fast retransmit mechanism to solve the problem. This approach has turned out to cause a problem with spurious sequence number updates during peer-initiated retransmits, and we have realized it may not be the best way to solve the above issue. We now restore the previous method, by updating the said field at the moment a frame is added to the outqueue. To retain the advantage of having a quick re-attempt based on local failure detection, we now scan through the part of the outqueue that had do be dropped, and restore the sequence counter for each affected connection to the most appropriate value. Signed-off-by: Jon Maloy <jmaloy(a)redhat.com> --- v2: - Re-spun loop in tcp_revert_seq() and some other changes based on feedback from Stefano Brivio. - Added paranoid test to avoid that seq_to_tap becomes lower than seq_ack_from_tap. v3: - Identical to v2. Called v3 because it was embedded in a series with that version. v4: - In tcp_revert_seq(), we read the sequence number from the TCP header instead of keeping a copy in struct tcp_buf_seq_update. - Since the only remaining field in struct tcp_buf_seq_update is a pointer to struct tcp_tap_conn, we eliminate the struct altogether, and make the tcp6/tcp3_buf_seq_update arrays into arrays of said pointer. - Removed 'paranoid' test in tcp_revert_seq. If it happens, it is not fatal, and will be caught by other code anyway. - Separated from the series again. --- tcp.c | 59 +++++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 37 insertions(+), 22 deletions(-) diff --git a/tcp.c b/tcp.c index 21d0af0..976dba8 100644 --- a/tcp.c +++ b/tcp.c @@ -410,16 +410,6 @@ static int tcp_sock_ns [NUM_PORTS][IP_VERSIONS]; */ static union inany_addr low_rtt_dst[LOW_RTT_TABLE_SIZE]; -/** - * tcp_buf_seq_update - Sequences to update with length of frames once sent - * @seq: Pointer to sequence number sent to tap-side, to be updated - * @len: TCP payload length - */ -struct tcp_buf_seq_update { - uint32_t *seq; - uint16_t len; -}; - /* Static buffers */ /** * struct tcp_payload_t - TCP header and data to send segments with payload @@ -461,7 +451,8 @@ static struct tcp_payload_t tcp4_payload[TCP_FRAMES_MEM]; static_assert(MSS4 <= sizeof(tcp4_payload[0].data), "MSS4 is greater than 65516"); -static struct tcp_buf_seq_update tcp4_seq_update[TCP_FRAMES_MEM]; +/* References tracking the owner connection of frames in the tap outqueue */ +static struct tcp_tap_conn *tcp4_frame_conns[TCP_FRAMES_MEM]; static unsigned int tcp4_payload_used; static struct tap_hdr tcp4_flags_tap_hdr[TCP_FRAMES_MEM]; @@ -483,7 +474,8 @@ static struct tcp_payload_t tcp6_payload[TCP_FRAMES_MEM]; static_assert(MSS6 <= sizeof(tcp6_payload[0].data), "MSS6 is greater than 65516"); -static struct tcp_buf_seq_update tcp6_seq_update[TCP_FRAMES_MEM]; +/* References tracking the owner connection of frames in the tap outqueue */ +static struct tcp_tap_conn *tcp6_frame_conns[TCP_FRAMES_MEM]; static unsigned int tcp6_payload_used; static struct tap_hdr tcp6_flags_tap_hdr[TCP_FRAMES_MEM]; @@ -1261,25 +1253,49 @@ static void tcp_flags_flush(const struct ctx *c) tcp4_flags_used = 0; } +/** + * tcp_revert_seq() - Revert affected conn->seq_to_tap after failed transmission + * @conns: Array of connection pointers corresponding to queued frames + * @frames: Two-dimensional array containing queued frames with sub-iovs + * @num_frames: Number of entries in the two arrays to be compared + */ +static void tcp_revert_seq(struct tcp_tap_conn **conns, struct iovec *frames, + int num_frames) +{ + int c, f; + + for (c = 0, f = 0; c < num_frames; c++, f += TCP_NUM_IOVS) { + struct tcp_tap_conn *conn = conns[c]; + struct tcphdr *th = frames[f + TCP_IOV_PAYLOAD].iov_base; + uint32_t seq = ntohl(th->seq); + + if (SEQ_LE(conn->seq_to_tap, seq)) + continue; + + conn->seq_to_tap = seq; + } +} + /** * tcp_payload_flush() - Send out buffers for segments with data * @c: Execution context */ static void tcp_payload_flush(const struct ctx *c) { - unsigned i; size_t m; m = tap_send_frames(c, &tcp6_l2_iov[0][0], TCP_NUM_IOVS, tcp6_payload_used); - for (i = 0; i < m; i++) - *tcp6_seq_update[i].seq += tcp6_seq_update[i].len; + if (m != tcp6_payload_used) + tcp_revert_seq(tcp6_frame_conns, &tcp6_l2_iov[m][0], + tcp6_payload_used - m); tcp6_payload_used = 0; m = tap_send_frames(c, &tcp4_l2_iov[0][0], TCP_NUM_IOVS, tcp4_payload_used); - for (i = 0; i < m; i++) - *tcp4_seq_update[i].seq += tcp4_seq_update[i].len; + if (m != tcp4_payload_used) + tcp_revert_seq(tcp4_frame_conns, &tcp4_l2_iov[m][0], + tcp4_payload_used - m); tcp4_payload_used = 0; } @@ -2129,10 +2145,11 @@ static int tcp_sock_consume(const struct tcp_tap_conn *conn, uint32_t ack_seq) static void tcp_data_to_tap(const struct ctx *c, struct tcp_tap_conn *conn, ssize_t dlen, int no_csum, uint32_t seq) { - uint32_t *seq_update = &conn->seq_to_tap; struct iovec *iov; size_t l4len; + conn->seq_to_tap = seq + dlen; + if (CONN_V4(conn)) { struct iovec *iov_prev = tcp4_l2_iov[tcp4_payload_used - 1]; const uint16_t *check = NULL; @@ -2142,8 +2159,7 @@ static void tcp_data_to_tap(const struct ctx *c, struct tcp_tap_conn *conn, check = &iph->check; } - tcp4_seq_update[tcp4_payload_used].seq = seq_update; - tcp4_seq_update[tcp4_payload_used].len = dlen; + tcp4_frame_conns[tcp4_payload_used] = conn; iov = tcp4_l2_iov[tcp4_payload_used++]; l4len = tcp_l2_buf_fill_headers(c, conn, iov, dlen, check, seq); @@ -2151,8 +2167,7 @@ static void tcp_data_to_tap(const struct ctx *c, struct tcp_tap_conn *conn, if (tcp4_payload_used > TCP_FRAMES_MEM - 1) tcp_payload_flush(c); } else if (CONN_V6(conn)) { - tcp6_seq_update[tcp6_payload_used].seq = seq_update; - tcp6_seq_update[tcp6_payload_used].len = dlen; + tcp6_frame_conns[tcp6_payload_used] = conn; iov = tcp6_l2_iov[tcp6_payload_used++]; l4len = tcp_l2_buf_fill_headers(c, conn, iov, dlen, NULL, seq); -- 2.42.0

2 3

[PATCH v3 0/3] Support for SO_PEEK_OFF socket option
by Jon Maloy 14 May '24

14 May '24

Rebased, plus latest changes based on feedback. Jon Maloy (3): tcp: move seq_to_tap update to when frame is queued tcp: leverage support of SO_PEEK_OFF socket option when available tcp: allow retransmit when peer receive window is zero tcp.c | 182 +++++++++++++++++++++++++++++++++++++++-------------- tcp_conn.h | 2 + 2 files changed, 138 insertions(+), 46 deletions(-) -- 2.42.0

3 12