buglwIP - A Lightweight TCP/IP stack - Bugs: bug #49631, Zero window and refused data...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #49631: Zero window and refused data problem

Submitted by:  Dirk Ziegelmeier <dziegel>
Submitted on:  Thu 17 Nov 2016 09:09:16 AM UTC  
 
Category: TCPSeverity: 3 - Normal
Item Group: Faulty BehaviourStatus: Fixed
Privacy: PublicAssigned to: Joel Cunningham <jcunningham>
Open/Closed: ClosedPlanned Release: 2.0.1
lwIP version: git head

(Jump to the original submission Jump to the original submission)

Fri 16 Dec 2016 04:09:38 PM UTC, comment #14:

Latest patch commit in cceea73c3f32f433f3d6b5c8212a8ca1f06cddab

I agree that supporting the the lwip <-> lwip case we start down a path of adding strange, stack dependent checks.

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Fri 16 Dec 2016 02:49:27 PM UTC, comment #13:

I understood the problems with lwip<->lwip. However, I don't know how we could solve this without losing the congestion-like "throttling" feature implied with dropping incoming segments when we have refused_data (send last ACK when announced window is < MSS would fix this, but that would be strange).

Can you push the patch?

Simon Goldschmidt <goldsimon>
Project Administrator
Fri 16 Dec 2016 12:08:48 AM UTC, comment #12:

Updated patch to remove check against tcplen == 1. This will still only trigger on a data segment because of the above check for pcb->refused_data != NULL && tcplen > 0. This is correct so that we aren't sending an empty ACK in response to an ACK/window update from the remote TCP

