May 2024 - dev

passt: new version 2024_05_23.765eb0b available
by Stefano Brivio 23 May '24

23 May '24

The new version with tag 2024_05_23.765eb0b includes the following changes: 765eb0b apparmor: Fix comments after PID file and AF_UNIX socket creation refactoring 0608ec4 conf, passt.h: Rename pid_file in struct ctx to pidfile c9b2413 conf, passt, tap: Open socket and PID files before switching UID/GID ba23b05 passt, util: Move opening of PID file to its own function 57d8aa8 util: Rename write_pidfile() to pidfile_write() cbca08c tap: Split tap_sock_unix_init() into opening and listening parts fcfb592 passt, tap: Don't use -1 as uninitialised value for fd_tap_listen d02bb6c tap: Move all-ones initialisation of mac_guest to tap_sock_init() 45b8632 conf: Don't lecture user about starting us as root 3f917b3 netlink, test: Ignore deprecated addresses cc801fb tcp: Remove interim 'tapside' field from connection 8a2accb flow: Record the pifs for each side of each flow 4357185 flow: Make side 0 always be the initiating side 0060acd flow: Clarify and enforce flow state transitions a631998 inany: Better helpers for using inany and specific family addrs together 7a832a8 flow: Properly type callbacks to protocol specific handlers 1a20370 util, tcp: Add helper to display socket addresses 3ff3a8a apparmor: Fix passt abstraction 6cdc9fd apparmor: allow netns paths on /tmp 80f7ff2 clang-tidy: Suppress macro to enum conversion warnings 29bd08f conf: Fix clang-tidy warning about using an undefined enum value 26c71db passt.c: explicitly include libgen.h for basename 623c2fd netlink: Don't duplicate routes referring to unrelated host interfaces https://passt.top/passt/log/?qt=range&q=2024_05_10.7288448..2024_05_23.765e… Packages: - Alpine Linux: https://pkgs.alpinelinux.org/packages?name=passt - Arch Linux: https://www.archlinux.org/packages/extra/x86_64/passt/ https://archlinuxarm.org/packages/aarch64/passt https://archlinuxarm.org/packages/armv7h/passt - Chimera: https://pkgs.chimera-linux.org/packages?name=passt - Debian tracker: https://tracker.debian.org/pkg/passt - Copr (CentOS Stream, EPEL, Fedora, Mageia): https://copr.fedorainfracloud.org/coprs/sbrivio/passt/build/7482357/ permanent mirror: https://passt.top/builds/copr/0^20240523.g765eb0b/ - Fedora updates: https://bodhi.fedoraproject.org/updates/?packages=passt - Gentoo versions: https://packages.gentoo.org/packages/net-misc/passt - GNU Guix: https://packages.guix.gnu.org/packages/passt/ - openSUSE: https://software.opensuse.org/package/passt - PLD Linux: https://git.pld-linux.org/cgi-bin/gitweb.cgi?p=packages/passt.git - Solus: https://github.com/getsolus/packages/tree/main/packages/p/passt - Ubuntu tracker: https://launchpad.net/ubuntu/+source/passt - Void Linux: https://voidlinux.org/packages/?q=passt - Static builds: - Package for other RPM-based distributions, x86_64 only: https://passt.top/builds/latest/x86_64/passt-g765eb0b-1.x86_64.rpm - x86_64 static binaries: https://passt.top/builds/latest/x86_64/ - Debian package, from x86_64 static build: https://passt.top/builds/latest/x86_64/passt_765eb0b-1_all.deb -- Stefano

1 0

[PATCH] apparmor: Fix comments after PID file and AF_UNIX socket creation refactoring
by Stefano Brivio 23 May '24

23 May '24

