May 2024 - dev

[PATCH] apparmor: Fix passt abstraction
by Maxime Bélair 21 May '24

21 May '24

Commit b686afa2 introduced the invalid apparmor rule `mount options=(rw, runbindable) /,` since runbindable mount rules cannot have a source. Therefore running aa-logprof/aa-genprof will trigger errors (see https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2065685) $ sudo aa-logprof ERROR: Operation {'runbindable'} cannot have a source. Source = AARE('/') This patch fixes it to the intended behavior. Signed-off-by: Maxime Bélair <maxime.belair(a)canonical.com> --- contrib/apparmor/abstractions/passt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/apparmor/abstractions/passt b/contrib/apparmor/abstractions/passt index 61ec32c..d245115 100644 --- a/contrib/apparmor/abstractions/passt +++ b/contrib/apparmor/abstractions/passt @@ -26,7 +26,7 @@ capability sys_ptrace, / r, # isolate_prefork(), isolation.c - mount options=(rw, runbindable) /, + mount options=(rw, runbindable) -> /, mount "" -> "/", mount "" -> "/tmp/", pivot_root "/tmp/" -> "/tmp/", -- 2.40.1

2 3

[PATCH 0/6] Final flow table preliminaries
by David Gibson 21 May '24

21 May '24

Here's a series of what I hope will be the last preliminaries for the flow table which can be applied before we take the cost of increasing the size of entries with generic address information. Changes since "Unified flow table v5": * Small fixes to state verification * New patch passing specific typed structures into deferred/timer callbacks * Terminology change from "forwarded side" to "target side" * Assorted wording and style tweaks based on Stefano's review David Gibson (6): flow: Properly type callbacks to protocol specific handlers inany: Better helpers for using inany and specific family addrs together flow: Clarify and enforce flow state transitions flow: Make side 0 always be the initiating side flow: Record the pifs for each side of each flow tcp: Remove interim 'tapside' field from connection flow.c | 209 ++++++++++++++++++++++++++++++++++----------------- flow.h | 118 +++++++++++++++++++++++++++-- flow_table.h | 10 +++ icmp.c | 18 ++--- icmp_flow.h | 2 +- inany.c | 17 +---- inany.h | 97 +++++++++++++++++++++++- pif.h | 1 - tcp.c | 68 ++++++++--------- tcp_conn.h | 7 +- tcp_splice.c | 25 +++--- 11 files changed, 417 insertions(+), 155 deletions(-) -- 2.45.1

1 6

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

21 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

2 6

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

21 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 | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ util.h | 14 ++++++++++++++ 3 files changed, 79 insertions(+), 14 deletions(-) Changes since v2: * Removed use of iprintf() helper function, use some macros instead. * Use sizeof("literal") approach to simplify some things. 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 diff --git a/tcp.c b/tcp.c index 21d0af06..efbbc1c0 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 849fa7f6..d66d366a 100644 --- a/util.c +++ b/util.c @@ -553,3 +553,59 @@ 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 + * @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; + socklen_t off = 0; + +#define IPRINTF(...) \ + do { \ + off += snprintf(dst + off, size - off, __VA_ARGS__); \ + if (off >= size) \ + return NULL; \ + } while (0) + +#define INTOP(af, addr) \ + do { \ + if (!inet_ntop((af), (addr), dst + off, size - off)) \ + return NULL; \ + off += strlen(dst + off); \ + } while (0) + + switch (family) { + case AF_INET: { + const struct sockaddr_in *sa4 = sa; + + INTOP(AF_INET, &sa4->sin_addr); + IPRINTF(":%hu", ntohs(sa4->sin_port)); + break; + } + + case AF_INET6: { + const struct sockaddr_in6 *sa6 = sa; + + IPRINTF("["); + INTOP(AF_INET6, &sa6->sin6_addr); + IPRINTF("]:%hu", ntohs(sa6->sin6_port)); + break; + } + + /* FIXME: Implement AF_UNIX */ + default: + errno = EAFNOSUPPORT; + return NULL; + } + +#undef IPRINTF +#undef INTOP + + return dst; +} diff --git a/util.h b/util.h index 264423bb..c8a357e3 100644 --- a/util.h +++ b/util.h @@ -180,6 +180,20 @@ static inline const char *af_name(sa_family_t af) } } +#define UINT16_STRLEN (sizeof("65535")) + +/* inet address (- '\0') + port (u16) (- '\0') + ':' + '\0' */ +#define SOCKADDR_INET_STRLEN \ + (INET_ADDRSTRLEN-1 + UINT16_STRLEN-1 + sizeof(":")) + +/* inet6 address (- '\0') + port (u16) (- '\0') + '[' + ']' + ':' + '\0' */ +#define SOCKADDR_INET6_STRLEN \ + (INET6_ADDRSTRLEN-1 + UINT16_STRLEN-1 + sizeof("[]:")) + +#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 -- 2.45.1

1 0

[PATCH] pasta.c: modify hostname when detaching new namespace
by Danish Prakash 20 May '24

20 May '24

