[PATCH v3 00/16] Simplify and correct handling of "spliced" UDP forwarding
The UDP "splicing" (forwarding packets from one L4 socket to another, rather than via the tuntap device) code assumes that any given UDP port in the init namespace will only communicate with a single port on the ns side at a time, and vice versa. This will often be the case, but since UDP is a connectionless protocol, it need not be. In fact it is not the case in our existing UDP bandwidth checks, although the specific configuration there means it's not harmful in that case. The failure mode in this case can be quite bad: we don't just fall back to an unoptimized oath, or drop packets, we will misdirect packets to the wrong destination. This series make some substantial simplifications to how we handle the splice forwarding, then corrects it to handle the case of multiple source ports sending to a single destination. This does come at a performance cost. It's not as large as I feared, and shouldn't affect the most common case where there is a 1 to 1 mapping between source and destination ports. I haven't yet been able to confirm the latter because the iperf3 bandwidth test we use *does* have interleaved streams with a common destination port. Based on the earlier series for dual stack TCP sockets. Changes since v2: * Minor style and comment revisions Changes since v1: * Added patches 12..16/16 fixing the delivery of packets, as well as just simplifying the mechanics David Gibson (16): udp: Also bind() connected ports for "splice" forwarding udp: Separate tracking of inbound and outbound packet flows udp: Always use sendto() rather than send() for forwarding spliced packets udp: Don't connect "forward" sockets for spliced flows udp: Remove the @bound field from union udp_epoll_ref udp: Split splice field in udp_epoll_ref into (mostly) independent bits udp: Don't create double sockets for -U port udp: Re-use fixed bound sockets for packet forwarding when possible udp: Don't explicitly track originating socket for spliced "connections" udp: Update UDP "connection" timestamps in both directions udp: Simplify udp_sock_handler_splice udp: Make UDP_SPLICE_FRAMES and UDP_TAP_FRAMES_MEM the same thing udp: Add helper to extract port from a sockaddr_in or sockaddr_in6 udp: Unify buffers for tap and splice paths udp: Split send half of udp_sock_handler_splice() from the receive half udp: Correct splice forwarding when receiving from multiple sources passt.h | 2 + udp.c | 519 +++++++++++++++++++++++++------------------------------- udp.h | 16 +- 3 files changed, 244 insertions(+), 293 deletions(-) -- 2.38.1
pasta handles "spliced" port forwarding by resending datagrams received on
a bound socket in the init namespace to a connected socket in the guest
namespace. This means there are actually three ports associated with each
"connection". First there's the source and destination ports of the
originating datagram. That's also the destination port of the forwarded
datagram, but the source port of the forwarded datagram is the kernel
allocated bound address of the connected socket.
However, by bind()ing as well as connect()ing the forwarding socket we can
choose the source port of the forwarded datagrams. By choosing it to match
the original source port we remove that surprising third port number and
no longer need to store port numbers in struct udp_splice_port.
As a bonus this means that the recipient of the packets will see the
original source port if they call getpeername(). This rarely matters, but
it can't hurt.
Signed-off-by: David Gibson
Each entry udp_splice_map[v6][N] keeps information about two essentially
unrelated packet flows. @ns_conn_sock, @ns_conn_ts and @init_bound_sock
track a packet flow from port N in the host init namespace to some other
port in the pasta namespace (the one @ns_conn_sock is connected to).
@init_conn_sock, @init_conn_ts and @ns_bound_sock track packet flow from
port N in the pasta namespace to some other port in the host init namespace
(the one @init_conn_sock is connected to).
Split udp_splice_map[][] into two separate tables for the two directions.
Each entry in each table is a 'struct udp_splice_flow' with @orig_sock
(previously the bound socket), @target_sock (previously the connected
socket) and @ts (the timeout for the target socket).
Signed-off-by: David Gibson
udp_sock_handler_splice() has two different ways of sending out packets
once it has determined the correct destination socket. For the originating
sockets (which are not connected) it uses sendto() to specify a specific
address. For the forward socket (which is connected) we use send().
However we know the correct destination address even for the forward socket
we do also know the correct destination address. We can use this to use
sendto() instead of send(), removing the need for two different paths and
some staging data structures.
Signed-off-by: David Gibson
Currently we connect() the socket we use to forward spliced UDP flows.
However, we now only ever use sendto() rather than send() on this socket
so there's not actually any need to connect it. Don't do so.
Rename a number of things that referred to "connect" or "conn" since that
would now be misleading.
Signed-off-by: David Gibson
We set this field, but nothing ever checked it.
Signed-off-by: David Gibson
The @splice field in union udp_epoll_ref can have a number of values for
different types of "spliced" packet flows. Split it into several single
bit fields with more or less independent meanings. The new @splice field
is just a boolean indicating whether the socket is associated with a
spliced flow, making it identical to the @splice fiend in tcp_epoll_ref.
The new bit @orig, indicates whether this is a socket which can originate
new udp packet flows (created with -u or -U) or a socket created on the
fly to handle reply socket. @ns indicates whether the socket lives in the
init namespace or the pasta namespace.
Making these bits more orthogonal to each other will simplify some future
cleanups.
Signed-off-by: David Gibson
For each IP version udp_socket() has 3 possible calls to sock_l4(). One
is for the "non-spliced" bound socket in the init namespace, one for the
"spliced" bound socket in the init namespace and one for the "spliced"
bound socket in the pasta namespace.
However when this is called to create a socket in the pasta namspeace there
is a logic error which causes it to take the path for the init side spliced
socket as well as the ns socket. This essentially tries to create two
identical sockets on the ns side. Unsurprisingly the second bind() call
fails according to strace.
Correct this to only attempt to open one socket within the ns.
Signed-off-by: David Gibson
When we look up udp_splice_to_ns[v6][src].target_sock in
udp_sock_handler_splice, all we really require of the socket is that it
be bound to port src in the pasta guest namespace. Similarly for
udp_splice_to_init but bound in the init namespace.
Usually these sockets are created temporarily by udp_splice_connect() and
cleaned up by udp_timer(). However, depending on the -u and -U options its
possible we have a permanent socket bound to the relevant port created by
udp_sock_init(). If such a socket exists, we could use it instead of
creating a temporary one. In fact we *must* use it, because we'll fail
trying to bind() a temporary one to the same port.
So allow this, store permanently bound sockets into udp_splice_to_{ns,init}
in udp_sock_init(). These won't get incorrectly removed by the timer
because we don't put a corresponding entry in the udp_act[] structure
which directs the timer what to clean up.
Signed-off-by: David Gibson
When we look up udp_splice_to_ns[][].orig_sock in udp_sock_handler_splice()
we're finding the socket on which the originating packet for the
"connection" was received on. However, we don't specifically need this
socket to be the originating one - we just need one that's bound to the
the source port of this reply packet in the init namespace. We can look
this up in udp_splice_to_init[v6][src].target_sock, whose defining
characteristic is exactly that. The same applies with init and ns swapped.
In practice, of course, the port we locate this way will always be the
originating port, since we couldn't have started this "connection" if it
wasn't.
Change this, and we no longer need the @orig_sock field at all. That
leaves just @target_sock which we rename to simply @sock. The whole
udp_splice_flow structure now more represents a single bound port than
a "flow" per se, so rename and recomment it accordingly. Likewise the
udp_splice_to_{ns,init} names are now misleading, since the ports in
those maps are used in both directions. Rename them to
udp_splice_{ns,init} indicating the location where the described
socket is bound.
Signed-off-by: David Gibson
A UDP pseudo-connection between port A in the init namespace and port B in
the pasta guest namespace involves two sockets: udp_splice_init[v6][B]
and udp_splice_ns[v6][A]. The socket which originated this "connection"
will be permanent but the other one will be closed on a timeout.
When we get a packet from the originating socket, we update the timeout on
the other socket, but we don't do the same when we get a reply packet from
the other socket. However any activity on the "connection" probably
indicates that it's still in use. Without this we could incorrectly time
out a "connection" if it's using a protocol which involves a single
initiating packet, but which then gets continuing replies from the target.
Correct this by updating the timeout on both sockets for a packet in either
direction. This also updates the timestamps for the permanent originating
sockets which is unnecessary, but harmless.
Signed-off-by: David Gibson
Previous cleanups mean that we can now rework some complex ifs in
udp_sock_handler_splice() into a simpler set.
Signed-off-by: David Gibson
These two constants have the same value, and there's not a lot of reason
they'd ever need to be different. Future changes will further integrate
the spliced and "tap" paths so that these need to be the same. So, merge
them into UDP_MAX_FRAMES.
Signed-off-by: David Gibson
udp_sock_handler_splice() has a somewhat clunky if to extract the port from
a socket address which could be either IPv4 or IPv6. Future changes are
going to make this even more clunky, so introduce a helper function to
do this extraction.
Signed-off-by: David Gibson
We maintain a set of buffers for UDP packets to be forwarded via the tap
interface in udp[46]_l2_buf. We then have a separate set of buffers for
packets to be "spliced" in udp_splice_buf[]. However, we only use one of
these at a time, so we can share the buffer space.
For the receiving splice packets we can not only re-use the data buffers
but also the udp[46]_l2_iov_sock and udp[46]_l2_mh_sock control structures.
For sending the splice packets we keep the same data buffers, but we need
specific control structures. We create udp[46]_iov_splice - we can't
reuse udp_l2_iov_sock[] because we need to write iov_len as we're writing
spliced packets, but the tap path expects iov_len to remain the same (it
only uses it for receive). Likewise we create udp[46]_mh_splice with the
mmsghdr structures for sending spliced packets. As well as needing to
reference different iovs, these need to all reference udp_splice_namebuf
instead of individual msg_name fields for each slot.
Signed-off-by: David Gibson
Move the part of udp_sock_handler_splice() concerned with sending out the
datagrams into a new udp_splice_sendfrom() helper. This will make later
cleanups easier.
Signed-off-by: David Gibson
udp_sock_handler_splice() reads a whole batch of datagrams at once with
recvmmsg(). It then forwards them all via a single socket on the other
side, based on the source port.
However, it's entirely possible that the datagrams in the set have
different source ports, and thus ought to be forwarded via different
sockets on the destination side. In fact this situation arises with the
iperf -P4 throughput tests in our own test suite. AFAICT we only get away
with this because iperf3 is strictly one way and doesn't send reply packets
which would be misdirected because of the incorrect source ports.
Alter udp_sock_handler_splice() to split the packets it receives into
batches with the same source address and send each batch with a separate
sendmmsg().
For now we only look for already contiguous batches, which means that if
there are multiple active flows interleaved this is likely to degenerate
to batches of size 1. For now this is the simplest way to correct the
behaviour and we can try to optimize later.
Signed-off-by: David Gibson
On Fri, Nov 25, 2022 at 06:29:16PM +1100, David Gibson wrote:
udp_sock_handler_splice() reads a whole batch of datagrams at once with recvmmsg(). It then forwards them all via a single socket on the other side, based on the source port.
However, it's entirely possible that the datagrams in the set have different source ports, and thus ought to be forwarded via different sockets on the destination side. In fact this situation arises with the iperf -P4 throughput tests in our own test suite. AFAICT we only get away with this because iperf3 is strictly one way and doesn't send reply packets which would be misdirected because of the incorrect source ports.
Alter udp_sock_handler_splice() to split the packets it receives into batches with the same source address and send each batch with a separate sendmmsg().
For now we only look for already contiguous batches, which means that if there are multiple active flows interleaved this is likely to degenerate to batches of size 1. For now this is the simplest way to correct the behaviour and we can try to optimize later.
Signed-off-by: David Gibson
Oops, replied to the wrong version. v3 still has the nasty bug, do not apply until I send a v4.
--- udp.c | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-)
diff --git a/udp.c b/udp.c index 899dcbb..eaf7a52 100644 --- a/udp.c +++ b/udp.c @@ -571,9 +571,9 @@ static void udp_splice_sendfrom(const struct ctx *c, struct mmsghdr *mmh, int n, static void udp_sock_handler_splice(const struct ctx *c, union epoll_ref ref, uint32_t events, const struct timespec *now) { - in_port_t src, dst = ref.r.p.udp.udp.port; + in_port_t dst = ref.r.p.udp.udp.port; + int v6 = ref.r.p.udp.udp.v6, n, i, m; struct mmsghdr *mmh_recv, *mmh_send; - int v6 = ref.r.p.udp.udp.v6, n, i;
if (!(events & EPOLLIN)) return; @@ -609,12 +609,23 @@ static void udp_sock_handler_splice(const struct ctx *c, union epoll_ref ref, }); }
- for (i = 0; i < n; i++) - mmh_send[i].msg_hdr.msg_iov->iov_len = mmh_recv[i].msg_len; - - src = sa_port(v6, mmh_recv[0].msg_hdr.msg_name); - udp_splice_sendfrom(c, mmh_send, n, src, ref.r.p.udp.udp.port, - v6, ref.r.p.udp.udp.ns, ref.r.p.udp.udp.orig, now); + for (i = 0; i < n; i += m) { + const struct mmsghdr *mmh = &mmh_recv[i]; + in_port_t src = sa_port(v6, mmh->msg_hdr.msg_name); + + m = 0; + do { + mmh_send[i + m].msg_hdr.msg_iov->iov_len = mmh->msg_len; + mmh++; + m++; + } while (sa_port(v6, mmh->msg_hdr.msg_name) == src);
Buffer overflow here, because I didn't check i + m against n. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
participants (1)
-
David Gibson