Now: - we don't open the PID file in main() anymore - PID file and AF_UNIX socket are opened by pidfile_open() and tap_sock_unix_open() - write_pidfile() becomes pidfile_write() Reported-by: Richard W.M. Jones <rjones(a)redhat.com> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- contrib/apparmor/abstractions/pasta | 2 +- contrib/apparmor/usr.bin.passt | 9 ++++++--- contrib/apparmor/usr.bin.pasta | 9 ++++++--- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/contrib/apparmor/abstractions/pasta b/contrib/apparmor/abstractions/pasta index 581ad1b..9f73bee 100644 --- a/contrib/apparmor/abstractions/pasta +++ b/contrib/apparmor/abstractions/pasta @@ -27,7 +27,7 @@ @{PROC}/@{pid}/net/udp r, @{PROC}/@{pid}/net/udp6 r, - @{run}/user/@{uid}/** rw, # pasta_open_ns(), main() + @{run}/user/@{uid}/** rw, # pasta_open_ns() @{PROC}/[0-9]*/ns/ r, # pasta_netns_quit_init(), @{PROC}/[0-9]*/ns/net r, # pasta_wait_for_ns(), diff --git a/contrib/apparmor/usr.bin.passt b/contrib/apparmor/usr.bin.passt index 564f82f..9568189 100644 --- a/contrib/apparmor/usr.bin.passt +++ b/contrib/apparmor/usr.bin.passt @@ -19,9 +19,12 @@ profile passt /usr/bin/passt{,.avx2} { include <abstractions/passt> # Alternatively: include <abstractions/user-tmp> - owner /tmp/** w, # tap_sock_unix_init(), pcap(), - # write_pidfile(), + owner /tmp/** w, # tap_sock_unix_open(), + # tap_sock_unix_init(), pcap(), + # pidfile_open(), + # pidfile_write(), # logfile_init() - owner @{HOME}/** w, # pcap(), write_pidfile() + owner @{HOME}/** w, # pcap(), pidfile_open(), + # pidfile_write() } diff --git a/contrib/apparmor/usr.bin.pasta b/contrib/apparmor/usr.bin.pasta index bdfeb71..2483968 100644 --- a/contrib/apparmor/usr.bin.pasta +++ b/contrib/apparmor/usr.bin.pasta @@ -19,10 +19,13 @@ profile pasta /usr/bin/pasta{,.avx2} flags=(attach_disconnected) { include <abstractions/pasta> # Alternatively: include <abstractions/user-tmp> - /tmp/** rw, # tap_sock_unix_init(), pcap(), - # write_pidfile(), + /tmp/** rw, # tap_sock_unix_open(), + # tap_sock_unix_init(), pcap(), + # pidfile_open(), + # pidfile_write(), # logfile_init(), # pasta_open_ns() - owner @{HOME}/** w, # pcap(), write_pidfile() + owner @{HOME}/** w, # pcap(), pidfile_open(), + # pidfile_write() } -- 2.43.0

2 1

[PATCH] netlink, test: Ignore deprecated addresses
by David Gibson 23 May '24

23 May '24