When invoking pasta without any arguments, it's difficult to tell whether we are in the new namespace or not leaving users a bit confused. This change modifies the host namespace to add a prefix "pasta-" to make it a bit more obvious. Signed-off-by: Danish Prakash <contact(a)danishpraka.sh> --- pasta.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/pasta.c b/pasta.c index 31e1e00..840a2b1 100644 --- a/pasta.c +++ b/pasta.c @@ -180,6 +180,8 @@ static int pasta_spawn_cmd(void *arg) { const struct pasta_spawn_cmd_arg *a; sigset_t set; + char hostname[HOST_NAME_MAX+1], pasta_hostname[HOST_NAME_MAX+1]; + char *hostname_prefix = "pasta-"; /* We run in a detached PID and mount namespace: mount /proc over */ if (mount("", "/proc", "proc", 0, NULL)) @@ -188,6 +190,17 @@ static int pasta_spawn_cmd(void *arg) if (write_file("/proc/sys/net/ipv4/ping_group_range", "0 0")) warn("Cannot set ping_group_range, ICMP requests might fail"); + if (gethostname(hostname, HOST_NAME_MAX+1) == 0) { + if ((strlen(hostname) + strlen(hostname_prefix)) > HOST_NAME_MAX) { + hostname[strlen(hostname)-strlen(hostname_prefix)] = '\0'; + } + sprintf(pasta_hostname, "%s%s", hostname_prefix, hostname); + + if (sethostname(pasta_hostname, strlen(pasta_hostname)) != 0) { + warn("Unable to set pasta-prefixed hostname"); + } + } + /* Wait for the parent to be ready: see main() */ sigemptyset(&set); sigaddset(&set, SIGUSR1); -- 2.45.1

1 0

[PATCH v5 00/19] RFC: Unified flow table
by David Gibson 20 May '24

20 May '24

This is a fourth draft of the first steps in implementing more general "connection" tracking, as described at: https://pad.passt.top/p/NewForwardingModel This series changes the TCP connection table and hash table into a more general flow table that can track other protocols as well. Each flow uniformly keeps track of all the relevant addresses and ports, which will allow for more robust control of NAT and port forwarding. ICMP is converted to use the new flow table. This doesn't include UDP, but I'm working on it right now and making progress. I'm posting this to give a head start on the review :) Caveats: * We significantly increase the size of a connection/flow entry Changes since v4: * flowside_from_af() no longer fills in unspecified addresses when passed NULL * Split and rename flow hash lookup function * Clarified flow state transitions, and enforced where practical * Made side 0 always the initiating side of a flow, rather than letting the protocol specific code decide * Separated pifs from flowside addresses to allow better structure packing Changes since v3: * Complex rebase on top of the many things that have happened upstream since v2. * Assorted other changes. * Replace TAPFSIDE() and SOCKFSIDE() macros with local variables. Changes since v2: * Cosmetic fixes based on review * Extra doc comments for enum flow_type * Rename flowside to flowaddrs which turns out to make more sense in light of future changes * Fix bug where the socket flowaddrs for tap initiated connections wasn't initialised to match the socket address we were using in the case of map-gw NAT * New flowaddrs_from_sock() helper used in most cases which is cleaner and should avoid bugs like the above * Using newer centralised workarounds for clang-tidy issue 58992 * Remove duplicate definition of FLOW_MAX as maximum flow type and maximum number of tracked flows * Rebased on newer versions of preliminary work (ICMP, flow based dispatch and allocation, bind/address cleanups) * Unified hash table as well as base flow table * Integrated ICMP Changes since v1: * Terminology changes - "Endpoint" address/port instead of "correspondent" address/port - "flowside" instead of "demiflow" * Actually move the connection table to a new flow table structure in new files * Significant rearrangement of earlier patchs on top of that new table, to reduce churn David Gibson (19): flow: Clarify and enforce flow state transitions flow: Make side 0 always be the initiating side flow: Record the pifs for each side of each flow tcp: Remove interim 'tapside' field from connection flow: Common data structures for tracking flow addresses flow: Populate address information for initiating side flow: Populate address information for non-initiating side tcp, flow: Remove redundant information, repack connection structures tcp: Obtain guest address from flowside tcp: Simplify endpoint validation using flowside information tcp_splice: Eliminate SPLICE_V6 flag tcp, flow: Replace TCP specific hash function with general flow hash flow, tcp: Generalise TCP hash table to general flow hash table tcp: Re-use flow hash for initial sequence number generation icmp: Use flowsides as the source of truth wherever possible icmp: Look up ping flows using flow hash icmp: Eliminate icmp_id_map flow, tcp: Flow based NAT and port forwarding for TCP flow, icmp: Use general flow forwarding rules for ICMP flow.c | 538 +++++++++++++++++++++++++++++++++++++++++++++------ flow.h | 149 +++++++++++++- flow_table.h | 21 ++ fwd.c | 110 +++++++++++ fwd.h | 12 ++ icmp.c | 98 ++++++---- icmp_flow.h | 1 - inany.h | 29 ++- passt.h | 3 + pif.h | 1 - tap.c | 11 -- tap.h | 1 - tcp.c | 484 ++++++++++++--------------------------------- tcp_conn.h | 36 ++-- tcp_splice.c | 97 ++-------- tcp_splice.h | 5 +- 16 files changed, 999 insertions(+), 597 deletions(-) -- 2.45.0

2 25

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

2024

2023

2022

2021

dev May 2024