[PANIC] lro + iscsi or lro + skb text search causes panic

[PANIC] lro + iscsi or lro + skb text search causes panic

[Brandeburg, Jesse]

I've filed this bugzilla a while ago.
http://bugzilla.kernel.org/show_bug.cgi?id=11804
now other customers are becoming interested as well

what happens is that when a device driver (inet_lro) hands an skb that has 
possibly multiple skb->data pointers, chained together with skb->next and 
each one possibly having pages attached, skb_seq_read called by iSCSI 
doesn't follow the chain as it should.  result is a panic.

to reproduce you just get lro enabled igb or ixgbe and try to connect to 
an iSCSI target.

BUG: unable to handle kernel NULL pointer dereference at 000005a8
IP: [<f8de64b2>] :iscsi_tcp:iscsi_tcp_recv+0x161/0x473
*pdpt = 0000000036533001 *pde = 0000000000000000
Oops: 0000 [#1] SMP
Modules linked in: crc32c libcrc32c iscsi_tcp libiscsi 
scsi_transport_iscsi
ixgbe netconsole inet_lro ipv6 af_packet button battery ac loop usbhid
ff_memless ehci_hcd uhci_hcd usbcore dm_mod bnx2 ext3 jbd edd fan thermal
processor thermal_sys sg megaraid_sas ata_piix libata dock piix sd_mod 
scsi_mod
ide_disk ide_core [last unloaded: iscsi_tcp]

Pid: 0, comm: swapper Not tainted (2.6.26-bigsmp #1)
EIP: 0060:[<f8de64b2>] EFLAGS: 00010202 CPU: 3
EIP is at iscsi_tcp_recv+0x161/0x473 [iscsi_tcp]
EAX: 0000002b EBX: f747dd48 ECX: 00000038 EDX: 00000000
ESI: 000005a8 EDI: f593db20 EBP: f751ca10 ESP: f747dd20
 DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
Process swapper (pid: 0, ti=f747c000 task=f745abe0 task.ti=f747c000)
Stack: f8de78e7 000000e0 f446c0c0 f6c35544 f751ca00 000005a8 00000000 
000000e0
       000005a8 08745958 00000000 00000a88 00000000 000005a8 f446c0c0 
f78ba0ac
       00000000 c0289617 00000000 00000000 05a80001 00007fff f78ba040 
000005a8
Call Trace:
 [<c0289617>] tcp_ack+0x15bd/0x1757
 [<c028391e>] tcp_read_sock+0x8c/0x1e0
 [<f8de6351>] iscsi_tcp_recv+0x0/0x473 [iscsi_tcp]
 [<f8de716a>] iscsi_tcp_data_ready+0x36/0x80 [iscsi_tcp]
 [<c028d1a2>] tcp_send_ack+0xab/0xaf
 [<c028c02e>] tcp_rcv_established+0x3b3/0x639
 [<c02909fb>] tcp_v4_do_rcv+0x22/0x16f
 [<c0292294>] tcp_v4_rcv+0x512/0x562
 [<c027b921>] ip_local_deliver_finish+0xb2/0x14a
 [<c027b852>] ip_rcv_finish+0x286/0x2a3
 [<f8ce9a93>] packet_rcv_spkt+0xb6/0xbd [af_packet]
 [<c0261889>] netif_receive_skb+0x2d0/0x33b
 [<f8afd5ca>] lro_flush+0x314/0x340 [inet_lro]
 [<f8afd636>] lro_flush_all+0x1b/0x28 [inet_lro]
 [<f8b410eb>] ixgbe_clean_rx_irq+0x73b/0x850 [ixgbe]
 [<f8b44183>] ixgbe_clean_rxonly+0x53/0xd0 [ixgbe]
 [<c0263521>] net_rx_action+0x8a/0x152
 [<c0124c6e>] __do_softirq+0x5d/0xc1
 [<c0124d04>] do_softirq+0x32/0x36
 [<c010663a>] do_IRQ+0x73/0x85
 [<c0109152>] mwait_idle+0x0/0x32
 [<c0105143>] common_interrupt+0x23/0x28
 [<c0109152>] mwait_idle+0x0/0x32
 [<c0109181>] mwait_idle+0x2f/0x32
 [<c0103535>] cpu_idle+0x88/0x9c
 =======================
Code: 24 14 0f 46 44 24 14 89 44 24 14 50 68 e7 78 de f8 e8 2e b3 33 c7 8b 
7d
08 03 7d 00 8b 4c 24 1c 8b 74 24 20 03 74 24 18 c1 e9 02 <f3> a5 8b 4c 24 
1c 83
e1 03 74 02 f3 a4 8b 4c 24 1c 01 4c 24 18
EIP: [<f8de64b2>] iscsi_tcp_recv+0x161/0x473 [iscsi_tcp] SS:ESP 
0068:f747dd20
Kernel panic - not syncing: Fatal exception in interrupt



skb_copy_bits is an example of the code flow that does work.

skb_seq_read appears to only be used by iSCSI and the skb text match 
support in tc/netfilter (aka skb_find_text)

skb_seq_read is so complex that it is not a simple job just to re-write it 
with a state machine switch statement, and I am unable to spend the time 
on it to fix it.  Can someone help?

I am also a little bit concerned that the recent effort to make GRO frames 
become more utilized in the stack may end up causing this issue to trigger 
as well.

We have test resources that can test patches with iSCSI.

Re: [PANIC] lro + iscsi or lro + skb text search causes panic

[Mike Christie]

Brandeburg, Jesse wrote:
> 
> skb_copy_bits is an example of the code flow that does work.
> 
> skb_seq_read appears to only be used by iSCSI and the skb text match 
> support in tc/netfilter (aka skb_find_text)
> 

There is no reason iscsi needs to use skb_seq_read. It used to use 
skb_copy_bits. I can convert iscsi to use skb_copy_bits again.

Is it easier to just convert the text search to not use skb_seq_read, 
and then just dump those functions?

Re: [PANIC] lro + iscsi or lro + skb text search causes panic

[Mike Christie]

[-- Attachment #1: Type: text/plain, Size: 726 bytes --]

Mike Christie wrote:
> Brandeburg, Jesse wrote:
>>
>> skb_copy_bits is an example of the code flow that does work.
>>
>> skb_seq_read appears to only be used by iSCSI and the skb text match 
>> support in tc/netfilter (aka skb_find_text)
>>
> 
> There is no reason iscsi needs to use skb_seq_read. It used to use 
> skb_copy_bits. I can convert iscsi to use skb_copy_bits again.
> 

Attached is a patch made against the scsi maintainer's tree (I think it 
should also apply to linus's) that converts iscsi to use skb_copy_bits. 
It is lightly tested. If there is no benefit in having skb_find_text use 
skb_seq_read maybe we can just kill it, so people do not have to 
maintain two helpers that provide similar functionality.

[-- Attachment #2: use-skb-copy-bits.patch --]
[-- Type: text/plain, Size: 4905 bytes --]

diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
index 23808df..b052c57 100644
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -74,7 +74,7 @@ static int iscsi_sw_tcp_recv(read_descriptor_t *rd_desc, struct sk_buff *skb,
 			     unsigned int offset, size_t len)
 {
 	struct iscsi_conn *conn = rd_desc->arg.data;
-	unsigned int consumed, total_consumed = 0;
+	unsigned int consumed, total_consumed = 0, in = skb->len - offset;
 	int status;
 
 	debug_tcp("in %d bytes\n", skb->len - offset);
@@ -84,7 +84,8 @@ static int iscsi_sw_tcp_recv(read_descriptor_t *rd_desc, struct sk_buff *skb,
 		consumed = iscsi_tcp_recv_skb(conn, skb, offset, 0, &status);
 		offset += consumed;
 		total_consumed += consumed;
-	} while (consumed != 0 && status != ISCSI_TCP_SKB_DONE);
+	} while (consumed != 0 && status != ISCSI_TCP_SKB_DONE &&
+		 consumed < in);
 
 	debug_tcp("read %d bytes status %d\n", skb->len - offset, status);
 	return total_consumed;
diff --git a/drivers/scsi/libiscsi_tcp.c b/drivers/scsi/libiscsi_tcp.c
index e7705d3..f91c79d 100644
--- a/drivers/scsi/libiscsi_tcp.c
+++ b/drivers/scsi/libiscsi_tcp.c
@@ -252,45 +252,6 @@ int iscsi_tcp_segment_done(struct iscsi_tcp_conn *tcp_conn,
 }
 EXPORT_SYMBOL_GPL(iscsi_tcp_segment_done);
 
-/**
- * iscsi_tcp_segment_recv - copy data to segment
- * @tcp_conn: the iSCSI TCP connection
- * @segment: the buffer to copy to
- * @ptr: data pointer
- * @len: amount of data available
- *
- * This function copies up to @len bytes to the
- * given buffer, and returns the number of bytes
- * consumed, which can actually be less than @len.
- *
- * If hash digest is enabled, the function will update the
- * hash while copying.
- * Combining these two operations doesn't buy us a lot (yet),
- * but in the future we could implement combined copy+crc,
- * just way we do for network layer checksums.
- */
-static int
-iscsi_tcp_segment_recv(struct iscsi_tcp_conn *tcp_conn,
-		       struct iscsi_segment *segment, const void *ptr,
-		       unsigned int len)
-{
-	unsigned int copy = 0, copied = 0;
-
-	while (!iscsi_tcp_segment_done(tcp_conn, segment, 1, copy)) {
-		if (copied == len) {
-			debug_tcp("iscsi_tcp_segment_recv copied %d bytes\n",
-				  len);
-			break;
-		}
-
-		copy = min(len - copied, segment->size - segment->copied);
-		debug_tcp("iscsi_tcp_segment_recv copying %d\n", copy);
-		memcpy(segment->data + segment->copied, ptr + copied, copy);
-		copied += copy;
-	}
-	return copied;
-}
-
 inline void
 iscsi_tcp_dgst_header(struct hash_desc *hash, const void *hdr, size_t hdrlen,
 		      unsigned char digest[ISCSI_DIGEST_SIZE])
@@ -850,8 +811,7 @@ int iscsi_tcp_recv_skb(struct iscsi_conn *conn, struct sk_buff *skb,
 {
 	struct iscsi_tcp_conn *tcp_conn = conn->dd_data;
 	struct iscsi_segment *segment = &tcp_conn->in.segment;
-	struct skb_seq_state seq;
-	unsigned int consumed = 0;
+	unsigned int copy = 0, copied = 0, len = skb->len - offset;
 	int rc = 0;
 
 	debug_tcp("in %d bytes\n", skb->len - offset);
@@ -867,30 +827,31 @@ int iscsi_tcp_recv_skb(struct iscsi_conn *conn, struct sk_buff *skb,
 		goto segment_done;
 	}
 
-	skb_prepare_seq_read(skb, offset, skb->len, &seq);
-	while (1) {
-		unsigned int avail;
-		const u8 *ptr;
-
-		avail = skb_seq_read(consumed, &ptr, &seq);
-		if (avail == 0) {
-			debug_tcp("no more data avail. Consumed %d\n",
-				  consumed);
+	while (!iscsi_tcp_segment_done(tcp_conn, segment, 1, copy)) {
+		if (copied == len) {
+			debug_tcp("iscsi_tcp_recv_skb finished skb "
+				  "copied %d bytes\n", len);
 			*status = ISCSI_TCP_SKB_DONE;
-			skb_abort_seq_read(&seq);
 			goto skb_done;
 		}
-		BUG_ON(segment->copied >= segment->size);
 
-		debug_tcp("skb %p ptr=%p avail=%u\n", skb, ptr, avail);
-		rc = iscsi_tcp_segment_recv(tcp_conn, segment, ptr, avail);
-		BUG_ON(rc == 0);
-		consumed += rc;
+		copy = min(len - copied, segment->size - segment->copied);
+		debug_tcp("iscsi_tcp_recv_skb copying %d (len %u copied %u "
+			  "segment size %u segment copied %u\n", copy,
+			  copied, segment->size, segment->copied);
 
-		if (segment->total_copied >= segment->total_size) {
-			skb_abort_seq_read(&seq);
-			goto segment_done;
+		rc = skb_copy_bits(skb, copied + offset,
+				   segment->data + segment->copied, copy);
+		if (rc) {
+			printk(KERN_ERR "iscsi_tcp_recv_skb bug. Could not "
+			       "copy skb data to command buffer. (len %u "
+			       "copied %u segment size %u segment copied %u)\n",
+				copy, copied, segment->size, segment->copied);
+			*status = ISCSI_TCP_CONN_ERR;
+			iscsi_conn_failure(conn, ISCSI_ERR_CONN_FAILED);
+			return 0;
 		}
+		copied += copy;
 	}
 
 segment_done:
@@ -906,8 +867,8 @@ segment_done:
 	/* The done() functions sets up the next segment. */
 
 skb_done:
-	conn->rxdata_octets += consumed;
-	return consumed;
+	conn->rxdata_octets += copied;
+	return copied;
 }
 EXPORT_SYMBOL_GPL(iscsi_tcp_recv_skb);
 

Re: [PANIC] lro + iscsi or lro + skb text search causes panic

[Mike Christie]

Mike Christie wrote:
> Mike Christie wrote:
>> Brandeburg, Jesse wrote:
>>>
>>> skb_copy_bits is an example of the code flow that does work.
>>>
>>> skb_seq_read appears to only be used by iSCSI and the skb text match 
>>> support in tc/netfilter (aka skb_find_text)
>>>
>>
>> There is no reason iscsi needs to use skb_seq_read. It used to use 
>> skb_copy_bits. I can convert iscsi to use skb_copy_bits again.
>>
> 
> Attached is a patch made against the scsi maintainer's tree (I think it 
> should also apply to linus's) that converts iscsi to use skb_copy_bits. 
> It is lightly tested. If there is no benefit in having skb_find_text use 
> skb_seq_read maybe we can just kill it, so people do not have to 
> maintain two helpers that provide similar functionality.
> 

There is a bug in this patch, but it just makes it a little less 
efficient. It should not screw up testing to verify that the oops is fixed.

Re: [PANIC] lro + iscsi or lro + skb text search causes panic

[David Miller]

From: Mike Christie <michaelc@cs.wisc.edu>
Date: Thu, 22 Jan 2009 22:22:42 -0600

> Attached is a patch made against the scsi maintainer's tree (I think
> it should also apply to linus's) that converts iscsi to use
> skb_copy_bits. It is lightly tested. If there is no benefit in
> having skb_find_text use skb_seq_read maybe we can just kill it, so
> people do not have to maintain two helpers that provide similar
> functionality.

Regardless of what we decide to do with iSCSI we have to make this
function work properly as long as there is a user in the tree.

This iterator can in fact be more efficient than using copy
bits since copy bits doesn't remember any state from previous
invocations whereas the iterator does.  So if you call it
multiple times on the same SKB that's a lot of wasted work.

In fact I'm pretty sure that's why we added it for textsearch,
because in that application we're constantly beating through
the same SKB via the testsearch state machine.

Therefore, keeping it's use for iSCSI would be beneficial.

Re: [PANIC] lro + iscsi or lro + skb text search causes panic

[Herbert Xu]

On Thu, Jan 22, 2009 at 12:55:21PM -0800, Brandeburg, Jesse wrote:
> I've filed this bugzilla a while ago.
> http://bugzilla.kernel.org/show_bug.cgi?id=11804
> now other customers are becoming interested as well

Does this patch help?

net: Fix frag_list handling in skb_seq_read

The frag_list handling was broken ini skb_seq_read:

1) We didn't add the stepped offset when looking at the head
are of fragments other than the first.

2) The frag index wasn't reset.

This patch fixes both issues.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index d7efaf9..ae03c7f 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2215,7 +2215,7 @@ unsigned int skb_seq_read(unsigned int consumed, const u8 **data,
 		return 0;
 
 next_skb:
-	block_limit = skb_headlen(st->cur_skb);
+	block_limit = skb_headlen(st->cur_skb) + st->stepped_offset;
 
 	if (abs_offset < block_limit) {
 		*data = st->cur_skb->data + abs_offset;
@@ -2260,6 +2260,7 @@ next_skb:
 	} else if (st->root_skb == st->cur_skb &&
 		   skb_shinfo(st->root_skb)->frag_list) {
 		st->cur_skb = skb_shinfo(st->root_skb)->frag_list;
+		st->frag_idx = 0;
 		goto next_skb;
 	}

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: [PANIC] lro + iscsi or lro + skb text search causes panic

[Brandeburg, Jesse]

Herbert Xu wrote:
> On Thu, Jan 22, 2009 at 12:55:21PM -0800, Brandeburg, Jesse wrote:
>> I've filed this bugzilla a while ago.
>> http://bugzilla.kernel.org/show_bug.cgi?id=11804
>> now other customers are becoming interested as well
> 
> Does this patch help?
> 
> net: Fix frag_list handling in skb_seq_read
> 
> The frag_list handling was broken ini skb_seq_read:
> 
> 1) We didn't add the stepped offset when looking at the head
> are of fragments other than the first.
> 
> 2) The frag index wasn't reset.
> 
> This patch fixes both issues.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Herbert, thanks! the patch looks promsing, but we have to setup the repro again.
I'll have a tester take a look at it tomorrow or Monday, and let you know then.

Thanks!
  Jesse

Re: [PANIC] lro + iscsi or lro + skb text search causes panic

[Mike Christie]

Herbert Xu wrote:
> On Thu, Jan 22, 2009 at 12:55:21PM -0800, Brandeburg, Jesse wrote:
>> I've filed this bugzilla a while ago.
>> http://bugzilla.kernel.org/show_bug.cgi?id=11804
>> now other customers are becoming interested as well
> 
> Does this patch help?
> 
> net: Fix frag_list handling in skb_seq_read
> 
> The frag_list handling was broken ini skb_seq_read:
> 
> 1) We didn't add the stepped offset when looking at the head
> are of fragments other than the first.
> 
> 2) The frag index wasn't reset.
> 
> This patch fixes both issues.
> 

Without the patch I do not get the oops Jesse saw. The iscsi driver logs 
in and I do not see a problem until running IO (scsi read commands). The 
iscsi code thinks there is a missing packet at the iscsi level and 
begins recovery at that level.

With the patch running against linus's git tree, my box locks up. You 
cannot ping it. I do not get a oops or anything in the logs, and the 
keyboard does not respond. I will try to get some oops output and more info.

Re: [PANIC] lro + iscsi or lro + skb text search causes panic

[David Miller]

From: Mike Christie <michaelc@cs.wisc.edu>
Date: Thu, 22 Jan 2009 18:04:11 -0600

> With the patch running against linus's git tree, my box locks
> up. You cannot ping it. I do not get a oops or anything in the logs,
> and the keyboard does not respond. I will try to get some oops
> output and more info.

Herbert, any idea offhand?

Re: [PANIC] lro + iscsi or lro + skb text search causes panic

[Herbert Xu]

On Sun, Jan 25, 2009 at 09:32:22PM -0800, David Miller wrote:
> From: Mike Christie <michaelc@cs.wisc.edu>
> Date: Thu, 22 Jan 2009 18:04:11 -0600
> 
> > With the patch running against linus's git tree, my box locks
> > up. You cannot ping it. I do not get a oops or anything in the logs,
> > and the keyboard does not respond. I will try to get some oops
> > output and more info.
> 
> Herbert, any idea offhand?

Yeah, I missed an offset update in there :) Here's a better version.

net: Fix frag_list handling in skb_seq_read

The frag_list handling was broken in skb_seq_read:

1) We didn't add the stepped offset when looking at the head
are of fragments other than the first.

2) We didn't take the stepped offset away when setting the data
pointer in the head area.

3) The frag index wasn't reset.

This patch fixes both issues.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index d7efaf9..d4d0e31 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2215,10 +2215,10 @@ unsigned int skb_seq_read(unsigned int consumed, const u8 **data,
 		return 0;
 
 next_skb:
-	block_limit = skb_headlen(st->cur_skb);
+	block_limit = skb_headlen(st->cur_skb) + st->stepped_offset;
 
 	if (abs_offset < block_limit) {
-		*data = st->cur_skb->data + abs_offset;
+		*data = st->cur_skb->data + (abs_offset - st->stepped_offset);
 		return block_limit - abs_offset;
 	}
 
@@ -2260,6 +2260,7 @@ next_skb:
 	} else if (st->root_skb == st->cur_skb &&
 		   skb_shinfo(st->root_skb)->frag_list) {
 		st->cur_skb = skb_shinfo(st->root_skb)->frag_list;
+		st->frag_idx = 0;
 		goto next_skb;
 	}
 
Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: [PANIC] lro + iscsi or lro + skb text search causes panic

[Brandeburg, Jesse]

Herbert Xu wrote:
> On Sun, Jan 25, 2009 at 09:32:22PM -0800, David Miller wrote:
>> From: Mike Christie <michaelc@cs.wisc.edu>
>> Date: Thu, 22 Jan 2009 18:04:11 -0600
>> 
>>> With the patch running against linus's git tree, my box locks
>>> up. You cannot ping it. I do not get a oops or anything in the logs,
>>> and the keyboard does not respond. I will try to get some oops
>>> output and more info.
>> 
>> Herbert, any idea offhand?
> 
> Yeah, I missed an offset update in there :) Here's a better version.
> 
> net: Fix frag_list handling in skb_seq_read
> 
> The frag_list handling was broken in skb_seq_read:
> 
> 1) We didn't add the stepped offset when looking at the head
> are of fragments other than the first.
> 
> 2) We didn't take the stepped offset away when setting the data
> pointer in the head area.
> 
> 3) The frag index wasn't reset.
> 
> This patch fixes both issues.

