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

["atepin@kraftway.ru" <atepin@kraftway.ru>]

Hello, Siyuan

If usable window becomes negative (UsableWnd < 0), we adjust SndNxt (Tcb->SndNxt = Right) and thus, our usable window becomes non-negative (UsableWnd = Tcb->SndUna + Tcb->SndWnd - Right). In other words, we are fixing usable window if it becomes negative and this way we provide robustness.

Examining this code again I realized, that I was wrong about bug here. I was thinking the bug here was not checking usable window, but in line

       if (TCP_SEQ_LT (Right, Tcb->SndNxt)) {     // <---- this line

-        Tcb->SndNxt = Right;
+        UsableWnd = Tcb->SndUna + Tcb->SndWnd - Tcb->SndNxt;
+        if (UsableWnd < 0) {
+          Tcb->SndNxt = Right;
+        }


  usable window is actually being checked. So the bug here is checking it against wrong values. Variable Right is assigned here
          //
          // Update window info
          //
         if (TCP_SEQ_LT (Tcb->SndWl1, Seg->Seq) ||
             ((Tcb->SndWl1 == Seg->Seq) && TCP_SEQ_LEQ (Tcb->SndWl2, Seg->Ack)))
        {

             Right = Seg->Ack + Seg->Wnd;  // <---- this seems to be wrong (see rfc1122 p.99)

using values from Seg structure. I am not sure about other checks where variable Right occurs, but in line

       if (TCP_SEQ_LT (Right, Tcb->SndNxt))

it should be changed to

       if (TCP_SEQ_LT (Tcb->SndUna + Tcb->SndWnd, Tcb->SndNxt))


Regards,
Andrey


On 12.04.2017 13:48, Fu, Siyuan wrote:

Hi, Andrey

Thanks for providing the detail data and now I understand the problem. The TCP driver was designed and implemented according RFC793, so I think it doesn't fully comply with the subsequent update ECR.

While I still have question about your patch, the RFC1122 said that the sending TCP must be robust against the window shrinking, per my understanding, this means we should guarantee the useable windows always be a positive number. But in your patch it actually doing the opposite thing, that make sure the UsableWnd always a negative number. That's why I'm confusing when I see the patch first time.

       if (TCP_SEQ_LT (Right, Tcb->SndNxt)) {

-        Tcb->SndNxt = Right;
+        UsableWnd = Tcb->SndUna + Tcb->SndWnd - Tcb->SndNxt;
+        if (UsableWnd < 0) {                  <-----  why "< 0" here ??
+          Tcb->SndNxt = Right;
+        }

Best Regards,
Siyuan


-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of atepin@kraftway.ru<mailto:atepin@kraftway.ru>
Sent: 2017年4月6日 23:34
To: Fu, Siyuan <siyuan.fu@intel.com><mailto:siyuan.fu@intel.com>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Ye, Ting <ting.ye@intel.com><mailto:ting.ye@intel.com>; Tian, Feng <feng.tian@intel.com><mailto:feng.tian@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com><mailto:jiaxin.wu@intel.com>
Subject: Re: [edk2] [PATCH 2/2] NetworkPkg/TcpDxe: Fix unconditional window shrinking
here
Hello, Siyuan,

I experienced actual deadlock while sending huge amounts of data from uefi to some server (tried Apache and IIS). At some point of transmission server starts to reduce its window size all the way to zero window size. After window update on server (when window size restores back to non-zero value) client starts to drop all packets from server because of future ACK (Seg.Ack > Tcb.SndNxt). This supposedly happened because uefi client mistakenly moved SndNxt to the left one or multiple times. According to RFC 1122 (section 4.2.2.16), when server shrinks rcv buffer, usable window becomes negative.

Below is a part of wireshark's dump of tcp transmission (with non-important parts removed).
In frames 4490-4496 server reduces window size from 24576 to 5623, while ACKing relatively old packets (from 566378 to 585202). Client then sends packet with SEQ 589546 (frame 4496). In frame 4497 server ACKs segment from frame 4489 and in next frame it ACKs segment from frame 4496 with ACK=590994 (which means he accepted all 1448 data bytes from 4496 frame?). But on all zero window size packets (frames 4498-4515) client replies with SEQ 590754 and after window update (frame 4516) we have a deadlock starting from frame 4517.

---

Frame 4489: 1514 bytes on wire (12112 bits), 1514 bytes captured (12112 bits) on interface 0 Transmission Control Protocol, Src Port: 1452 (1452), Dst Port: 80 (80), Seq: 588098, Ack: 26, Len: 1448
    Sequence number: 588098    (relative sequence number)
    [Next sequence number: 589546    (relative sequence number)]
    Acknowledgment number: 26    (relative ack number)
    Window size value: 32767
    [Calculated window size: 2097088]
    [Window size scaling factor: 64]
    TCP segment data (1448 bytes)

Frame 4490: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 80 (80), Dst Port: 1452 (1452), Seq: 26, Ack: 566378, Len: 0
    Sequence number: 26    (relative sequence number)
    Acknowledgment number: 566378    (relative ack number)
    Window size value: 96
    [Calculated window size: 24576]
    [Window size scaling factor: 256]

Frame 4491: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 80 (80), Dst Port: 1452 (1452), Seq: 26, Ack: 570722, Len: 0
    Sequence number: 26    (relative sequence number)
    Acknowledgment number: 570722    (relative ack number)
    Window size value: 79
    [Calculated window size: 20224]
    [Window size scaling factor: 256]

Frame 4492: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 80 (80), Dst Port: 1452 (1452), Seq: 26, Ack: 573618, Len: 0
    Sequence number: 26    (relative sequence number)
    Acknowledgment number: 573618    (relative ack number)
    Window size value: 67
    [Calculated window size: 17152]
    [Window size scaling factor: 256]

Frame 4493: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 80 (80), Dst Port: 1452 (1452), Seq: 26, Ack: 577962, Len: 0
    Sequence number: 26    (relative sequence number)
    Acknowledgment number: 577962    (relative ack number)
    Window size value: 50
    [Calculated window size: 12800]
    [Window size scaling factor: 256]

Frame 4494: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 80 (80), Dst Port: 1452 (1452), Seq: 26, Ack: 582306, Len: 0
    Sequence number: 26    (relative sequence number)
    Acknowledgment number: 582306    (relative ack number)
    Window size value: 33
    [Calculated window size: 8448]
    [Window size scaling factor: 256]

Frame 4495: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 80 (80), Dst Port: 1452 (1452), Seq: 26, Ack: 585202, Len: 0
    Sequence number: 26    (relative sequence number)
    Acknowledgment number: 585202    (relative ack number)
    Window size value: 22
    [Calculated window size: 5632]
    [Window size scaling factor: 256]

Frame 4496: 1514 bytes on wire (12112 bits), 1514 bytes captured (12112 bits) on interface 0 Transmission Control Protocol, Src Port: 1452 (1452), Dst Port: 80 (80), Seq: 589546, Ack: 26, Len: 1448
    Sequence number: 589546    (relative sequence number)
    [Next sequence number: 590994    (relative sequence number)]
    Acknowledgment number: 26    (relative ack number)
    Window size value: 32767
    [Calculated window size: 2097088]
    [Window size scaling factor: 64]
    TCP segment data (1448 bytes)

Frame 4497: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 80 (80), Dst Port: 1452 (1452), Seq: 26, Ack: 589546, Len: 0
    Sequence number: 26    (relative sequence number)
    Acknowledgment number: 589546    (relative ack number)
    Window size value: 5
    [Calculated window size: 1280]
    [Window size scaling factor: 256]

Frame 4498: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 80 (80), Dst Port: 1452 (1452), Seq: 26, Ack: 590994, Len: 0
    Sequence number: 26    (relative sequence number)
    Acknowledgment number: 590994    (relative ack number)
    Window size value: 0
    [Calculated window size: 0]
    [Window size scaling factor: 256]

Frame 4500: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 1452 (1452), Dst Port: 80 (80), Seq: 590754, Ack: 26, Len: 0
    Sequence number: 590754    (relative sequence number)
    Acknowledgment number: 26    (relative ack number)
    Window size value: 32767
    [Calculated window size: 2097088]
    [Window size scaling factor: 64]

Frame 4501: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 80 (80), Dst Port: 1452 (1452), Seq: 26, Ack: 590994, Len: 0
    Sequence number: 26    (relative sequence number)
    Acknowledgment number: 590994    (relative ack number)
    Window size value: 0
    [Calculated window size: 0]
    [Window size scaling factor: 256]

Frame 4502: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 1452 (1452), Dst Port: 80 (80), Seq: 590754, Ack: 26, Len: 0
    Sequence number: 590754    (relative sequence number)
    Acknowledgment number: 26    (relative ack number)
    Window size value: 32767
    [Calculated window size: 2097088]
    [Window size scaling factor: 64]

Frame 4503: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 80 (80), Dst Port: 1452 (1452), Seq: 26, Ack: 590994, Len: 0
    Sequence number: 26    (relative sequence number)
    Acknowledgment number: 590994    (relative ack number)
    Window size value: 0
    [Calculated window size: 0]
    [Window size scaling factor: 256]


Frame 4504: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 1452 (1452), Dst Port: 80 (80), Seq: 590754, Ack: 26, Len: 0
    Sequence number: 590754    (relative sequence number)
    Acknowledgment number: 26    (relative ack number)
    Window size value: 32767
    [Calculated window size: 2097088]
    [Window size scaling factor: 64]

Frame 4505: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 80 (80), Dst Port: 1452 (1452), Seq: 26, Ack: 590994, Len: 0
    Sequence number: 26    (relative sequence number)
    Acknowledgment number: 590994    (relative ack number)
    Window size value: 0
    [Calculated window size: 0]
    [Window size scaling factor: 256]

Frame 4506: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 1452 (1452), Dst Port: 80 (80), Seq: 590754, Ack: 26, Len: 0
    Sequence number: 590754    (relative sequence number)
    Acknowledgment number: 26    (relative ack number)
    Window size value: 32767
    [Calculated window size: 2097088]
    [Window size scaling factor: 64]

Frame 4507: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 80 (80), Dst Port: 1452 (1452), Seq: 26, Ack: 590994, Len: 0
    Sequence number: 26    (relative sequence number)
    Acknowledgment number: 590994    (relative ack number)
    Window size value: 0
    [Calculated window size: 0]
    [Window size scaling factor: 256]

Frame 4508: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 1452 (1452), Dst Port: 80 (80), Seq: 590754, Ack: 26, Len: 0
    Sequence number: 590754    (relative sequence number)
    Acknowledgment number: 26    (relative ack number)
    Window size value: 32767
    [Calculated window size: 2097088]
    [Window size scaling factor: 64]

Frame 4509: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 80 (80), Dst Port: 1452 (1452), Seq: 26, Ack: 590994, Len: 0
    Sequence number: 26    (relative sequence number)
    Acknowledgment number: 590994    (relative ack number)
    Window size value: 0
    [Calculated window size: 0]
    [Window size scaling factor: 256]

Frame 4510: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 1452 (1452), Dst Port: 80 (80), Seq: 590754, Ack: 26, Len: 0
    Sequence number: 590754    (relative sequence number)
    Acknowledgment number: 26    (relative ack number)
    Window size value: 32767
    [Calculated window size: 2097088]
    [Window size scaling factor: 64]

Frame 4511: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 80 (80), Dst Port: 1452 (1452), Seq: 26, Ack: 590994, Len: 0
    Sequence number: 26    (relative sequence number)
    Acknowledgment number: 590994    (relative ack number)
    Window size value: 0
    [Calculated window size: 0]
    [Window size scaling factor: 256]

Frame 4512: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 1452 (1452), Dst Port: 80 (80), Seq: 590754, Ack: 26, Len: 0
    Sequence number: 590754    (relative sequence number)
    Acknowledgment number: 26    (relative ack number)
    Window size value: 32767
    [Calculated window size: 2097088]
    [Window size scaling factor: 64]

Frame 4513: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 80 (80), Dst Port: 1452 (1452), Seq: 26, Ack: 590994, Len: 0
    Sequence number: 26    (relative sequence number)
    Acknowledgment number: 590994    (relative ack number)
    Window size value: 0
    [Calculated window size: 0]
    [Window size scaling factor: 256]

Frame 4514: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 1452 (1452), Dst Port: 80 (80), Seq: 590754, Ack: 26, Len: 0
    Sequence number: 590754    (relative sequence number)
    Acknowledgment number: 26    (relative ack number)
    Window size value: 32767
    [Calculated window size: 2097088]
    [Window size scaling factor: 64]

Frame 4515: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 80 (80), Dst Port: 1452 (1452), Seq: 26, Ack: 590994, Len: 0
    Sequence number: 26    (relative sequence number)
    Acknowledgment number: 590994    (relative ack number)
    Window size value: 0
    [Calculated window size: 0]
    [Window size scaling factor: 256]

Frame 4516: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 80 (80), Dst Port: 1452 (1452), Seq: 26, Ack: 590994, Len: 0
    Sequence number: 26    (relative sequence number)
    Acknowledgment number: 590994    (relative ack number)
    Window size value: 260
    [Calculated window size: 66560]
    [Window size scaling factor: 256]

Frame 4517: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 1452 (1452), Dst Port: 80 (80), Seq: 590754, Ack: 26, Len: 0
    Sequence number: 590754    (relative sequence number)
    Acknowledgment number: 26    (relative ack number)
    Window size value: 32767
    [Calculated window size: 2097088]
    [Window size scaling factor: 64]

Frame 4518: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 1452 (1452), Dst Port: 80 (80), Seq: 590754, Ack: 26, Len: 0
    Sequence number: 590754    (relative sequence number)
    Acknowledgment number: 26    (relative ack number)
    Window size value: 32767
    [Calculated window size: 2097088]
    [Window size scaling factor: 64]

Frame 4520: 66 bytes on wire (528 bits), 66 bytes captured (528 bits) on interface 0 Transmission Control Protocol, Src Port: 80 (80), Dst Port: 1452 (1452), Seq: 26, Ack: 590994, Len: 0
    Sequence number: 26    (relative sequence number)
    Acknowledgment number: 590994    (relative ack number)
    Window size value: 260
    [Calculated window size: 66560]
    [Window size scaling factor: 256]

Regards,
Andrey



On 05.04.2017 12:47, Fu, Siyuan wrote:

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<mailto:atepin@kraftway.ru><mailto:atepin@kraftway.ru><mailto:atepin@kraftway.ru>
Sent: 2017年3月28日 15:20
To: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org><mailto:edk2-devel@lists.01.org><mailto: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><mailto:atepin@kraftway.ru><mailto:atepin@kraftway.ru><mailto: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<mailto:edk2-devel@lists.01.org><mailto:edk2-devel@lists.01.org><mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel