aboutgitcodebugslistschat
path: root/tcp.c
Commit message (Collapse)AuthorAgeFilesLines
* 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>
* epoll: Split handling of TCP timerfds into its own handler functionDavid Gibson2023-08-131-11/+4
| | | | | | | | | | | | tcp_sock_handler() actually handles several different types of fd events. This includes timerfds that aren't sockets at all. The handling of these has essentially nothing in common with the other cases. So, give the TCP timers there own epoll_type value and dispatch directly to their handler. This also means we can remove the timer field from tcp_epoll_ref, the information it encoded is now implicit in the epoll_type value. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* epoll: Generalize epoll_ref to cover things other than socketsDavid Gibson2023-08-131-11/+11
| | | | | | | | | | | | | | The epoll_ref type includes fields for the IP protocol of a socket, and the socket fd. However, we already have a few things in the epoll which aren't protocol sockets, and we may have more in future. Rename these fields to an abstract "fd type" and file descriptor for more generality. Similarly, rather than using existing IP protocol numbers for the type, introduce our own number space. For now these just correspond to the supported protocols, but we'll expand on that in future. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* Use C11 anonymous members to make poll refs less verbose to useDavid Gibson2023-08-041-23/+23
| | | | | | | | | | | | union epoll_ref has a deeply nested set of structs and unions to let us subdivide it into the various different fields we want. This means that referencing elements can involve an awkward long string of intermediate fields. Using C11 anonymous structs and unions lets us do this less clumsily. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* passt: Relicense to GPL 2.0, or any later versionStefano Brivio2023-04-061-1/+1
| | | | | | | | | | | | | | | | | | | In practical terms, passt doesn't benefit from the additional protection offered by the AGPL over the GPL, because it's not suitable to be executed over a computer network. Further, restricting the distribution under the version 3 of the GPL wouldn't provide any practical advantage either, as long as the passt codebase is concerned, and might cause unnecessary compatibility dilemmas. Change licensing terms to the GNU General Public License Version 2, or any later version, with written permission from all current and past contributors, namely: myself, David Gibson, Laine Stump, Andrea Bolognani, Paul Holzinger, Richard W.M. Jones, Chris Kuhn, Florian Weimer, Giuseppe Scrivano, Stefan Hajnoczi, and Vasiliy Ulyanov. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Clear ACK_FROM_TAP_DUE also on unchanged ACK sequence from peerStefano Brivio2023-03-291-3/+5
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Since commit cc6d8286d104 ("tcp: Reset ACK_FROM_TAP_DUE flag only as needed, update timer"), we don't clear ACK_FROM_TAP_DUE whenever we process an ACK segment, but, more correctly, only if we're really not waiting for a further ACK segment, that is, only if the acknowledged sequence matches what we sent. In the new function implementing this, tcp_update_seqack_from_tap(), we also reset the retransmission counter and store the updated ACK sequence. Both should be done iff forward progress is acknowledged, implied by the fact that the new ACK sequence is greater than the one we previously stored. At that point, it looked natural to also include the statements that clear and set the ACK_FROM_TAP_DUE flag inside the same conditional block: if we're not making forward progress, the need for an ACK, or lack thereof, should remain unchanged. There might be cases where this isn't true, though: without the previous commit 4e73e9bd655c ("tcp: Don't special case the handling of the ack of a syn"), this would happen if a tap-side client initiated a connection, and the server didn't send any data. At that point we would never, in the established state of the connection, call tcp_update_seqack_from_tap() with reported forward progress. That issue itself is fixed by the previous commit, now, but clearing ACK_FROM_TAP_DUE only on ACK sequence progress doesn't really follow any logic. Clear the ACK_FROM_TAP_DUE flag regardless of reported forward progress. If we clear it when it's already unset, conn_flag() will do nothing with it. This doesn't fix any known functional issue, rather a conceptual one. Fixes: cc6d8286d104 ("tcp: Reset ACK_FROM_TAP_DUE flag only as needed, update timer") Reported-by: David Gibson <david@gibson.dropbear.id.au> Analysed-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Don't special case the handling of the ack of a synDavid Gibson2023-03-291-2/+2
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | TCP treats the SYN packets as though they occupied 1 byte in the logical data stream described by the sequence numbers. That is, the very first ACK (or SYN-ACK) each side sends should acknowledge a sequence number one greater than the initial sequence number given in the SYN or SYN-ACK it's responding to. In passt we were tracking that by advancing conn->seq_to_tap by one when we send a SYN or SYN-ACK (in tcp_send_flag()). However, we also initialized conn->seq_ack_from_tap, representing the acks we've already seen from the tap side, to ISN+1, meaning we treated it has having acknowledged the SYN before it actually did. There were apparently reasons for this in earlier versions, but it causes problems now. Because of this when we actually did receive the initial ACK or SYN-ACK, we wouldn't see the acknoweldged serial number as advancing, and so wouldn't clear the ACK_FROM_TAP_DUE flag. In most cases we'd get away because subsequent packets would clear the flag. However if one (or both) sides didn't send any data, the other side would (correctly) keep sending ISN+1 as the acknowledged sequence number, meaning we would never clear the ACK_FROM_TAP_DUE flag. That would mean we'd treat the connection as if we needed to retransmit (although we had 0 bytes to retransmit), and eventaully (after around 30s) reset the connection due to too many retransmits. Specifically this could cause the iperf3 throughput tests in the testsuite to fail if set for a long enough test period. Correct this by initializing conn->seq_ack_from_tap to the ISN and only advancing it when we actually get the first ACK (or SYN-ACK). Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Clarify allowed state for tcp_data_from_tap()David Gibson2023-03-291-0/+5
| | | | | | | | | | | | | | Comments suggest that this should only be called for an ESTABLISHED connection. However, it's non-trivial to ascertain that from the actual control flow in the caller. Add an ASSERT() to make it very clear that this is only called in ESTABLISHED state. In fact, there were some circumstances where it could be called on a CLOSED connection. In a sense that is "established", but with that assert this does require specific (trivial) handling to avoid a spurious abort(). Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Don't reset ACK_TO_TAP_DUE on any ACK, reschedule timer as needed2023_03_21.1ee2f7cStefano Brivio2023-03-211-4/+8
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | This is mostly symmetric with commit cc6d8286d104 ("tcp: Reset ACK_FROM_TAP_DUE flag only as needed, update timer"): we shouldn't reset the ACK_TO_TAP_DUE flag on any inbound ACK segment, but only once we acknowledge everything we received from the guest or the container. If we don't, a client might unnecessarily hold off further data, especially during slow start, and in general we won't converge to the usable bandwidth. This is very visible especially with traffic tests on links with non-negligible latency, such as in the reported issue. There, a public iperf3 server sometimes aborts the test due do what appears to be a low iperf3's --rcv-timeout (probably less than a second). Even if this doesn't happen, the throughput will converge to a fraction of the usable bandwidth. Clear ACK_TO_TAP_DUE if we acknowledged everything, set it if we didn't, and reschedule the timer in case the flag is still set as the timer expires. While at it, decrease the ACK timer interval to 10ms. A 50ms interval is short enough for any bandwidth-delay product I had in mind (local connections, or non-local connections with limited bandwidth), but here I am, testing 1gbps transfers to a peer with 100ms RTT. Indeed, we could eventually make the timer interval dependent on the current window and estimated bandwidth-delay product, but at least for the moment being, 10ms should be long enough to avoid any measurable syscall overhead, yet usable for any real-world application. Reported-by: Lukas Mrtvy <lukas.mrtvy@gmail.com> Link: https://bugs.passt.top/show_bug.cgi?id=44 Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: When a connection flag it set, don't negate it for debug printStefano Brivio2023-03-211-1/+1
| | | | | | | | | Fix a copy and paste typo I added in commit 5474bc5485d8 ("tcp, tcp_splice: Get rid of false positive CWE-394 Coverity warning from fls()") and --debug altogether. Fixes: 5474bc5485d8 ("tcp, tcp_splice: Get rid of false positive CWE-394 Coverity warning from fls()") Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* Fix false positive if cppcheck doesn't give a false positiveDavid Gibson2023-03-211-1/+1
| | | | | | | | | | | | | | | da46fdac "tcp: Suppress knownConditionTrueFalse cppcheck false positive" introduced a suppression to work around a cppcheck bug causing a false positive warning. However, the suppression will itself cause a spurious unmatchedSuppression warning if used with a version of cppcheck from before the bug was introduced. That includes the packaged version of cppcheck in Fedora. Suppress the unmatchedSuppression as well. Fixes: da46fdac3605 ("tcp: Suppress knownConditionTrueFalse cppcheck false positive") Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* Work around weird false positives with cppcheck-2.9.1David Gibson2023-03-211-1/+1
| | | | | | | | | | | | | | | | | | Commit 89e38f55 "treewide: Fix header includes to build with musl" added extra #includes to work with musl. Unfortunately with the cppcheck version I'm using (cppcheck-2.9-1.fc37.x86_64 in Fedora 37) this causes weird false positives: specifically cppcheck seems to hit a #error in <bits/unistd.h> complaining about including it directly instead of via <unistd.h> (which is not something we're doing). I have no idea why that would be happening; but I'm guessing it has to be a bug in the cpp implementation in that cppcheck version. In any case, it's possible to work around this by moving the include of <unistd.h> before the include of <signal.h>. So, do that. Fixes: 89e38f55405d ("treewide: Fix header includes to build with musl") 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-10 09:29:38 +0000