This is basically a rename that now calls an apple an apple,
except user code can no longer change it. It's not obvious if
user code getting the socket handle from the socket is very useful,
but it's not harmful either, so why not.
The previous implementation made no sense. It is obvious that
poll_at() should use the same mechanisms to decide whether dispatch()
should be called as dispatch() itself uses to decide whether to send
anything.
This fixes numerous busy looping issues that arise if the return
value of poll() is used for waiting.
1. Apart from non-empty transmit buffer, a state which transmits
a FIN flag should also be considerd. Otherwise, closing a socket
with an empty transmit buffer may retransmit the FIN flag forever.
2. Timeout poll requests should only be overridden by timer poll
requests when the latter is earlier.
After this change, if an 1s timeout is set on a socket that received
no packet for 2s, it will be instantly aborted, which seems
more reasonable. Also, this makes the `self.remote_last_ts.is_none()`
branch in TcpSocket::dispatch() actually behave as described.
This also makes TcpSocket::{send,recv}_slice slightly more efficient
in case when the slice wraps around the corresponding buffer,
halving the necessary amount of calls.
This is prohibited by RFC 1122. Instead, aggregate them, and respond
once in the egress function.
However, still instantly send challenge ACKs to trigger fast
retransmit.
Commit 21282fdb was not completely sufficient because e.g. receiving
one octet and then blocking meant that an ACK with window length
of one is then sent, and this isn't motivating the other TCP stacks
all that much.
In any case, preemptively notifying the peer of window size increase
can only be beneficial, modulo network congestion.
Zero is a valid sequence number, treating it as an absence of value
isn't any good. This is unlikely to cause any real harm but it
just isn't good practice, nor does it make for understandable code.
Before this commit, if the amount of data in the buffer caused it
to be split among many outgoing packets, and retransmit timer
was active, the socket would behave very erratically and flood
the peer.
Before this commit, anything that touched RawSocket or TapInterface
worked partly by accident and partly because of a horrible crutch
that resulted in massive latencies as well as inevitable packet loss
every time an ARP request had to be issued. Also, there was no way
to use poll() other than by continuously calling it in a busy loop.
After this commit, poll() indicates when the earliest timer expires,
and so the caller can sleep until that moment (or until packets
arrive).
Note that there is a subtle problem remaining: every time poll()
is called, every socket with a pending outbound packet whose
IP address doesn't correspond to a MAC address will send a new
ARP request, resulting in potentially a whole lot of such requests.
ARP rate limiting is a separate topic though.
We don't transmit anything in FIN-WAIT-2, so we don't need the timer
running, or we'll get spurious log messages about retransmissions.
This also makes logic cleaner, although with no functional change.
This is done for simplification. FIN implies PSH, RST doesn't have
any meaning with PSH, and SYN|PSH only makes sense in the context
of TCP Fast Open, in the context of which, any data in the original
SYN already implies PSH.
This commit completely reworks packet dispatch in TCP sockets,
and brings significant improvements to processing as well.
In particular:
* Challenge ACKs now do not reset retransmit timer; instead,
TcpSocket::process directly returns a TcpRepr without altering
any internal state at all.
* Retransmit and close (aka TIME-WAIT) timers are unified
and restructured into a enum that actually matches semantics
of the timers.
* If a packet cannot be emitted, no internal state is changed.
* The dispatch of RST packets in case of connection abort
is brought in line with dispatch of all other packets.
* Packet dispatch now follows a series of steps with clean
separation of concerns, like packet processing:
1. If we should retransmit, update state to assume that
all in-flight packets are lost.
2. Prepare the packet that would be sent next, considering
the in-flight packets, if any.
3. Check if the packet contains anything new, or it's the same
as the one already in flight. If it is, bail.
4. Finalize and try to actually transmit the packet.
If we can't do that, bail.
5. Update the internal state to reflect that the packet
we've just sent is in flight.
The use of this type has several drawbacks:
* It does not allow distinguishing between different error
conditions. In fact, we wrongly conflated some of them
before this commit.
* It does not allow propagation via ? and requires manual use
of map_err, which is especially tiresome for downstream code.
* It prevents us from expanding the set of error conditions
even if right now we have only one.
* It prevents us from blanket using Result<T> everywhere
(a nitpick at most).
Instead, use Result<T, Error> everywhere, and differentiate error
conditions where applicable.
The changes in this commit affect the following scenario:
* Remote end sends octets 1..2, they are delivered and buffered
on local end;
* Remote end sends octets 3..4, they are lost;
* Remote end sends octets 5..6, they are delivered but cannot
be buffered on local end because we don't perform reassembly.
Before this commit, we would silently drop the segment with octets
5..6, relying on retransmission timer on the remote end. This works,
but can result in severe decrease in throughput. After this commit,
we send a duplicate ACK, which may trigger fast retransmit, if
implemented by the congestion control algorithm on the remote end.
This is a form of an uninitialized read bug; although safe it caused
panics. In short, transmit buffers received from the network stack
should be considered uninitialized (in practice they will often
contain previously transmitted packets or parts thereof). Wrapping
them with the only method we had (e.g. Ipv4Packet) treated the buffer
as if it contained a valid incoming packet, which can easily fail
with Error::Truncated.
This commit splits every `fn new(buffer: T) -> Result<Self, Error>`
method on a `Packet` into three smaller ones:
* `fn check_len(&self) -> Result<(), Error>`, purely a validator;
* `fn new(T) -> Self`, purely a wrapper;
* `fn new_checked(T) -> Result<Self, Error>`, a validating wrapper.
This makes it easy to process ingress packets (using `new_checked`),
egress packets (using `new`), and, if needed, maintain the invariants
at any point during packet construction (using `check_len`).
Fixes#17.
This is a conservative bound; if we don't have enough buffers
to receive more than four segments, clearly we shouldn't advertise
our ability to.
It however will only work reliably with exactly one TCP connection
continuously receiving; for two, another window adjustment mechanism
will be needed for reliable reception.
Previously, sockets could get stuck in the CLOSING state, after
the sequence described in the new test_mutual_close_with_data_2.
The root cause was that some state machine transitions got
folded into ACK handling.
Now, all transitions are handled in the same match statement,
and ACK handling is broken up around it.
I haven't realized that a feature `log` with an optional crate
dependency `log` activates that dependency, and added the prefix
to avoid a "clash". This is unnecessary.
Before this commit, IP payload length was calculated by subtracting
the IP header length from the total underlying buffer length, which
fails if the underlying buffer has padding, e.g. like Ethernet
does.
Egor Karavaev
whitequark
Steffen Butzer