| Message ID | alpine.DEB.2.00.1007191319010.13002@wel-95.cs.helsinki.fi |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Le lundi 19 juillet 2010 à 14:16 +0300, Ilpo Järvinen a écrit : > Thanks for testing. > > DaveM, I think this oops was introduced for 2.6.28 (in > 08ebd1721ab8fd362e90ae17b461c07b23fa2824 it seems, to be exact) so to > stables it should go too please. I've only tweaked the message (so no need > for Lennart to retest v2 :-)). > > -- > [PATCHv2] tcp: fix crash in tcp_xmit_retransmit_queue > > It can happen that there are no packets in queue while calling > tcp_xmit_retransmit_queue(). tcp_write_queue_head() then returns > NULL and that gets deref'ed to get sacked into a local var. > > There is no work to do if no packets are outstanding so we just > exit early. > > This oops was introduced by 08ebd1721ab8fd (tcp: remove tp->lost_out > guard to make joining diff nicer). > But prior to commit 08ebd1721ab8fd3, we were not testing tp->packets_out, but tp->lost_out if it was 0, we were not doing the tcp_for_write_queue_from() loop. Not sure it makes a difference ? > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> > Reported-by: Lennart Schulte <lennart.schulte@nets.rwth-aachen.de> > Tested-by: Lennart Schulte <lennart.schulte@nets.rwth-aachen.de> > --- > net/ipv4/tcp_output.c | 3 +++ > 1 files changed, 3 insertions(+), 0 deletions(-) > > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c > index b4ed957..7ed9dc1 100644 > --- a/net/ipv4/tcp_output.c > +++ b/net/ipv4/tcp_output.c > @@ -2208,6 +2208,9 @@ void tcp_xmit_retransmit_queue(struct sock *sk) > int mib_idx; > int fwd_rexmitting = 0; > > + if (!tp->packets_out) > + return; > + > if (!tp->lost_out) > tp->retransmit_high = tp->snd_una; > -- 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 Mon, 19 Jul 2010, Eric Dumazet wrote: > Le lundi 19 juillet 2010 à 14:16 +0300, Ilpo Järvinen a écrit : > > > Thanks for testing. > > > > DaveM, I think this oops was introduced for 2.6.28 (in > > 08ebd1721ab8fd362e90ae17b461c07b23fa2824 it seems, to be exact) so to > > stables it should go too please. I've only tweaked the message (so no need > > for Lennart to retest v2 :-)). > > > > -- > > [PATCHv2] tcp: fix crash in tcp_xmit_retransmit_queue > > > > It can happen that there are no packets in queue while calling > > tcp_xmit_retransmit_queue(). tcp_write_queue_head() then returns > > NULL and that gets deref'ed to get sacked into a local var. > > > > There is no work to do if no packets are outstanding so we just > > exit early. > > > > This oops was introduced by 08ebd1721ab8fd (tcp: remove tp->lost_out > > guard to make joining diff nicer). > > > > But prior to commit 08ebd1721ab8fd3, we were not testing > tp->packets_out, but tp->lost_out That's right, but back then we were not testing it for the same purpose. > if it was 0, we were not doing the tcp_for_write_queue_from() loop. This invariant _should_ be true all the time: lost_out <= packets_out ...and if it's not we would get Leak printouts every now and then. Thus is packets_out is zero no NULL defer with the if lost_out either. The other loop too (in pre 08eb kernels) will work because of earlier mentioned send_head check side-effects. > Not sure it makes a difference ? This difference is well thought and intentional, I didn't use different one by accident. We want to make sure we won't use NULL from tcp_write_queue_head() while the pre 08ebd1721ab8fd3 kernels was interested mainly whether the first loop should run or not (and of course ends up avoid the null deref too but it's more optimization like thing in there, ie., if there's no lost packets no work to-do). The deref could have been fixed by moving TCP_SKB_CB(skb)->sacked a bit later but that would again make us depend on the side-effect of the send_head check (in the case of packets_out being zero and wq empty) which is something I don't like too much.
Le lundi 19 juillet 2010 à 20:25 +0300, Ilpo Järvinen a écrit : > This difference is well thought and intentional, I didn't use different > one by accident. We want to make sure we won't use NULL from > tcp_write_queue_head() while the pre 08ebd1721ab8fd3 kernels was > interested mainly whether the first loop should run or not (and of course > ends up avoid the null deref too but it's more optimization like > thing in there, ie., if there's no lost packets no work to-do). The deref > could have been fixed by moving TCP_SKB_CB(skb)->sacked a bit later but > that would again make us depend on the side-effect of the send_head check > (in the case of packets_out being zero and wq empty) which is something I > don't like too much. > Thanks Ilpo. Do you know in what exact circumstance the bug triggers ? It's hard to believe thousand of machines on the Internet never hit it :( Maybe another problem in congestion control ? -- 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
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Mon, 19 Jul 2010 19:39:08 +0200 > Do you know in what exact circumstance the bug triggers ? > > It's hard to believe thousand of machines on the Internet never hit > it :( > > Maybe another problem in congestion control ? This is something to investigate, but the conditions under which tcp_fastretrans_alert() (the main invoker of tcp_xmit_retransmit_queue()) does it's thing are complicated enough that I'm going to add this fix for the time being and push it out to stable too. Thanks everyone. -- 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 Mon, 19 Jul 2010, David Miller wrote: > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Mon, 19 Jul 2010 19:39:08 +0200 > > > Do you know in what exact circumstance the bug triggers ? > > > > It's hard to believe thousand of machines on the Internet never hit > > it :( > > > > Maybe another problem in congestion control ? > > This is something to investigate, but the conditions under which > tcp_fastretrans_alert() (the main invoker of tcp_xmit_retransmit_queue()) > does it's thing are complicated enough that I'm going to add this fix > for the time being and push it out to stable too. This is so true. ...So far I've managed to twice rule out of the possibility of this being really triggerable (ie., it would mean Lennart's out of tree changes broke it), and once in the middle came into opposite conclusion. Thus by majority voting we can deduce that it won't happen - how reassuring :-/. It seems that tcp_try_undo_recovery causes return if TCP remained in CA_Loss/CA_Recovery and that tcp_time_to_recover won't really let past return either under normal circumstances (more details below), and tcp_simple_retransmit requires lost_out to change; seems safe in mainline to me. Hmm... It seems that I've just solved another report too. ...Somebody a while back found out that setting reordering sysctl to zero (ie. to a value which does not make too much sense) crashed the kernel. It seems that at least then tcp_time_to_recover() would return true and trigger this bug (though I'm not sure if that's the only breakage to happen). Also worth to keep in mind is the bugzilla entry ("New freez in TCP" or something like that) so I'm not really sure I could say for sure nobody never hit it. The bugzilla one goes away by disable SACK (at least for some) but it might mix two different issues. It seems that there really are two different issues, the other may have something to do with SACK though there are other variables then involved, e.g., the changes in retransmission logic/timing, so it's impossible to say if the SACK disable really "fixed" the bugzilla one or not. Also Tejun's ->next == NULL finding points out to a different bug than this Lennart's one.
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c index b4ed957..7ed9dc1 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c @@ -2208,6 +2208,9 @@ void tcp_xmit_retransmit_queue(struct sock *sk) int mib_idx; int fwd_rexmitting = 0; + if (!tp->packets_out) + return; + if (!tp->lost_out) tp->retransmit_high = tp->snd_una;