If we get an error from epoll_ctl() in tcp_splice_epoll_ctl() we goto the
'delete' path where we remove both sockets from the epoll set and return
an error. There are several problems with this:
- We 'return -errno' after the EPOLL_CTL_DEL operations, which means the
deleting epoll_ctl() calls may have overwritten the errno values which
actually triggered the failures.
- The call from conn_flag_do() occurs when the CLOSING flag is set, in
which case we go do the delete path regardless of error. In that case
the 'return errno' is meaningless since we don't expect the EPOLL_CTL_DEL
operations to fail and we ignore the return code anyway.
- All other calls to tcp_splice_epoll_ctl() check the return code and if
non-zero immediately call conn_flag(..., CLOSING) which will call
tcp_splice_epoll_ctl() again explicitly to remove the sockets from epoll.
That means removing them when the error first occurs is redundant.
- We never specifically report an error on the epoll_ctl() operations. We
just set the connection to CLOSING, more or less silently killing it.
This could make debugging difficult in the unlikely even that we get a
failure here.
Re-organise tcp_splice_epoll_ctl() to just log a message then return in the
error case, and only EPOLL_CTL_DEL when explicitly asked to with the
CLOSING flag.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
tcp_splice_conn_update() calls tcp_splice_epoll_ctl() twice: first ignoring
the return value, then checking it. This serves no purpose. If the first
call succeeds, the second call will do exactly the same thing again, since
nothing has changed in conn. If the first call fails, then
tcp_splice_epoll_ctl() itself will EPOLL_CTL_DEL both fds, meaning when
the second call tries to EPOLL_CTL_MOD them it will necessarily fail.
It appears that this duplication was introduced by accident in an
otherwise unrelated patch.
Fixes: bb708111 ("treewide: Packet abstraction with mandatory boundary checks")
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
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>
For certain socket types, we record in the epoll ref whether they're
sockets in the namespace, or on the host. We now have the notion of "pif"
to indicate what "place" a socket is associated with, so generalise the
simple one-bit 'ns' to a pif id.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
We have several possible ways of communicating with other entities. We use
sockets to communicate with the host and other network sites, but also in
a different context to communicate "spliced" channels to a namespace. We
also use a tuntap device or a qemu socket to communicate with the namespace
or guest.
For the time being these are just defined implicitly by how we structure
things. However, there are other communication channels we want to use in
future (e.g. virtio-user), and we want to allow more flexible forwarding
between those. To accomplish that we're going to want a specific way of
referring to those channels.
Introduce the concept of a "passt/pasta interface" or "pif" representing a
specific channel to communicate network data. Each pif is assumed to be
associated with a specific network namespace in the broad sense (that is
as a place where IP addresses have a consistent meaning - not the Linux
specific sense). But there could be multiple pifs communicating with the
same namespace (e.g. the spliced and tap interfaces in pasta).
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
udp_sock_init() has a number of paths that initialise uref differently.
However some of the fields are initialised the same way in all of them.
Move those fields into the original initialiser to save a few lines.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
get_bound_ports_*() now only use their context and ns parameters to
determine which forwarding maps they're operating on. Each function needs
the map they're actually updating, as well as the map for the other
direction, to avoid creating forwarding loops. The UDP function also
requires the corresponding TCP map, to implement the behaviour where we
forward UDP ports of the same number as bound TCP ports for tools like
iperf3.
Passing those maps directly as parameters simplifies the code without
making the callers life harder, because those already know the relevant
maps. IMO, invoking these functions in terms of where they're looking for
updated forwarding also makes more logical sense than in terms of where
they're looking for bound ports. Given that new way of looking at the
functions, also rename them to port_fwd_scan_*().
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Currently we store /proc/net fds used to implement automatic port
forwarding in the proc_net_{tcp,udp} fields of the main context structure.
However, in fact each of those is associated with a particular direction
of forwarding, and we already have struct port_fwd which collects all
other information related to a particular direction of port forwarding.
We can simplify things a bit by moving the /proc fds into struct port_fwd.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Currently get_bound_ports() takes a parameter to determine if it scans for
UDP or TCP bound ports, but in fact there's almost nothing in common
between those two paths. The parameter appears primarily to have been
a convenience for when we needed to invoke this function via NS_CALL().
Now that we don't need that, split it into separate TCP and UDP versions.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
When we want to scan for bound ports in the namespace we use NS_CALL() to
run get_bound_ports() in the namespace. However, the only thing it
actually needed to be in the namespace for was to open the /proc/net file
it was scanning. Since we now always pre-open those, we no longer need
to switch to the namespace for the actual get_bound_ports() calls.
That in turn means that tcp_port_detect() doesn't need to run in the ns
either, and we can just replace it with inline calls to get_bound_ports().
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
procfs_scan_listen() can either use an already opened fd for a /proc/net
file, or it will open it. So, effectively it will open the file on the
first call, then re-use the fd in subsequent calls. However, it's not
possible to open the /proc/net files after we isolate our filesystem in
isolate_prefork(). That means that for each /proc/net file we must call
procfs_scan_listen() at least once before isolate_prefork(), or it won't
work afterwards.
That happens to be the case, but it's a pretty fragile requirement. To
make this more robust, instead always pre-open the /proc files we will need
in get_bounds_port_init() and have procfs_scan_listen() just use those.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Most of our helpers which need to enter the pasta network namespace are
quite specialised. Add one which is rather general - it just open()s a
given file in the namespace context and returns the fd back to the main
namespace. This will have some future uses.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
procfs_scan_listen() does some slightly clunky logic to deduce the fd it
wants to use, the path it wants to open and the state it's looking for
based on parameters for protocol, IP version and whether we're in the
namespace.
However, the caller already has to make choices based on similar parameters
so it can just pass in the things that procfs_scan_listen() needs directly.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
The implementation of scanning /proc files to do automatic port forwarding
is a bit awkwardly split between procfs_scan_listen() in util.c,
get_bound_ports() and related functions in conf.c and the initial setup
code in conf().
Consolidate all of this into port_fwd.h, which already has some related
definitions, and a new port_fwd.c.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Initialisation of the forwarding mode variables is complicated a bit by the
fact that we have different defaults for passt and pasta modes. This leads
to some debateably duplicated code between those two cases.
More significantly, however, currently the final setting of the mode
variable is interleaved with the code to actually start automatic scanning
when that's selected. This essentially mingles "policy" code (setting the
default mode), with implementation code (making that happen). That's a bit
inflexible if we want to change policies in future.
Disentangle these two pieces, and use a slightly different construction to
make things briefer as well.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
With current selinux-policy-37.22-1.fc37.noarch, and presumably any
future update for Fedora 37, the user_namespace class is not
available, so statements using it prevent the policy from being
loaded.
If a class is not defined in the base policy, any related permission
is assumed to be enabled, so we can safely drop those.
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2237996
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
...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>
It looks like we need it as workaround for this situation, readily
reproducible at least with a 6.5 Linux kernel, with default rmem_max
and wmem_max values:
- an iperf3 client on the host sends about 160 KiB, typically
segmented into five frames by passt. We read this data using
MSG_PEEK
- the iperf3 server on the guest starts receiving
- meanwhile, the host kernel advertised a zero-sized window to the
sender, as expected
- eventually, the guest acknowledges all the data sent so far, and
we drop it from the buffer, courtesy of tcp_sock_consume(), using
recv() with MSG_TRUNC
- the client, however, doesn't get an updated window value, and
even keepalive packets are answered with zero-window segments,
until the connection is closed
It looks like dropping data from a socket using MSG_TRUNC doesn't
cause a recalculation of the window, which would be expected as a
result of any receiving operation that invalidates data on a buffer
(that is, not with MSG_PEEK).
Strangely enough, setting TCP_WINDOW_CLAMP via setsockopt(), even to
the previous value we clamped to, forces a recalculation of the
window which is advertised to the sender.
I couldn't quite confirm this issue by following all the possible
code paths in the kernel, yet. If confirmed, this should be fixed in
the kernel, but meanwhile this workaround looks robust to me (and it
will be needed for backward compatibility anyway).
Reported-by: Matej Hrica <mhrica@redhat.com>
Link: https://bugs.passt.top/show_bug.cgi?id=74
Analysed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
cppcheck 2.12.0 (and maybe some other versions) things this if condition
is always true, which is demonstrably not true. Work around the bug for
now.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Recent enough cppcheck versions (at least as of cppcheck 2.12) give this
error processing conf.c:
conf.c:1179:1: information: ValueFlow analysis is limited in conf. Use --check-level=exhaustive if full analysis is wanted. [checkLevelNormal]
Adding --check-level=exhaustive doesn't seem to significantly increase the
cppcheck run time for us, so enable it when possible, suppressing that
warning.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
In a couple of places in conf(), we use a local 'fwd' variable to reference
one of our forwarding tables. The value depends on which command line
option we're currently looking at, and is initialized rather cryptically
from an assignment side-effect within the if condition checking that
option.
Newer versions of cppcheck complain about this assignment being an always
true condition, but in any case it's both clearer and slightly shorter to
use separate if branches for the two cases and set the forwarding parameter
more directly.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
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>
We have a bunch of variants of the siphash functions for different data
sizes. The callers, in tcp.c, need to pack the various values they want to
hash into a temporary structure, then call the appropriate version. We can
avoid the copy into the temporary by directly using the incremental
siphash functions.
The length specific hash functions also have an undocumented constraint
that the data pointer they take must, in fact, be aligned to avoid
unaligned accesses, which may cause crashes on some architectures.
So, prefer the incremental approach and remove the length-specific
functions.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
A number of checksum and hash functions require workarounds for the odd
behaviour of Type-Baased Alias Analysis. We have a detailed comment about
this on siphash_8b() and other functions reference that.
Move the main comment to csume_16b() instead, because we're going to
reorganise things in siphash.c.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Move a bunch of code from siphash.c to siphash.h, making it available to
other modules. This will allow places which need hashes of more complex
objects to construct them incrementally.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
To improve type safety, encapsulate the internal state of the SipHash
algorithm into a dedicated structure type.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
The PREAMBLE macro sets up the SipHash initial internal state. It also
defines that state as a variable, which isn't macro hygeinic. With
previous changes simplifying this premable, it's now possible to replace it
with a macro which simply expands to a C initialisedrfor that state.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
The SipHash algorithm starts with initializing the 32 bytes of internal
state with some magic numbers XORed with the hash key. However, our
implementation has a bug - rather than XORing the hash key, it *sets* the
initial state to copies of the key.
I don't know if that affects any of the cryptographic properties of SipHash
but it's not what we should be doing. Fix it.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
The POSTAMBLE macro implements the finalisation steps of SipHash. It
relies on some variables in the environment, including returning the final
hash value that way. That isn't great hygeine.
In addition the PREAMBLE macro takes a length parameter which is used only
to initialize the 'b' value that's not used until the finalisation and is
also sometimes modified in a non-obvious way by the callers.
The 'b' value is always composed from the total length of the hash input
plus up to 7 bytes of "tail" data - that is the remainder of the hash input
after a multiple of 8 bytes has been consumed.
Simplify all this by replacing the POSTAMBLE macro with a siphash_final()
function which takes the length and tail data as parameters and returns the
final hash value.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
We have macros or inlines for a number of common operations in the siphash
functions. However, in a number of places we still open code feeding
another 64-bits of data into the hash function: an xor, followed by 2
rounds of shuffling, followed by another xor.
Implement an inline function for this, which results in somewhat shortened
code.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
The SIPROUND(n) macro implements n rounds of SipHash shuffling. It relies
on 'v' and '__i' variables being available in the context it's used in
which isn't great hygeine. Replace it with an inline function instead.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Some of the siphas_*b() functions return 64-bit results, others 32-bit
results, with no obvious pattern. siphash_32b() also appears to do this
incorrectly - taking the 64-bit hash value and simply returning it
truncated, rather than folding the two halves together.
Since SipHash proper is defined to give a 64-bit hash, make all of them
return 64-bit results. In the one caller which needs a 32-bit value,
tcp_seq_init() do the fold down to 32-bits ourselves.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
We have several workarounds for a clang-tidy bug where the checker doesn't
recognize that a number of system calls write to - and therefore initialise
- a socket address. We can't neatly use a suppression, because the bogus
warning shows up some time after the actual system call, when we access
a field of the socket address which clang-tidy erroneously thinks is
uninitialised.
Consolidate these workarounds into one place by using macros to implement
wrappers around affected system calls which add a memset() of the sockaddr
to silence clang-tidy. This removes the need for the individual memset()
workarounds at the callers - and the somewhat longwinded explanatory
comments.
We can then use a #define to not include the hack in "real" builds, but
only consider it for clang-tidy.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
A classic gotcha of the standard C library is that its unwise to call any
variable 'index' because it will shadow the standard string library
function index(3). This can cause warnings from cppcheck amongst others,
and it also means that if the variable is removed you tend to get confusing
type errors (or sometimes nothing at all) instead of a nice simple "name is
not defined" error.
Strictly speaking this only occurs if <string.h> is included, but that
is so common that as a rule it's best to just avoid it always. We
have a number of places which hit this trap, so rename variables and
parameters to avoid it.
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
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>
It turns out we never used 'clen' until commit 1f24d3efb4 ("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: b439984641 ("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>
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>
The regular expression I used when relicensing to GPLv2+ missed this.
Fixes: ca2749e1bd ("passt: Relicense to GPL 2.0, or any later version")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
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>
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>
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>
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>
>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>
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>
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>
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>
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>
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>