1-3 (both, :-)) doesn't seem to work here, I applied this and still got a 
panic when doing a quick test before I went home today.

I can get panic trace from netconsole tomorrow.

Re: [PANIC] lro + iscsi or lro + skb text search causes panic

[Herbert Xu]

On Mon, Jan 26, 2009 at 05:40:08PM -0800, Brandeburg, Jesse wrote:
>
> 1-3 (both, :-)) doesn't seem to work here, I applied this and still got a 
> panic when doing a quick test before I went home today.
> 
> I can get panic trace from netconsole tomorrow.

OK, looking at the original trace it looks like none of the bugs
I've fixed actually explain it.  The crash looks more like a ref
count bug, which means that the issue isn't so much about the
skb_seq_read interface (which should be fixed in any case), but
in the iSCSI code itself.

More digging is required.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PANIC] lro + iscsi or lro + skb text search causes panic

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 27 Jan 2009 09:30:22 +1100

> net: Fix frag_list handling in skb_seq_read
> 
> The frag_list handling was broken in skb_seq_read:
> 
> 1) We didn't add the stepped offset when looking at the head
> are of fragments other than the first.
> 
> 2) We didn't take the stepped offset away when setting the data
> pointer in the head area.
> 
> 3) The frag index wasn't reset.
> 
> This patch fixes both issues.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

