aboutgitcodebugslistschat
Commit message (Collapse)AuthorAgeFilesLines
...
* tcp: Update tap specific header too in tcp_fill_headers[46]()David Gibson2024-05-021-14/+14
| | | | | | | | | | | | | | tcp_fill_headers[46]() fill most of the headers, but the tap specific header (the frame length for qemu sockets) is filled in afterwards. Filling this as well: * Removes a little redundancy between the tcp_send_flag() and tcp_data_to_tap() path * Makes calculation of the correct length a little easier * Removes the now misleadingly named 'vnet_len' variable in tcp_send_flag() Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* iov: Helper macro to construct iovs covering existing variables or fieldsDavid Gibson2024-05-024-21/+16
| | | | | | | | | | | Laurent's recent changes mean we use IO vectors much more heavily in the TCP code. In many of those cases, and few others around the code base, individual iovs of these vectors are constructed to exactly cover existing variables or fields. We can make initializing such iovs shorter and clearer with a macro for the purpose. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap, tcp: (Re-)abstract TAP specific header handlingDavid Gibson2024-05-022-25/+42
| | | | | | | | | | | | | | | Recent changes to the TCP code (reworking of the buffer handling) have meant that it now (again) deals explicitly with the MODE_PASST specific vnet_len field, instead of using the (partial) abstractions provided by the tap layer. The abstractions we had don't work for the new TCP structure, so make some new ones that do: tap_hdr_iov() which constructs an iovec suitable for containing (just) the TAP specific header and tap_hdr_update() which updates it as necessary per-packet. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Simplify packet length calculation when preparing headersDavid Gibson2024-05-021-16/+10
| | | | | | | | | tcp_fill_headers[46]() compute the L3 packet length from the L4 packet length, then their caller tcp_l2_buf_fill_headers() converts it back to the L4 packet length. We can just use the L4 length throughout. Signed-off-by: David Gibson <david@gibson.dropbear.id.au>eewwee Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* treewide: Standardise variable names for various packet lengthsDavid Gibson2024-05-0212-193/+194
| | | | | | | | | | | | | | | | | | At various points we need to track the lengths of a packet including or excluding various different sets of headers. We don't always use the same variable names for doing so. Worse in some places we use the same name for different things: e.g. tcp_fill_headers[46]() use ip_len for the length including the IP headers, but then tcp_send_flag() which calls it uses it to mean the IP payload length only. To improve clarity, standardise on these names: dlen: L4 protocol payload length ("data length") l4len: plen + length of L4 protocol header l3len: l4len + length of IPv4/IPv6 header l2len: l3len + length of L2 (ethernet) header Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* checksum: Make csum_ip4_header() take a host endian lengthDavid Gibson2024-05-024-6/+8
| | | | | | | | | | | | | | csum_ip4_header() takes the packet length as a network endian value. In general it's very error-prone to pass non-native-endian values as a raw integer. It's particularly bad here because this differs from other checksum functions (e.g. proto_ipv4_header_psum()) which take host native lengths. It turns out all the callers have easy access to the native endian value, so switch it to use host order like everything else. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* treewide: Remove misleading and redundant endianness notesDavid Gibson2024-05-023-16/+16
| | | | | | | | | | | | | | | In general, it's much less error-prone to have the endianness of values implied by the type, rather than just noting it in comments. We can't always easily avoid it, because C, but we can do so when possible. struct in_addr and in6_addr are always encoded network endian, so noting it explicitly isn't useful. Remove them. In some cases we also have endianness notes on uint8_t parameters, which doesn't make sense: for a single byte endianness is irrelevant. Remove those too. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Remove unused structs tap_msg, tap_l4_msgDavid Gibson2024-05-021-20/+0
| | | | | | | | | Use of these structures was removed in bb708111833e ("treewide: Packet abstraction with mandatory boundary checks"). Remove the stale declarations. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Split tap specific and L2 (ethernet) headersDavid Gibson2024-05-022-21/+23
| | | | | | | | | | | | | | In some places (well, actually only UDP now) we use struct tap_hdr to represent both tap backend specific and L2 ethernet headers. Handling these together seemed like a good idea at the time, but Laurent's changes in the TCP code working towards vhost-user support suggest that treating them separately is more useful, more often. Alter struct tap_hdr to represent only the TAP backend specific headers. Updated related helpers and the UDP code to match. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* checksum: Use proto_ipv6_header_psum() for ICMPv6 as wellDavid Gibson2024-05-021-4/+2
| | | | | | | | | | | 7df624e79 ("checksum: introduce functions to compute the header part checksum for TCP/UDP") introduced a helper to compute the partial checksum for the IPv6 pseudo-header used in L4 protocol checksums. It used it in csum_udp6() for UDP packets, but not in csum_icmp6() for the identical calculation in csum_icmp6() for ICMPv6 packets. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* netlink: Fix iterations over nexthop objectsStefano Brivio2024-05-021-3/+9
| | | | | | | | | | | | | | | | | | | | Somewhat confusingly, RTNH_NEXT(), as defined by <linux/rtnetlink.h>, doesn't take an attribute length parameter like RTA_NEXT() does, and I just modelled loops over nexthops after RTA loops, forgetting to decrease the remaining length we pass to RTNH_OK(). In practice, this didn't cause issue in any of the combinations I checked, at least without the next patch. We seem to be the only user of RTNH_OK(): even iproute2 has an open-coded version of it in print_rta_multipath() (ip/iproute.c). Introduce RTNH_NEXT_AND_DEC(), similar to RTA_NEXT(), and use it. Fixes: 6c7623d07bbd ("netlink: Add support to fetch default gateway from multipath routes") Fixes: f4e38b5cd232 ("netlink: Adjust interface index inside copied nexthop objects too") Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* netlink: Use IFA_F_NODAD also while duplicating addresses from the host2024_04_26.d03c4e2Stefano Brivio2024-04-261-0/+6
| | | | | | | | | | | | | | | | | | | ...not just for a single set address (legacy operation with --no-copy-addrs). I forgot to add this to nl_addr_dup(). Note that we can have two version of flags: the 8-bit ifa_flags in ifaddrmsg, and the newer 32-bit version as IFA_FLAGS attribute, which is given priority if present. Make sure IFA_F_NODAD is set in both. Without this, a Podman user reports, something on the lines of: pasta --config-net -- ping -c1 -6 passt.top would fail as the kernel would start Duplicate Address Detection once we configure the address, which can't really work (and doesn't make sense) in this case. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* netlink: For IPv4, IFA_LOCAL is the interface address, not IFA_ADDRESSStefano Brivio2024-04-261-1/+2
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | See the comment to the unnamed enum in linux/if_addr.h, which currently states: /* * Important comment: * IFA_ADDRESS is prefix address, rather than local interface address. * It makes no difference for normally configured broadcast interfaces, * but for point-to-point IFA_ADDRESS is DESTINATION address, * local address is supplied in IFA_LOCAL attribute. * * [...] */ if we fetch IFA_ADDRESS, and we have a point-to-point link with a peer address configured, we'll source the peer address as "our" address, and refuse to resolve it in arp(). This was reported with pasta and a tun upstream interface configured by OpenVPN in "p2p" topology: the target namespace will have similar addresses and routes as the host, which is fine, and will try to resolve the point-to-point peer address (because it's the default gateway). Given that we configure it as our address (only internally, not visibly in the namespace), we'll fail to resolve that and traffic doesn't go anywhere. Note that this is not the case for IPv6: there, IFA_ADDRESS is the actual, local address of the interface, and IFA_LOCAL is not necessarily present, so the comment in linux/if_addr.h doesn't apply either. Link: https://github.com/containers/podman/issues/22320 Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* test: Make log truncation test more robustDavid Gibson2024-04-251-2/+3
| | | | | | | | | | | | | | | | | | | | | | | test/pasta_options/log_to_file checks that pasta truncates its log file when started. It does that by starting pasta with a log file once, then starting it again and checking that after the second round, the log file has only one line: the startup banner from the second invocation. However, this test will break if the second invocation logs any additional messages at startup. This can easily happen on a host with multiple network interfaces due to the "Multiple default route" informational messages added in 639fdf06e ("netlink: Fix selection of template interface"). I believe it could also happen on a host without IPv6 connectivity due to the "Couldn't pick external interface" messages, though I haven't confirmed this. Make the log file test more robust, by not testing for a single line, but instead explicitly testing for the PID of the second pasta invocation in the banner line. Link: https://bugs.passt.top/show_bug.cgi?id=88 Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* test: Slight simplification to pasta log testsDavid Gibson2024-04-251-6/+2
| | | | | | | | | | | | | test/pasta_options/log_to_file contains a couple of rudimentary tests where we start pasta with an interactive shell, then immediately exit it. We can achieve the same thing by using /bin/true as the command to pasta. This also means that waiting for pasta to start, waiting for the executed command to complete and for pasta to clean up are all handled by simply waiting for pasta to complete in the foreground, so there's no need for an additional sleep. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* udp: Correctly look up outbound socket with port remappingsDavid Gibson2024-04-251-2/+3
| | | | | | | | | | | | | | | | | | | | | | | Commit bb9bf0bb ("tcp, udp: Don't precompute port remappings in epoll references") changed the epoll reference for UDP sockets to include the bound port as seen by the socket itself, rather than the bound port it would be translated to on the guest side. As a side effect, it also means that udp_tap_map[] is indexed by the bound port on the host side, rather than on the guest side. This is consistent and a good idea, however we forgot to account for it when finding the correct outgoing socket for packets originating in the guest. This means that if forwarding UDP inbound with a port number change, reply packets would be misdirected. Fix this by applying the reverse mapping before looking up the socket in udp_tap_handler(). While we're at it, use 'port' directly instead of 'uref.port' in udp_sock_init(). Those now always have the same value - failing to realise that is the same error as above. Reported-by: Laurent Jacquot <jk@lutty.net> Link: https://bugs.passt.top/show_bug.cgi?id=87 Fixes: bb9bf0bb8f57 ("tcp, udp: Don't precompute port remappings in epoll references") Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Replace TCP buffer structure by an iovec arrayLaurent Vivier2024-04-192-268/+264
| | | | | | | | | | | | | To be able to provide pointers to TCP headers and IP headers without worrying about alignment in the structure, split the structure into several arrays and point to each part of the frame using an iovec array. Using iovec also allows us to simply ignore the first entry when the vnet length header is not needed. Signed-off-by: Laurent Vivier <lvivier@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf: Don't fail if the template interface doesn't have a MAC addressStefano Brivio2024-04-192-4/+9
| | | | | | | | | | | | | | | | ...simply resort to using locally-administered address (LAA) as host-side source, instead. Pick 02:00:00:00:00:00, to make it clear that we don't actually care about that address, and also to match the 00 (Administratively Assigned Identifier) quadrant of SLAP (RFC 8948). Otherwise, pasta refuses to start if the template is a tun or Wireguard interface. Link: https://bugs.passt.top/show_bug.cgi?id=49 Link: https://github.com/containers/podman/issues/22320 Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf: We're interested in the MAC address, not in the MAC itselfStefano Brivio2024-04-191-2/+2
| | | | | Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* pasta, util: Align stack area for clones to maximum natural alignmentStefano Brivio2024-04-192-2/+5
| | | | | | | | | | | | | | | | Given that we use this stack pointer as a location to store arbitrary data types from the cloned process, we need to guarantee that its alignment matches any of those possible data types. runsisi reports that pasta gets a SIGBUS in pasta_open_ns() on aarch64, where the alignment requirement for stack pointers is a 16 bytes (same as the size of a long double), and similar requirements actually apply to most architectures we run on. Reported-by: runsisi <runsisi@hust.edu.cn> Link: https://bugs.passt.top/show_bug.cgi?id=85 Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* treewide: Compilers' name for armv6l and armv7l is "arm"Stefano Brivio2024-04-115-9/+8
| | | | | | | | | | | | | | When I switched from 'uname -m' to 'gcc -dumpmachine' to fetch the architecture name for, among others, seccomp.sh, I didn't realise that "armv6l" and "armv7l" are just Linux kernel names -- compilers just call that "arm". Fix the "syscalls" annotation we use to define seccomp profiles accordingly, otherwise pasta will be terminated on sigreturn() on armv6l and armv7l. Fixes: 213c397492bd ("passt, pasta: Run-time selection of AVX2 build") Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* test: Verify that podman tests are using the pasta binary we expect2024_04_05.954589bDavid Gibson2024-04-051-2/+9
| | | | | | | | | | | | Paul Holzinger pointed out that when we invoke the podman tests inside the passt testsuite, the way we point podman at the newly built pasta binary is kind of indirect. It's therefore prudent to check that podman is actually using the binary we expect it to - in particular that it is using the binary built in this tree, not some system installed pasta binary. Suggested-by: Paul Holzinger <pholzing@redhat.com> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* test: catatonit may not be in $PATHDavid Gibson2024-04-051-1/+1
| | | | | | | | | | | | | The pasta_podman/bats test script looks for 'catatonit' amongst other tools to be avaiiliable on the host. However, while the podman tests do require catatonit, it doesn't necessarily need to be in the regular path. For example Fedora and RHEL place catatonit in /usr/libexec and podman finds it there fine. Therefore, remove it as an htools dependency. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* test: Build and download podman as a test assetDavid Gibson2024-04-053-6/+13
| | | | | | | | | | | | | | | | | | | | | The pasta_podman/bats test scrpt downloads and builds podman, then runs its pasta specific tests. Downloading from within a test case has some drawbacks: * It can be very tedious if you have poor connectivity to the server * It makes a test that's ostensibly for pasta itself dependent on the state of the github server * It precludes runnning the tests in an isolated network environment The same concerns largely apply to building podman too, because it's pretty common for Go builds to download dependencies themselves. Therefore move the download and build of podman from the test itself, to the Makefile where we prepare other test assets. To avoid cryptic failures if something went wrong with the build, make running the test dependent on having the built podman binary. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* test: Make sure to update mbuto repositoryDavid Gibson2024-04-051-1/+7
| | | | | | | | | We download and use mbuto to build trivial boot images for our VM tests. However, if mbuto is already cloned, we won't update it to the current version. Add some make logic to ensure that we do this. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* cppcheck: Explicitly give files to checkDavid Gibson2024-04-052-3/+3
| | | | | | | | | | | | | | | | | | | | | | | Currently "make cppcheck" invokes cppcheck on ".", so it will check all the .c and .h files it can find in the source tree. This isn't ideal, because it can find files that aren't actually part of the real build, or even stale files which aren't in git. More practically, some upcoming changes are looking at downloading other source trees for some tests. Static errors in there is Not Our Problem, so checking them is both slow and pointless. So, change the Makefile to invoke cppcheck only on the specific source files that are part of the build. For some reason in this format the badBitmaskCheck warnings in seccomp.h which were suppressed by 5beb3472e ("cppcheck: Avoid errors due to zeroes in bitwise ORs") no longer trigger. That means we get unmatchedSuppression warnings instead. We add an unmatchedSuppression suppression instead of simply removing the original suppressions, just in case this odd behaviour isn't the same for all cppcheck versions. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* netlink: Ignore routes to link-local addresses for selecting interfaceDavid Gibson2024-04-052-1/+23
| | | | | | | | | | | | | | | | | | | | Since f919dc7a4b1c ("conf, netlink: Don't require a default route to start"), and since 639fdf06ede ("netlink: Fix selection of template interface") less buggily, we haven't required a default route on the host in order to operate. Instead, if we lack a default route we'll pick an interface with any route, as long as there's only one such interface. If there's more than one, we don't have a good criterion to pick, so we give up with an informational message. Paul Holzinger pointed out that this code considers it ambiguous even if all but one of the interfaces has only routes to link-local addresses (fe80::/10). A route to link-local addresses isn't really useful from pasta's point of view, so ignore them instead. This removes a misleading message in many cases, and a spurious failure in some cases. Suggested-by: Paul Holzinger <pholzing@redhat.com> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* util: Add helper to return name of address familyDavid Gibson2024-04-052-3/+21
| | | | | | | | | | | | We have a few places where we want to include the name of the internet protocol version (IPv4 or IPv6) in a message, which we handle with an open-coded ?: expression. This seems like something that might be more widely useful, so make a trivial helper to return the correct string based on the address family. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* netlink: Adjust interface index inside copied nexthop objects tooStefano Brivio2024-04-051-4/+11
| | | | | | | | | | | | | | | | | | | | As pasta duplicates host routes into the target namespaces, interface indices might not match, so we go through RTA_OIF attributes and fix them up to match the identifier in the namespace. But RTA_OIF is not the ony attribute specifying interfaces for routes: multipath routes use RTA_MULTIPATH attributes with nexthop objects, which contain in turn interface indices. Fix them up as well. If we don't, and we have at least two host interfaces, and the host interface we use as template isn't the first one (hence the mismatching indices), we'll fail to insert multipath routes with nexthop objects, and ultimately refuse to start as the kernel unexpectedly gives us ENODEV. Link: https://github.com/containers/podman/issues/22192 Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* apparmor: Fix access to procfs namespace entries in pasta's abstractionDanish Prakash2024-04-051-0/+3
| | | | | | | | | | | | | | | | | | | | | From an original patch by Danish Prakash. With commit ff22a78d7b52 ("pasta: Don't try to watch namespaces in procfs with inotify, use timer instead"), if a filesystem-bound target namespace is passed on the command line, we'll grab a handle on its parent directory. That commit, however, didn't introduce a matching AppArmor rule. Add it here. To access a network namespace procfs entry, we also need a 'ptrace' rule. See commit 594dce66d3bb ("isolation: keep CAP_SYS_PTRACE when required") for details as to when we need this -- essentially, it's about operation with Buildah. Reported-by: Jörg Sonnenberger <joerg@bec.de> Link: https://github.com/containers/buildah/issues/5440 Link: https://bugzilla.suse.com/show_bug.cgi?id=1221840 Fixes: ff22a78d7b52 ("pasta: Don't try to watch namespaces in procfs with inotify, use timer instead") Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* apparmor: Expand scope of @{run}/user access, allow writing PID files tooStefano Brivio2024-04-051-1/+1
| | | | | | | | | | | | | | | With Podman's custom networks, pasta will typically need to open the target network namespace at /run/user/<UID>/containers/networks: grant access to anything under /run/user/<UID> instead of limiting it to some subpath. Note that in this case, Podman will need pasta to write out a PID file, so we need write access, for similar locations, too. Reported-by: Jörg Sonnenberger <joerg@bec.de> Link: https://github.com/containers/buildah/issues/5440 Link: https://bugzilla.suse.com/show_bug.cgi?id=1221840 Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* apparmor: Add mount rule with explicit, empty source in passt abstractionStefano Brivio2024-04-051-0/+1
| | | | | | | | | | | | | For the policy to work as expected across either AppArmor commit 9d3f8c6cc05d ("parser: fix parsing of source as mount point for propagation type flags") and commit 300889c3a4b7 ("parser: fix option flag processing for single conditional rules"), we need one mount rule with matching mount options as "source" (that is, without source), and one without mount options and an explicit, empty source. Link: https://github.com/containers/buildah/issues/5440 Link: https://bugzilla.suse.com/show_bug.cgi?id=1221840 Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* README.md: Alpine, Guix and OpenSUSE now have packages for passtStefano Brivio2024-04-051-1/+3
| | | | | Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* tcp: Unconditionally force ACK for all !SYN, !RST packets2024_03_26.4988e2bDavid Gibson2024-03-261-5/+1
| | | | | | | | | | | | | | | | | | | | | | | Currently we set ACK on flags packets only when the acknowledged byte pointer has advanced, or we hadn't previously set a window. This means in particular that we can send a window update with no ACK flag, which doesn't appear to be correct. RFC 9293 requires a receiver to ignore such a packet [0], and indeed it appears that every non-SYN, non-RST packet should have the ACK flag. The reason for the existing logic, rather than always forcing an ACK seems to be to avoid having the packet mistaken as a duplicate ACK which might trigger a fast retransmit. However, earlier tests in the function mean we won't reach here if we don't have either an advance in the ack pointer - which will already set the ACK flag, or a window update - which shouldn't trigger a fast retransmit. [0] https://www.ietf.org/rfc/rfc9293.html#section-3.10.7.4-2.5.2.1 Link: https://github.com/containers/podman/issues/22146 Link: https://bugs.passt.top/show_bug.cgi?id=84 Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Never automatically add the ACK flag to RST packetsDavid Gibson2024-03-261-1/+1
| | | | | | | | | | | | | | | | | tcp_send_flag() will sometimes force on the ACK flag for all !SYN packets. This doesn't make sense for RST packets, where plain RST and RST+ACK have somewhat different meanings. AIUI, RST+ACK indicates an abrupt end to a connection, but acknowledges data already sent. Plain RST indicates an abort, when one end receives a packet that doesn't seem to make sense in the context of what it knows about the connection. All of the cases where we send RSTs are the second, so we don't want an ACK flag, but we currently could add one anyway. Change that, so we won't add an ACK to an RST unless the caller explicitly requests it. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Rearrange logic for setting ACK flag in tcp_send_flag()David Gibson2024-03-261-5/+4
| | | | | | | | | | We have different paths for controlling the ACK flag for the SYN and !SYN paths. This amounts to sometimes forcing on the ACK flag in the !SYN path regardless of options. We can rearrange things to explicitly be that which will make things neater for some future changes. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Split handling of DUP_ACK from ACKDavid Gibson2024-03-261-2/+2
| | | | | | | | | | | | | | | The DUP_ACK flag to tcp_send_flag() has two effects: first it forces the setting of the ACK flag in the packet, even if we otherwise wouldn't. Secondly, it causes a duplicate of the flags packet to be sent immediately after the first. Setting the ACK flag to tcp_send_flag() also has the first effect, so instead of having DUP_ACK also do that, pass both flags when we need both operations. This slightly simplifies the logic of tcp_send_flag() in a way that makes some future changes easier. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* util: fix confusion between offset in the iovec array and in the entry2024_03_20.71dd405Laurent Vivier2024-03-201-4/+5
| | | | | | | | | | | | | | | | | | | | In write_remainder() 'skip' is the offset to start the operation from in the iovec array. In iov_skip_bytes(), 'skip' is also the offset in the iovec array but 'offset' is the first unskipped byte in the iovec entry. As write_remainder() uses 'skip' for both, 'skip' is reset to the first unskipped byte in the iovec entry rather to staying the first unskipped byte in the iovec array. Fix the problem by introducing a new variable not to overwrite 'skip' on each loop. Fixes: 8bdb0883b441 ("util: Add write_remainder() helper") Signed-off-by: Laurent Vivier <lvivier@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* netlink: Fix selection of template interfaceDavid Gibson2024-03-202-26/+40
| | | | | | | | | | | | | | | | | | | | | | | | | | Since f919dc7a4b1c ("conf, netlink: Don't require a default route to start"), if there is only one host interface with routes, we will pick that as the template interface, even if there are no default routes for an IP version. Unfortunately this selection had a serious flaw: in some cases it would 'return' in the middle of an nl_foreach() loop, meaning we wouldn't consume all the netlink responses for our query. This could cause later netlink operations to fail as we read leftover responses from the aborted query. Rewrite the interface detection to avoid this problem. While we're there: * Perform detection of both default and non-default routes in a single pass, avoiding an ugly goto * Give more detail on error and working but unusual paths about the situation (no suitable interface, multiple possible candidates, etc.). Fixes: f919dc7a4b1c ("conf, netlink: Don't require a default route to start") Link: https://bugs.passt.top/show_bug.cgi?id=83 Link: https://github.com/containers/podman/issues/22052 Link: https://bugzilla.redhat.com/show_bug.cgi?id=2270257 Signed-off-by: David Gibson <david@gibson.dropbear.id.au> [sbrivio: Use info(), not warn() for somewhat expected cases where one IP version has no default routes, or no routes at all] Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* netlink: Fix handling of NLMSG_DONE in nl_route_dup()2024_03_19.d35bcbeDavid Gibson2024-03-191-9/+8
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | A recent kernel change 87d381973e49 ("genetlink: fit NLMSG_DONE into same read() as families") changed netlink behaviour so that the NLMSG_DONE terminating a bunch of responses can go in the same datagram as those responses, rather than in a separate one. Our netlink code is supposed to handle that behaviour, and indeed does so for most cases, using the nl_foreach() macro. However, there was a subtle error in nl_route_dup() which doesn't work with this change. f00b1534 ("netlink: Don't try to get further datagrams in nl_route_dup() on NLMSG_DONE") attempted to fix this, but has its own subtle error. The problem arises because nl_route_dup(), unlike other cases doesn't just make a single pass through all the responses to a netlink request. It needs to get all the routes, then make multiple passes through them. We don't really have anywhere to buffer multiple datagrams, so we only support the case where all the routes fit in a single datagram - but we need to fail gracefully when that's not the case. After receiving the first datagram of responses (with nl_next()) we have a first loop scanning them. It needs to exit when either we run out of messages in the datagram (!NLMSG_OK()) or when we get a message indicating the last response (nl_status() <= 0). What we do after the loop depends on which exit case we had. If we saw the last response, we're done, but otherwise we need to receive more datagrams to discard the rest of the responses. We attempt to check for that second case by re-checking NLMSG_OK(nh, status). However in the got-last-response case, we've altered status from the number of remaining bytes to the error code (usually 0). That means NLMSG_OK() now returns false even if it didn't during the loop check. To fix this we need separate variables for the number of bytes left and the final status code. We also checked status after the loop, but this was redundant: we can only exit the loop with NLMSG_OK() == true if status <= 0. Reported-by: Martin Pitt <mpitt@redhat.com> Fixes: f00b153414b1 ("netlink: Don't try to get further datagrams in nl_route_dup() on NLMSG_DONE") Fixes: 4d6e9d0816e2 ("netlink: Always process all responses to a netlink request") Link: https://github.com/containers/podman/issues/22052 Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* fedora: Switch license identifier to SPDX2024_03_18.615d370Dan Čermák2024-03-181-1/+1
| | | | | | | | The spec file patch by Dan Čermák was originally contributed at: https://src.fedoraproject.org/rpms/passt/pull-request/1 Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* udp: Translate source address of resolver only for DNS remapped queriesStefano Brivio2024-03-181-6/+15
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | Paul reports that if pasta is configured with --dns-forward, and the container queries a resolver which is configured on the host directly, without using the address given for --dns-forward, we'll translate the source address of the response pretending it's coming from the address passed as --dns-forward, and the client will discard the reply. That is, $ cat /etc/resolv.conf 198.51.100.1 $ pasta --config-net --dns-forward 192.0.2.1 nslookup passt.top will not work, because we change the source address of the reply from 198.51.100.1 to 192.0.2.1. But the client contacted 198.51.100.1, and it's from that address that it expects an answer. Add a PORT_DNS_FWD flag for tap-facing ports, which is triggered by activity in the opposite direction as the other flags. If the tap-facing port was seen sending a DNS query that was remapped, we'll remap the source address of the response, otherwise we'll leave it unaffected. Reported-by: Paul Holzinger <pholzing@redhat.com> Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* conf, netlink: Don't require a default route to startStefano Brivio2024-03-183-21/+59
| | | | | | | | | | | | | | | | | | | | There might be isolated testing environments where default routes and global connectivity are not needed, a single interface has all non-loopback addresses and routes, and still passt and pasta are expected to work. In this case, it's pretty obvious what our upstream interface should be, so go ahead and select the only interface with at least one route, disabling DHCP and implying --no-map-gw as the documentation already states. If there are multiple interfaces with routes, though, refuse to start, because at that point it's really not clear what we should do. Reported-by: Martin Pitt <mpitt@redhat.com> Link: https://github.com/containers/podman/issues/21896 Signed-off-by: Stefano brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* netlink: Don't try to get further datagrams in nl_route_dup() on NLMSG_DONEStefano Brivio2024-03-181-1/+2
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Martin reports that, with Fedora Linux kernel version kernel-core-6.9.0-0.rc0.20240313gitb0546776ad3f.4.fc41.x86_64, including commit 87d381973e49 ("genetlink: fit NLMSG_DONE into same read() as families"), pasta doesn't exit once the network namespace is gone. Actually, pasta is completely non-functional, at least with default options, because nl_route_dup(), which duplicates routes from the parent namespace into the target namespace at start-up, is stuck on a second receive operation for RTM_GETROUTE. However, with that commit, the kernel is now able to fit the whole response, including the NLMSG_DONE message, into a single datagram, so no further messages will be received. It turns out that commit 4d6e9d0816e2 ("netlink: Always process all responses to a netlink request") accidentally relied on the fact that we would always get at least two datagrams as a response to RTM_GETROUTE. That is, the test to check if we expect another datagram, is based on the 'status' variable, which is 0 if we just parsed NLMSG_DONE, but we'll also expect another datagram if NLMSG_OK on the last message is false. But NLMSG_OK with a zero length is always false. The problem is that we don't distinguish if status is zero because we got a NLMSG_DONE message, or because we processed all the available datagram bytes. Introduce an explicit check on NLMSG_DONE. We should probably refactor this slightly, for example by introducing a special return code from nl_status(), but this is probably the least invasive fix for the issue at hand. Reported-by: Martin Pitt <mpitt@redhat.com> Link: https://github.com/containers/podman/issues/22052 Fixes: 4d6e9d0816e2 ("netlink: Always process all responses to a netlink request") Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Tested-by: Paul Holzinger <pholzing@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* tap: Rename tap_iov_{base,len}David Gibson2024-03-143-17/+17
| | | | | | | | | | | These two functions are typically used to calculate values to go into the iov_base and iov_len fields of a struct iovec. They don't have to be used for that, though. Rename them in terms of what they actually do: calculate the base address and total length of the complete frame, including both L2 and tap specific headers. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Implement tap_send() "slow path" in terms of fast pathDavid Gibson2024-03-143-25/+19
| | | | | | | | | | | | | | | | Most times we send frames to the guest it goes via tap_send_frames(). However "slow path" protocols - ARP, ICMP, ICMPv6, DHCP and DHCPv6 - go via tap_send(). As well as being a semantic duplication, tap_send() contains at least one serious problem: it doesn't properly handle short sends, which can be fatal on the qemu socket connection, since frame boundaries will get out of sync. Rewrite tap_send() to call tap_send_frames(). While we're there, rename it tap_send_single() for clarity. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Simplify some casts in the tap "slow path" functionsDavid Gibson2024-03-141-23/+18
| | | | | | | | We can both remove some variables which differ from others only in type, and slightly improve type safety. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tap: Extend tap_send_frames() to allow multi-buffer framesDavid Gibson2024-03-144-37/+59
| | | | | | | | | | | tap_send_frames() takes a vector of buffers and requires exactly one frame per buffer. We have future plans where we want to have multiple buffers per frame in some circumstances, so extend tap_send_frames() to take the number of buffers per frame as a parameter. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> [sbrivio: Improve comment to rembufs calculation] Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* passt, log: Call __openlog() earlier, log to stderr until we detachStefano Brivio2024-03-142-8/+8
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Paul reports that, with commit 15001b39ef1d ("conf: set the log level much earlier"), early messages aren't reported to standard error anymore. The reason is that, once the log mask is changed from LOG_EARLY, we don't force logging to stderr, and this mechanism was abused to have early errors on stderr. Now that we drop LOG_EARLY earlier on, this doesn't work anymore. Call __openlog() as soon as we know the mode we're running as, using LOG_PERROR. Then, once we detach, if we're not running from an interactive terminal and logging to standard error is not forced, drop LOG_PERROR from the options. While at it, check if the standard error descriptor refers to a terminal, instead of checking standard output: if the user redirects standard output to /dev/null, they might still want to see messages from standard error. Further, make sure we don't print messages to standard error reporting that we couldn't log to the system logger, if we didn't open a connection yet. That's expected. Reported-by: Paul Holzinger <pholzing@redhat.com> Fixes: 15001b39ef1d ("conf: set the log level much earlier") Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* pcap: Use clock_gettime() instead of gettimeofday()Stefano Brivio2024-03-142-13/+14
| | | | | | | | | | | | | | | POSIX.1-2008 declared gettimeofday() as obsolete, but I'm a dinosaur. Usually, C libraries translate that to the clock_gettime() system call anyway, but this doesn't happen in Jon's environment, and, there, seccomp happily kills pasta(1) when started with --pcap, because we didn't add gettimeofday() to our seccomp profiles. Use clock_gettime() instead. Reported-by: Jon Maloy <jmaloy@redhat.com> Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
generated by cgit v1.2.3 (git 2.25.1) at 2024-11-09 23:47:49 +0000