* tcp_use_frto crashes on empty tcp_write_queue
@ 2013-02-13 21:06 Zoltan Kiss
2013-02-13 21:10 ` Zoltan Kiss
0 siblings, 1 reply; 6+ messages in thread
From: Zoltan Kiss @ 2013-02-13 21:06 UTC (permalink / raw)
To: netfilter-devel@vger.kernel.org; +Cc: Frediano Ziglio
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
--- a/net/ipv4/tcp_timer.c Wed Feb 13 15:02:50 2013 +0000
+++ b/net/ipv4/tcp_timer.c Wed Feb 13 15:03:18 2013 +0000
@@ -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
^ permalink raw reply [flat|nested] 6+ messages in thread
* tcp_use_frto crashes on empty tcp_write_queue
@ 2013-02-13 21:09 Zoltan Kiss
2013-02-13 21:37 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Zoltan Kiss @ 2013-02-13 21:09 UTC (permalink / raw)
To: netdev; +Cc: Frediano Ziglio
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
--- a/net/ipv4/tcp_timer.c Wed Feb 13 15:02:50 2013 +0000
+++ b/net/ipv4/tcp_timer.c Wed Feb 13 15:03:18 2013 +0000
@@ -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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: tcp_use_frto crashes on empty tcp_write_queue
2013-02-13 21:06 tcp_use_frto crashes on empty tcp_write_queue Zoltan Kiss
@ 2013-02-13 21:10 ` Zoltan Kiss
0 siblings, 0 replies; 6+ messages in thread
From: Zoltan Kiss @ 2013-02-13 21:10 UTC (permalink / raw)
To: netfilter-devel@vger.kernel.org
Apologies, I've sent this to the wrong list!
On 13/02/13 21:06, Zoltan Kiss wrote:
> 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()
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: tcp_use_frto crashes on empty tcp_write_queue
2013-02-13 21:09 Zoltan Kiss
@ 2013-02-13 21:37 ` Eric Dumazet
2013-02-13 22:14 ` Zoltan Kiss
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2013-02-13 21:37 UTC (permalink / raw)
To: Zoltan Kiss; +Cc: netdev, Frediano Ziglio
On Wed, 2013-02-13 at 21:09 +0000, Zoltan Kiss wrote:
> 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
> --- a/net/ipv4/tcp_timer.c Wed Feb 13 15:02:50 2013 +0000
> +++ b/net/ipv4/tcp_timer.c Wed Feb 13 15:03:18 2013 +0000
> @@ -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
This doesn't seem to be a standard kernel.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: tcp_use_frto crashes on empty tcp_write_queue
2013-02-13 21:37 ` Eric Dumazet
@ 2013-02-13 22:14 ` Zoltan Kiss
2013-02-13 23:24 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Zoltan Kiss @ 2013-02-13 22:14 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev@vger.kernel.org, Frediano Ziglio
Hi,
On 13/02/13 21:37, Eric Dumazet wrote:
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: tcp_use_frto crashes on empty tcp_write_queue
2013-02-13 22:14 ` Zoltan Kiss
@ 2013-02-13 23:24 ` Eric Dumazet
0 siblings, 0 replies; 6+ messages in thread
From: Eric Dumazet @ 2013-02-13 23:24 UTC (permalink / raw)
To: Zoltan Kiss; +Cc: netdev@vger.kernel.org, Frediano Ziglio
On Wed, 2013-02-13 at 22:14 +0000, Zoltan Kiss wrote:
> Hi,
>
> On 13/02/13 21:37, Eric Dumazet wrote:
> > 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()?
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 ?
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-02-13 23:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-02-13 21:06 tcp_use_frto crashes on empty tcp_write_queue Zoltan Kiss
2013-02-13 21:10 ` Zoltan Kiss
-- strict thread matches above, loose matches on Subject: below --
2013-02-13 21:09 Zoltan Kiss
2013-02-13 21:37 ` Eric Dumazet
2013-02-13 22:14 ` Zoltan Kiss
2013-02-13 23:24 ` Eric Dumazet
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.