When we retrieve or copy host addresses we can include deprecated addresses, which is not what we want. Adjust our logic to exclude them. Similarly our tests can retrieve deprecated addresses, so exclude them there too. I hit this in practice because my router sometimes temporarily advertises an fd00:: prefix before the real delegated IPv6 prefix. The deprecated address can hang around for some time messing up my tests. Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- netlink.c | 5 +++-- test/passt/dhcp | 2 +- test/passt/ndp | 2 +- test/pasta/dhcp | 2 +- test/pasta/ndp | 2 +- test/two_guests/basic | 2 +- 6 files changed, 8 insertions(+), 7 deletions(-) diff --git a/netlink.c b/netlink.c index b1c0ccef..d3bea68c 100644 --- a/netlink.c +++ b/netlink.c @@ -696,7 +696,7 @@ int nl_addr_get(int s, unsigned int ifi, sa_family_t af, struct rtattr *rta; size_t na; - if (ifa->ifa_index != ifi) + if (ifa->ifa_index != ifi || ifa->ifa_flags & IFA_F_DEPRECATED) continue; for (rta = IFA_RTA(ifa), na = IFA_PAYLOAD(nh); RTA_OK(rta, na); @@ -833,7 +833,8 @@ int nl_addr_dup(int s_src, unsigned int ifi_src, ifa = (struct ifaddrmsg *)NLMSG_DATA(nh); if (rc < 0 || ifa->ifa_scope == RT_SCOPE_LINK || - ifa->ifa_index != ifi_src) + ifa->ifa_index != ifi_src || + ifa->ifa_flags & IFA_F_DEPRECATED) continue; ifa->ifa_index = ifi_dst; diff --git a/test/passt/dhcp b/test/passt/dhcp index 53ee641e..3ec2faf2 100644 --- a/test/passt/dhcp +++ b/test/passt/dhcp @@ -50,7 +50,7 @@ check [ "__SEARCH__" = "__HOST_SEARCH__" ] test DHCPv6: address guest /sbin/dhclient -6 __IFNAME__ gout ADDR6 ip -j -6 addr show|jq -rM '[.[] | select(.ifname == "__IFNAME__").addr_info[] | select(.prefixlen == 128).local] | .[0]' -hout HOST_ADDR6 ip -j -6 addr show|jq -rM '[.[] | select(.ifname == "__HOST_IFNAME6__").addr_info[] | select(.scope == "global").local] | .[0]' +hout HOST_ADDR6 ip -j -6 addr show|jq -rM '[.[] | select(.ifname == "__HOST_IFNAME6__").addr_info[] | select(.scope == "global" and .deprecated != true).local] | .[0]' check [ "__ADDR6__" = "__HOST_ADDR6__" ] test DHCPv6: route diff --git a/test/passt/ndp b/test/passt/ndp index 7b2dbfef..6bf8af30 100644 --- a/test/passt/ndp +++ b/test/passt/ndp @@ -23,7 +23,7 @@ check [ -n "__IFNAME__" ] test SLAAC: prefix gout ADDR6 ip -j -6 addr show|jq -rM '[.[] | select(.ifname == "__IFNAME__").addr_info[] | select(.scope == "global" and .prefixlen == 64).local] | .[0]' gout PREFIX6 sipcalc __ADDR6__/64 | grep prefix | cut -d' ' -f4 -hout HOST_ADDR6 ip -j -6 addr show|jq -rM '[.[] | select(.ifname == "__HOST_IFNAME6__").addr_info[] | select(.scope == "global").local] | .[0]' +hout HOST_ADDR6 ip -j -6 addr show|jq -rM '[.[] | select(.ifname == "__HOST_IFNAME6__").addr_info[] | select(.scope == "global" and .deprecated != true).local] | .[0]' hout HOST_PREFIX6 sipcalc __HOST_ADDR6__/64 | grep prefix | cut -d' ' -f4 check [ "__PREFIX6__" = "__HOST_PREFIX6__" ] diff --git a/test/pasta/dhcp b/test/pasta/dhcp index 112633ab..41556b85 100644 --- a/test/pasta/dhcp +++ b/test/pasta/dhcp @@ -37,7 +37,7 @@ test DHCPv6: address ns /sbin/dhclient -6 --no-pid __IFNAME__ hout HOST_IFNAME6 ip -j -6 route show|jq -rM '[.[] | select(.dst == "default").dev] | .[0]' nsout ADDR6 ip -j -6 addr show|jq -rM '[.[] | select(.ifname == "__IFNAME__").addr_info[] | select(.prefixlen == 128).local] | .[0]' -hout HOST_ADDR6 ip -j -6 addr show|jq -rM '[.[] | select(.ifname == "__HOST_IFNAME6__").addr_info[] | select(.scope == "global").local] | .[0]' +hout HOST_ADDR6 ip -j -6 addr show|jq -rM '[.[] | select(.ifname == "__HOST_IFNAME6__").addr_info[] | select(.scope == "global" and .deprecated != true).local] | .[0]' check [ __ADDR6__ = __HOST_ADDR6__ ] test DHCPv6: route diff --git a/test/pasta/ndp b/test/pasta/ndp index 2a8afe62..d45ff7b8 100644 --- a/test/pasta/ndp +++ b/test/pasta/ndp @@ -23,7 +23,7 @@ sleep 2 test SLAAC: prefix nsout ADDR6 ip -j -6 addr show|jq -rM '[.[] | select(.ifname == "__IFNAME__").addr_info[] | select(.scope == "global" and .prefixlen == 64).local] | .[0]' nsout PREFIX6 sipcalc __ADDR6__/64 | grep prefix | cut -d' ' -f4 -hout HOST_ADDR6 ip -j -6 addr show|jq -rM ['.[] | select(.ifname == "__IFNAME__").addr_info[] | select(.scope == "global").local] | .[0]' +hout HOST_ADDR6 ip -j -6 addr show|jq -rM ['.[] | select(.ifname == "__IFNAME__").addr_info[] | select(.scope == "global" and .deprecated != true).local] | .[0]' hout HOST_PREFIX6 sipcalc __HOST_ADDR6__/64 | grep prefix | cut -d' ' -f4 check [ "__PREFIX6__" = "__HOST_PREFIX6__" ] diff --git a/test/two_guests/basic b/test/two_guests/basic index fa0608bc..4d49e851 100644 --- a/test/two_guests/basic +++ b/test/two_guests/basic @@ -41,7 +41,7 @@ guest1 /sbin/dhclient -6 __IFNAME1__ guest2 /sbin/dhclient -6 __IFNAME2__ g1out ADDR1_6 ip -j -6 addr show|jq -rM '[.[] | select(.ifname == "__IFNAME1__").addr_info[] | select(.prefixlen == 128).local] | .[0]' g2out ADDR2_6 ip -j -6 addr show|jq -rM '[.[] | select(.ifname == "__IFNAME2__").addr_info[] | select(.prefixlen == 128).local] | .[0]' -hout HOST_ADDR6 ip -j -6 addr show|jq -rM '[.[] | select(.ifname == "__HOST_IFNAME6__").addr_info[] | select(.scope == "global").local] | .[0]' +hout HOST_ADDR6 ip -j -6 addr show|jq -rM '[.[] | select(.ifname == "__HOST_IFNAME6__").addr_info[] | select(.scope == "global" and .deprecated != true).local] | .[0]' check [ "__ADDR1_6__" = "__HOST_ADDR6__" ] check [ "__ADDR2_6__" = "__HOST_ADDR6__" ] -- 2.45.1

2 1

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

23 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

2 7

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

23 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

2 1

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

22 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 4

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

22 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 7

[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