oops in tcp_xmit_retransmit_queue() w/ v2.6.32.15

We've been seeing oops in tcp_xmit_retransmit_queue() w/ 2.6.32.15.

Does anything ring a bell?

We've been seeing oops in tcp_xmit_retransmit_queue() w/ 2.6.32.15. Please see the attached photoshoot. This is happening on a HPC cluster and very interestingly caused by one particular job. How long it takes isn't clear yet (at least more than a day) but when it happens it happens on a lot of machines in relatively short time.

With a bit of disassemblying, I've found that the oops is happening during tcp_for_write_queue_from() because the skb->next points to NULL.

void tcp_xmit_retransmit_queue(struct sock *sk)
{
...

	if (tp->retransmit_skb_hint) {
		skb = tp->retransmit_skb_hint;
		last_lost = TCP_SKB_CB(skb)->end_seq;
		if (after(last_lost, tp->retransmit_high))
			last_lost = tp->retransmit_high;
	} else {
		skb = tcp_write_queue_head(sk);
		last_lost = tp->snd_una;
	}

 =>	tcp_for_write_queue_from(skb, sk) {
		 __u8 sacked = TCP_SKB_CB(skb)->sacked;

		 if (skb == tcp_send_head(sk))
			 break;
		 /* we could do better than to assign each time */
		 if (hole == NULL)

This can happen for one of the following reasons,

1. tp->retransmit_skb_hint is NULL and tcp_write_queue_head() is NULL too. ie. tcp_xmit_retransmit_queue() is called on an empty write queue for some reason.

2. tp->retransmit_skb_hint is pointing to a skb which is not on the write_queue. ie. somebody forgot to update hint while removing the skb from the write queue.

3. The hint is pointing to a skb on the list but the list itself is corrupt.

I added some debug code and the crash is happening when tp->retransmit_skb_hint is not NULL but tp->retransmit_skb_hint->next is NULL. So, #1 is out; unfortunately, I didn't have debug code in place to discern between #2 and #3.

Does anything ring a bell? This is a production system and debugging affects quite a number of people. I can put debug code in to discern between #2 and #3 but I'm basically shooting in the dark and it would be great if someone has a better idea.

Quoted 33 lines · Tejun Heo

We've been seeing oops in tcp_xmit_retransmit_queue() w/ 2.6.32.15. Please see the attached photoshoot. This is happening on a HPC cluster and very interestingly caused by one particular job. How long it takes isn't clear yet (at least more than a day) but when it happens it happens on a lot of machines in relatively short time.

With a bit of disassemblying, I've found that the oops is happening during tcp_for_write_queue_from() because the skb->next points to NULL.

void tcp_xmit_retransmit_queue(struct sock *sk)
{
...

	if (tp->retransmit_skb_hint) {
		skb = tp->retransmit_skb_hint;
		last_lost = TCP_SKB_CB(skb)->end_seq;
		if (after(last_lost, tp->retransmit_high))
			last_lost = tp->retransmit_high;
	} else {
		skb = tcp_write_queue_head(sk);
		last_lost = tp->snd_una;
	}

 =>	tcp_for_write_queue_from(skb, sk) {
		 __u8 sacked = TCP_SKB_CB(skb)->sacked;

		 if (skb == tcp_send_head(sk))
			 break;
		 /* we could do better than to assign each time */
		 if (hole == NULL)

This can happen for one of the following reasons,

Quoted 7 lines

1. tp->retransmit_skb_hint is NULL and tcp_write_queue_head() is NULL too. ie. tcp_xmit_retransmit_queue() is called on an empty write queue for some reason.

2. tp->retransmit_skb_hint is pointing to a skb which is not on the write_queue. ie. somebody forgot to update hint while removing the skb from the write queue.

Once again I've read the unlinkers through, and only thing that could cause this is tcp_send_synack (others do deal with the hints) but I think Eric already proposed a patch to that but we never got anywhere due to some counterargument why it wouldn't take place (too far away for me to remember, see archives about the discussions). ...But if you want be dead sure some WARN_ON there might not hurt. Also the purging of the whole queue was a similar suspect I then came across (but that would only materialize with sk reuse happening e.g., with nfs which the other guys weren't using).

Quoted 8 lines

3. The hint is pointing to a skb on the list but the list itself is corrupt.

I added some debug code and the crash is happening when tp->retransmit_skb_hint is not NULL but tp->retransmit_skb_hint->next is NULL. So, #1 is out; unfortunately, I didn't have debug code in place to discern between #2 and #3.

Quoted 4 lines

Does anything ring a bell? This is a production system and debugging affects quite a number of people. I can put debug code in to discern between #2 and #3 but I'm basically shooting in the dark and it would be great if someone has a better idea.

Thanks for taking this up. I've been kind of waiting somebody to show up who actually has some way of reproducing it. Once I had one guy in the hook but his ability to reproduce was for some reason lost when he tried with a debug patch [1].

I now realize that the debug patch should probably also print the write queue too when the problem is caught in order to discern the cases you mention.

Something along these lines:

tcp_for_write_queue(skb, sk) {
	printk("skb %p (%u-%u) next %p prev %p sacked %u\n", ...);
}

Anyway, my debugging patch should be such that in a lucky case it avoids crashing the system too, though price to pay might then be a stuck connection. In case #3 I'd expect the box to die elsewhere in TCP code pretty soon anyway so it depends whether avoiding oops is really so useful, but if you're lucky other mechanism in TCP will recover the lost one for you (basically RTO driven retransmission).

Quoted 43 lines · Ilpo Järvinen

Quoted 33 lines · Tejun Heo

We've been seeing oops in tcp_xmit_retransmit_queue() w/ 2.6.32.15. Please see the attached photoshoot. This is happening on a HPC cluster and very interestingly caused by one particular job. How long it takes isn't clear yet (at least more than a day) but when it happens it happens on a lot of machines in relatively short time.

With a bit of disassemblying, I've found that the oops is happening during tcp_for_write_queue_from() because the skb->next points to NULL.

void tcp_xmit_retransmit_queue(struct sock *sk)
{
...

	if (tp->retransmit_skb_hint) {
		skb = tp->retransmit_skb_hint;
		last_lost = TCP_SKB_CB(skb)->end_seq;
		if (after(last_lost, tp->retransmit_high))
			last_lost = tp->retransmit_high;
	} else {
		skb = tcp_write_queue_head(sk);
		last_lost = tp->snd_una;
	}

 =>	tcp_for_write_queue_from(skb, sk) {
		 __u8 sacked = TCP_SKB_CB(skb)->sacked;

		 if (skb == tcp_send_head(sk))
			 break;
		 /* we could do better than to assign each time */
		 if (hole == NULL)

This can happen for one of the following reasons,

Quoted 7 lines

1. tp->retransmit_skb_hint is NULL and tcp_write_queue_head() is NULL too. ie. tcp_xmit_retransmit_queue() is called on an empty write queue for some reason.

2. tp->retransmit_skb_hint is pointing to a skb which is not on the write_queue. ie. somebody forgot to update hint while removing the skb from the write queue.

Quoted 10 lines

Once again I've read the unlinkers through, and only thing that could cause this is tcp_send_synack (others do deal with the hints) but I think Eric already proposed a patch to that but we never got anywhere due to some counterargument why it wouldn't take place (too far away for me to remember, see archives about the discussions). ...But if you want be dead sure some WARN_ON there might not hurt. Also the purging of the whole queue was a similar suspect I then came across (but that would only materialize with sk reuse happening e.g., with nfs which the other guys weren't using).

Hmm.

This sounds familiar to me, but I cannot remember the discussion you mention or the patch.

Or maybe it was the TCP transaction thing ? (including data in SYN or SYN-ACK packet)

Quoted 55 lines · Eric Dumazet

Quoted 43 lines · Ilpo Järvinen

Quoted 33 lines · Tejun Heo

We've been seeing oops in tcp_xmit_retransmit_queue() w/ 2.6.32.15. Please see the attached photoshoot. This is happening on a HPC cluster and very interestingly caused by one particular job. How long it takes isn't clear yet (at least more than a day) but when it happens it happens on a lot of machines in relatively short time.

With a bit of disassemblying, I've found that the oops is happening during tcp_for_write_queue_from() because the skb->next points to NULL.

void tcp_xmit_retransmit_queue(struct sock *sk)
{
...

	if (tp->retransmit_skb_hint) {
		skb = tp->retransmit_skb_hint;
		last_lost = TCP_SKB_CB(skb)->end_seq;
		if (after(last_lost, tp->retransmit_high))
			last_lost = tp->retransmit_high;
	} else {
		skb = tcp_write_queue_head(sk);
		last_lost = tp->snd_una;
	}

 =>	tcp_for_write_queue_from(skb, sk) {
		 __u8 sacked = TCP_SKB_CB(skb)->sacked;

		 if (skb == tcp_send_head(sk))
			 break;
		 /* we could do better than to assign each time */
		 if (hole == NULL)

This can happen for one of the following reasons,

Quoted 7 lines

1. tp->retransmit_skb_hint is NULL and tcp_write_queue_head() is NULL too. ie. tcp_xmit_retransmit_queue() is called on an empty write queue for some reason.

2. tp->retransmit_skb_hint is pointing to a skb which is not on the write_queue. ie. somebody forgot to update hint while removing the skb from the write queue.

Quoted 10 lines

Once again I've read the unlinkers through, and only thing that could cause this is tcp_send_synack (others do deal with the hints) but I think Eric already proposed a patch to that but we never got anywhere due to some counterargument why it wouldn't take place (too far away for me to remember, see archives about the discussions). ...But if you want be dead sure some WARN_ON there might not hurt. Also the purging of the whole queue was a similar suspect I then came across (but that would only materialize with sk reuse happening e.g., with nfs which the other guys weren't using).

Quoted 7 lines

Hmm.

This sounds familiar to me, but I cannot remember the discussion you mention or the patch.

Or maybe it was the TCP transaction thing ? (including data in SYN or SYN-ACK packet)

Hmm, I cannot find where we reset restransmit_skb_hint in tcp_mtu_probe(), if we call tcp_unlink_write_queue().

if (skb->len <= copy) {
	/* We've eaten all the data from this skb.
	 * Throw it away. */
	TCP_SKB_CB(nskb)->flags |= TCP_SKB_CB(skb)->flags;
<<>>	tcp_unlink_write_queue(skb, sk);
	sk_wmem_free_skb(sk, skb);
} else {

Sorry if this was already discussed. We might add a comment here in anycase ;)

Quoted 56 lines · Eric Dumazet

Quoted 55 lines · Eric Dumazet

Quoted 53 lines · Ilpo Järvinen

Quoted 33 lines · Tejun Heo

We've been seeing oops in tcp_xmit_retransmit_queue() w/ 2.6.32.15. Please see the attached photoshoot. This is happening on a HPC cluster and very interestingly caused by one particular job. How long it takes isn't clear yet (at least more than a day) but when it happens it happens on a lot of machines in relatively short time.

With a bit of disassemblying, I've found that the oops is happening during tcp_for_write_queue_from() because the skb->next points to NULL.

void tcp_xmit_retransmit_queue(struct sock *sk)
{
...

	if (tp->retransmit_skb_hint) {
		skb = tp->retransmit_skb_hint;
		last_lost = TCP_SKB_CB(skb)->end_seq;
		if (after(last_lost, tp->retransmit_high))
			last_lost = tp->retransmit_high;
	} else {
		skb = tcp_write_queue_head(sk);
		last_lost = tp->snd_una;
	}

 =>	tcp_for_write_queue_from(skb, sk) {
		 __u8 sacked = TCP_SKB_CB(skb)->sacked;

		 if (skb == tcp_send_head(sk))
			 break;
		 /* we could do better than to assign each time */
		 if (hole == NULL)

This can happen for one of the following reasons,

Quoted 7 lines

1. tp->retransmit_skb_hint is NULL and tcp_write_queue_head() is NULL too. ie. tcp_xmit_retransmit_queue() is called on an empty write queue for some reason.

2. tp->retransmit_skb_hint is pointing to a skb which is not on the write_queue. ie. somebody forgot to update hint while removing the skb from the write queue.

Once again I've read the unlinkers through, and only thing that could cause this is tcp_send_synack (others do deal with the hints) but I think Eric already proposed a patch to that but we never got anywhere due to some counterargument why it wouldn't take place (too far away for me to remember, see archives about the discussions). ...But if you want be dead sure some WARN_ON there might not hurt. Also the purging of the whole queue was a similar suspect I then came across (but that would only materialize with sk reuse happening e.g., with nfs which the other guys weren't using).

Quoted 7 lines

Quoted 7 lines

Hmm.

This sounds familiar to me, but I cannot remember the discussion you mention or the patch.

Or maybe it was the TCP transaction thing ? (including data in SYN or SYN-ACK packet)

Quoted 12 lines

Hmm, I cannot find where we reset restransmit_skb_hint in tcp_mtu_probe(), if we call tcp_unlink_write_queue().

if (skb->len <= copy) {
	/* We've eaten all the data from this skb.
	 * Throw it away. */
	TCP_SKB_CB(nskb)->flags |= TCP_SKB_CB(skb)->flags;
<<>>	tcp_unlink_write_queue(skb, sk);
	sk_wmem_free_skb(sk, skb);
} else {

Quoted 2 lines

Sorry if this was already discussed. We might add a comment here in anycase ;)

Just in case, here is a patch for this issue, if Tejun wants to try it.

Thanks

[PATCH] tcp: tcp_mtu_probe() and retransmit hints

When removing an skb from tcp write queue, we must take care of various hints that could be kept on this skb. tcp_mtu_probe() misses this cleanup.

lkml reference : http://lkml.org/lkml/2010/7/8/63

Reported-by Tejun Heo <[email protected]>

Signed-off-by Eric Dumazet <[email protected]>

---

net/ipv4/tcp_output.c +0 -0

index b4ed957..187453f 100644

net/ipv4/tcp_output.c +0 -0

 			sk_wmem_free_skb(sk, skb);
 		} else {
 			TCP_SKB_CB(nskb)->flags |= TCP_SKB_CB(skb)->flags &

Quoted 62 lines · Eric Dumazet

Quoted 61 lines · Eric Dumazet

Quoted 60 lines · Eric Dumazet

Quoted 43 lines · Ilpo Järvinen

Quoted 33 lines · Tejun Heo

We've been seeing oops in tcp_xmit_retransmit_queue() w/ 2.6.32.15. Please see the attached photoshoot. This is happening on a HPC cluster and very interestingly caused by one particular job. How long it takes isn't clear yet (at least more than a day) but when it happens it happens on a lot of machines in relatively short time.

With a bit of disassemblying, I've found that the oops is happening during tcp_for_write_queue_from() because the skb->next points to NULL.

void tcp_xmit_retransmit_queue(struct sock *sk)
{
...

	if (tp->retransmit_skb_hint) {
		skb = tp->retransmit_skb_hint;
		last_lost = TCP_SKB_CB(skb)->end_seq;
		if (after(last_lost, tp->retransmit_high))
			last_lost = tp->retransmit_high;
	} else {
		skb = tcp_write_queue_head(sk);
		last_lost = tp->snd_una;
	}

 =>	tcp_for_write_queue_from(skb, sk) {
		 __u8 sacked = TCP_SKB_CB(skb)->sacked;

		 if (skb == tcp_send_head(sk))
			 break;
		 /* we could do better than to assign each time */
		 if (hole == NULL)

This can happen for one of the following reasons,

Quoted 7 lines

1. tp->retransmit_skb_hint is NULL and tcp_write_queue_head() is NULL too. ie. tcp_xmit_retransmit_queue() is called on an empty write queue for some reason.

2. tp->retransmit_skb_hint is pointing to a skb which is not on the write_queue. ie. somebody forgot to update hint while removing the skb from the write queue.

Quoted 10 lines

Once again I've read the unlinkers through, and only thing that could cause this is tcp_send_synack (others do deal with the hints) but I think Eric already proposed a patch to that but we never got anywhere due to some counterargument why it wouldn't take place (too far away for me to remember, see archives about the discussions). ...But if you want be dead sure some WARN_ON there might not hurt. Also the purging of the whole queue was a similar suspect I then came across (but that would only materialize with sk reuse happening e.g., with nfs which the other guys weren't using).

Hmm.

This sounds familiar to me, but I cannot remember the discussion you mention or the patch.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Or maybe it was the TCP transaction thing ? (including data in SYN or SYN-ACK packet)

No. That's another thing. ...I've already found it with google today but cannot seem to find it again. I thought I used tcp_make_synack eric but for some reason I only get these transaction fix hits. I'll keep looking.

Quoted 67 lines · Ilpo Järvinen

Quoted 64 lines · Eric Dumazet

Quoted 63 lines · Eric Dumazet

Quoted 62 lines · Eric Dumazet

Quoted 43 lines · Ilpo Järvinen

Quoted 33 lines · Tejun Heo

We've been seeing oops in tcp_xmit_retransmit_queue() w/ 2.6.32.15. Please see the attached photoshoot. This is happening on a HPC cluster and very interestingly caused by one particular job. How long it takes isn't clear yet (at least more than a day) but when it happens it happens on a lot of machines in relatively short time.

With a bit of disassemblying, I've found that the oops is happening during tcp_for_write_queue_from() because the skb->next points to NULL.

void tcp_xmit_retransmit_queue(struct sock *sk)
{
...

	if (tp->retransmit_skb_hint) {
		skb = tp->retransmit_skb_hint;
		last_lost = TCP_SKB_CB(skb)->end_seq;
		if (after(last_lost, tp->retransmit_high))
			last_lost = tp->retransmit_high;
	} else {
		skb = tcp_write_queue_head(sk);
		last_lost = tp->snd_una;
	}

 =>	tcp_for_write_queue_from(skb, sk) {
		 __u8 sacked = TCP_SKB_CB(skb)->sacked;

		 if (skb == tcp_send_head(sk))
			 break;
		 /* we could do better than to assign each time */
		 if (hole == NULL)

This can happen for one of the following reasons,

Quoted 7 lines

1. tp->retransmit_skb_hint is NULL and tcp_write_queue_head() is NULL too. ie. tcp_xmit_retransmit_queue() is called on an empty write queue for some reason.

2. tp->retransmit_skb_hint is pointing to a skb which is not on the write_queue. ie. somebody forgot to update hint while removing the skb from the write queue.

Quoted 10 lines

Once again I've read the unlinkers through, and only thing that could cause this is tcp_send_synack (others do deal with the hints) but I think Eric already proposed a patch to that but we never got anywhere due to some counterargument why it wouldn't take place (too far away for me to remember, see archives about the discussions). ...But if you want be dead sure some WARN_ON there might not hurt. Also the purging of the whole queue was a similar suspect I then came across (but that would only materialize with sk reuse happening e.g., with nfs which the other guys weren't using).

Hmm.

This sounds familiar to me, but I cannot remember the discussion you mention or the patch.

Or maybe it was the TCP transaction thing ? (including data in SYN or SYN-ACK packet)

Quoted 3 lines

No. That's another thing. ...I've already found it with google today but cannot seem to find it again. I thought I used tcp_make_synack eric but for some reason I only get these transaction fix hits. I'll keep looking.

Right, this one:

http://kerneltrap.org/mailarchive/linux-netdev/2009/10/29/6259073

I'm testing new reordering algorithms in a virtual testbed, that is the nodes are emulated with xen and all the network parameters can be tuned with queues.
With one of the algorithms I also got tracebacks which include tcp_xmit_retransmit_queue. It only happens with ACK loss. The kernel version however is 2.6.31.
When I read this thread I tried the debug patch and got the following:

[ 2754.413150] NULL head, pkts 0
[ 2754.413156] Errors caught so far 1

Hope that is of any help.

Quoted 8 lines · Lennart Schulte

I'm testing new reordering algorithms in a virtual testbed, that is the nodes are emulated with xen and all the network parameters can be tuned with queues.
With one of the algorithms I also got tracebacks which include tcp_xmit_retransmit_queue. It only happens with ACK loss. The kernel version however is 2.6.31.
When I read this thread I tried the debug patch and got the following:

Quoted 5 lines

[ 2754.413150] NULL head, pkts 0
[ 2754.413156] Errors caught so far 1

Hope that is of any help.

Not sure I understand.

Are you saying you reproduce same tcp_xmit_retransmit_queue() bug, with a special tc qdisc/class droppping some ACK frames ?

Could it be some sched problem and incorrect return codes in case of congestion ?

Since tcp_xmit_retransmit_queue also gets skb == NULL I'm pretty sure it is the same bug.
Up to now I only experienced the problem with ACK loss (without ACK loss the test ran about 30min without problems, with ACK loss it had paniced within 10min).
The data sender only has a HTB queue for traffic shaping (set to 20 Mbit/s). The ACK loss is done by another router.
The setup looks like this. This way it seems to be the most realistic.

o sender with HTB
|
|
o netem queue for forward path delay
|
o netem queue for a queue limit
|
o netem queue for backward path delay
|
o netem queue for ACK loss
|
|
o receiver with HTB

Perhaps now it is a little big clearer.

Quoted 2 lines

Quoted 2 lines

[ 2754.413150] NULL head, pkts 0
[ 2754.413156] Errors caught so far 1

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

[ 2754.413150] NULL head, pkts 0
[ 2754.413156] Errors caught so far 1

Thanks for reporting the results.

Could you post the oops too or double check do the timestamps really match (and there wasn't more "Errors caught" prints in between)? Since this condition doesn't seem to crash the kernel as also send_head should be NULL, which saves the day here exiting the loop (unless send head would too be corrupt).

Quoted 4 lines · Ilpo Järvinen

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

[ 2754.413150] NULL head, pkts 0
[ 2754.413156] Errors caught so far 1

Quoted 7 lines

Thanks for reporting the results.

Could you post the oops too or double check do the timestamps really match (and there wasn't more "Errors caught" prints in between)? Since this condition doesn't seem to crash the kernel as also send_head should be NULL, which saves the day here exiting the loop (unless send head would too be corrupt).

I can try to do some more testing, perhaps then I will get other results. But until now I've always gotten something like above.

With the debug patch the kernel doesn't crash, but I have an oops from a run before the patch:

[PATCH] 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.

There may still be another bug affecting this same function.

Signed-off-by Ilpo Järvinen<[email protected]>

Reported-by Lennart Schulte<[email protected]>

---
  net/ipv4/tcp_output.c |    3 +++
  1 files changed, 3 insertions(+), 0 deletions(-)

net/ipv4/tcp_output.c +3 -0

index b4ed957..7ed9dc1 100644
@@ -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;
 

I ran tests for about 2 hours with this patch and I got no output from the debug patch. This seems to have solved at least my problem :)

Thanks!

Quoted 10 lines

[PATCH] 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.

There may still be another bug affecting this same function.

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).

Signed-off-by Ilpo Järvinen <[email protected]>

Reported-by Lennart Schulte <[email protected]>

Tested-by Lennart Schulte <[email protected]>

---
 net/ipv4/tcp_output.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

net/ipv4/tcp_output.c +3 -0

index b4ed957..7ed9dc1 100644
@@ -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;
 

Quoted 17 lines · Ilpo Järvinen

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.

Quoted 3 lines

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 ?

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.

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 ?

From: Eric Dumazet <[email protected]>
Date: Mon, 19 Jul 2010 19:39:08 +0200

Quoted 5 lines

Do you know in what exact circumstance the bug triggers ?

It's hard to believe thousand of machines on the Internet never hit it :(

Quoted 1 line

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.

Besides, Tejun has also found that it's hint->next ptr which is NULL in his case so this won't solve his case anyway. Tejun, can you confirm whether it was retransmit_skb_hint->next being NULL on _entry time_ to tcp_xmit_retransmit_queue() or later on in the loop after the updates done by the loop itself to the hint (or that your testing didn't conclude either)?

Hello,

Quoted 6 lines · Ilpo Järvinen

Besides, Tejun has also found that it's hint->next ptr which is NULL in his case so this won't solve his case anyway. Tejun, can you confirm whether it was retransmit_skb_hint->next being NULL on _entry time_ to tcp_xmit_retransmit_queue() or later on in the loop after the updates done by the loop itself to the hint (or that your testing didn't conclude either)?

Sorry about the delay. I was traveling last week. Unfortunately, I don't know whether ->next was NULL on entry or not. I hacked up the following ugly patch for the next test run. It should have everything which has come up till now + list and hint sanity checking before starting processing them. I'm planning on deploying it w/ crashdump enabled in several days. If I've missed something, please let me know.

net/ipv4/tcp_output.c +98 -4

index b4ed957..1c8b1e0 100644
@@ -2190,6 +2190,53 @@ static int tcp_can_forward_retransmit(struct sock *sk)
 	return 1;
 }
 
+static void print_queue(struct sock *sk, struct sk_buff *old, struct sk_buff *hole)
+{
+	struct tcp_sock *tp = tcp_sk(sk);
+	struct sk_buff *skb, *prev;
+	bool do_panic = false;
+
+	skb = tcp_write_queue_head(sk);
+	prev = (struct sk_buff *)(&sk->sk_write_queue);
+
+	if (skb == NULL) {
+		printk("XXX NULL head, pkts %u\n", tp->packets_out);
+		do_panic = true;
+	}
+
+	printk("XXX head %p tail %p sendhead %p oldhint %p now %p hole %p high %u\n",
+	       tcp_write_queue_head(sk), tcp_write_queue_tail(sk),
+	       tcp_send_head(sk), old, tp->retransmit_skb_hint, hole,
+	       tp->retransmit_high);
+
+	while (skb) {
+		printk("XXX skb %p (%u-%u) next %p prev %p sacked %u\n",
+		       skb, TCP_SKB_CB(skb)->seq, TCP_SKB_CB(skb)->end_seq,
+		       skb->next, skb->prev, TCP_SKB_CB(skb)->sacked);
+		if (prev != skb->prev) {
+			printk("XXX Inconsistent prev\n");
+			do_panic = true;
+		}
+
+		if (skb == tcp_write_queue_tail(sk)) {
+			if (skb->next != (struct sk_buff *)(&sk->sk_write_queue)) {
+				printk("XXX Improper next at tail\n");
+				do_panic = true;
+			}
+			break;
+		}
+
+		prev = skb;
+		skb = skb->next;
+	}
+	if (!skb) {
+		printk("XXX Encountered unexpected NULL\n");
+		do_panic = true;
+	}
+	if (do_panic)
+		panic("XXX panicking");
+}
+
 /* This gets called after a retransmit timeout, and the initially
  * retransmitted data is acknowledged.  It tries to continue
  * resending the rest of the retransmit queue, until either

@@ -2198,19 +2245,53 @@ static int tcp_can_forward_retransmit(struct sock *sk)
  * based retransmit packet might feed us FACK information again.
  * If so, we use it to avoid unnecessarily retransmissions.
  */
+static unsigned int caught_it;
+
 void tcp_xmit_retransmit_queue(struct sock *sk)
 {
 	const struct inet_connection_sock *icsk = inet_csk(sk);
 	struct tcp_sock *tp = tcp_sk(sk);
-	struct sk_buff *skb;
+	struct sk_buff *skb, *prev;
 	struct sk_buff *hole = NULL;
+	struct sk_buff *old = tp->retransmit_skb_hint;
 	u32 last_lost;
 	int mib_idx;
 	int fwd_rexmitting = 0;
+	bool saw_hint = false;
+
+	if (!tp->packets_out) {
+		if (net_ratelimit())
+			printk("XXX !tp->packets_out, retransmit_skb_hint=%p, write_queue_head=%p\n",
+			       tp->retransmit_skb_hint, tcp_write_queue_head(sk));
+		return;
+	}
 
 	if (!tp->lost_out)
 		tp->retransmit_high = tp->snd_una;
 
+	for (skb = tcp_write_queue_head(sk),
+	     prev = (struct sk_buff *)&sk->sk_write_queue;
+	     skb != (struct sk_buff *)&sk->sk_write_queue;
+	     prev = skb, skb = skb->next) {
+		if (prev != skb->prev) {
+			printk("XXX sanity check: prev corrupt\n");
+			print_queue(sk, old, hole);
+		}
+		if (skb == tp->retransmit_skb_hint)
+			saw_hint = true;
+		if (skb == tcp_write_queue_tail(sk) &&
+		    skb->next != (struct sk_buff *)(&sk->sk_write_queue)) {
+			printk("XXX sanity check: end corrupt\n");
+			print_queue(sk, old, hole);
+		}
+	}
+	if (tp->retransmit_skb_hint && !saw_hint) {
+		printk("XXX sanity check: retransmit_skb_hint=%p is not on list, claring hint\n",
+		       tp->retransmit_skb_hint);
+		print_queue(sk, old, hole);
+		tp->retransmit_skb_hint = NULL;
+	}
+
 	if (tp->retransmit_skb_hint) {
 		skb = tp->retransmit_skb_hint;
 		last_lost = TCP_SKB_CB(skb)->end_seq;

@@ -2218,7 +2299,17 @@ void tcp_xmit_retransmit_queue(struct sock *sk)
 			last_lost = tp->retransmit_high;
 	} else {
 		skb = tcp_write_queue_head(sk);
-		last_lost = tp->snd_una;
+		if (skb)
+			last_lost = tp->snd_una;
+	}
+
+checknull:
+	if (skb == NULL) {
+		print_queue(sk, old, hole);
+		caught_it++;
+		if (net_ratelimit())
+			printk("XXX Errors caught so far %u\n", caught_it);
+		return;
 	}
 
 	tcp_for_write_queue_from(skb, sk) {

@@ -2261,7 +2352,7 @@ begin_fwd:
 		} else if (!(sacked & TCPCB_LOST)) {
 			if (hole == NULL && !(sacked & (TCPCB_SACKED_RETRANS|TCPCB_SACKED_ACKED)))
 				hole = skb;
-			continue;
+			goto cont;
 
 		} else {
 			last_lost = TCP_SKB_CB(skb)->end_seq;

@@ -2272,7 +2363,7 @@ begin_fwd:
 		}
 
 		if (sacked & (TCPCB_SACKED_ACKED|TCPCB_SACKED_RETRANS))
-			continue;
+			goto cont;
 
 		if (tcp_retransmit_skb(sk, skb))
 			return;

@@ -2282,6 +2373,9 @@ begin_fwd:
 			inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS,
 						  inet_csk(sk)->icsk_rto,
 						  TCP_RTO_MAX);
+cont:
+		skb = skb->next;
+		goto checknull;
 	}
 }
 

Hello,

Quoted 6 lines · Ilpo Järvinen

Besides, Tejun has also found that it's hint->next ptr which is NULL in his case so this won't solve his case anyway. Tejun, can you confirm whether it was retransmit_skb_hint->next being NULL on _entry time_ to tcp_xmit_retransmit_queue() or later on in the loop after the updates done by the loop itself to the hint (or that your testing didn't conclude either)?

Sorry about the delay. I was traveling last week. Unfortunately, I don't know whether ->next was NULL on entry or not. I hacked up the following ugly patch for the next test run. It should have everything which has come up till now + list and hint sanity checking before starting processing them. I'm planning on deploying it w/ crashdump enabled in several days. If I've missed something, please let me know.

Quoted 7 lines

Sorry about the delay. I was traveling last week. Unfortunately, I don't know whether ->next was NULL on entry or not. I hacked up the following ugly patch for the next test run. It should have everything which has come up till now + list and hint sanity checking before starting processing them. I'm planning on deploying it w/ crashdump enabled in several days. If I've missed something, please let me know.

Any news on this one?

Quoted 8 lines · Ilpo Järvinen

Quoted 7 lines

Sorry about the delay. I was traveling last week. Unfortunately, I don't know whether ->next was NULL on entry or not. I hacked up the following ugly patch for the next test run. It should have everything which has come up till now + list and hint sanity checking before starting processing them. I'm planning on deploying it w/ crashdump enabled in several days. If I've missed something, please let me know.

Quoted 1 line

Any news on this one?

Unfortunately, we haven't been able to reproduce the problem anymore.

It could be (but not likely given that none of the debugging messages is triggering) that I was mistaken and the previously posted fixed the issue. The network used by the cluster went through some changes at the time and there have been issues with packet losses. Given that the problem needs packet losses to trigger, it's likely that packet loss pattern here changed such that the patterns of packet losses which trigger the problem aren't happening anymore. (Carsten, Henning, please feel free to fill in if I'm missing something).

Quoted 1 line

Unfortunately, we haven't been able to reproduce the problem anymore.

With my debug patch or not at all?

Quoted 8 lines

It could be (but not likely given that none of the debugging messages is triggering) that I was mistaken and the previously posted fixed the issue. The network used by the cluster went through some changes at the time and there have been issues with packet losses. Given that the problem needs packet losses to trigger, it's likely that packet loss pattern here changed such that the patterns of packet losses which trigger the problem aren't happening anymore. (Carsten, Henning, please feel free to fill in if I'm missing something).

That might well be true, however, you're already a second guy who cannot reproduce it with the debug patch so I would not rule out other possibilities unless you've tried without debug patch too since the changes?

Quoted 3 lines · Ilpo Järvinen

Quoted 1 line

Unfortunately, we haven't been able to reproduce the problem anymore.

With my debug patch or not at all?

With the ugly merged patch I posted previously in this thread which contained debug messages if any of the worked around condition triggers.

Quoted 9 lines

Quoted 8 lines

It could be (but not likely given that none of the debugging messages is triggering) that I was mistaken and the previously posted fixed the issue. The network used by the cluster went through some changes at the time and there have been issues with packet losses. Given that the problem needs packet losses to trigger, it's likely that packet loss pattern here changed such that the patterns of packet losses which trigger the problem aren't happening anymore. (Carsten, Henning, please feel free to fill in if I'm missing something).

Quoted 4 lines

That might well be true, however, you're already a second guy who cannot reproduce it with the debug patch so I would not rule out other possibilities unless you've tried without debug patch too since the changes?

Unfortunately, I can't really tell one way or the other at this point. Carsten will be back in a few days. I'll ask him for more details.