Re: [PATCH 2/2] NetworkPkg/TcpDxe: Fix unconditional window shrinking

["Fu, Siyuan" <siyuan.fu@intel.com>]

Hi, Andrey

When the client received such a segment that moves the right edge of send window back to the left said, the client actually have no idea about whether the server is "shrinking the window" or just "reducing window size". Actually, if the server reduced the advertised windows size too much, which takes back his previous permission of sending a number of bytes, from the client side, the server is indeed shrink the receive window. This is discouraged by RFC793, while it says the client must prepare for the peer to do such kind of thing.

Back to the original code, move the SndNxt to the "Right" will let client to retransmit some bytes of data (from Right to SndNxt), but I don't see a condition that it will cause deadlock. While if we adopt this patch, the SndNxt is unchanged when we got a positive usable window, and in the meanwhile, if server did shrink the window and dropped the data from Right to SndNxt, this piece of data won't be retransmit by the client anymore, this is actually a deadlock.

So please provide more details if you did observe a deadlock, maybe an example would help to understand the problem.

Thanks,
Siyuan

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of atepin@kraftway.ru
Sent: 2017年3月28日 15:20
To: edk2-devel@lists.01.org
Subject: [edk2] [PATCH 2/2] NetworkPkg/TcpDxe: Fix unconditional window shrinking

Moving Right window edge to the left on sender side without additional checks leads to the situation when sender assumes the receiver shrunk its rcv buffer, when, in fact, it only reduced window size. This is a TCP deadlock situation. Receiver ACKs proper segment, while sender discards it for future ACK. Add check for negative usable window to prevent erroneous window shrinking.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Andrey Tepin <atepin@kraftway.ru>
---
 NetworkPkg/TcpDxe/TcpInput.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/NetworkPkg/TcpDxe/TcpInput.c b/NetworkPkg/TcpDxe/TcpInput.c index 04c8a82..11b3eb8 100644
--- a/NetworkPkg/TcpDxe/TcpInput.c
+++ b/NetworkPkg/TcpDxe/TcpInput.c
@@ -738,6 +738,7 @@ TcpInput (
   TCP_SEQNO   Right;
   TCP_SEQNO   Urg;
   UINT16      Checksum;
+  INT32       UsableWnd;
 
   ASSERT ((Version == IP_VERSION_4) || (Version == IP_VERSION_6));
 
@@ -1307,7 +1308,10 @@ TcpInput (
 
       if (TCP_SEQ_LT (Right, Tcb->SndNxt)) {
 
-        Tcb->SndNxt = Right;
+        UsableWnd = Tcb->SndUna + Tcb->SndWnd - Tcb->SndNxt;
+        if (UsableWnd < 0) {
+          Tcb->SndNxt = Right;
+        }
 
         if (Right == Tcb->SndUna) {
 
--
1.9.1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel