aboutgitcodebugslistschat
path: root/tap.c
Commit message (Collapse)AuthorAgeFilesLines
* iov: Add helper to find skip over first n bytes of an io vectorDavid Gibson2024-02-291-7/+5
| | | | | | | | | Several of the IOV functions in iov.c, and also tap_send_frames_passt() needs to determine which buffer element a byte offset into an IO vector lies in. Split this out into a helper function iov_skip_bytes(). Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* treewide: Make a bunch of pointer variables pointers to constDavid Gibson2024-01-161-6/+6
| | | | | | | | | | Sufficiently recent cppcheck (I'm using 2.13.0) seems to have added another warning for pointer variables which could be pointer to const but aren't. Use this to make a bunch of variables const pointers where they previously weren't for no particular reason. Signed-off-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-6/+6
| | | | | | | | 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>
* tap, pasta: Handle short writes to /dev/tap2023_11_10.5ec3634David Gibson2023-11-101-1/+7
| | | | | | | | | | | | | | | tap_send_frames_pasta() sends frames to the namespace by sending them to our the /dev/tap device. If that write() returns an error, we already handle it. However we don't handle the case where the write() returns short, meaning we haven't successfully transmitted the whole frame. I don't know if this can ever happen with the kernel tap device, but we should at least report the case so we don't get a cryptic failure. For the purposes of the return value for tap_send_frames_pasta() we treat this case as though it was an error (on the grounds that a partial frame is no use to the namespace). Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
* tap, pasta: Handle incomplete tap sends for pasta tooDavid Gibson2023-11-101-3/+1
| | | | | | | | | | | | | | | | | | | | Since a469fc39 ("tcp, tap: Don't increase tap-side sequence counter for dropped frames") we've handled more gracefully the case where we get data from the socket side, but are temporarily unable to send it all to the tap side (e.g. due to full buffers). That code relies on tap_send_frames() returning the number of frames it successfully sent, which in turn gets it from tap_send_frames_passt() or tap_send_frames_pasta(). While tap_send_frames_passt() has returned that information since b62ed9ca ("tap: Don't pcap frames that didn't get sent"), tap_send_frames_pasta() always returns as though it succesfully sent every frame. However there certainly are cases where it will return early without sending all frames. Update it report that properly, so that the calling functions can handle it properly. Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
* pif: Pass originating pif to tap handler functionsDavid Gibson2023-11-071-10/+16
| | | | | | | | | | | 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>
* tcp, tap: Don't increase tap-side sequence counter for dropped framesStefano Brivio2023-10-041-3/+7
| | | | | | | | | | | | | | | | | | | | | | | | | | ...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>
* cppcheck: Make many pointers constDavid Gibson2023-10-041-7/+7
| | | | | | | | | 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>
* udp, tap: Correctly advance through packets in udp_tap_handler()David Gibson2023-09-081-12/+8
| | | | | | | | | | | | | | | | | | 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 DUP sequence calling udp_tap_handler(). Or so it appears. In fact, udp_tap_handler() doesn't take an index and always starts with packet 0 of the sequence, even if called repeatedly. 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. Correct this by adding an index parameter to udp_tap_handler() and altering the loops in tap.c to step through the pool properly. 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-8/+17
| | | | | | | | | | | | | | | | | | | | | | | | | 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>
* tap: fix uses of l3_len in tap4_handler()Stas Sergeev2023-09-071-2/+2
| | | | | | | | | | | | | | | | | l3_len was calculated from the ethernet frame size, and it was assumed to be equal to the length stored in an IP packet. But if the ethernet frame is padded, then l3_len calculated that way can only be used as a bound check to validate the length stored in an IP header. It should not be used for calculating the l4_len. This patch makes sure the small padded ethernet frames are properly processed, by trusting the length stored in an IP header. Link: https://bugs.passt.top/show_bug.cgi?id=73 Signed-off-by: Stas Sergeev <stsp2@yandex.ru> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp, udp: Don't pre-fill IPv4 destination address in headersDavid Gibson2023-08-221-5/+3
| | | | | | | | | | | | | | | | | 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>
* tap: Pass source address to protocol handler functionsDavid Gibson2023-08-221-8/+11
| | | | | | | | | | | 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>
* tap: Don't clobber source address in tap6_handler()David Gibson2023-08-221-2/+0
| | | | | | | | | | | | | | In tap6_handler() saddr is initialized to the IPv6 source address from the incoming packet. However part way through, but before organizing the packet into a "sequence" we set it unconditionally to the guest's assigned address. We don't do anything equivalent for IPv4. This doesn't make a lot of sense: if the guest is using a different source address it makes sense to consider these different sequences of packets and we shouldn't try to combine them together. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Fix format specifier in tap4_is_fragment() warningStefano Brivio2023-08-161-1/+2
| | | | | | | | Spotted by Coverity, relatively harmless. Fixes: e01759e2fab0 ("tap: Explicitly drop IPv4 fragments, and give a warning") Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* epoll: Use different epoll types for passt and pasta tap fdsDavid Gibson2023-08-131-24/+15
| | | | | | | | | | | Currently we have a single epoll event type for the "tap" fd, which could be either a handle on a /dev/net/tun device (pasta) or a connected Unix socket (passt). However for the two modes we call different handler functions. Simplify this a little by using different epoll types and dispatching directly to the correct handler function. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* epoll: Split listening Unix domain socket into its own typeDavid Gibson2023-08-131-11/+4
| | | | | | | | | | | | | tap_handler() actually handles events on three different types of object: the /dev/tap character device (pasta), a connected Unix domain socket (passt) or a listening Unix domain socket (passt). The last, in particular, really has no handling in common with the others, so split it into its own epoll type and directly dispatch to the relevant handler from the top level. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* epoll: Always use epoll_ref for the epoll data variableDavid Gibson2023-08-131-4/+12
| | | | | | | | | | | | | | | | | | | | | | epoll_ref contains a variety of information useful when handling epoll events on our sockets, and we place it in the epoll_event data field returned by epoll. However, for a few other things we use the 'fd' field in the standard union of types for that data field. This actually introduces a bug which is vanishingly unlikely to hit in practice, but very nasty if it ever did: theoretically if we had a very large file descriptor number for fd_tap or fd_tap_listen it could overflow into bits that overlap with the 'proto' field in epoll_ref. With some very bad luck this could mean that we mistakenly think an event on a regular socket is an event on fd_tap or fd_tap_listen. More practically, using different (but overlapping) fields of the epoll_data means we can't unify dispatch for the various different objects in the epoll. Therefore use the same epoll_ref as the data for the tap fds and the netns quit fd, adding new fd type values to describe them. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Fold reset handling into tap_handler_passt()David Gibson2023-08-131-36/+32
| | | | | | | | | | | We call tap_sock_reset() if tap_handler_passt() fails, or if we get an error event on the socket. Fold that logic into tap_handler() passt itself which simplifies the caller. It also makes it clearer that we had a redundant EPOLL_CTL_DEL and close() in one of the reset paths, so fix that too. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Fold reset handling into tap_handler_pasta()David Gibson2023-08-131-15/+15
| | | | | | | | | If tap_handler_pasta() fails, we reset the connection. But in the case of pasta the "reset" is just a fatal error. Fold the die() calls directly into tap_handler_pasta() for simplicity. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Clean up behaviour for errors on listening Unix socketDavid Gibson2023-08-131-4/+8
| | | | | | | | | | | | | | | | | | | | | We call tap_sock_unix_new() to handle a new connection to the qemu socket if we get an EPOLLIN event on c->fd_tap_listen. If we get any other event on the fd, we'll fall through to the "tap reset" path. But that won't do anything relevant to the listening socket, it will just close the already connected socket. Furthermore, the only other event we're subscribed to for the listening socket is EPOLLRDHUP, which doesn't apply to a non connected socket. Remove EPOLLRDHUP from the subscribed events. We don't need to explicitly add EPOLLERR, because errors are always reported. There's no obvious case that would cause an error on a listening socket anyway, and it's not obvious how we'd recover, treat it as a fatal error if it ever does happen. Finally, fold all this handling into the tap_sock_unix_new() function, there's no real reason to split it between there and tap_handler(). Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Clean up tap reset pathDavid Gibson2023-08-131-23/+29
| | | | | | | | | | | | | | | | | | | | | | In tap_handler() if we get an error on the tap device or socket, we use tap_sock_init() to re-initialise it. However, what we actually need for this reset case has remarkably little in common with the case where we're initialising for the first time: * Re-initialising the packet pools is unnecessary * The case of a passed in fd (--fd) isn't relevant * We don't even call this for pasta mode * We will never re-call tap_sock_unix_init() because we never clear fd_tap_listen In fact the only thing we do in tap_sock_init() relevant to the reset case is to remove the fd from the epoll and close it... which isn't used in the first initialisation case. So make a new tap_sock_reset() function just for this case, and simplify tap_sock_init() slightly as being used only for the first time case. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: fix seq->p.count limitLaurent Vivier2023-08-131-6/+6
| | | | | | | | | | | | | The number of items in pool_l4_t is defined to UIO_MAXIOV, not TAP_SEQS. TAP_SEQS is the number of the sequences. Fix the value used to compare seq->p.count with. Fixes: 37c228ada88b ("tap, tcp, udp, icmp: Cut down on some oversized buffers") Signed-off-by: Laurent Vivier <lvivier@redhat.com> [sbrivio: s/messages/sequences/ in commit message, extend initialisation of packets in pool to UIO_MAXIOV items] Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Remove unnecessary global tun_ns_fdDavid Gibson2023-08-041-7/+3
| | | | | | | | | | | | | | tap_ns_tun(), which runs in an ephemeral thread puts the fd it opens into the global variable tun_ns_fd to communicate it back to the main thread in tap_sock_tun_init(). However, the only thing tap_sock_tun_init() does with it is copies it to c->fd_tap and everything else uses it from there. tap_ns_tun() already has access to the context structure, so we might as well store the value directly in there rather than having a global as an intermediate. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: More detailed error reporting in tap_ns_tun()David Gibson2023-08-041-9/+16
| | | | | | | | | | | | | | | | There are several possible failure points in tap_ns_tun(), but if anything goes wrong, we just set tun_ns_fd to -1 resulting in the same error message. Add more detailed error reporting to the various failure points. At the same time, we know this is only called from tap_sock_tun_init() which will terminate pasta if we fail, so we can simplify things a little because we don't need to close() the fd on the failure paths. Link: https://bugs.passt.top/show_bug.cgi?id=69 Link: https://github.com/containers/podman/issues/19428 Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* util: Make ns_enter() a void function and report setns() errorsDavid Gibson2023-08-041-2/+2
| | | | | | | | | | | | | | | | ns_enter() returns an integer... but it's always zero. If we actually fail the function doesn't return. Therefore it makes more sense for this to be a function returning void, and we can remove the cases where we pointlessly checked its return value. In addition ns_enter() is usually called from an ephemeral thread created by NS_CALL(). That means that the exit(EXIT_FAILURE) there usually won't be reported (since NS_CALL() doesn't wait() for the thread). So, use die() instead to print out some information in the unlikely event that our setns() here does fail. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Explicitly drop IPv4 fragments, and give a warningDavid Gibson2023-07-071-0/+31
| | | | | | | | | | | | | | | | We don't handle defragmentation of IP packets coming from the tap side, and we're unlikely to any time soon (with our large MTU, it's not useful for practical use cases). Currently, however, we simply ignore the fragmentation flags and treat fragments as though they were whole IP packets. This isn't ideal and can lead to rather cryptic behaviour if we do receive IP fragments. Change the code to explicitly drop fragmented packets, and print a rate limited warning if we do encounter them. Link: https://bugs.passt.top/show_bug.cgi?id=62 Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: With pasta, don't reset on tap errors, handle write failuresStefano Brivio2023-06-231-5/+19
| | | | | | | | | | | | | | | | | | | | | | | | | | | Since commit 0515adceaa8f ("passt, pasta: Namespace-based sandboxing, defer seccomp policy application"), it makes no sense to close and reopen the tap device on error: we don't have access to /dev/net/tun after the initial setup phase. If we hit ENOBUFS while writing (as reported: in one case because the kernel actually ran out of memory, with another case under investigation), or ENOSPC, we're supposed to drop whatever data we were trying to send: there's no room for it. Handle EINTR just like we handled EAGAIN/EWOULDBLOCK: there's no particular reason why sending the same data should fail again. Anything else I can think of would be an unrecoverable error: exit with failure then. While at it, drop a useless cast on the write() call: it takes a const void * anyway. Reported-by: Gianluca Stivan <me@yawnt.com> Reported-by: Chris Kuhn <kuhnchris@kuhnchris.eu> Fixes: 0515adceaa8f ("passt, pasta: Namespace-based sandboxing, defer seccomp policy application") Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* tap: Don't update ip6.addr_seen to ::David Gibson2023-05-171-1/+1
| | | | | | | | | | | | | | | | | | | | | When we receive packets from the tap side, we update the addr_seen fields to reflect the last known address of the guest or ns. For ip4.addr_seen we, sensibly, only update if the address we've just seen isn't 0 (0.0.0.0). This case can occur during early DHCP transactions. We have no equivalent case for IPv6. We're less likely to hit this, because DHCPv6 uses link-local addresses, however we can see an source address of :: with certain multicast operations. This can bite us if we try to make an incoming connection very early after starting pasta with --config-net: we may have only seen some of those multicast packets, updated addr_seen to :: and not had any "real" packets to update it to a global address. I've seen this with some of the avocado test conversions. In any case, it can never make sense to update addr_seen to ::, so explicitly exclude that case. 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>
* 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>
* treewide: Fix header includes to build with muslChris Kuhn2023-03-091-0/+1
| | | | | | | | | | | | | | | Roughly inspired from a patch by Chris Kuhn: fix up includes so that we can build against musl: glibc is more lenient as headers generally include a larger amount of other headers. Compared to the original patch, I only included what was needed directly in C files, instead of adding blanket includes in local header files. It's a bit more involved, but more consistent with the current (not ideal) situation. Reported-by: Chris Kuhn <kuhnchris+github@kuhnchris.eu> Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* log, conf, tap: Define die() as err() plus exit(), drop cppcheck workaroundsStefano Brivio2023-02-271-5/+1
| | | | | | | | | | | If we define die() as a variadic macro, passing __VA_ARGS__ to err(), and calling exit() outside err() itself, we can drop the workarounds introduced in commit 36f0199f6ef4 ("conf, tap: Silence two false positive invalidFunctionArg from cppcheck"). Suggested-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>
* conf, tap: Silence two false positive invalidFunctionArg from cppcheckStefano Brivio2023-02-161-1/+5
| | | | | | | | The newly introduced die() calls exit(), but cppcheck doesn't see it and warns about possibly invalid arguments used after the check which triggers die(). Add return statements to silence the warnings. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Eliminate goto from tap_handler()David Gibson2023-02-161-12/+7
| | | | | | | | The goto here really doesn't improve clarity or brevity at all. Use a clearer construct. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Don't pcap frames that didn't get sentDavid Gibson2023-02-161-10/+20
| | | | | | | | | | | | | | In tap_send_frames() we send a number of frames to the tap device, then also write them to the pcap capture file (if configured). However the tap send can partially fail (short write()s or similar), meaning that some of the requested frames weren't actually sent, but we still write those frames to the capture file. We do give a debug message in this case, but it's misleading to add frames that we know weren't sent to the capture file. Rework to avoid this. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Use single counter for iov elements in tap_send_frames_pasta()Stefano Brivio2023-02-161-3/+3
| | | | | | | | | | | | | David points out that using multiple counters to go over the iov array, namely 'i' and 'iov', makes mistakes easier. We can't just use 'iov', unless we reserve an element with zero iov_len at the end, which isn't really justified. Simply use 'i' to iterate over the array. Link: https://archives.passt.top/passt-dev/Y+mfenvLn3VJ7Dg5@yekko/ Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* convert all remaining err() followed by exit() to die()Laine Stump2023-02-161-20/+10
| | | | | | | | This actually leaves us with 0 uses of err(), but someone could want to use it in the future, so we may as well leave it around. Signed-off-by: Laine Stump <laine@redhat.com> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Send frames after the first one in tap_send_frames_pasta()Stefano Brivio2023-02-121-1/+2
| | | | | | | ...instead of repeatedly sending out the first one in iov. Fixes: e21ee41ac35a ("tcp: Combine two parts of pasta tap send path together") Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Improve handling of partial frame sendsDavid Gibson2023-01-231-13/+36
| | | | | | | | | | | | | | | | | | | In passt mode, when writing frames to the qemu socket, we might get a short send. If we ignored this and carried on, the qemu socket would get out of sync, because the bytes we actually sent wouldn't correspond to the length header we already sent. tap_send_frames_passt() handles that by doing a a blocking send to complete the message, but it has a few flaws: * We only attempt to resend once: although it's unlikely in practice, nothing prevents the blocking send() from also being short * We print a debug error if send() returns non-zero.. but send() returns the number of bytes sent, so we actually want it to return the length of the remaining data. Correct those flaws and also be a bit more thorough about reporting problems here. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Use different io vector bases depending on tap typeDavid Gibson2023-01-231-3/+2
| | | | | | | | | | | | | Currently tap_send_frames() expects the frames it is given to include the vnet_len field, even in pasta mode which doesn't use it (although it need not be initialized in that case). To match, tap_iov_base() and tap_iov_len() construct the frame in that way. This will inconvenience future changes, so alter things to set the buffers to include just the frame needed by the tap backend type. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Add "tap headers" abstractionDavid Gibson2023-01-231-0/+15
| | | | | | | | | | | | | | | | Currently both the TCP and UDP code need to deal in various places with the details of the L2 headers, and also the tap-specific "vnet_len" header. This makes abstracting the tap interface to new backends (e.g. vhost-user or tun) more difficult. To improve this abstraction, create a new 'tap_hdr' structure which represents both L2 (always Ethernet at the moment, but might be vary in future) and any additional tap specific headers (such as the qemu socket's vnet_len field). Provide helper functions and macros to initialize, update and use it. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap, tcp: Move tap send path to tap.cDavid Gibson2023-01-231-0/+84
| | | | | | | | | | The functions which do the final steps of sending TCP packets on through the tap interface - tcp_l2_buf_flush*() - no longer have anything that's actually specific to TCP in them, other than comments and names. Move them all to tap.c. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* passt, tap: Process data on the socket before HUP/ERR eventsRichard W.M. Jones2022-11-251-3/+3
| | | | | | | | | | | | | | | In the case where the client writes a packet and then closes the socket, because we receive EPOLLIN|EPOLLRDHUP together we have a choice of whether to close the socket immediately, or read the packet and then close the socket. Choose the latter. This should improve fuzzing coverage and arguably is a better choice even for regular use since dropping packets on close is bad. See-also: https://archives.passt.top/passt-dev/20221117171805.3746f53a@elisabeth/ Signed-off-by: Richard W.M. Jones <rjones@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* passt, tap: Add --fd optionRichard W.M. Jones2022-11-251-0/+9
| | | | | | | | | This passes a fully connected stream socket to passt. Signed-off-by: Richard W.M. Jones <rjones@redhat.com> [sbrivio: reuse fd_tap instead of adding a new descriptor, imply --one-off on --fd, add to optstring and usage()] Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* style: Minor corrections to function commentsDavid Gibson2022-11-251-3/+3
| | | | | | | Some style issues and a typo. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Revert recently added checks in tap_handler_passt()Stefano Brivio2022-11-161-16/+4
| | | | | | | | | | | | | | | This reverts commit 198f87835dc4 ("tap: Return -EIO from tap_handler_passt() on inconsistent packet stream") and commit 510dace86ccf ("tap: Keep stream consistent if qemu length descriptor spans two recv() calls"). I can hit occasional failures in perf/passt_tcp tests where we seem to be getting excess data at the end of a recv(), and for some reason I couldn't figure out yet, if we just ignore it, subsequent recv() calls from qemu return correct data. If we close the connection, qemu can't talk to us anymore, of course. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* arp, tap, util: Don't use perror() after seccomp filter is installedStefano Brivio2022-11-161-3/+3
| | | | | | | | | | | | | | | If stderr is closed, after we fork to background, glibc's implementation of perror() will try to re-open it by calling dup(), upon which the seccomp filter causes the process to terminate, because dup() is not included in the list of allowed syscalls. Replace perror() calls that might happen after isolation_postfork(). We could probably replace all of them, but early ones need a bit more attention as we have to check whether log.c functions work in early stages. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* tap: Return -EIO from tap_handler_passt() on inconsistent packet streamStefano Brivio2022-11-101-2/+2
| | | | | | | | While it's important to fail in that case, it makes little sense to fail quietly: it's better to tell qemu explicitly that something went wrong and that we won't recover, by closing the socket. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Keep stream consistent if qemu length descriptor spans two recv() callsStefano Brivio2022-11-101-3/+15
| | | | | | | | | | | | | | | | | | | | | | | | | I got all paranoid after triggering a divide-by-zero general protection fault in passt with a qemu version without the virtio_net TX hang fix, while flooding UDP. I start thinking this was actually coming from some random changes I was playing with, but before reaching this conclusion I reviewed once more the relatively short path in tap_handler_passt() before we start using packet_*() functions, and found this. Never observed in practice, but artificially reproduced with changes in qemu's socket interface: if we don't receive from qemu a complete length descriptor in one recv() call, or if we receive a partial one at the end of one call, we currently disregard the rest, which would make the stream inconsistent. Nothing really bad happens, except that from that point on we would disregard all the packets we get until, if ever, we get the stream back in sync by chance. Force reading a complete packet length descriptor with a blocking recv(), if needed -- not just a complete packet later. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
generated by cgit v1.2.3 (git 2.25.1) at 2024-11-16 07:35:35 +0000