I see, the code is only clearing the fragment index when it's
advancing from one SKB to the next while already in the middle of a
->frag_list, not when transitioning past the root skb in such a list.

I bet some weird cases happen when "consumed" it's advanced by
the caller the entire length of data returned by the previous
skb_seq_read().

It all seems to be designed to work for that case, however.

Anyways, Herbert's patch looks definitely correct but until
we've gotten these crashes and hangs solved I don't want
to apply it.

Re: [PANIC] lro + iscsi or lro + skb text search causes panic

[Mike Christie]

Herbert Xu wrote:
> On Sun, Jan 25, 2009 at 09:32:22PM -0800, David Miller wrote:
>> From: Mike Christie <michaelc@cs.wisc.edu>
>> Date: Thu, 22 Jan 2009 18:04:11 -0600
>>
>>> With the patch running against linus's git tree, my box locks
>>> up. You cannot ping it. I do not get a oops or anything in the logs,
>>> and the keyboard does not respond. I will try to get some oops
>>> output and more info.
>> Herbert, any idea offhand?
> 
> Yeah, I missed an offset update in there :) Here's a better version.
> 
> net: Fix frag_list handling in skb_seq_read
> 
> The frag_list handling was broken in skb_seq_read:
> 
> 1) We didn't add the stepped offset when looking at the head
> are of fragments other than the first.
> 
> 2) We didn't take the stepped offset away when setting the data
> pointer in the head area.
> 
> 3) The frag index wasn't reset.
> 
> This patch fixes both issues.
> 

