Message ID | 1356129948.21834.8002.camel@edumazet-glaptop |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
On Fri, Dec 21, 2012 at 5:45 PM, Eric Dumazet <erdnetdev@gmail.com> wrote: > On Fri, 2012-12-21 at 14:49 -0500, Zhiyun Qian wrote: > >> If I am not mistaken, line 6142 in kernel v3.7.1 corresponds to >> tcp_rcv_state_process(). According to the comments, "This function >> implements the receiving procedure of RFC 793 for all states except >> ESTABLISHED and TIME_WAIT." Are you referring to a different kernel >> version? > > You are not mistaken, it seems code is too permissive. > > We should reject a frame without ACK flag while in ESTABLISHED state. > > Thats explicitly stated in RFC 973. > > Then we should make all possible safety checks before even sending a > frame or changing socket variables. I completely agree! > > (For instance the tests done in tcp_ack() should be done before calling > tcp_validate_incoming()) > It seems that it is not straightforward to simply move tcp_ack() before tcp_validate_incoming() as tcp_ack() currently assumes the tcp sequence number is already validated and it may adjust certain states purely depending on the ACK number. I guess the solution is to extract all safety checks out and put at the very beginning. The rest of the code in tcp_validate_incoming() and tcp_ack() may still need to perform the redundant checks since if some state changes are dependent on the sequence number or ACK number (e.g., window update). I'm willing to help on this. Perhaps I can draft an initial patch and you can help take a look before I submit it? > John Dykstra in commit 96e0bf4b5193d0 (tcp: Discard segments that ack > data not yet sent) did a step into right direction, but missed this. > > Current code assumes the incoming frame is mostly legitimate. > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > index a136925..2ea4937 100644 > --- a/net/ipv4/tcp_input.c > +++ b/net/ipv4/tcp_input.c > @@ -5551,7 +5551,7 @@ slow_path: > return 0; > > step5: > - if (th->ack && tcp_ack(sk, skb, FLAG_SLOWPATH) < 0) > + if (!th->ack || tcp_ack(sk, skb, FLAG_SLOWPATH) < 0) > goto discard; > > /* ts_recent update must be made after we are sure that the packet > > Neat change. This should enforce the ACK flag and ACK number check for every packet received in established state. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2012-12-21 at 18:52 -0500, Zhiyun Qian wrote: > It seems that it is not straightforward to simply move tcp_ack() > before tcp_validate_incoming() as tcp_ack() currently assumes the tcp > sequence number is already validated and it may adjust certain states > purely depending on the ACK number. I guess the solution is to extract > all safety checks out and put at the very beginning. The rest of the > code in tcp_validate_incoming() and tcp_ack() may still need to > perform the redundant checks since if some state changes are dependent > on the sequence number or ACK number (e.g., window update). > > I'm willing to help on this. Perhaps I can draft an initial patch and > you can help take a look before I submit it? I began coding a serie of patches. Part of the problem comes from a 'fast path' idea that was nice 20 years ago but no longer a real killer these days. So some tests are duplicated, others are not correctly done. For example, the fast path misses the more complete tests done in tcp_ack() Its a big mess. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2012-12-21 at 16:01 -0800, Eric Dumazet wrote:
> I began coding a serie of patches.
My goal is to make very small patches, to ease code review, and
eventually come to a state where we have more factorized code.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
I see. If you need any help, just let me know! E.g., I can definitely help review the patches. -Zhiyun On Fri, Dec 21, 2012 at 7:04 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Fri, 2012-12-21 at 16:01 -0800, Eric Dumazet wrote: > >> I began coding a serie of patches. > > My goal is to make very small patches, to ease code review, and > eventually come to a state where we have more factorized code. > > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index a136925..2ea4937 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -5551,7 +5551,7 @@ slow_path: return 0; step5: - if (th->ack && tcp_ack(sk, skb, FLAG_SLOWPATH) < 0) + if (!th->ack || tcp_ack(sk, skb, FLAG_SLOWPATH) < 0) goto discard; /* ts_recent update must be made after we are sure that the packet