Skip to contentSkip to conversation

tcp_use_frto crashes on empty tcp_write_queue

PATCH Zoltan Kiss 2 participants 4 messages Feb 13, 2013

Subsystems (3)
Networking Network drivers TCP
lore
Share & tools
lore

Zoltan Kiss

author
#0 lore raw

Hi,

I see the following WARN and then crash on 2.6.32.12:

<4>WARNING: at net/ipv4/tcp_timer.c:293 tcp_retransmit_timer+0x5dd/0x630()

...

<1>BUG: unable to handle kernel NULL pointer dereference at (null) <1>IP: [<c033f2b5>] tcp_use_frto+0x45/0x90
...
<0>Call Trace:
<4> [<c0343ab9>] ? tcp_retransmit_timer+0xd9/0x630
<4> [<c0120e58>] ? __wake_up_common+0x48/0x70
<4> [<c0344580>] ? tcp_write_timer+0xe0/0x1a0
<4> [<c0137fe1>] ? run_timer_softirq+0x151/0x200
<4> [<c02af069>] ? maybe_schedule_tx_action+0x39/0x40 <4> [<c03444a0>] ? tcp_write_timer+0x0/0x1a0
<4> [<c013359a>] ? __do_softirq+0xba/0x180
<4> [<c015e7a7>] ? move_native_irq+0x47/0x50
...

I've checked the code, tcp_use_frto() crashes because

skb = tcp_write_queue_head(sk);

returns a NULL, as the queue is empty, and in the next line:

if (tcp_skb_is_last(sk, skb))
===>
static inline bool tcp_skb_is_last(const struct sock *sk,
const struct sk_buff *skb)

{
    return skb_queue_is_last(&sk->sk_write_queue, skb);
}
===>
static inline bool skb_queue_is_last(const struct sk_buff_head *list,
                     const struct sk_buff *skb)
{
    return (skb->next == (struct sk_buff *) list);
}

That skb->next cause the NULL pointer dereference.

I've checked this in upstream, and it seems this would fail in the same way. Wouldn't it be more reasonable to return from tcp_retransmit_timer() instead of just signing a WARN? Something like this:

