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

login
register
mail settings
Submitter Herbert Xu
Date Jan. 26, 2009, 10:30 p.m.
Message ID <20090126223022.GA24046@gondor.apana.org.au>
Download mbox | patch
Permalink /patch/20346/
State Accepted
Delegated to: David Miller
Headers show

Comments

Herbert Xu - Jan. 26, 2009, 10:30 p.m.
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>

Cheers,
Jesse Brandeburg - Jan. 27, 2009, 1:40 a.m.
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.--
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
Herbert Xu - Jan. 27, 2009, 3:01 a.m.
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,
David Miller - Jan. 27, 2009, 5:52 a.m.
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.
--
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
Mike Christie - Jan. 27, 2009, 6:12 a.m.
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.
--
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
Herbert Xu - Jan. 28, 2009, 9:25 p.m.
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,
David Miller - Jan. 30, 2009, 12:13 a.m.
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.
--
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

Patch

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;
 	}