OpenAFS Master Repository branch, master, updated. openafs-devel-1_9_0-82-g7b20494
Gerrit Code Review
Fri, 18 Dec 2020 13:25:26 -0500
The following commit has been merged in the master branch:
Author: Andrew Deason <firstname.lastname@example.org>
Date: Tue Aug 27 22:58:23 2019 -0500
rx: Avoid lastReceiveTime update for invalid ACKs
Currently, we ignore ACK packets in a few cases:
- If the ACK appears to move the window backwards (if firstPacket is
smaller than call->tfirst).
- If the ACK appears to have been received out of order (if
previousPacket is smaller than call->tprev).
- If the ACK packet appears truncated.
In all of these cases, we ignore the ACK packet completely in our ACK
processing code (rxi_ReceiveAckPacket), but we still process the
packet at higher levels (rxi_ReceivePacket). Notably, this means we
update call->lastReceiveTime after rxi_ReceiveAckPacket returns, even
for ACK packets we haven't really looked at.
Normally this does not cause any noticeable problems, because such
packets should either never be encountered, or only consist of a small
number of packets that are mixed in with valid packets.
However, if our peer is a server, and it is restarted in the middle of
a call, our peer may exclusively send us packets that fall into the
above categories. (This does not happen if our peer is a client,
because clients just ignore packets for calls they do not recognize.)
Consider a call where a client is sending data to a server, and the
server restarts after the client has sent a DATA packet with sequence
number 1000. The server may then start responding to the client with ACKs
with firstPacket set to 1, since the restarted server has no knowledge
of the call's state.
In this case, a firstPacket of 1 is well below where our window was,
so all of the ACKs from the server are ignored. But we keep updating
call->lastReceiveTime for all of these packets, and so the call stays
alive forever until an idle-dead or hard-dead timeout activates (if
any are set).
As another example, consider the case where a client is sending data
to a server, and the server receives a full window of packets (say, 16
packets), has not yet passed any data to the application yet, and the
server restarts. The restarted server then starts responding to the
client with ACKs with firstPacket set to 1, and previousPacket set to
0. We also ignore all of the ACKs from the server in this case,
because even though firstPacket looks sane, it looks like
previousPacket has gone backwards. We still update
call->lastReceiveTime for each ignored ACK we get, keeping the call
Before commit 4e71409f (Rx: Reject out of order ACK packets) was
introduced in 1.6.0, neither of these issues could occur. That commit
introduced the issue specifically if previousPacket goes backwards;
that is, if the server restarts before firstPacket moves forwards.
Commit 8d359e6d (rx: Remove duplicate out of order ACK check) in 1.8.0
introduced the issue when 'firstPacket' goes backwards, since
previously the FIRSTACKOFFSET-based check caused us to ignore those
packets without updating call->lastReceiveTime. That is, if the server
restarts after firstPacket moves forwards.
In this commit, we still ignore packets in the above cases, but we
also avoid updating lastReceiveTime when we update such packets, to
make sure that we do not keep a call alive solely from receiving these
Alternatively, we could change our logic to immediately abort calls
where firstPacket moves backwards (since this violates the Rx
protocol), or to not ignore some packets where previousPacket goes
backwards (since these calls may be recoverable). And we could also
skip updating lastReceiveTime for invalid packets of other types. But
for now, this commit just avoids updating lastReceiveTime for invalid
ACK packets, in order to just try to restore our behavior before
1.6.0, while still retaining the benefits of ignoring out-of-order
ACKs. Further changes in this area can potentially be handled
separately by future commits.
Also increment the spuriousPacketsRead counter for packets that we
ignore in this way (which we used to do for some packets before commit
8d359e6d), so we are not entirely silent about ignoring them.
Written in collaboration with email@example.com.
Tested-by: BuildBot <firstname.lastname@example.org>
Reviewed-by: Cheyenne Wills <email@example.com>
Reviewed-by: Benjamin Kaduk <firstname.lastname@example.org>
src/rx/rx.c | 26 ++++++++++++++++++--------
1 files changed, 18 insertions(+), 8 deletions(-)
OpenAFS Master Repository