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

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

Hi, Andrey

I’d like to compare the original code logic and your suggested change:

Original:
Right = Seg->Ack + Seg->Wnd;
...
if (TCP_SEQ_LT (Right, Tcb->SndNxt)) {                      <- check if “Seg->Ack    + Seg->Wnd    – SndNxt < 0”
Change to:
if (TCP_SEQ_LT (Tcb->SndUna + Tcb->SndWnd, Tcb->SndNxt))    <- check if “Tcb->SndUna + Tcb->SndWnd – SndNxt < 0”

For each received segment, TCP driver will set SND.UNA <- SEG.ACK, and SND.WND <- SEG.WND. So the original code is always checking the server advised Right edge and adjust the SndNxt, before apply the new right edge to the Tcb, then the useable window would never become a negative value. While your patch use the TCB’s SndUna and SndWnd, which means it will accept a negative window for the first time, then adjust it when receiving a new segment.

If the TCP client received a series of segment that continuous shrink the receive window, just like the frame 4492-4497, I think above 2 method will get same result, that the SndNxt will move to the left, and send out zero window probe segment. Then I have 2 question here:

1.      The client send out frame 4496 with data Seq to 590994, which already beyond the server’s current receive window (this means the server has advised the right edge large than 590994 in previous packet, but not list here??). But server did acked all the data in frame 4498, this is the first thing that looks very strange to me.

2.      The server re-open the receive window in frame 4520, with ACK=590994. As I mentioned above, the SndNxt has already been moved to the left of 590994, no matter we choose which method to adjust the SndNxt, so this frame will always be ignored and become a deadlock.
So, if the problem is solved by the patch, something must be wrong in my above understanding. Do you also have a sample file to show how the patch could solve the dead lock, especially for the packets after server re-open the receive windows?

Best Regards,
Siyuan

From: Fu, Siyuan
Sent: 2017年4月14日 9:04
To: atepin@kraftway.ru; edk2-devel@lists.01.org
Cc: Ye, Ting <ting.ye@intel.com>; Tian, Feng <feng.tian@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
Subject: RE: [PATCH 2/2] NetworkPkg/TcpDxe: Fix unconditional window shrinking

Hi, Andrey

Thanks, that explains my confusion to the patch.

Per my understanding, to against the useable windows to be a negative window means the code should check the value before assign the new right edge, and reject that change, not to correct it after the useable windows already becomes a negative one.

Could you please help to file a bug in edk2 Bugzilla so we can easily trace it?
https://tianocore.acgmultimedia.com/

Best Regards,
Siyuan

From: atepin@kraftway.ru<mailto:atepin@kraftway.ru> [mailto:atepin@kraftway.ru]
Sent: 2017年4月13日 22:33
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: [PATCH 2/2] NetworkPkg/TcpDxe: Fix unconditional window shrinking


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