aboutgitcodebugslistschat
Commit message (Collapse)AuthorAgeFilesLines
* 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>
* dhcp: Actually note down the length of options received by the clientStefano Brivio2023-09-271-0/+1
| | | | | | | | | | | | | | | It turns out we never used 'clen' until commit 1f24d3efb499 ("dhcp: support BOOTP clients"), and we always ignored option 55 (Parameter Request List), while, according to RFC 2132, we MUST try to insert the requested options in the order requested by the client. The commit mentioned above made this visible because now every client is reported as sending a DHCPREQUEST as an old BOOTP client, based on the lack of option 53 (that is, zero length). Fixes: b439984641ed ("merd: ARP and DHCP handlers, connection tracking fixes") Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* dhcpv6: Properly separate domain names in search listStefano Brivio2023-09-271-7/+17
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | To prepare the DHCPv6 domain search list option, we go over the flattened list of domains, and replace both dots and zero bytes with a counter of bytes in the next label, implementing the encoding specified by section 3.1 of RFC 1035. If there are multiple domains in the list, however, zero bytes serve as markers for the end of a domain name, and we'll replace them with the length of the first label of the next domain, plus one. This is wrong. We should only convert the dots before the labels. To distinguish between label separators and domain names separators, for simplicity, introduce a dot before the first label of every domain we copy to form the list. All dots are then replaced by label lengths, and separators (zero bytes) remain as they are. As we do this, we need to make sure we don't replace the trailing dot, if present: that's already a separator. Skip copying it, and just add separators as needed. Now that we don't copy those, though, we might end up with zero-length domains: skip them, as they're meaningless anyway. And as we might skip domains, we can't use the index 'i' to check if we're at the beginning of the option -- use 'srch' instead. This is very similar to how we prepare the list for NDP option 31, except that we don't need padding (RFC 8106, 5.2) here, and we should refactor this into common functions, but it probably makes sense to rework the NDP responder (https://bugs.passt.top/show_bug.cgi?id=21) first. Reported-by: Sebastian Mitterle <smitterl@redhat.com> Link: https://bugs.passt.top/show_bug.cgi?id=75 Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* util: Fix licensing information display in --version2023_09_08.05627dcStefano Brivio2023-09-081-2/+2
| | | | | | | The regular expression I used when relicensing to GPLv2+ missed this. Fixes: ca2749e1bd52 ("passt: Relicense to GPL 2.0, or any later version") Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* 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>
* udp, tap: Correctly advance through packets in udp_tap_handler()David Gibson2023-09-083-20/+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 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-083-22/+33
| | | | | | | | | | | | | | | | | | | | | | | | | 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>
* test: Add Podman system test with bats for pasta2023_09_07.ee58f37Stefano Brivio2023-09-073-2/+27
| | | | | | | | | | | | | | | | | | | Ugly as hell, but we keep breaking things otherwise, and I keep forgetting to run this manually (as long as it's based on my local Podman setup, that's the only alternative). We need to clone the Podman repository as distribution packages don't contain test scripts, typically. While at it, build the latest version which is what really matters. As we're planning anyway to revamp the test framework, I'd be inclined to just add this without too many thoughts, and have it as a nice-to-have requirement reminder for the new framework. Link: https://github.com/containers/podman/pull/19699 Suggested-by: Paul Holzinger <pholzing@redhat.com> Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
* dhcp: support BOOTP clientsStas Sergeev2023-09-071-2/+2
| | | | | | | | | | BOOTP clients do not use tagged messages for requests. As such, any message without the DHCP option 53, should be considered a BOOTP request. Link: https://bugs.passt.top/show_bug.cgi?id=72 Signed-off-by: Stas Sergeev <stsp2@yandex.ru> 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>
* fedora: Replace pasta hard links by separate buildsStefano Brivio2023-09-071-6/+16
| | | | | | | | | | | | | | | | | | | | | | | | | The hard link trick didn't actually fix the issue with SELinux file contexts properly: as opposed to symbolic links, SELinux now correctly associates types to the labels that are set -- except that those labels are now shared, so we can end up (depending on how rpm(8) extracts the archives) with /usr/bin/passt having a pasta_exec_t context. This got rather confusing as running restorecon(8) seemed to fix up labels -- but that's simply toggling between passt_exec_t and pasta_exec_t for both links, because each invocation will just "fix" the file with the mismatching context. Replace the hard links with two separate builds of the binary, as suggested by David. The build is reproducible, so we pass "-pasta" in the VERSION for pasta's build. This is wasteful but better than the alternative. Just copying the binary over would otherwise cause issues with debuginfo packages due to duplicate Build-IDs -- and rpmbuild(8) also warns about them. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* apparmor: Add pasta's own profileStefano Brivio2023-09-073-10/+31
| | | | | | | | | | | | | | | | | | | If pasta and pasta.avx2 are hard links to passt and passt.avx2, AppArmor will attach their own profiles on execution, and we can restrict passt's profile to what it actually needs. Note that pasta needs to access all the resources that passt needs, so the pasta abstraction still includes passt's one. I plan to push the adaptation required for the Debian package in commit 5bb812e79143 ("debian/rules: Override pasta symbolic links with hard links"), on Salsa. If other distributions need to support AppArmor profiles they can follow a similar approach. The profile itself will be installed, there, via dh_apparmor, in a separate commit, b52557fedcb1 ("debian/rules: Install new pasta profile using dh_apparmor"). Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* apparmor: Allow pasta to remount /proc, access entries under its own copyStefano Brivio2023-09-071-0/+7
| | | | | | | | | | Since commit b0e450aa8500 ("pasta: Detach mount namespace, (re)mount procfs before spawning command"), we need to explicitly permit mount of /proc, and access to entries under /proc/PID/net (after remount, that's what AppArmor sees as path). Fixes: b0e450aa8500 ("pasta: Detach mount namespace, (re)mount procfs before spawning command") Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* apparmor: Allow read-only access to uid_mapStefano Brivio2023-09-071-0/+2
| | | | | | | | | | | | | | Starting with commit 770d1a4502dd ("isolation: Initially Keep CAP_SETFCAP if running as UID 0 in non-init"), the lack of this rule became more apparent as pasta needs to access uid_map in procfs even as non-root. However, both passt and pasta needs this, in case they are started as root, so add this directly to passt's abstraction (which is sourced by pasta's profile too). Fixes: 770d1a4502dd ("isolation: Initially Keep CAP_SETFCAP if running as UID 0 in non-init") Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* apparmor: Explicitly pass options we use while remounting root filesystemStefano Brivio2023-09-071-1/+1
| | | | | | | | | | | | | | | | | | | | | | | | | As a result of AppArmor commit d4b0fef10a4a ("parser: fix rule flag generation change_mount type rules"), we can't expect anymore to get permission to mount() / read-write, with MS_REC | MS_UNBINDABLE ("runbindable", in AppArmor terms), if we don't explicitly pass those flags as options. It used to work by mistake. Now, the reasonable expectation would be that we could just change the existing rule into: mount options=(rw, runbindable) "" -> /, ...but this now fails to load too, I think as a result of AppArmor commit 9d3f8c6cc05d ("parser: fix parsing of source as mount point for propagation type flags"). It works with 'rw' alone, but 'runbindable' is indeed a propagation type flag. Skip the source specification, it doesn't add anything meaningful to the rule anyway. Reported-by: Paul Holzinger <pholzing@redhat.com> Link: https://github.com/containers/podman/pull/19751 Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* apparmor: Use abstractions/nameservice to deal with symlinked resolv.confStefano Brivio2023-09-061-2/+1
| | | | | | | | | | | | | While abstractions/nameservice appeared too broad and overkill for our simple need (read-only resolv.conf access), it properly deals with symlinked resolv.conf files generated by systemd-resolved, NetworkManager or suchlike. If we just grant read-only access to /etc/resolv.conf, we'll fail to read nameserver information in rather common configurations, because AppArmor won't follow the symlink. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* pasta: Strip RTA_PREFSRC when copying routes to the namespace2023_08_23.a7e4bfbDavid Gibson2023-08-231-1/+14
| | | | | | | | | | | | | | | | | | | | | Host routes can include a preferred source address (RTA_PREFSRC), which must be one of the host's addresses. However when using pasta with -a the namespace might be given a different address, not on the host. This seems to occur pretty routinely depending on the network configuration systems in place on the host. With --config-net we will try to copy host routes to the namespace. If one of those includes an RTA_PREFSRC, but the namespace doesn't have the host address, this will fail with -EINVAL, causing pasta to fail. Fix this by stripping off RTA_PREFSRC attributes from routes as we copy them to the namespace. This is by no means infallible, bit it should at least handle common cases for the time being. Link: https://bugs.passt.top/show_bug.cgi?id=71 Link: https://github.com/containers/podman/pull/19699#issuecomment-1688769287 Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* netlink: Set IFA_ADDRESS, not just IFA_LOCAL, while adding IPv4 addressesStefano Brivio2023-08-231-0/+1
| | | | | | | | | Otherwise, we actually configure the address, but it's not usable because no local route is added by the kernel. Link: https://github.com/containers/podman/pull/19699 Fixes: cfe7509e5c16 ("netlink: Use struct in_addr for IPv4 addresses, not bare uint32_t") Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* tcp: Remove broken pressure calculations for tcp_defer_handler()David Gibson2023-08-223-13/+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>
* inany: Add missing double include guard to inany.hDavid Gibson2023-08-221-0/+5
| | | | | | | This was overlooked when the file was created. 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-223-7/+9
| | | | | | | | | | | | | | | | | | | 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-228-32/+15
| | | | | | | | | | | | | | | | | 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-223-52/+27
| | | | | | | | | | | | | | | | | | | | | | 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-222-52/+53
| | | | | | | | | | | | | | | | | | | | | | | | | | | 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-227-32/+48
| | | | | | | | | | | 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>
* selinux: Fix domain transitions for typical commands pasta might run2023_08_18.0af928eStefano Brivio2023-08-181-1/+18
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | ...now it gets ugly. If we use pasta without an existing target namespace, and run commands directly or spawn a shell, and keep the pasta_t domain when we do, they won't be able to do much: a shell might even start, but it's not going to be usable, or to even display a prompt. Ideally, pasta should behave like a shell when it spawns a command: start as unconfined_t and automatically transition to whatever domain is associated in the specific policy for that command. But we can't run as unconfined_t, of course. It would seem natural to switch to unconfined_t "just before", so that the default transitions happen. But transitions can only happen when we execvp(), and that's one single transition -- not two. That is, this approach would work for: pasta -- sh -c 'ip address show' but not for: pasta -- ip address show If we configure a transition to unconfined_t when we run ip(8), we'll really try to start that as unconfined_t -- but unconfined_t isn't allowed as entrypoint for ip(8) itself, and execvp() will fail. However, there aren't many different types of binaries pasta might commonly run -- for example, we're unlikely to see pasta used to run a mount(8) command. Explicitly set up domain transition for common stuff -- switching to unconfined_t for bin_t and shells works just fine, ip(8), ping(8), arping(8) and similar need a different treatment. While at it, allow commands we spawn to inherit resource limits and signal masks, because that's what happens by default, and don't require AT_SECURE sanitisation of the environment (because that won't happen by default). Slightly unrelated: we also need to explicitly allow pasta_t to use TTYs, not just PTYs, otherwise we can't keep stdin and stdout open for shells. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* selinux: Allow pasta_t to read nsfs entriesStefano Brivio2023-08-181-0/+2
| | | | | | | | | This is needed to monitor filesystem-bound namespaces and quit when they're gone -- this feature never really worked with SELinux. Fixes: 745a9ba4284c ("pasta: By default, quit if filesystem-bound net namespace goes away") Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Acked-by: Richard W.M. Jones <rjones@redhat.com>
* selinux: Add rules for sysctl and /proc/net accessesStefano Brivio2023-08-182-0/+4
| | | | | | | | | That's what we actually need to check networking-related sysctls, to scan for bound ports, and to manipulate bits of network configuration inside pasta's target namespaces. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Acked-by: Richard W.M. Jones <rjones@redhat.com>
* selinux: Update policy to fix user/group settingsStefano Brivio2023-08-182-4/+13
| | | | | | | | | Somehow most of this used to work on older kernels, but now we need to explicitly permit setuid, setgid, and setcap capabilities, as well as read-only access to passwd (as we support running under a given login name) and sssd library facilities. Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* selinux: Fix user namespace creation after breaking kernel changeStefano Brivio2023-08-182-0/+4
| | | | | | | | | | | | | | | | | Kernel commit ed5d44d42c95 ("selinux: Implement userns_create hook") seems to just introduce a new functionality, but given that SELinux implements a form of mandatory access control, introducing the new permission breaks any application (shipping with SELinux policies) that needs to create user namespaces, such as passt and pasta for sandboxing purposes. Add the new 'allow' rules. They appear to be backward compatible, kernel-wise, and the policy now requires the new 'user_namespace' class to build, but that's something distributions already ship. Reported-by: Richard W.M. Jones <rjones@redhat.com> Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
* selinux: Use explicit paths for binaries in file contextStefano Brivio2023-08-182-2/+4
| | | | | | | | | There's no reason to use wildcards, and we don't want any similarly-named binary (not that I'm aware of any) to risk being associated to passt_exec_t and pasta_exec_t by accident. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
* fedora: Install pasta as hard link to ensure SELinux file context matchStefano Brivio2023-08-181-0/+7
| | | | | | | | | | | | | | The Makefile installs symbolic links by default, which actually worked at some point (not by design) with SELinux, but at least on recent kernel versions it doesn't anymore: override pasta (and pasta.avx2) with hard links. Otherwise, even if the links are labeled as pasta_exec_t, SELinux will "resolve" them to passt_exec_t, and we'll have pasta running as passt_t instead of pasta_t. Signed-off-by: Stefano Brivio <sbrivio@redhat.com> Acked-by: Richard W.M. Jones <rjones@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>
* netlink: Don't propagate host address expiry to the containerDavid Gibson2023-08-161-1/+3
| | | | | | | | | | | | | | | | | | | | | | | When we copy addresses from the host to the container in nl_addr_dup(), we copy all the address's attributes, including IFA_CACHEINFO, which controls the address's lifetime. If the host address is managed by, for example, DHCP, it will typically have a finite lifetime. When we copy that lifetime to the pasta container, that lifetime will remain, meaning the kernel will eventually remove the address, typically some hours later. The container, however, won't have the DHCP client or whatever was managing and maintaining the address in the host, so it will just lose connectivity. Long term, we may want to monitor host address changes and reflect them to the guest. But for now, we just want to take a snapshot of the host's address and set those in the container permanently. We can accomplish that by stripping off the IFA_CACHEINFO attribute as we copy addresses. Link: https://github.com/containers/podman/issues/19405 Link: https://bugs.passt.top/show_bug.cgi?id=70 Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* netlink: Correctly calculate attribute length for address messagesDavid Gibson2023-08-161-2/+2
| | | | | | | | | | | | In nl_addr_get() and nl_addr_dup() we step the attributes attached to each RTM_NEWADDR message with a loop initialised with IFA_RTA() and RTM_PAYLOAD() macros. RTM_PAYLOAD(), however is for RTM_NEWROUTE messages (struct rtmsg), not RTM_NEWADDR messages (struct ifaddrmsg). Consequently it miscalculates the size and means we can skip some attributes. Switch to IFA_PAYLOAD() which we should be using here. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* netlink: Remove redundant check on nlmsg_typeDavid Gibson2023-08-161-3/+0
| | | | | | | | | | | In the loop within nl_addr_dup() we check and skip for any messages that aren't of type RTM_NEWADDR. This is a leftover that was missed in the recent big netlink cleanup. In fact we already check for the message type in the nl_foreach_oftype() macro, so the explicit test is redudant. Remove it. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* conf: Demote overlapping port ranges error to a warningDavid Gibson2023-08-131-4/+3
| | | | | | | | | | | | | | | | | | | | We give a fatal error if the port ranges from any port forwarding specifiers overlap. This occurs even if those port ranges are specifically bound to different addresses, so there's not really any overlap. Right now, we can't 100% handle this case correctly, because our data structures don't have a way to represent per-address forwarding. However, there are a number of cases that will actually work just fine: e.g. mapping the same port to the same port on two different addresses (say :: and 127.0.0.1). We have long term plans to fix this properly, but that is still some time away. For the time being, demote this error to a warning so that the cases that already work will be allowed. Link: https://bugs.passt.top/show_bug.cgi?id=56 Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* epoll: Use different epoll types for passt and pasta tap fdsDavid Gibson2023-08-134-30/+30
| | | | | | | | | | | 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-134-16/+15
| | | | | | | | | | | | | 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: Split handling of listening TCP sockets into their own handlerDavid Gibson2023-08-137-45/+55
| | | | | | | | | | | | | | | | | 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-134-14/+12
| | | | | | | | | | | | 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: Tiny cleanup to udp_sock_handler()David Gibson2023-08-132-3/+2
| | | | | | | | | Move the test for c->no_udp into the function itself, rather than in the dispatching switch statement to better localize the UDP specific logic, and make for greated consistency with other handler functions. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
* epoll: Split handling of ICMP and ICMPv6 socketsDavid Gibson2023-08-133-56/+70
| | | | | | | | | | | | | | | | We have different epoll type values for ICMP and ICMPv6 sockets, but they both call the same handler function, icmp_sock_handler(). However that function does essentially nothing in common for the two cases. So, split it into icmp_sock_handler() and icmpv6_sock_handler() and dispatch them separately from the top level. While we're there remove some parameters that the function was never using anyway. Also move the test for c->no_icmp into the functions, so that all the logic specific to ICMP is within the handler, rather than in the top level dispatch code. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
generated by cgit v1.2.3 (git 2.25.1) at 2024-11-14 20:33:27 +0000