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

Submitter:  Dirk Ziegelmeier <dziegel>
Submitted:  Thu 17 Nov 2016 09:09:16 AM UTC
   
 
Category:  TCP Severity:  3 - Normal
Item Group:  Faulty Behaviour Status:  Fixed
Privacy:  Public Assigned to:  jcunningham
Open/Closed:  Closed Planned Release:  2.0.1
lwIP version:  git head

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>
Group Member
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>
Group 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>
Group Member
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:


The sending TCP must be prepared to accept from the user and send at
least one octet of new data even if the send window is zero.  The
sending TCP must regularly retransmit to the receiving TCP even when
the window is zero.  Two minutes is recommended for the retransmission
interval when the window is zero.  This retransmission is essential to
guarantee that when either TCP has a zero window the re-opening of the
window will be reliably reported to the other.

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).


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>
Group Member
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>
Group 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>
Group 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():


tcp_in.c: 1361
if (TCP_SEQ_BETWEEN(seqno, pcb->rcv_nxt,
                    pcb->rcv_nxt + pcb->rcv_wnd - 1)) {


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>
Group Member
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>
Group Member
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>
Group 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>
Group Member
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>
Group 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>
Group Member
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>
Group 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>
Group Member
Thu 17 Nov 2016 09:09:16 AM UTC, original submission:  
Dirk Ziegelmeier <dziegel>
Group administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-12-16 jcunningham StatusNone Fixed
        Open/ClosedOpen Closed
    2016-12-16 jcunningham Attached File- Added 0001-bug-49631-handle-zero-window-probe-and-refused_data.patch, #39245
    2016-12-10 jcunningham Attached File- Added 0001-First-pass-at-fixing-refused_data-with-zero-window-p.patch, #39191
    2016-12-09 dziegel Planned ReleaseNone 2.0.1
    2016-12-08 goldsimon Assigned togoldsimon jcunningham

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code