From: Andrew Deason <adeason@sinenomine.net>
Date: Wed, 28 Aug 2019 22:12:53 +0000 (-0500)
Subject: rx: Introduce ack_is_valid
X-Git-Tag: openafs-devel-1_9_1~54
X-Git-Url: http://git.openafs.org/?p=openafs.git;a=commitdiff_plain;h=f6490629e1239c412002f316804c656c9be61400

rx: Introduce ack_is_valid

Take some of our existing logic for ignoring invalid ACK packets and
split it out into a separate function, ack_is_valid. This just makes
it easier to add more complex logic in here and write longer comments
explaining the decisions.

Note that the bug mentioned regarding the previousPacket field was
introduced in IBM AFS 3.5, and was fixed in OpenAFS in commit bbf92017
(rx: rxi_ReceiveDataPacket do not set rprev on drop), included in
OpenAFS 1.6.23.

This commit incurs no functional change; it is just code
reorganization.

Change-Id: Idd569c6bc0c475e700935cf86780a04ab24102f4
Reviewed-on: https://gerrit.openafs.org/13874
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 73fb9c8..76d7d69 100644
--- a/src/rx/rx.c
+++ b/src/rx/rx.c
@@ -4249,6 +4249,41 @@ rx_ack_reason(int reason)
 }
 #endif
 
+static_inline int
+ack_is_valid(struct rx_call *call, afs_uint32 first, afs_uint32 prev)
+{
+    if (first < call->tfirst) {
+	/*
+	 * The peer indicated that the window went backwards. That's not
+	 * allowed; the window can only move forwards.
+	 */
+	return 0;
+    }
+
+    if (first == call->tfirst && prev < call->tprev) {
+	/*
+	 * The peer said the last DATA packet it received was seq X, but it
+	 * already told us before that it had received data after X. This is
+	 * probably just an out-of-order ACK, and so we can ignore it.
+	 */
+	if (prev >= call->tfirst + call->twind) {
+	    /*
+	     * Some peers (OpenAFS libafs before 1.6.23) mistakenly set the
+	     * previousPacket field to a serial number, not a sequence number.
+	     * The sequence number the peer told us about is further than our
+	     * transmit window, so it cannot possibly be correct; it's probably
+	     * actually a serial number. Don't ignore packets based on this;
+	     * the previousPacket information is not accurate.
+	     */
+	    return 1;
+	}
+
+	return 0;
+    }
+
+    /* Otherwise, the ack looks valid. */
+    return 1;
+}
 
 /* The real smarts of the whole thing.  */
 static struct rx_packet *
@@ -4287,14 +4322,7 @@ rxi_ReceiveAckPacket(struct rx_call *call, struct rx_packet *np,
     prev = ntohl(ap->previousPacket);
     serial = ntohl(ap->serial);
 
-    /*
-     * Ignore ack packets received out of order while protecting
-     * against peers that set the previousPacket field to a packet
-     * serial number instead of a sequence number.
-     */
-    if (first < call->tfirst ||
-        (first == call->tfirst && prev < call->tprev && prev < call->tfirst
-	 + call->twind)) {
+    if (!ack_is_valid(call, first, prev)) {
 	return np;
     }