net/ipv4/tcp_timer.c Wed Feb 13 15:03:18 2013 +0000 renamed from net/ipv4/tcp_timer.c Wed Feb 13 15:02:50 2013 +0000 +1 -3
diff -r 7a748d2cb9f1 -r bb8257f0730a net/ipv4/tcp_timer.c
@@ -287,11 +287,9 @@ void tcp_retransmit_timer(struct sock *s
     struct tcp_sock *tp = tcp_sk(sk);
     struct inet_connection_sock *icsk = inet_csk(sk);
 
-    if (!tp->packets_out)
+    if (!tp->packets_out || tcp_write_queue_empty(sk))
         goto out;
 
-    WARN_ON(tcp_write_queue_empty(sk));
-
     if (!tp->snd_wnd && !sock_flag(sk, SOCK_DEAD) &&
         !((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV))) {
         /* Receiver dastardly shrinks window. Our retransmits

Credit goes to Frediano for the patch.

Regards,

Zoli

#1 lore raw
Quoted 54 lines · Zoltan Kiss

Hi,

I see the following WARN and then crash on 2.6.32.12:

<4>WARNING: at net/ipv4/tcp_timer.c:293 tcp_retransmit_timer+0x5dd/0x630()

...

<1>BUG: unable to handle kernel NULL pointer dereference at (null) <1>IP: [<c033f2b5>] tcp_use_frto+0x45/0x90
...
<0>Call Trace:
<4> [<c0343ab9>] ? tcp_retransmit_timer+0xd9/0x630
<4> [<c0120e58>] ? __wake_up_common+0x48/0x70
<4> [<c0344580>] ? tcp_write_timer+0xe0/0x1a0
<4> [<c0137fe1>] ? run_timer_softirq+0x151/0x200
<4> [<c02af069>] ? maybe_schedule_tx_action+0x39/0x40 <4> [<c03444a0>] ? tcp_write_timer+0x0/0x1a0
<4> [<c013359a>] ? __do_softirq+0xba/0x180
<4> [<c015e7a7>] ? move_native_irq+0x47/0x50
...

I've checked the code, tcp_use_frto() crashes because

skb = tcp_write_queue_head(sk);

returns a NULL, as the queue is empty, and in the next line:

if (tcp_skb_is_last(sk, skb))
===>
static inline bool tcp_skb_is_last(const struct sock *sk,
const struct sk_buff *skb)

{
     return skb_queue_is_last(&sk->sk_write_queue, skb);
}
===>
static inline bool skb_queue_is_last(const struct sk_buff_head *list,
                      const struct sk_buff *skb)
{
     return (skb->next == (struct sk_buff *) list);
}

That skb->next cause the NULL pointer dereference.

I've checked this in upstream, and it seems this would fail in the same way. Wouldn't it be more reasonable to return from tcp_retransmit_timer() instead of just signing a WARN? Something like this:

diff -r 7a748d2cb9f1 -r bb8257f0730a net/ipv4/tcp_timer.c

net/ipv4/tcp_timer.c Wed Feb 13 15:03:18 2013 +0000 renamed from net/ipv4/tcp_timer.c Wed Feb 13 15:02:50 2013 +0000 +0 -0
@@ -287,11 +287,9 @@ void tcp_retransmit_timer(struct sock *s
      struct tcp_sock *tp = tcp_sk(sk);
      struct inet_connection_sock *icsk = inet_csk(sk);

net/ipv4/tcp_timer.c Wed Feb 13 15:03:18 2013 +0000 void tcp_retransmit_timer(struct sock *s · line 295

Quoted 10 lines
net/ipv4/tcp_timer.c Wed Feb 13 15:03:18 2013 +0000 renamed from net/ipv4/tcp_timer.c Wed Feb 13 15:02:50 2013 +0000 +1 -3
 
-    if (!tp->packets_out)
+    if (!tp->packets_out || tcp_write_queue_empty(sk))
          goto out;
 
-    WARN_ON(tcp_write_queue_empty(sk));
-
      if (!tp->snd_wnd && !sock_flag(sk, SOCK_DEAD) &&
          !((1 << sk->sk_state) & (TCPF_SYN_SENT | TCPF_SYN_RECV))) {
          /* Receiver dastardly shrinks window. Our retransmits

This doesn't seem to be a standard kernel.

author
#2 lore raw

Hi,

Quoted 1 line · Eric Dumazet

This doesn't seem to be a standard kernel.

Indeed, this is a XenServer 6.0.2 kernel, which contains changes, however the TCP parts are barely modified. The related code path were not changed.
I'm trying to determine if this bug could be fixed in an obvious way, so let me rephrase my question: if the socket write queue is empty, shouldn't we just stop going further instead of dropping a WARN? Or, if there is a reason to do so, shouldn't we check at least that returned pointer in tcp_use_frto()?

Regards,

Zoltan Kiss

#3 lore raw
Quoted 5 lines · Zoltan Kiss

Hi,

Quoted 1 line · Eric Dumazet

This doesn't seem to be a standard kernel.

Quoted 8 lines

Indeed, this is a XenServer 6.0.2 kernel, which contains changes, however the TCP parts are barely modified. The related code path were not changed.
I'm trying to determine if this bug could be fixed in an obvious way, so let me rephrase my question: if the socket write queue is empty, shouldn't we just stop going further instead of dropping a WARN? Or, if there is a reason to do so, shouldn't we check at least that returned pointer in tcp_use_frto()?

You could change the WARN_ON() to BUG_ON(), as there is a severe bug in your tree (and possibly in current trees as well, but its hard to say, given 2.6.32 is probably missing some tcp fixes)

Trying to hide the bug wont really help.

How write_queue can be empty and packets_out not null ?