aboutgitcodebugslistschat
path: root/tcp.c
Commit message (Collapse)AuthorAgeFilesLines
* tcp: Don't defer hash table removal2023_12_04.b86afe3David Gibson2023-12-041-3/+7
| | | | | | | | | | | | | | | | | | | When a TCP connection is closed, we mark it by setting events to CLOSED, then some time later we do final cleanups: closing sockets, removing from the hash table and so forth. This does mean that when making a hash lookup we need to exclude any apparent matches that are CLOSED, since they represent a stale connection. This can happen in practice if one connection closes and a new one with the same endpoints is started shortly afterward. Checking for CLOSED is quite specific to TCP however, and won't work when we extend the hash table to more general flows. So, alter the code to immediately remove the connection from the hash table when CLOSED, although we still defer closing sockets and other cleanup. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: "TCP" hash secret doesn't need to be TCP specificDavid Gibson2023-12-041-33/+2
| | | | | | | | | | | | | The TCP state structure includes a 128-bit hash_secret which we use for SipHash calculations to mitigate attacks on the TCP hash table and initial sequence number. We have plans to use SipHash in places that aren't TCP related, and there's no particular reason they'd need their own secret. So move the hash_secret to the general context structure. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* flow,tcp: Use epoll_ref type including flow and sideDavid Gibson2023-12-041-3/+8
| | | | | | | | | | | | | | | | | | | | | | | | | | | Currently TCP uses the 'flow' epoll_ref field for both connected sockets and timers, which consists of just the index of the relevant flow (connection). This is just fine for timers, for while it obviously works, it's subtly incomplete for sockets on spliced connections. In that case we want to know which side of the connection the event is occurring on as well as which connection. At present, we deduce that information by looking at the actual fd, and comparing it to the fds of the sockets on each side. When we use the flow table for more things, we expect more cases where something will need to know a specific side of a specific flow for an event, but nothing more. Therefore add a new 'flowside' epoll_ref field, with exactly that information. We use it for TCP connected sockets. This allows us to directly know the side for spliced connections. For "tap" connections, it's pretty meaningless, since the side is always the socket side. It still makes logical sense though, and it may become important for future flow table work. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* flow,tcp: Generalise TCP epoll_ref to generic flowsDavid Gibson2023-12-041-5/+5
| | | | | | | | | | | | | | TCP uses three different epoll object types: one for connected sockets, one for timers and one for listening sockets. Listening sockets really need information that's specific to TCP, so need their own epoll_ref field. Timers and connected sockets, however, only need the connection (flow) they're associated with. As we expand the use of the flow table, we expect that to be true for more epoll fds. So, rename the "TCP" epoll_ref field to be a "flow" epoll_ref field that can be used both for TCP and for other future cases. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Remove unneccessary bounds check in tcp_timer_handler()David Gibson2023-12-041-2/+2
| | | | | | | | | In tcp_timer_handler() we use conn_at_idx() to interpret the flow index from the epoll reference. However, this will never be NULL - we always put a valid index into the epoll_ref. Simplify slightly based on this. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* flow, tcp: Add logging helpers for connection related messagesDavid Gibson2023-12-041-44/+38
| | | | | | | | | | | | | | | | | | | Most of the messages logged by the TCP code (be they errors, debug or trace messages) are related to a specific connection / flow. We're fairly consistent about prefixing these with the type of connection and the connection / flow index. However there are a few places where we put the index later in the message or omit it entirely. The template with the prefix is also a little bulky to carry around for every message, particularly for spliced connections. To help keep this consistent, introduce some helpers to log messages linked to a specific flow. It takes the flow as a parameter and adds a uniform prefix to each message. This makes things slightly neater now, but more importantly will help keep formatting consistent as we add more things to the flow table. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* flow: Make unified version of flow table compactionDavid Gibson2023-12-041-42/+4
| | | | | | | | | | | tcp_table_compact() will move entries in the connection/flow table to keep it compact when other entries are removed. The moved entries need not have the same type as the flow removed, so it needs to be able to handle moving any type of flow. Therefore, move it to flow.c rather than being purportedly TCP specific. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* flow, tcp: Consolidate flow pointer<->index helpersDavid Gibson2023-12-041-34/+33
| | | | | | | | | | | | | | | | | | | Both tcp.c and tcp_splice.c define CONN_IDX() variants to find the index of their connection structures in the connection table, now become the unified flow table. We can easily combine these into a common helper. While we're there, add some trickery for some additional type safety. They also define their own CONN() versions, which aren't so easily combined since they need to return different types, but we can have them use a common helper. In the process, we standardise on always using an unsigned type to store the connection / flow index, which makes more sense. tcp.c's conn_at_idx() remains for now, but we change its parameter to unsigned to match. That in turn means we can remove a check for negative values from it. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* flow, tcp: Move TCP connection table to unified flow tableDavid Gibson2023-12-041-45/+42
| | | | | | | | | | | | | We want to generalise "connection" tracking to things other than true TCP connections. Continue implenenting this by renaming the TCP connection table to the "flow table" and moving it to flow.c. The definitions are split between flow.h and flow_table.h - we need this separation to avoid circular dependencies: the definitions in flow.h will be needed by many headers using the flow mechanism, but flow_table.h needs all those protocol specific headers in order to define the full flow table entry. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* flow, tcp: Generalise connection typesDavid Gibson2023-12-041-18/+45
| | | | | | | | | | | | Currently TCP connections use a 1-bit selector, 'spliced', to determine the rest of the contents of the structure. We want to generalise the TCP connection table to other types of flows in other protocols. Make a start on this by replacing the tcp_conn_common structure with a new flow_common structure with an enum rather than a simple boolean indicating the type of flow. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: remove useless assignmentLaurent Vivier2023-12-041-1/+0
| | | | | | | | | | | | | | | | | | | | In tcp_send_flag(), a4826ee04b76 has replaced: th->doff = sizeof(*th) / 4; th->doff += OPT_MSS_LEN / 4; th->doff += (1 + OPT_WS_LEN) / 4; by optlen = OPT_MSS_LEN + 1 + OPT_WS_LEN; th->doff = (sizeof(*th) + optlen) / 4; but forgot to remove the useless "th->doff += (1 + OPT_WS_LEN) / 4;" Fixes: a4826ee04b76 ("tcp: Defer and coalesce all segments with no data (flags) to handler") Signed-off-by: Laurent Vivier <lvivier@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* treewide: Use 'z' length modifier for size_t/ssize_t conversionsStefano Brivio2023-12-021-1/+2
| | | | | | | | Types size_t and ssize_t are not necessarily long, it depends on the architecture. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* valgrind: Adjust suppression for MSG_TRUNC with NULL bufferDavid Gibson2023-11-191-0/+9
| | | | | | | | | | | | | | | | | | | | | | | | | valgrind complains if we pass a NULL buffer to recv(), even if we use MSG_TRUNC, in which case it's actually safe. For a long time we've had a valgrind suppression for this. It singles out the recv() in tcp_sock_consume(), the only place we use MSG_TRUNC. However, tcp_sock_consume() only has a single caller, which makes it a prime candidate for inlining. If inlined, it won't appear on the stack and valgrind won't match the correct suppression. It appears that certain compiler versions (for example gcc-13.2.1 in Fedora 39) will inline this function even with the -O0 we use for valgrind builds. This breaks the suppression leading to a spurious failure in the tests. There's not really any way to adjust the suppression itself without making it overly broad (we don't want to match other recv() calls). So, as a hack explicitly prevent inlining of this function when we're making a valgrind build. To accomplish this add an explicit -DVALGRIND when making such a build. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Simplify away tcp_port_rebind()David Gibson2023-11-191-29/+12
| | | | | | | | | | | | | | | | | tcp_port_rebind() is desgined to be called from NS_CALL() and has two disjoint cases: one where it enters the namespace (outbound forwards) and one where it doesn't (inbound forwards). We only actually need the NS_CALL() framing for the outbound case, for inbound we can just call tcp_port_do_rebind() directly. So simplify tcp_port_rebind() to tcp_port_rebind_outbound(), allowing us to eliminate an awkward parameters structure. With that done we can safely rename tcp_port_do_rebind() to tcp_port_rebind() for brevity. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Use common helper for rebinding inbound and outbound portsDavid Gibson2023-11-191-47/+45
| | | | | | | | | tcp_port_rebind() has two cases with almost but not quite identical code. Simplify things a bit by factoring this out into a single parameterised helper, tcp_port_do_rebind(). Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Don't use TCP_WINDOW_CLAMPDavid Gibson2023-11-101-56/+9
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | On the L2 tap side, we see TCP headers and know the TCP window that the ultimate receiver is advertising. In order to avoid unnecessary buffering within passt/pasta (or by the kernel on passt/pasta's behalf) we attempt to advertise that window back to the original sock-side sender using TCP_WINDOW_CLAMP. However, TCP_WINDOW_CLAMP just doesn't work like this. Prior to kernel commit 3aa7857fe1d7 ("tcp: enable mid stream window clamp"), it simply had no effect on established sockets. After that commit, it does affect established sockets but doesn't behave the way we need: * It appears to be designed only to shrink the window, not to allow it to re-expand. * More importantly, that commit has a serious bug where if the setsockopt() is made when the existing kernel advertised window for the socket happens to be zero, it will now become locked at zero, stopping any further data from being received on the socket. Since this has never worked as intended, simply remove it. It might be possible to re-implement the intended behaviour by manipulating SO_RCVBUF, so we leave a comment to that effect. This kernel bug is the underlying cause of both the linked passt bug and the linked podman bug. We attempted to fix this before with passt commit d3192f67 ("tcp: Force TCP_WINDOW_CLAMP before resetting STALLED flag"). However while that commit masked the bug for some cases, it didn't really address the problem. Fixes: d3192f67c492 ("tcp: Force TCP_WINDOW_CLAMP before resetting STALLED flag") Link: https://github.com/containers/podman/issues/20170 Link: https://bugs.passt.top/show_bug.cgi?id=74 Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Rename and small cleanup to tcp_clamp_window()David Gibson2023-11-101-11/+10
| | | | | | | | | | | | tcp_clamp_window() is _mostly_ about using TCP_WINDOW_CLAMP to control the sock side advertised window, but it is also responsible for actually updating the conn->wnd_from_tap value. Rename to tcp_tap_window_update() to reflect that broader purpose, and pull the logic that's not TCP_WINDOW_CLAMP related out to the front. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* log: Enable format warningsDavid Gibson2023-11-071-5/+8
| | | | | | | | | | | | | | | | | | | | | | | | | | logmsg() takes printf like arguments, but because it's not a built in, the compiler won't generate warnings if the format string and parameters don't match. Enable those by using the format attribute. Strictly speaking this is a gcc extension, but I believe it is also supported by some other common compilers. We already use some other attributes in various places. For now, just use it and we can worry about compilers that don't support it if it comes up. This exposes some warnings from existing callers, both in gcc and in clang-tidy: - Some are straight out bugs, which we correct - It's occasionally useful to invoke the logging functions with an empty string, which gcc objects to, so disable that specific warning in the Makefile - Strictly speaking the C standard requires that the parameter for a %p be a (void *), not some other pointer type. That's only likely to cause problems in practice on weird architectures with different sized representations for pointers to different types. Nonetheless add the casts to make it happy. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Remove remaining declaration of tcp_l2_mhLaurent Vivier2023-11-071-6/+0
| | | | | | | | | | | Use of tcp_l2_mh has been removed in commit 38fbfdbcb95d, but its declaration and initialization are always in the code. Remove them as they are useless. Fixes: 38fbfdbcb95d ("tcp: Get rid of iov with cached MSS, drop sendmmsg(), add deferred flush") Signed-off-by: Laurent Vivier <lvivier@redhat.com> Acked-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* pif: Pass originating pif to tap handler functionsDavid Gibson2023-11-071-1/+5
| | | | | | | | | | | For now, packets passed to the various *_tap_handler() functions always come from the single "tap" interface. We want to allow the possibility to broaden that in future. As preparation for that, have the code in tap.c pass the pif id of the originating interface to each of those handler functions. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* pif: Record originating pif in listening socket refsDavid Gibson2023-11-071-2/+3
| | | | | | | | | | For certain socket types, we record in the epoll ref whether they're sockets in the namespace, or on the host. We now have the notion of "pif" to indicate what "place" a socket is associated with, so generalise the simple one-bit 'ns' to a pif id. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* port_fwd: Simplify get_bound_ports_*() to port_fwd_scan_*()David Gibson2023-11-071-2/+2
| | | | | | | | | | | | | | | | | | | | get_bound_ports_*() now only use their context and ns parameters to determine which forwarding maps they're operating on. Each function needs the map they're actually updating, as well as the map for the other direction, to avoid creating forwarding loops. The UDP function also requires the corresponding TCP map, to implement the behaviour where we forward UDP ports of the same number as bound TCP ports for tools like iperf3. Passing those maps directly as parameters simplifies the code without making the callers life harder, because those already know the relevant maps. IMO, invoking these functions in terms of where they're looking for updated forwarding also makes more logical sense than in terms of where they're looking for bound ports. Given that new way of looking at the functions, also rename them to port_fwd_scan_*(). Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* port_fwd: Split TCP and UDP cases for get_bound_ports()David Gibson2023-11-071-2/+2
| | | | | | | | | | | | Currently get_bound_ports() takes a parameter to determine if it scans for UDP or TCP bound ports, but in fact there's almost nothing in common between those two paths. The parameter appears primarily to have been a convenience for when we needed to invoke this function via NS_CALL(). Now that we don't need that, split it into separate TCP and UDP versions. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* port_fwd: Don't NS_CALL get_bound_ports()David Gibson2023-11-071-36/+2
| | | | | | | | | | | | | | When we want to scan for bound ports in the namespace we use NS_CALL() to run get_bound_ports() in the namespace. However, the only thing it actually needed to be in the namespace for was to open the /proc/net file it was scanning. Since we now always pre-open those, we no longer need to switch to the namespace for the actual get_bound_ports() calls. That in turn means that tcp_port_detect() doesn't need to run in the ns either, and we can just replace it with inline calls to get_bound_ports(). Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* port_fwd: Move automatic port forwarding code to port_fwd.[ch]David Gibson2023-11-071-1/+0
| | | | | | | | | | | | | The implementation of scanning /proc files to do automatic port forwarding is a bit awkwardly split between procfs_scan_listen() in util.c, get_bound_ports() and related functions in conf.c and the initial setup code in conf(). Consolidate all of this into port_fwd.h, which already has some related definitions, and a new port_fwd.c. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp, tap: Don't increase tap-side sequence counter for dropped framesStefano Brivio2023-10-041-6/+34
| | | | | | | | | | | | | | | | | | | | | | | | | | ...so that we'll retry sending them, instead of more-or-less silently dropping them. This happens quite frequently if our sending buffer on the UNIX domain socket is heavily constrained (for instance, by the 208 KiB default memory limit). It might be argued that dropping frames is part of the expected TCP flow: we don't dequeue those from the socket anyway, so we'll eventually retransmit them. But we don't need the receiver to tell us (by the way of duplicate or missing ACKs) that we couldn't send them: we already know as sendmsg() reports that. This seems to considerably increase throughput stability and throughput itself for TCP connections with default wmem_max values. Unfortunately, the 16 bits left as padding in the frame descriptors we use internally aren't enough to uniquely identify for which connection we should update sequence numbers: create a parallel array of pointers to sequence numbers and L4 lengths, of TCP_FRAMES_MEM size, and go through it after calling sendmsg(). Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* tcp: Force TCP_WINDOW_CLAMP before resetting STALLED flagStefano Brivio2023-10-041-5/+24
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | It looks like we need it as workaround for this situation, readily reproducible at least with a 6.5 Linux kernel, with default rmem_max and wmem_max values: - an iperf3 client on the host sends about 160 KiB, typically segmented into five frames by passt. We read this data using MSG_PEEK - the iperf3 server on the guest starts receiving - meanwhile, the host kernel advertised a zero-sized window to the sender, as expected - eventually, the guest acknowledges all the data sent so far, and we drop it from the buffer, courtesy of tcp_sock_consume(), using recv() with MSG_TRUNC - the client, however, doesn't get an updated window value, and even keepalive packets are answered with zero-window segments, until the connection is closed It looks like dropping data from a socket using MSG_TRUNC doesn't cause a recalculation of the window, which would be expected as a result of any receiving operation that invalidates data on a buffer (that is, not with MSG_PEEK). Strangely enough, setting TCP_WINDOW_CLAMP via setsockopt(), even to the previous value we clamped to, forces a recalculation of the window which is advertised to the sender. I couldn't quite confirm this issue by following all the possible code paths in the kernel, yet. If confirmed, this should be fixed in the kernel, but meanwhile this workaround looks robust to me (and it will be needed for backward compatibility anyway). Reported-by: Matej Hrica <mhrica@redhat.com> Link: https://bugs.passt.top/show_bug.cgi?id=74 Analysed-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* tcp: Fix comment to tcp_sock_consume()Stefano Brivio2023-10-041-1/+1
| | | | | | | | | Note that tcp_sock_consume() doesn't update ACK sequence counters anymore. Fixes: cc6d8286d104 ("tcp: Reset ACK_FROM_TAP_DUE flag only as needed, update timer") Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* cppcheck: Work around bug in cppcheck 2.12.0David Gibson2023-10-041-0/+7
| | | | | | | | | cppcheck 2.12.0 (and maybe some other versions) things this if condition is always true, which is demonstrably not true. Work around the bug for now. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* cppcheck: Make many pointers constDavid Gibson2023-10-041-9/+9
| | | | | | | | | Newer versions of cppcheck (as of 2.12.0, at least) added a warning for pointers which could be declared to point at const data, but aren't. Based on that, make many pointers throughout the codebase const. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* siphash: Use incremental rather than all-at-once siphash functionsDavid Gibson2023-09-301-22/+10
| | | | | | | | | | | | | | | | | | We have a bunch of variants of the siphash functions for different data sizes. The callers, in tcp.c, need to pack the various values they want to hash into a temporary structure, then call the appropriate version. We can avoid the copy into the temporary by directly using the incremental siphash functions. The length specific hash functions also have an undocumented constraint that the data pointer they take must, in fact, be aligned to avoid unaligned accesses, which may cause crashes on some architectures. So, prefer the incremental approach and remove the length-specific functions. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* siphash: Make siphash functions consistently return 64-bit resultsDavid Gibson2023-09-301-3/+4
| | | | | | | | | | | | | | Some of the siphas_*b() functions return 64-bit results, others 32-bit results, with no obvious pattern. siphash_32b() also appears to do this incorrectly - taking the 64-bit hash value and simply returning it truncated, rather than folding the two halves together. Since SipHash proper is defined to give a 64-bit hash, make all of them return 64-bit results. In the one caller which needs a 32-bit value, tcp_seq_init() do the fold down to 32-bits ourselves. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* util: Consolidate and improve workarounds for clang-tidy issue 58992David Gibson2023-09-271-7/+1
| | | | | | | | | | | | | | | | | | | | | We have several workarounds for a clang-tidy bug where the checker doesn't recognize that a number of system calls write to - and therefore initialise - a socket address. We can't neatly use a suppression, because the bogus warning shows up some time after the actual system call, when we access a field of the socket address which clang-tidy erroneously thinks is uninitialised. Consolidate these workarounds into one place by using macros to implement wrappers around affected system calls which add a memset() of the sockaddr to silence clang-tidy. This removes the need for the individual memset() workarounds at the callers - and the somewhat longwinded explanatory comments. We can then use a #define to not include the hack in "real" builds, but only consider it for clang-tidy. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* Avoid shadowing index(3)David Gibson2023-09-271-7/+7
| | | | | | | | | | | | | | | | | A classic gotcha of the standard C library is that its unwise to call any variable 'index' because it will shadow the standard string library function index(3). This can cause warnings from cppcheck amongst others, and it also means that if the variable is removed you tend to get confusing type errors (or sometimes nothing at all) instead of a nice simple "name is not defined" error. Strictly speaking this only occurs if <string.h> is included, but that is so common that as a rule it's best to just avoid it always. We have a number of places which hit this trap, so rename variables and parameters to avoid it. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Always send an ACK segment once the handshake is completedStefano Brivio2023-09-271-1/+1
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The reporter is running a SMTP server behind pasta, and the client waits for the server's banner before sending any data. In turn, the server waits for our ACK after sending SYN,ACK, which never comes. If we use the ACK_IF_NEEDED indication to tcp_send_flag(), given that there's no pending data, we delay sending the ACK segment at the end of the three-way handshake until we have some data to send to the server. This was actually intended, as I thought we would lower the latency for new connections, but we can't assume that the client will start sending data first (SMTP is the typical example where this doesn't happen). And, trying out this patch with SSH (where the client starts sending data first), the reporter actually noticed we have a lower latency by forcing an ACK right away. Comparing a capture before the patch: 13:07:14.007704 IP 10.1.2.1.42056 > 10.1.2.140.1234: Flags [S], seq 1797034836, win 65535, options [mss 4096,nop,wscale 7], length 0 13:07:14.007769 IP 10.1.2.140.1234 > 10.1.2.1.42056: Flags [S.], seq 2297052481, ack 1797034837, win 65480, options [mss 65480,nop,wscale 7], length 0 13:07:14.008462 IP 10.1.2.1.42056 > 10.1.2.140.1234: Flags [.], seq 1:22, ack 1, win 65535, length 21 13:07:14.008496 IP 10.1.2.140.1234 > 10.1.2.1.42056: Flags [.], ack 22, win 512, length 0 13:07:14.011799 IP 10.1.2.140.1234 > 10.1.2.1.42056: Flags [P.], seq 1:515, ack 22, win 512, length 514 and after: 13:10:26.165364 IP 10.1.2.1.59508 > 10.1.2.140.1234: Flags [S], seq 4165939595, win 65535, options [mss 4096,nop,wscale 7], length 0 13:10:26.165391 IP 10.1.2.140.1234 > 10.1.2.1.59508: Flags [S.], seq 985607380, ack 4165939596, win 65480, options [mss 65480,nop,wscale 7], length 0 13:10:26.165418 IP 10.1.2.1.59508 > 10.1.2.140.1234: Flags [.], ack 1, win 512, length 0 13:10:26.165683 IP 10.1.2.1.59508 > 10.1.2.140.1234: Flags [.], seq 1:22, ack 1, win 512, length 21 13:10:26.165698 IP 10.1.2.140.1234 > 10.1.2.1.59508: Flags [.], ack 22, win 512, length 0 13:10:26.167107 IP 10.1.2.140.1234 > 10.1.2.1.59508: Flags [P.], seq 1:515, ack 22, win 512, length 514 the latency between the initial SYN segment and the first data transmission actually decreases from 792µs to 334µs. This is not statistically relevant as we have a single measurement, but it can't be that bad, either. Reported-by: cr3bs (from IRC) Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* tcp: Correct handling of FIN,ACK followed by SYNDavid Gibson2023-09-081-1/+1
| | | | | | | | | | | | | | | | | | | | When the guest tries to establish a connection, it could give up on it by sending a FIN,ACK instead of a plain ACK to our SYN,ACK. It could then make a new attempt to establish a connection with the same addresses and ports with a new SYN. Although it's unlikely, it could send the 2nd SYN very shortly after the FIN,ACK resulting in both being received in the same batch of packets from the tap interface. Currently, we don't handle that correctly, when we receive a FIN,ACK on a not fully established connection we discard the remaining packets in the batch, and so will never process the 2nd SYN. Correct this by returning 1 from tcp_tap_handler() in this case, so we'll just consume the FIN,ACK and continue to process the rest of the batch. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Consolidate paths where we initiate reset on tap interfaceDavid Gibson2023-09-081-22/+25
| | | | | | | | | | | | | | | | | | | There are a number of conditions where we will issue a TCP RST in response to something unexpected we received from the tap interface. These occur in both tcp_data_from_tap() and tcp_tap_handler(). In tcp_tap_handler() use a 'goto out of line' technique to consolidate all these paths into one place. For the tcp_data_from_tap() cases use a negative return code and direct that to the same path in tcp_tap_handler(), its caller. In this case we want to discard all remaining packets in the batch we have received: even if they're otherwise good, they'll be invalidated when the guest receives the RST we're sending. This is subtly different from the case where we *receive* an RST, where we could in theory get a new SYN immediately afterwards. Clarify that with a common on the now common reset path. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Correctly handle RST followed rapidly by SYNDavid Gibson2023-09-081-2/+2
| | | | | | | | | | | | | | | | | Although it's unlikely in practice, the guest could theoretically reset one TCP connection then immediately start a new one with the same addressses and ports, such that we get an RST then a SYN in the same batch of received packets in tcp_tap_handler(). We don't correctly handle that unlikely case, because when we receive the RST, we discard any remaining packets in the batch so we'd never see the SYN. This could happen in either tcp_tap_handler() or tcp_data_from_tap(). Correct that by returning 1, so that the caller will continue calling tcp_tap_handler() on subsequent packets allowing us to process any subsequent SYN. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Return consumed packet count from tcp_data_from_tap()David Gibson2023-09-081-10/+15
| | | | | | | | | | Currently tcp_data_from_tap() is assumed to consume all packets remaining in the packet pool it is given. However there are some edge cases where that's not correct. In preparation for fixing those, change it to return a count of packets consumed and use that in its caller. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Never hash match closed connectionsDavid Gibson2023-09-081-1/+1
| | | | | | | | | | | | | | | >From a practical point of view, when a TCP connection ends, whether by FIN or by RST, we set the CLOSED event, then some time later we remove the connection from the hash table and clean it up. However, from a protocol point of view, once it's closed, it's gone, and any new packets with matching addresses and ports are either forming a new connection, or are invalid packets to discard. Enforce these semantics in the TCP hash logic by never hash matching closed connections. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Remove some redundant packet_get() operationsDavid Gibson2023-09-081-10/+4
| | | | | | | | | | | | Both tcp_data_from_tap() and tcp_tap_handler() call packet_get() to get the entire L4 packet length, then immediately call it again to check that the packet is long enough to include a TCP header. The features of packet_get() let us easily combine these together, we just need to adjust the length slightly, because we want the value to include the TCP header length. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp, tap: Correctly advance through packets in tcp_tap_handler()David Gibson2023-09-081-13/+15
| | | | | | | | | | | | | | | | | | | | | | | | | In both tap4_handler() and tap6_handler(), once we've sorted incoming l3 packets into "sequences", we then step through all the packets in each TCP sequence calling tcp_tap_handler(). Or so it appears. In fact, tcp_tap_handler() doesn't take an index and always looks at packet 0 of the sequence, except when it calls tcp_data_from_tap() to process data packets. It appears to be written with the idea that the struct pool is a queue, from which it consumes packets as it processes them, but that's not how the pool data structure works - they are more like an array of packets. We only get away with this, because setup packets for TCP tend to come in separate batches (because we need to reply in between) and so we only get a bunch of packets for the same connection together when they're data packets (tcp_data_from_tap() has its own loop through packets). Correct this by adding an index parameter to tcp_tap_handler() and altering the loops in tap.c to step through the pool properly. Link: https://bugs.passt.top/show_bug.cgi?id=68 Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Remove broken pressure calculations for tcp_defer_handler()David Gibson2023-08-221-9/+0
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | tcp_defer_handler() performs a potentially expensive linear scan of the connection table. So, to mitigate the cost of that we skip if if we're not under at least moderate pressure: either 30% of available connections or 30% (estimated) of available fds used. But, the calculation for this has been broken since it was introduced: we calculate "max_conns" based on c->tcp.conn_count, not TCP_MAX_CONNS, meaning we only exit early if conn_count is less than 30% of itself, i.e. never. If that calculation is "corrected" to be based on TCP_MAX_CONNS, it completely tanks the TCP CRR times for passt - from ~60ms to >1000ms on my laptop. My guess is that this is because in the case of many short lived connections, we're letting the table become much fuller before compacting it. That means that other places which perform a table scan now have to do much, much more. For the time being, simply remove the tests, since they're not doing anything useful. We can reintroduce them more carefully if we see a need for them. This also removes the only user of c->tcp.splice_conn_count, so that can be removed as well. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Move in_epoll flag out of common connection structureDavid Gibson2023-08-221-3/+3
| | | | | | | | | | | | | | | | | | | The in_epoll boolean is one of only two fields (currently) in the common structure shared between tap and spliced connections. It seems like it belongs there, because both tap and spliced connections use it, and it has roughly the same meaning. Roughly, however, isn't exactly: which fds this flag says are in the epoll varies between the two connection types, and are in type specific fields. So, it's only possible to meaningfully use this value locally in type specific code anyway. This common field is going to get in the way of more widespread generalisation of connection / flow tracking, so move it to separate fields in the tap and splice specific structures. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp, udp: Don't pre-fill IPv4 destination address in headersDavid Gibson2023-08-221-7/+1
| | | | | | | | | | | | | | | | | Because packets sent on the tap interface will always be going to the guest/namespace, we more-or-less know what address they'll be going to. So we pre-fill this destination address in our header buffers for IPv4. We can't do the same for IPv6 because we could need either the global or link-local address for the guest. In future we're going to want more flexibility for the destination address, so this pre-filling will get in the way. Change the flow so we always fill in the IPv4 destination address for each packet, rather than prefilling it from proto_update_l2_buf(). In fact for TCP we already redundantly filled the destination for each packet anyway. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp, udp: Don't include destination address in partially precomputed csumsDavid Gibson2023-08-221-40/+21
| | | | | | | | | | | | | | | | | | | | | | We partially prepopulate IP and TCP header structures including, amongst other things the destination address, which for IPv4 is always the known address of the guest/namespace. We partially precompute both the IPv4 header checksum and the TCP checksum based on this. In future we're going to want more flexibility with controlling the destination for IPv4 (as we already do for IPv6), so this precomputed value gets in the way. Therefore remove the IPv4 destination from the precomputed checksum and fold it into the checksum update when we actually send a packet. Doing this means we no longer need to recompute those partial sums when the destination address changes ({tcp,udp}_update_l2_buf()) and instead the computation can be moved to compile time. This means while we perform slightly more computations on each packet, we slightly reduce the amount of memory we need to access. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Consistent usage of ports in tcp_seq_init()David Gibson2023-08-221-2/+2
| | | | | | | | | | | | | | | In tcp_seq_init() the meaning of "src" and "dst" isn't really clear since it's used for connections in both directions. However, these values are just feeding a hash, so as long as we're consistent and include all the information we want, it doesn't really matter. Oddly, for the "src" side we supply the (tap side) forwarding address but the (tap side) endpoint port. This again doesn't really matter, but it's confusing. So swap this with dstport, so "src" is always forwarding and "dst" is always endpoint. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: More precise terms for addresses and portsDavid Gibson2023-08-221-46/+47
| | | | | | | | | | | | | | | | | | | | | | | | | | | In a number of places the comments and variable names we use to describe addresses and ports are ambiguous. It's not sufficient to describe a port as "tap-facing" or "socket-facing", because on both the tap side and the socket side there are two ports for the two ends of the connection. Similarly, "local" and "remote" aren't particularly helpful, because it's not necessarily clear whether we're talking from the point of view of the guest/namespace, the host, or passt itself. This patch makes a number of changes to be more precise about this. It introduces two new terms in aid of this: A "forwarding" address (or port) refers to an address which is local from the point of view of passt itself. That is a source address for traffic sent by passt, whether it's to the guest via the tap interface or to a host on the internet via a socket. The "endpoint" address (or port) is the reverse: a remote address from passt's point of view, the destination address for traffic sent by passt. Between them the "side" (either tap/guest-facing or sock/host-facing) and forwarding vs. endpoint unambiguously describes which address or port we're talking about. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Pass source address to protocol handler functionsDavid Gibson2023-08-221-11/+17
| | | | | | | | | | | The tap code passes the IPv4 or IPv6 destination address of packets it receives to the protocol specific code. Currently that protocol code doesn't use the source address, but we want it to in future. So, in preparation, pass the IPv4/IPv6 source address of tap packets to those functions as well. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* epoll: Split handling of listening TCP sockets into their own handlerDavid Gibson2023-08-131-29/+22
| | | | | | | | | | | | | | | | | tcp_sock_handler() handles both listening TCP sockets, and connected TCP sockets, but what it needs to do in those cases has essentially nothing in common. Therefore, give listening sockets their own epoll_type value and dispatch directly to their own handler from the top level. Furthermore, the two handlers need essentially entirely different information from the reference: we re-(ab)used the index field in the tcp_epoll_ref to indicate the port for the listening socket, but that's not the same meaning. So, switch listening sockets to their own reference type which we can lay out as we please. That lets us remove the listen and outbound fields from the normal (connected) tcp_epoll_ref, reducing it to just the connection table index. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
generated by cgit v1.2.3 (git 2.25.1) at 2024-11-14 11:04:30 +0000