It oopsd for me in skb_seq_read. addr2line said it was 
linux-2.6/net/core/skbuff.c:2228, which is this line:


while (st->frag_idx < skb_shinfo(st->cur_skb)->nr_frags) {


I added some printks in there and it looks like we hit this:

         } else if (st->root_skb == st->cur_skb &&
                    skb_shinfo(st->root_skb)->frag_list) {
                 st->cur_skb = skb_shinfo(st->root_skb)->frag_list;
                 st->frag_idx = 0;
                 goto next_skb;
         }


Then when we hit the goto and start again, and we oops when we hit that 
"while (st->frag_idx < skb_shinfo(st->cur_skb)->nr_frags)" line.

Re: [PANIC] lro + iscsi or lro + skb text search causes panic

[Herbert Xu]

On Wed, Jan 28, 2009 at 05:47:57PM +0530, Shyam_Iyer@Dell.com wrote:
> 
> --- skbuff.c.orig	2009-01-29 01:12:03.000000000 +0530
> +++ skbuff.c	2009-01-29 01:34:57.000000000 +0530
> @@ -2039,15 +2039,15 @@
>  		st->frag_data = NULL;
>  	}
>  
> -	if (st->cur_skb->next) {
> -		st->cur_skb = st->cur_skb->next;
> -		st->frag_idx = 0;
> -		goto next_skb;
> -	} else if (st->root_skb == st->cur_skb &&
> +	if (st->root_skb == st->cur_skb &&
>  		   skb_shinfo(st->root_skb)->frag_list) {
>  		st->cur_skb = skb_shinfo(st->root_skb)->frag_list;
>  		st->frag_idx=0;
>  		goto next_skb;
> +	} else if (st->cur_skb->next) {
> +		st->cur_skb = st->cur_skb->next;
> +		st->frag_idx = 0;
> +		goto next_skb;
>  	}
>  
>  	return 0;

Good catch!

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PANIC] lro + iscsi or lro + skb text search causes panic

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 29 Jan 2009 08:25:26 +1100

