| Message ID | 1345380682.5158.201.camel@edumazet-glaptop |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
> Could you test following patch ?
Yes, sure. Thanks for the quick fix! I should have reported it earlier
(so as to get it fixed earlier) once find the bisect will be time consuming..
Please be patient -- it will take 1-2 days to confirm the bug fix.
Thanks,
Fengguang
--
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 Sun, Aug 19, 2012 at 8:51 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: <snip...> > Hi Fengguang, thanks for this report. > > Hmm, this looks like sk_reset_timer() is called on a socket, and timer > triggers _before_ the sock_hold() > > So the timer handler decrements sk_refcnt to 0 and calls sk_free() > > Its probably a bug introduced (or uncovered) by commit 6f458dfb40 (tcp: > improve latencies of timer triggered events) > > I always found sk_reset_timer() a bit racy... > > void sk_reset_timer(struct sock *sk, struct timer_list* timer, > unsigned long expires) > { > if (!mod_timer(timer, expires)) > sock_hold(sk); // MIGHT BE TOO LATE > } > > Following should be safer... > > void sk_reset_timer(struct sock *sk, struct timer_list* timer, > unsigned long expires) > { > sock_hold(sk); > if (mod_timer(timer, expires)) > sock_put(sk); > } > > Could you test following patch ? > > By the way, there is a typo in tcp_delack_timer() > (should use TCP_DELACK_TIMER_DEFERRED instead of > TCP_WRITE_TIMER_DEFERRED) > > > Thanks ! > > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c > index 7678237..6278a11 100644 > --- a/net/ipv4/tcp_ipv4.c > +++ b/net/ipv4/tcp_ipv4.c > @@ -417,10 +417,12 @@ void tcp_v4_err(struct sk_buff *icmp_skb, u32 info) > > if (code == ICMP_FRAG_NEEDED) { /* PMTU discovery (RFC1191) */ > tp->mtu_info = info; > - if (!sock_owned_by_user(sk)) > + if (!sock_owned_by_user(sk)) { > tcp_v4_mtu_reduced(sk); > - else > - set_bit(TCP_MTU_REDUCED_DEFERRED, &tp->tsq_flags); > + } else { > + if (!test_and_set_bit(TCP_MTU_REDUCED_DEFERRED, &tp->tsq_flags)) > + sock_hold(sk); > + } > goto out; > } > > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c > index 20dfd89..d046326 100644 > --- a/net/ipv4/tcp_output.c > +++ b/net/ipv4/tcp_output.c > @@ -910,14 +910,18 @@ void tcp_release_cb(struct sock *sk) > if (flags & (1UL << TCP_TSQ_DEFERRED)) > tcp_tsq_handler(sk); > > - if (flags & (1UL << TCP_WRITE_TIMER_DEFERRED)) > + if (flags & (1UL << TCP_WRITE_TIMER_DEFERRED)) { > tcp_write_timer_handler(sk); > - > - if (flags & (1UL << TCP_DELACK_TIMER_DEFERRED)) > + __sock_put(sk); > + } > + if (flags & (1UL << TCP_DELACK_TIMER_DEFERRED)) { > tcp_delack_timer_handler(sk); > - > - if (flags & (1UL << TCP_MTU_REDUCED_DEFERRED)) > + __sock_put(sk); > + } > + if (flags & (1UL << TCP_MTU_REDUCED_DEFERRED)) { > sk->sk_prot->mtu_reduced(sk); > + __sock_put(sk); > + } > } > EXPORT_SYMBOL(tcp_release_cb); > > diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c > index 6df36ad..b774a03 100644 > --- a/net/ipv4/tcp_timer.c > +++ b/net/ipv4/tcp_timer.c > @@ -252,7 +252,8 @@ static void tcp_delack_timer(unsigned long data) > inet_csk(sk)->icsk_ack.blocked = 1; > NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_DELAYEDACKLOCKED); > /* deleguate our work to tcp_release_cb() */ > - set_bit(TCP_WRITE_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags); > + if (!test_and_set_bit(TCP_DELACK_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags)) > + sock_hold(sk); > } > bh_unlock_sock(sk); > sock_put(sk); > @@ -481,7 +482,8 @@ static void tcp_write_timer(unsigned long data) > tcp_write_timer_handler(sk); Will it still has problem if code goes here without sock_hold(sk)? > } else { > /* deleguate our work to tcp_release_cb() */ > - set_bit(TCP_WRITE_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags); > + if (!test_and_set_bit(TCP_WRITE_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags)) > + sock_hold(sk); > } > bh_unlock_sock(sk); > sock_put(sk); > -- 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
Hello, On Sun, 19 Aug 2012, Eric Dumazet wrote: > Hmm, this looks like sk_reset_timer() is called on a socket, and timer > triggers _before_ the sock_hold() > > So the timer handler decrements sk_refcnt to 0 and calls sk_free() > > Its probably a bug introduced (or uncovered) by commit 6f458dfb40 (tcp: > improve latencies of timer triggered events) > > I always found sk_reset_timer() a bit racy... > > void sk_reset_timer(struct sock *sk, struct timer_list* timer, > unsigned long expires) > { > if (!mod_timer(timer, expires)) > sock_hold(sk); // MIGHT BE TOO LATE > } > > Following should be safer... Above code is fine as long as caller holds reference. Your change for tcp_release_cb looks correct. Also, may be tcp_v4_mtu_reduced is missing a check for TCP_CLOSE state? tcp_v6_mtu_reduced already has such check. > void sk_reset_timer(struct sock *sk, struct timer_list* timer, > unsigned long expires) > { This should not be needed: > sock_hold(sk); > if (mod_timer(timer, expires)) > sock_put(sk); > } Regards -- Julian Anastasov <ja@ssi.bg> -- 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_ipv4.c b/net/ipv4/tcp_ipv4.c index 7678237..6278a11 100644 --- a/net/ipv4/tcp_ipv4.c +++ b/net/ipv4/tcp_ipv4.c @@ -417,10 +417,12 @@ void tcp_v4_err(struct sk_buff *icmp_skb, u32 info) if (code == ICMP_FRAG_NEEDED) { /* PMTU discovery (RFC1191) */ tp->mtu_info = info; - if (!sock_owned_by_user(sk)) + if (!sock_owned_by_user(sk)) { tcp_v4_mtu_reduced(sk); - else - set_bit(TCP_MTU_REDUCED_DEFERRED, &tp->tsq_flags); + } else { + if (!test_and_set_bit(TCP_MTU_REDUCED_DEFERRED, &tp->tsq_flags)) + sock_hold(sk); + } goto out; } diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c index 20dfd89..d046326 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c @@ -910,14 +910,18 @@ void tcp_release_cb(struct sock *sk) if (flags & (1UL << TCP_TSQ_DEFERRED)) tcp_tsq_handler(sk); - if (flags & (1UL << TCP_WRITE_TIMER_DEFERRED)) + if (flags & (1UL << TCP_WRITE_TIMER_DEFERRED)) { tcp_write_timer_handler(sk); - - if (flags & (1UL << TCP_DELACK_TIMER_DEFERRED)) + __sock_put(sk); + } + if (flags & (1UL << TCP_DELACK_TIMER_DEFERRED)) { tcp_delack_timer_handler(sk); - - if (flags & (1UL << TCP_MTU_REDUCED_DEFERRED)) + __sock_put(sk); + } + if (flags & (1UL << TCP_MTU_REDUCED_DEFERRED)) { sk->sk_prot->mtu_reduced(sk); + __sock_put(sk); + } } EXPORT_SYMBOL(tcp_release_cb); diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c index 6df36ad..b774a03 100644 --- a/net/ipv4/tcp_timer.c +++ b/net/ipv4/tcp_timer.c @@ -252,7 +252,8 @@ static void tcp_delack_timer(unsigned long data) inet_csk(sk)->icsk_ack.blocked = 1; NET_INC_STATS_BH(sock_net(sk), LINUX_MIB_DELAYEDACKLOCKED); /* deleguate our work to tcp_release_cb() */ - set_bit(TCP_WRITE_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags); + if (!test_and_set_bit(TCP_DELACK_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags)) + sock_hold(sk); } bh_unlock_sock(sk); sock_put(sk); @@ -481,7 +482,8 @@ static void tcp_write_timer(unsigned long data) tcp_write_timer_handler(sk); } else { /* deleguate our work to tcp_release_cb() */ - set_bit(TCP_WRITE_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags); + if (!test_and_set_bit(TCP_WRITE_TIMER_DEFERRED, &tcp_sk(sk)->tsq_flags)) + sock_hold(sk); } bh_unlock_sock(sk); sock_put(sk);