(file #39245)

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Thu 15 Dec 2016 11:21:59 PM UTC, comment #11:

I'm fine with that. I re-read RFC 793 and it doesn't specify that a zero window probe MUST only be 1 byte. I must have remembered incorrectly:

My only remaining concern was that if LwIP is the sender and it's engage in treating the window as zero rather than splitting the next unsent segment to fit within the remaining window, this logic will not correctly identify the packet as a "zero window probe".

This fix is still an improvement though as it does fix the issue for other stacks which will fully close the receive window

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Thu 15 Dec 2016 12:00:55 PM UTC, comment #10:

This would be the last item for 2.0.1, which I'd like to release to start a phase of shorter bugfixing releases (2.0.0 is nearly 2 months old already, and there have been some more or less critical things done since then).

Everyone OK with sending empty ACK if announced window is zero?

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 14 Dec 2016 02:07:20 PM UTC, comment #9:

Having thought about this, I think we can ignore tcplen and just send an empty ACK if rcv_ann_wnd is zero.

This still does not fix the case where we havfe refused_data without a zero window, but I guess normal retransmission should handle this good enough?

Simon Goldschmidt <goldsimon>
Project Administrator
Sat 10 Dec 2016 12:40:33 AM UTC, comment #8:

I studied the tcp input path and the distance between the refused_data check and the check against receive window. Switching these two is going to be a much more involved and risky due to the amount of logic that separates the two checks.

refused_data is checked very early in tcp_input() and the window check is done much later in tcp_receive():

Instead, I came up with an alternate fix: try to target the refused_data zero window probe case and respond with an empty ACK (containing RCV.NXT) since this is the standards violation

This turned out to be somewhat similar to what the original reporter (Oleg) had suggested, except his patch didn't target zero window probes and just sent an ACK with RCV.NXT for every refused segment

The one caveat my change has is that it won't respond to the 1 byte segment if the sending TCP is LwIP and is running the persist timer because it can't truly fill the window (see fix for bug #49533). This is because the receiving TCP can't identify a zero window probe if rcv_ann_wnd is non-zero. Technically in this contrived case and there is no standards violation because the window is not closed and the 1 byte segment is not a legitimate zero window probe

My patch is a first pass at addressing the standards violation. Oleg, can you try the patch in your port? It should apply to 1.4.1 I believe

(file #39191)

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Thu 08 Dec 2016 11:31:16 PM UTC, comment #7:

Yes, I think I can provide a patch that checks our announced window (instead of actual window) before accepting segments.

Might take me a little while to setup a test environment with LwIP as receiver where the window goes to zero and if the original reporter can test, that would be perfect :)

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Thu 08 Dec 2016 08:26:21 PM UTC, comment #6:

After all, the problem stays the same, wheter we have refused_data or not: having it gets us one segment further and we'll have to ignore (not ACK) the next segment.

You're right we should fix the case where we have (or announced) a zero window (wih or without refused_data) and we're probably good.

Is it OK to assign this to you?

Simon Goldschmidt <goldsimon>
Project Administrator
Thu 08 Dec 2016 04:28:13 PM UTC, comment #5:

If we did the proposed announced window check before the refused_data check, then it wouldn't matter whether refused_data was non-NULL or not because any zero window probe would be responded to with an ACK containing RCV.NEXT and then the data segment dropped. I was suggesting it as a smaller scope fix that addresses the standards violation.

I'm fine with removing the refused_data and switching the order, that seems like the logical ordering that doesn't leave us in this situation where we generated the ACK but failed to buffer the segment

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Tue 06 Dec 2016 01:28:54 PM UTC, comment #4:

> Maybe another way to fix this issue would be to not accept
> data when the advertised window is closed rcv_ann_wnd == 0


I don't know if that wlone would help: we could have refused_data in that case, too, as that is independent of the announced window (application can't accept data).

Back to removing refused_data:
My theory was that if we rewrote the incoming data handling to first deliver the incoming segment and only ACK the data if the application accepted it, this would behave like segment loss due to congestion (in fact, we have some kind of congestion: not on the wire, but in the consuming application).

Of course, we would still have to respond to zero window probes (if our window actually is zero; real or announced) and to segments not containing data.

Simon Goldschmidt <goldsimon>
Project Administrator
Mon 28 Nov 2016 06:12:12 PM UTC, comment #3:

Simon,

I'm not sure that removing refused_data would fix Oleg's problem because the data being dropped isn't a normal data segment, but instead a zero window probe and dropping that will lead to a connection closure (dropping a normal data segment would also lead to connection closure after all retransmissions have been sent and dropped)

If we had the following sequence, we'd still need an ACK containing RCV.NEXT

1. LwIP receiver closes window
2. Application reads < 1 MSS of data (no window update sent, but some space now open)
3. Sender sends 1 byte zero window probe
4. LwIP accepts and ACKs probe (advancing RCV.NEXT by 1)
5. Steps 3/4 repeat until application refuses new 1 byte data
6. LwIP silently drops window probe

Step 6 should instead be, zero-window probe data is not passed to application and ACK containing current RCV.NEXT is sent

Maybe another way to fix this issue would be to not accept data when the advertised window is closed rcv_ann_wnd == 0 (true when 0 <= rcv_wnd < MIN( 1 MSS, TCP_WND / 2)). Then the zero window probe would follow the same path as when rcv_wnd is actually 0. A receiving TCP doesn't have to buffer the probe even if there is window space but the advertised window is still closed

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Thu 24 Nov 2016 08:23:25 AM UTC, comment #2:

To sum it up in short, the correct behaviour would be:

Even if we have refused_data, we should send an ACK if we announce a zero window.

I'm OK with not announcing anything (keep it as is) if we do have refused_data.

In the end, it might be worth thinking about removing refused_data and changing the order of recv-callback vs. ACK so that data that would have been stored to refused_data is NOT ACKed and thus retransmitted later...?

Simon Goldschmidt <goldsimon>
Project Administrator
Wed 23 Nov 2016 08:27:46 PM UTC, comment #1:

From Simon and I discussion in the attached email:

"Joel Cunningham wrote:

> [..] So LwIP is behaving correctly for when the window is < 1 MSS. For wnd > 1 MSS
> (regardless of update threshold) we should be using the current window value in the ACK


Right. We should fix that...

Simon"

After looking at tcp_update_rcv_ann_wnd() in tcp.c, the logic is implemented correctly. If the new window is greater than the smaller of TCP_WND/2 or pcb->mss, we update pcb->rcv_ann_wnd, which is used for the window value reported in the TCP header.

So the original poster (Oleg) must not have been reading more than 1 MSS out of the receive buffer before his issue happened (or maybe this was broken on 1.4.1). LwIP is correct to not update the advertised window until TCP_WND/2 or pcb->mss is available.

I read over the original issue reported and there may be a problem with zero-window probes once refused_data is set. I looked over RFC 793 which includes the zero window probe details:

"When the receiving TCP has a zero window and a segment arrives it must
still send an acknowledgment showing its next expected sequence number
and current window (zero)."

If I'm understanding Oleg correctly, if we are advertising a zero window (whether or not we have some space < pcb->mss) we should be sending an ACK in response to the probe. If we have space in the receive buffer (0 < wnd < pcb->mss) the ACK value would cover the probe's 1 byte if accepted into the buffer, otherwise we would not increment the ACK field. Either way, an ACK should be sent. I've seen both these cases from a Windows host, where the zero window probe was truly ACKed and other cases where the ACK value was not advanced past the probe.

Now if the issue is about refused_data when we aren't advertising a zero window, then I think the standards don't offer guidance because we're in a situation where we can't handle the senders segmentation given our advertised window.

Joel Cunningham <jcunningham>
Project MemberIn charge of this item.
Thu 17 Nov 2016 09:09:16 AM UTC, original submission:

See

http://lwip.100.n7.nabble.com/Zero-window-and-refused-data-problem-td27713.html

Dirk Ziegelmeier <dziegel>
Project Member

 

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by jcunningham (Posted a comment)
  • -unavailable- added by dziegel (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 6 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 16 Dec 2016 04:09:38 PM UTCjcunninghamStatusNone=>Fixed
      Open/ClosedOpen=>Closed
    Fri 16 Dec 2016 12:08:48 AM UTCjcunninghamAttached File-=>Added 0001-bug-49631-handle-zero-window-probe-and-refused_data.patch, #39245
    Sat 10 Dec 2016 12:40:33 AM UTCjcunninghamAttached File-=>Added 0001-First-pass-at-fixing-refused_data-with-zero-window-p.patch, #39191
    Fri 09 Dec 2016 08:30:26 PM UTCdziegelPlanned ReleaseNone=>2.0.1
    Thu 08 Dec 2016 08:26:21 PM UTCgoldsimonAssigned togoldsimon=>jcunningham

    Back to the top


    Powered by Savane 3.1-cleanup1