> On Wed, Jan 28, 2009 at 05:47:57PM +0530, Shyam_Iyer@Dell.com wrote:
> > 
> > --- skbuff.c.orig	2009-01-29 01:12:03.000000000 +0530
> > +++ skbuff.c	2009-01-29 01:34:57.000000000 +0530
> > @@ -2039,15 +2039,15 @@
> >  		st->frag_data = NULL;
> >  	}
> >  
> > -	if (st->cur_skb->next) {
> > -		st->cur_skb = st->cur_skb->next;
> > -		st->frag_idx = 0;
> > -		goto next_skb;
> > -	} else if (st->root_skb == st->cur_skb &&
> > +	if (st->root_skb == st->cur_skb &&
> >  		   skb_shinfo(st->root_skb)->frag_list) {
> >  		st->cur_skb = skb_shinfo(st->root_skb)->frag_list;
> >  		st->frag_idx=0;
> >  		goto next_skb;
> > +	} else if (st->cur_skb->next) {
> > +		st->cur_skb = st->cur_skb->next;
> > +		st->frag_idx = 0;
> > +		goto next_skb;
> >  	}
> >  
> >  	return 0;
> 
> Good catch!
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Excellent work everyone.  I applied both Herbert's and
Shyam's patches, and will queue them up for -stable as well.

Thanks again.

Re: [PANIC] lro + iscsi or lro + skb text search causes panic

[Mike Christie]

Mike Christie wrote:
> Herbert Xu wrote:
>> On Thu, Jan 22, 2009 at 12:55:21PM -0800, Brandeburg, Jesse wrote:
>>> I've filed this bugzilla a while ago.
>>> http://bugzilla.kernel.org/show_bug.cgi?id=11804
>>> now other customers are becoming interested as well
>>
>> Does this patch help?
>>
>> net: Fix frag_list handling in skb_seq_read
>>
>> The frag_list handling was broken ini skb_seq_read:
>>
>> 1) We didn't add the stepped offset when looking at the head
>> are of fragments other than the first.
>>
>> 2) The frag index wasn't reset.
>>
>> This patch fixes both issues.
>>
> 
> Without the patch I do not get the oops Jesse saw. The iscsi driver logs 
> in and I do not see a problem until running IO (scsi read commands). The 
> iscsi code thinks there is a missing packet at the iscsi level and 
> begins recovery at that level.
> 
> With the patch running against linus's git tree, my box locks up. You 
> cannot ping it. I do not get a oops or anything in the logs, and the 
> keyboard does not respond. I will try to get some oops output and more 
> info.
>

I am not able to get anything. Box is gone.

Jesse, without the patch you guys were getting an oops trace right? Did 
you get one with Herbert's patch?

