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>

diff --git a/src/rx/rx.c b/src/rx/rx.c
index 76d7d69..b008400 100644 (file)
--- a/src/rx/rx.c
+++ b/src/rx/rx.c
@@ -135,7 +135,7 @@ static struct rx_packet
                               struct rx_call **newcallp);
 static struct rx_packet
        *rxi_ReceiveAckPacket(struct rx_call *call, struct rx_packet *np,
-                             int istack);
+                             int istack, int *a_invalid);
 static struct rx_packet
        *rxi_ReceiveResponsePacket(struct rx_connection *conn,
                                   struct rx_packet *np, int istack);
@@ -3405,6 +3405,7 @@ rxi_ReceivePacket(struct rx_packet *np, osi_socket socket,
     struct rx_connection *conn;
     int type;
     int unknownService = 0;
+    int invalid = 0;
 #ifdef RXDEBUG
     char *packetType;
 #endif
@@ -3595,7 +3596,7 @@ rxi_ReceivePacket(struct rx_packet *np, osi_socket socket,
                (void)rxi_SendAck(call, 0, np->header.serial,
                                  RX_ACK_PING_RESPONSE, 1);
        }
-       np = rxi_ReceiveAckPacket(call, np, 1);
+       np = rxi_ReceiveAckPacket(call, np, 1, &invalid);
        break;
     case RX_PACKET_TYPE_ABORT: {
        /* An abort packet: reset the call, passing the error up to the user. */
@@ -3628,11 +3629,16 @@ rxi_ReceivePacket(struct rx_packet *np, osi_socket socket,
        np = rxi_SendCallAbort(call, np, 1, 0);
        break;
     };
-    /* Note when this last legitimate packet was received, for keep-alive
-     * processing.  Note, we delay getting the time until now in the hope that
-     * the packet will be delivered to the user before any get time is required
-     * (if not, then the time won't actually be re-evaluated here). */
-    call->lastReceiveTime = clock_Sec();
+    if (invalid) {
+       if (rx_stats_active)
+           rx_atomic_inc(&rx_stats.spuriousPacketsRead);
+    } else {
+       /*
+        * Note when this last legitimate packet was received, for keep-alive
+        * processing.
+        */
+       call->lastReceiveTime = clock_Sec();
+    }
     MUTEX_EXIT(&call->lock);
     putConnection(conn);
     return np;
@@ -4288,7 +4294,7 @@ ack_is_valid(struct rx_call *call, afs_uint32 first, afs_uint32 prev)
 /* The real smarts of the whole thing.  */
 static struct rx_packet *
 rxi_ReceiveAckPacket(struct rx_call *call, struct rx_packet *np,
-                    int istack)
+                    int istack, int *a_invalid)
 {
     struct rx_ackPacket *ap;
     int nAcks;
@@ -4309,6 +4315,8 @@ rxi_ReceiveAckPacket(struct rx_call *call, struct rx_packet *np,
     int pktsize = 0;            /* Set if we need to update the peer mtu */
     int conn_data_locked = 0;
 
+    *a_invalid = 1;
+
     if (rx_stats_active)
         rx_atomic_inc(&rx_stats.ackPacketsRead);
     ap = (struct rx_ackPacket *)rx_DataOf(np);
@@ -4328,6 +4336,8 @@ rxi_ReceiveAckPacket(struct rx_call *call, struct rx_packet *np,
 
     call->tprev = prev;
 
+    *a_invalid = 0;
+
     if (np->header.flags & RX_SLOW_START_OK) {
        call->flags |= RX_CALL_SLOW_START_OK;
     }