| Message ID | 1505840757.29839.77.camel@edumazet-glaptop3.roam.corp.google.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] tcp: fastopen: fix on syn-data transmit failure | expand |
On Tue, Sep 19, 2017 at 10:05 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > From: Eric Dumazet <edumazet@google.com> > > Our recent change exposed a bug in TCP Fastopen Client that syzkaller > found right away [1] > > When we prepare skb with SYN+DATA, we attempt to transmit it, > and we update socket state as if the transmit was a success. > > In socket RTX queue we have two skbs, one with the SYN alone, > and a second one containing the DATA. > > When (malicious) ACK comes in, we now complain that second one had no > skb_mstamp. > > The proper fix is to make sure that if the transmit failed, we do not > pretend we sent the DATA skb, and make it our send_head. > > When 3WHS completes, we can now send the DATA right away, without having > to wait for a timeout. > > [1] > WARNING: CPU: 0 PID: 100189 at net/ipv4/tcp_input.c:3117 tcp_clean_rtx_queue+0x2057/0x2ab0 net/ipv4/tcp_input.c:3117() > > WARN_ON_ONCE(last_ackt == 0); > > Modules linked in: > CPU: 0 PID: 100189 Comm: syz-executor1 Not tainted > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 > 0000000000000000 ffff8800b35cb1d8 ffffffff81cad00d 0000000000000000 > ffffffff828a4347 ffff88009f86c080 ffffffff8316eb20 0000000000000d7f > ffff8800b35cb220 ffffffff812c33c2 ffff8800baad2440 00000009d46575c0 > Call Trace: > [<ffffffff81cad00d>] __dump_stack > [<ffffffff81cad00d>] dump_stack+0xc1/0x124 > [<ffffffff812c33c2>] warn_slowpath_common+0xe2/0x150 > [<ffffffff812c361e>] warn_slowpath_null+0x2e/0x40 > [<ffffffff828a4347>] tcp_clean_rtx_queue+0x2057/0x2ab0 n > [<ffffffff828ae6fd>] tcp_ack+0x151d/0x3930 > [<ffffffff828baa09>] tcp_rcv_state_process+0x1c69/0x4fd0 > [<ffffffff828efb7f>] tcp_v4_do_rcv+0x54f/0x7c0 > [<ffffffff8258aacb>] sk_backlog_rcv > [<ffffffff8258aacb>] __release_sock+0x12b/0x3a0 > [<ffffffff8258ad9e>] release_sock+0x5e/0x1c0 > [<ffffffff8294a785>] inet_wait_for_connect > [<ffffffff8294a785>] __inet_stream_connect+0x545/0xc50 > [<ffffffff82886f08>] tcp_sendmsg_fastopen > [<ffffffff82886f08>] tcp_sendmsg+0x2298/0x35a0 > [<ffffffff82952515>] inet_sendmsg+0xe5/0x520 > [<ffffffff8257152f>] sock_sendmsg_nosec > [<ffffffff8257152f>] sock_sendmsg+0xcf/0x110 > > Fixes: 8c72c65b426b ("tcp: update skb->skb_mstamp more carefully") > Fixes: 783237e8daf1 ("net-tcp: Fast Open client - sending SYN-data") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Reported-by: Dmitry Vyukov <dvyukov@google.com> > Cc: Neal Cardwell <ncardwell@google.com> > Cc: Yuchung Cheng <ycheng@google.com> Acked-by: Yuchung Cheng <ycheng@google.com> Thanks Eric for fixing this. The current arrangement of SYN plus data packet seems to cause more code for error cases. I am wondering a (subsequent) refactoring patch can make it simpler by updating the states after a successful transmission (instead of update and revert). > --- > net/ipv4/tcp_output.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c > index 517d737059d18d8821b65dcdf54d9bb3448784c2..0bc9e46a53696578eb6e911f2f75e6b34c80894f 100644 > --- a/net/ipv4/tcp_output.c > +++ b/net/ipv4/tcp_output.c > @@ -3389,6 +3389,10 @@ static int tcp_send_syn_data(struct sock *sk, struct sk_buff *syn) > goto done; > } > > + /* data was not sent, this is our new send_head */ > + sk->sk_send_head = syn_data; > + tp->packets_out -= tcp_skb_pcount(syn_data); > + > fallback: > /* Send a regular SYN with Fast Open cookie request option */ > if (fo->cookie.len > 0) > @@ -3441,6 +3445,11 @@ int tcp_connect(struct sock *sk) > */ > tp->snd_nxt = tp->write_seq; > tp->pushed_seq = tp->write_seq; > + buff = tcp_send_head(sk); > + if (unlikely(buff)) { > + tp->snd_nxt = TCP_SKB_CB(buff)->seq; > + tp->pushed_seq = TCP_SKB_CB(buff)->seq; > + } > TCP_INC_STATS(sock_net(sk), TCP_MIB_ACTIVEOPENS); > > /* Timer for repeating the SYN until an answer. */ > >
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Tue, 19 Sep 2017 10:05:57 -0700 > From: Eric Dumazet <edumazet@google.com> > > Our recent change exposed a bug in TCP Fastopen Client that syzkaller > found right away [1] > > When we prepare skb with SYN+DATA, we attempt to transmit it, > and we update socket state as if the transmit was a success. > > In socket RTX queue we have two skbs, one with the SYN alone, > and a second one containing the DATA. > > When (malicious) ACK comes in, we now complain that second one had no > skb_mstamp. > > The proper fix is to make sure that if the transmit failed, we do not > pretend we sent the DATA skb, and make it our send_head. > > When 3WHS completes, we can now send the DATA right away, without having > to wait for a timeout. > > [1] ... > Fixes: 8c72c65b426b ("tcp: update skb->skb_mstamp more carefully") > Fixes: 783237e8daf1 ("net-tcp: Fast Open client - sending SYN-data") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Reported-by: Dmitry Vyukov <dvyukov@google.com> Applied, thanks Eric.
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c index 517d737059d18d8821b65dcdf54d9bb3448784c2..0bc9e46a53696578eb6e911f2f75e6b34c80894f 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c @@ -3389,6 +3389,10 @@ static int tcp_send_syn_data(struct sock *sk, struct sk_buff *syn) goto done; } + /* data was not sent, this is our new send_head */ + sk->sk_send_head = syn_data; + tp->packets_out -= tcp_skb_pcount(syn_data); + fallback: /* Send a regular SYN with Fast Open cookie request option */ if (fo->cookie.len > 0) @@ -3441,6 +3445,11 @@ int tcp_connect(struct sock *sk) */ tp->snd_nxt = tp->write_seq; tp->pushed_seq = tp->write_seq; + buff = tcp_send_head(sk); + if (unlikely(buff)) { + tp->snd_nxt = TCP_SKB_CB(buff)->seq; + tp->pushed_seq = TCP_SKB_CB(buff)->seq; + } TCP_INC_STATS(sock_net(sk), TCP_MIB_ACTIVEOPENS); /* Timer for repeating the SYN until an answer. */