RE: [PANIC] lro + iscsi or lro + skb text search causes panic

[Shyam_Iyer]

[-- Attachment #1: Type: text/plain, Size: 3614 bytes --]



Mike Christie wrote:
> Herbert Xu wrote:
>> On Sun, Jan 25, 2009 at 09:32:22PM -0800, David Miller wrote:
>>> From: Mike Christie <michaelc@cs.wisc.edu>
>>> Date: Thu, 22 Jan 2009 18:04:11 -0600
>>>
>>>> With the patch running against linus's git tree, my box locks up. 
>>>> You cannot ping it. I do not get a oops or anything in the logs,
and 
>>>> the keyboard does not respond. I will try to get some oops output 
>>>> and more info.
>>> Herbert, any idea offhand?
>> 
>> Yeah, I missed an offset update in there :) Here's a better version.
>> 
>> net: Fix frag_list handling in skb_seq_read
>> 
>> The frag_list handling was broken in skb_seq_read:
>> 
>> 1) We didn't add the stepped offset when looking at the head are of 
>> fragments other than the first.
>> 
>> 2) We didn't take the stepped offset away when setting the data 
>> pointer in the head area.
>> 
>> 3) The frag index wasn't reset.
>> 
>> This patch fixes both issues.
>> 

>It oopsd for me in skb_seq_read. addr2line said it was
linux-2.6/net/core/skbuff.c:2228, which is this line:


>while (st->frag_idx < skb_shinfo(st->cur_skb)->nr_frags) {


>I added some printks in there and it looks like we hit this:

>         } else if (st->root_skb == st->cur_skb &&
>                   skb_shinfo(st->root_skb)->frag_list) {
>                 st->cur_skb = skb_shinfo(st->root_skb)->frag_list;
>                 st->frag_idx = 0;
>                 goto next_skb;
>        }



Actually I did some testing and added a few printks and found that the
st->cur_skb->data was 0 and hence the ptr used by iscsi_tcp was null.
This caused the kernel panic.

 	if (abs_offset < block_limit) {
-		*data = st->cur_skb->data + abs_offset;
+		*data = st->cur_skb->data + (abs_offset -
st->stepped_offset);

 I enabled the debug_tcp and with a few printks found that the code in
my scenario did not go to the next_skb label as described by Mike and
could find that the sequence being followed was this -

It hit this if condition -
 if (st->cur_skb->next) {
                st->cur_skb = st->cur_skb->next;
                st->frag_idx = 0;
                goto next_skb;

And so, now the st pointer is shifted to the next skb whereas actually
it should have hit the second else if first since the data is in the
frag_list.

else if (st->root_skb == st->cur_skb &&
                   skb_shinfo(st->root_skb)->frag_list) {
                st->cur_skb = skb_shinfo(st->root_skb)->frag_list;
                goto next_skb;
        }


Reversing the two conditions the attached patch fixes the issue for me
on top of Herbert's patches.  I have done the testing on the ixgbe
adapter itself and verified the fix using some amount of data transfer
as well.

Signed-off-by: Shyam Iyer <shyam_iyer@dell.com>

--- skbuff.c.orig	2009-01-29 01:12:03.000000000 +0530
+++ skbuff.c	2009-01-29 01:34:57.000000000 +0530
@@ -2039,15 +2039,15 @@
 		st->frag_data = NULL;
 	}
 
-	if (st->cur_skb->next) {
-		st->cur_skb = st->cur_skb->next;
-		st->frag_idx = 0;
-		goto next_skb;
-	} else if (st->root_skb == st->cur_skb &&
+	if (st->root_skb == st->cur_skb &&
 		   skb_shinfo(st->root_skb)->frag_list) {
 		st->cur_skb = skb_shinfo(st->root_skb)->frag_list;
 		st->frag_idx=0;
 		goto next_skb;
+	} else if (st->cur_skb->next) {
+		st->cur_skb = st->cur_skb->next;
+		st->frag_idx = 0;
+		goto next_skb;
 	}
 
 	return 0;


 
P.S: Currently sending through an email client that can't do proper
patch formatting so please excuse the patch in an attachement.


[-- Attachment #2: skbuff_patch --]
[-- Type: application/octet-stream, Size: 604 bytes --]

--- skbuff.c.orig	2009-01-29 01:12:03.000000000 +0530
+++ skbuff.c	2009-01-29 01:34:57.000000000 +0530
@@ -2039,15 +2039,15 @@
 		st->frag_data = NULL;
 	}
 
-	if (st->cur_skb->next) {
-		st->cur_skb = st->cur_skb->next;
-		st->frag_idx = 0;
-		goto next_skb;
-	} else if (st->root_skb == st->cur_skb &&
+	if (st->root_skb == st->cur_skb &&
 		   skb_shinfo(st->root_skb)->frag_list) {
 		st->cur_skb = skb_shinfo(st->root_skb)->frag_list;
 		st->frag_idx=0;
 		goto next_skb;
+	} else if (st->cur_skb->next) {
+		st->cur_skb = st->cur_skb->next;
+		st->frag_idx = 0;
+		goto next_skb;
 	}
 
 	return 0;

Re: [PANIC] lro + iscsi or lro + skb text search causes panic

[Mike Christie]

Shyam_Iyer@Dell.com wrote:
> 
> Mike Christie wrote:
>> Herbert Xu wrote:
>>> On Sun, Jan 25, 2009 at 09:32:22PM -0800, David Miller wrote:
>>>> From: Mike Christie <michaelc@cs.wisc.edu>
>>>> Date: Thu, 22 Jan 2009 18:04:11 -0600
>>>>
>>>>> With the patch running against linus's git tree, my box locks up. 
>>>>> You cannot ping it. I do not get a oops or anything in the logs,
> and 
>>>>> the keyboard does not respond. I will try to get some oops output 
>>>>> and more info.
>>>> Herbert, any idea offhand?
>>> Yeah, I missed an offset update in there :) Here's a better version.
>>>
>>> net: Fix frag_list handling in skb_seq_read
>>>
>>> The frag_list handling was broken in skb_seq_read:
>>>
>>> 1) We didn't add the stepped offset when looking at the head are of 
>>> fragments other than the first.
>>>
>>> 2) We didn't take the stepped offset away when setting the data 
>>> pointer in the head area.
>>>
>>> 3) The frag index wasn't reset.
>>>
>>> This patch fixes both issues.
>>>
> 
>> It oopsd for me in skb_seq_read. addr2line said it was
> linux-2.6/net/core/skbuff.c:2228, which is this line:
> 
> 
>> while (st->frag_idx < skb_shinfo(st->cur_skb)->nr_frags) {
> 
> 
>> I added some printks in there and it looks like we hit this:
> 
>>         } else if (st->root_skb == st->cur_skb &&
>>                   skb_shinfo(st->root_skb)->frag_list) {
>>                 st->cur_skb = skb_shinfo(st->root_skb)->frag_list;
>>                 st->frag_idx = 0;
>>                 goto next_skb;
>>        }
> 
> 
> 
> Actually I did some testing and added a few printks and found that the
> st->cur_skb->data was 0 and hence the ptr used by iscsi_tcp was null.
> This caused the kernel panic.




