OpenAFS Master Repository branch, master, updated. openafs-devel-1_9_0-82-g7b20494

[Gerrit Code Review]

The following commit has been merged in the master branch:
commit 7b204946010673506e0f74991f59a0865292199c
Author: Andrew Deason <adeason@sinenomine.net>
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.)
    For example:
    
    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
    alive.
    
    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
    invalid packets.
    
    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 mvitale@sinenomine.net.
    
    Change-Id: Ibf11bcb2417d481ab80cf4104f2862d1d6502bf4
    Reviewed-on: https://gerrit.openafs.org/13875
    Tested-by: BuildBot <buildbot@rampaginggeek.com>
    Reviewed-by: Cheyenne Wills <cwills@sinenomine.net>
    Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>

 src/rx/rx.c |   26 ++++++++++++++++++--------
 1 files changed, 18 insertions(+), 8 deletions(-)

-- 
OpenAFS Master Repository