Yeah, that is what Jesse saw too. I never got a null ptr though. That is 
probably why he oopsed, but I could login but would get what looked like 
corrupted packets instead of oopsing.




> 
>  	if (abs_offset < block_limit) {
> -		*data = st->cur_skb->data + abs_offset;
> +		*data = st->cur_skb->data + (abs_offset -
> st->stepped_offset);
> 
>  I enabled the debug_tcp and with a few printks found that the code in
> my scenario did not go to the next_skb label as described by Mike and
> could find that the sequence being followed was this -
> 
> It hit this if condition -
>  if (st->cur_skb->next) {
>                 st->cur_skb = st->cur_skb->next;
>                 st->frag_idx = 0;
>                 goto next_skb;



Yeah, I must have been cross eyed that night. I was hitting this too, 
and that caused me to hit the loop again.


> 
> And so, now the st pointer is shifted to the next skb whereas actually
> it should have hit the second else if first since the data is in the
> frag_list.
> 
> else if (st->root_skb == st->cur_skb &&
>                    skb_shinfo(st->root_skb)->frag_list) {
>                 st->cur_skb = skb_shinfo(st->root_skb)->frag_list;
>                 goto next_skb;
>         }
> 
> 
> Reversing the two conditions the attached patch fixes the issue for me
> on top of Herbert's patches.  I have done the testing on the ixgbe
> adapter itself and verified the fix using some amount of data transfer
> as well.
> 
> Signed-off-by: Shyam Iyer <shyam_iyer@dell.com>
> 
> --- skbuff.c.orig	2009-01-29 01:12:03.000000000 +0530
> +++ skbuff.c	2009-01-29 01:34:57.000000000 +0530
> @@ -2039,15 +2039,15 @@
>  		st->frag_data = NULL;
>  	
>  
> -	if (st->cur_skb->next) {
> -		st->cur_skb = st->cur_skb->next;
> -		st->frag_idx = 0;
> -		goto next_skb;
> -	} else if (st->root_skb == st->cur_skb &&
> +	if (st->root_skb == st->cur_skb &&
>  		   skb_shinfo(st->root_skb)->frag_list) {
>  		st->cur_skb = skb_shinfo(st->root_skb)->frag_list;
>  		st->frag_idx=0;


I think you diffed this against the wrong source. In the upstream code 
this is
st->frag_idx = 0;

so the patch does not apply.


>  		goto next_skb;
> +	} else if (st->cur_skb->next) {
> +		st->cur_skb = st->cur_skb->next;
> +		st->frag_idx = 0;
> +		goto next_skb;
>  	}
>  
>  	return 0;
> 

Nice catch. Patch works for me. Thanks!