Is it a possible bug in dev_gro_receive()?

Is it a possible bug in dev_gro_receive()?

[Xin Xiaohui]

I looked into the code dev_gro_receive(), found the code here:
if the frags[0] is pulled to 0, then the page will be released,
and memmove() frags left.
Is that right? I'm not sure if memmove do right or not, but
frags[0].size is never set after memove at least. what I think
a simple way is not to do anything if we found frags[0].size == 0.
The patch is as followed.

Or am I missing something here?

---
 net/core/dev.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 264137f..28cdbbf 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2730,13 +2730,6 @@ pull:
 
 		skb_shinfo(skb)->frags[0].page_offset += grow;
 		skb_shinfo(skb)->frags[0].size -= grow;
-
-		if (unlikely(!skb_shinfo(skb)->frags[0].size)) {
-			put_page(skb_shinfo(skb)->frags[0].page);
-			memmove(skb_shinfo(skb)->frags,
-				skb_shinfo(skb)->frags + 1,
-				--skb_shinfo(skb)->nr_frags);
-		}
 	}
 
 ok:
-- 
1.5.4.4

Re: Is it a possible bug in dev_gro_receive()?

[Jarek Poplawski]

Xin Xiaohui wrote:
> I looked into the code dev_gro_receive(), found the code here:
> if the frags[0] is pulled to 0, then the page will be released,
> and memmove() frags left.
> Is that right? I'm not sure if memmove do right or not, but
> frags[0].size is never set after memove at least. what I think
> a simple way is not to do anything if we found frags[0].size == 0.
> The patch is as followed.
> 
> Or am I missing something here?

I think, you're right, but fixing memmove looks nicer to me:

 -	--skb_shinfo(skb)->nr_frags);
 +	--skb_shinfo(skb)->nr_frags * sizeof(skb_frag_t));

Jarek P.

> 
> ---
>  net/core/dev.c |    7 -------
>  1 files changed, 0 insertions(+), 7 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 264137f..28cdbbf 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2730,13 +2730,6 @@ pull:
>  
>  		skb_shinfo(skb)->frags[0].page_offset += grow;
>  		skb_shinfo(skb)->frags[0].size -= grow;
> -
> -		if (unlikely(!skb_shinfo(skb)->frags[0].size)) {
> -			put_page(skb_shinfo(skb)->frags[0].page);
> -			memmove(skb_shinfo(skb)->frags,
> -				skb_shinfo(skb)->frags + 1,
> -				--skb_shinfo(skb)->nr_frags);
> -		}
>  	}
>  
>  ok:

Re: Is it a possible bug in dev_gro_receive()?

[Herbert Xu]

On Mon, Aug 02, 2010 at 10:29:06AM +0000, Jarek Poplawski wrote:
> Xin Xiaohui wrote:
> > I looked into the code dev_gro_receive(), found the code here:
> > if the frags[0] is pulled to 0, then the page will be released,
> > and memmove() frags left.
> > Is that right? I'm not sure if memmove do right or not, but
> > frags[0].size is never set after memove at least. what I think
> > a simple way is not to do anything if we found frags[0].size == 0.
> > The patch is as followed.
> > 
> > Or am I missing something here?
> 
> I think, you're right, but fixing memmove looks nicer to me:
> 
>  -	--skb_shinfo(skb)->nr_frags);
>  +	--skb_shinfo(skb)->nr_frags * sizeof(skb_frag_t));

I agree with the diagnosis and your proposed fix.

Thanks for catching this Xiaohui!

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: Is it a possible bug in dev_gro_receive()?

[Xin, Xiaohui]

>-----Original Message-----
>From: Jarek Poplawski [mailto:jarkao2@gmail.com]
>Sent: Monday, August 02, 2010 6:29 PM
>To: Xin, Xiaohui
>Cc: netdev@vger.kernel.org; herbert@gondor.apana.org.au; davem@davemloft.net
>Subject: Re: Is it a possible bug in dev_gro_receive()?
>
>Xin Xiaohui wrote:
>> I looked into the code dev_gro_receive(), found the code here:
>> if the frags[0] is pulled to 0, then the page will be released,
>> and memmove() frags left.
>> Is that right? I'm not sure if memmove do right or not, but
>> frags[0].size is never set after memove at least. what I think
>> a simple way is not to do anything if we found frags[0].size == 0.
>> The patch is as followed.
>>
>> Or am I missing something here?
>
>I think, you're right, but fixing memmove looks nicer to me:
>
> -	--skb_shinfo(skb)->nr_frags);
> +	--skb_shinfo(skb)->nr_frags * sizeof(skb_frag_t));
>
>Jarek P.

Is there a little hurt of performance to do memmove() if skb_shinfo(skb)->nr_frags is large?
We're now working on the zero-copy patches based on napi_gro_frags() interface, and in 
this case, we have found a lot of skbs which frags[0] is pulled to 0. And after the memmove is
fixed, each frags[x].size is needed to modify too.
So I think don't do anything is better. Or is there any side effect with a null page in the stack?

Thanks
Xiaohui
>
>>
>> ---
>>  net/core/dev.c |    7 -------
>>  1 files changed, 0 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 264137f..28cdbbf 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -2730,13 +2730,6 @@ pull:
>>
>>  		skb_shinfo(skb)->frags[0].page_offset += grow;
>>  		skb_shinfo(skb)->frags[0].size -= grow;
>> -
>> -		if (unlikely(!skb_shinfo(skb)->frags[0].size)) {
>> -			put_page(skb_shinfo(skb)->frags[0].page);
>> -			memmove(skb_shinfo(skb)->frags,
>> -				skb_shinfo(skb)->frags + 1,
>> -				--skb_shinfo(skb)->nr_frags);
>> -		}
>>  	}
>>
>>  ok:
>
>

Re: Is it a possible bug in dev_gro_receive()?

[Jarek Poplawski]

On Tue, Aug 03, 2010 at 10:33:24AM +0800, Xin, Xiaohui wrote:
> >-----Original Message-----
> >From: Jarek Poplawski [mailto:jarkao2@gmail.com]
> >Sent: Monday, August 02, 2010 6:29 PM
> >To: Xin, Xiaohui
> >Cc: netdev@vger.kernel.org; herbert@gondor.apana.org.au; davem@davemloft.net
> >Subject: Re: Is it a possible bug in dev_gro_receive()?
> >
> >Xin Xiaohui wrote:
> >> I looked into the code dev_gro_receive(), found the code here:
> >> if the frags[0] is pulled to 0, then the page will be released,
> >> and memmove() frags left.
> >> Is that right? I'm not sure if memmove do right or not, but
> >> frags[0].size is never set after memove at least. what I think
> >> a simple way is not to do anything if we found frags[0].size == 0.
> >> The patch is as followed.
> >>
> >> Or am I missing something here?
> >
> >I think, you're right, but fixing memmove looks nicer to me:
> >
> > -	--skb_shinfo(skb)->nr_frags);
> > +	--skb_shinfo(skb)->nr_frags * sizeof(skb_frag_t));
> >
> >Jarek P.
> 
> Is there a little hurt of performance to do memmove() if skb_shinfo(skb)->nr_frags is large?
> We're now working on the zero-copy patches based on napi_gro_frags() interface, and in 
> this case, we have found a lot of skbs which frags[0] is pulled to 0. And after the memmove is
> fixed, each frags[x].size is needed to modify too.
> So I think don't do anything is better. Or is there any side effect with a null page in the stack?

Even if it's better, generally you should separate fixes from
optimizations. On the other hand, it was expected to be "unlikely" by
design, so you should probably explain more why it has to be changed
here too.

Thanks,
Jarek P.

> 
> Thanks
> Xiaohui
> >
> >>
> >> ---
> >>  net/core/dev.c |    7 -------
> >>  1 files changed, 0 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/net/core/dev.c b/net/core/dev.c
> >> index 264137f..28cdbbf 100644
> >> --- a/net/core/dev.c
> >> +++ b/net/core/dev.c
> >> @@ -2730,13 +2730,6 @@ pull:
> >>
> >>  		skb_shinfo(skb)->frags[0].page_offset += grow;
> >>  		skb_shinfo(skb)->frags[0].size -= grow;
> >> -
> >> -		if (unlikely(!skb_shinfo(skb)->frags[0].size)) {
> >> -			put_page(skb_shinfo(skb)->frags[0].page);
> >> -			memmove(skb_shinfo(skb)->frags,
> >> -				skb_shinfo(skb)->frags + 1,
> >> -				--skb_shinfo(skb)->nr_frags);
> >> -		}
> >>  	}
> >>
> >>  ok:
> >
> >
> 

RE: Is it a possible bug in dev_gro_receive()?

[Xin, Xiaohui]

Jarek,
Seems community agree with your patch more.
So may you send out your patch then? Thanks!
Some of my related patches still need this fix.

Thanks
Xiaohui

>-----Original Message-----
>From: Jarek Poplawski [mailto:jarkao2@gmail.com]
>Sent: Tuesday, August 03, 2010 2:45 PM
>To: Xin, Xiaohui
>Cc: netdev@vger.kernel.org; herbert@gondor.apana.org.au; davem@davemloft.net
>Subject: Re: Is it a possible bug in dev_gro_receive()?
>
>On Tue, Aug 03, 2010 at 10:33:24AM +0800, Xin, Xiaohui wrote:
>> >-----Original Message-----
>> >From: Jarek Poplawski [mailto:jarkao2@gmail.com]
>> >Sent: Monday, August 02, 2010 6:29 PM
>> >To: Xin, Xiaohui
>> >Cc: netdev@vger.kernel.org; herbert@gondor.apana.org.au; davem@davemloft.net
>> >Subject: Re: Is it a possible bug in dev_gro_receive()?
>> >
>> >Xin Xiaohui wrote:
>> >> I looked into the code dev_gro_receive(), found the code here:
>> >> if the frags[0] is pulled to 0, then the page will be released,
>> >> and memmove() frags left.
>> >> Is that right? I'm not sure if memmove do right or not, but
>> >> frags[0].size is never set after memove at least. what I think
>> >> a simple way is not to do anything if we found frags[0].size == 0.
>> >> The patch is as followed.
>> >>
>> >> Or am I missing something here?
>> >
>> >I think, you're right, but fixing memmove looks nicer to me:
>> >
>> > -	--skb_shinfo(skb)->nr_frags);
>> > +	--skb_shinfo(skb)->nr_frags * sizeof(skb_frag_t));
>> >
>> >Jarek P.
>>
>> Is there a little hurt of performance to do memmove() if skb_shinfo(skb)->nr_frags is
>large?
>> We're now working on the zero-copy patches based on napi_gro_frags() interface, and in
>> this case, we have found a lot of skbs which frags[0] is pulled to 0. And after the memmove
>is
>> fixed, each frags[x].size is needed to modify too.
>> So I think don't do anything is better. Or is there any side effect with a null page in the
>stack?
>
>Even if it's better, generally you should separate fixes from
>optimizations. On the other hand, it was expected to be "unlikely" by
>design, so you should probably explain more why it has to be changed
>here too.
>
>Thanks,
>Jarek P.
>
>>
>> Thanks
>> Xiaohui
>> >
>> >>
>> >> ---
>> >>  net/core/dev.c |    7 -------
>> >>  1 files changed, 0 insertions(+), 7 deletions(-)
>> >>
>> >> diff --git a/net/core/dev.c b/net/core/dev.c
>> >> index 264137f..28cdbbf 100644
>> >> --- a/net/core/dev.c
>> >> +++ b/net/core/dev.c
>> >> @@ -2730,13 +2730,6 @@ pull:
>> >>
>> >>  		skb_shinfo(skb)->frags[0].page_offset += grow;
>> >>  		skb_shinfo(skb)->frags[0].size -= grow;
>> >> -
>> >> -		if (unlikely(!skb_shinfo(skb)->frags[0].size)) {
>> >> -			put_page(skb_shinfo(skb)->frags[0].page);
>> >> -			memmove(skb_shinfo(skb)->frags,
>> >> -				skb_shinfo(skb)->frags + 1,
>> >> -				--skb_shinfo(skb)->nr_frags);
>> >> -		}
>> >>  	}
>> >>
>> >>  ok:
>> >
>> >
>>

Re: Is it a possible bug in dev_gro_receive()?

[Jarek Poplawski]

On Tue, Aug 10, 2010 at 04:11:54PM +0800, Xin, Xiaohui wrote:
> Jarek,
> Seems community agree with your patch more.
> So may you send out your patch then? Thanks!
> Some of my related patches still need this fix.

Hmm... But there was no my patch. Only a tiny, cosmetical suggestion
to your patch. I'd be glad if you add some credit or my "Acked-by",
of course. But if you really have a big problem, e.g. you don't like
my suggestion, please confirm.

Thanks,
Jarek P.

> 
> Thanks
> Xiaohui
> 
> >-----Original Message-----
> >From: Jarek Poplawski [mailto:jarkao2@gmail.com]
> >Sent: Tuesday, August 03, 2010 2:45 PM
> >To: Xin, Xiaohui
> >Cc: netdev@vger.kernel.org; herbert@gondor.apana.org.au; davem@davemloft.net
> >Subject: Re: Is it a possible bug in dev_gro_receive()?
> >
> >On Tue, Aug 03, 2010 at 10:33:24AM +0800, Xin, Xiaohui wrote:
> >> >-----Original Message-----
> >> >From: Jarek Poplawski [mailto:jarkao2@gmail.com]
> >> >Sent: Monday, August 02, 2010 6:29 PM
> >> >To: Xin, Xiaohui
> >> >Cc: netdev@vger.kernel.org; herbert@gondor.apana.org.au; davem@davemloft.net
> >> >Subject: Re: Is it a possible bug in dev_gro_receive()?
> >> >
> >> >Xin Xiaohui wrote:
> >> >> I looked into the code dev_gro_receive(), found the code here:
> >> >> if the frags[0] is pulled to 0, then the page will be released,
> >> >> and memmove() frags left.
> >> >> Is that right? I'm not sure if memmove do right or not, but
> >> >> frags[0].size is never set after memove at least. what I think
> >> >> a simple way is not to do anything if we found frags[0].size == 0.
> >> >> The patch is as followed.
> >> >>
> >> >> Or am I missing something here?
> >> >
> >> >I think, you're right, but fixing memmove looks nicer to me:
> >> >
> >> > -	--skb_shinfo(skb)->nr_frags);
> >> > +	--skb_shinfo(skb)->nr_frags * sizeof(skb_frag_t));
> >> >
> >> >Jarek P.
> >>
> >> Is there a little hurt of performance to do memmove() if skb_shinfo(skb)->nr_frags is
> >large?
> >> We're now working on the zero-copy patches based on napi_gro_frags() interface, and in
> >> this case, we have found a lot of skbs which frags[0] is pulled to 0. And after the memmove
> >is
> >> fixed, each frags[x].size is needed to modify too.
> >> So I think don't do anything is better. Or is there any side effect with a null page in the
> >stack?
> >
> >Even if it's better, generally you should separate fixes from
> >optimizations. On the other hand, it was expected to be "unlikely" by
> >design, so you should probably explain more why it has to be changed
> >here too.
> >
> >Thanks,
> >Jarek P.
> >
> >>
> >> Thanks
> >> Xiaohui
> >> >
> >> >>
> >> >> ---
> >> >>  net/core/dev.c |    7 -------
> >> >>  1 files changed, 0 insertions(+), 7 deletions(-)
> >> >>
> >> >> diff --git a/net/core/dev.c b/net/core/dev.c
> >> >> index 264137f..28cdbbf 100644
> >> >> --- a/net/core/dev.c
> >> >> +++ b/net/core/dev.c
> >> >> @@ -2730,13 +2730,6 @@ pull:
> >> >>
> >> >>  		skb_shinfo(skb)->frags[0].page_offset += grow;
> >> >>  		skb_shinfo(skb)->frags[0].size -= grow;
> >> >> -
> >> >> -		if (unlikely(!skb_shinfo(skb)->frags[0].size)) {
> >> >> -			put_page(skb_shinfo(skb)->frags[0].page);
> >> >> -			memmove(skb_shinfo(skb)->frags,
> >> >> -				skb_shinfo(skb)->frags + 1,
> >> >> -				--skb_shinfo(skb)->nr_frags);
> >> >> -		}
> >> >>  	}
> >> >>
> >> >>  ok:
> >> >
> >> >
> >>

[PATCH] net: Fix a memmove bug in dev_gro_receive()

[Jarek Poplawski]

[was: Re: Is it a possible bug in dev_gro_receive()?]
On Tue, Aug 10, 2010 at 08:34:26AM +0000, Jarek Poplawski wrote:
> On Tue, Aug 10, 2010 at 04:11:54PM +0800, Xin, Xiaohui wrote:
> > Jarek,
> > Seems community agree with your patch more.
> > So may you send out your patch then? Thanks!
> > Some of my related patches still need this fix.
> 
> Hmm... But there was no my patch. Only a tiny, cosmetical suggestion
> to your patch. I'd be glad if you add some credit or my "Acked-by",
> of course. But if you really have a big problem, e.g. you don't like
> my suggestion, please confirm.

Hmm#2... OK, it's probably something with my English, but since it
seems to take too long, here it is. Xiaohui, I hope you'll send your
"Signed-off-by" at least.

Thanks,
Jarek P.

PS: I know, there is a bit too long line...
--------------------------->

>Xin Xiaohui wrote:
> I looked into the code dev_gro_receive(), found the code here:
> if the frags[0] is pulled to 0, then the page will be released,
> and memmove() frags left.
> Is that right? I'm not sure if memmove do right or not, but
> frags[0].size is never set after memove at least. what I think
> a simple way is not to do anything if we found frags[0].size == 0.
> The patch is as followed.
...

This version of the patch fixes the bug directly in memmove.

Reported-by: "Xin, Xiaohui" <xiaohui.xin@intel.com>
Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

---

diff --git a/net/core/dev.c b/net/core/dev.c
index 1ae6543..3721fbb 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3143,7 +3143,7 @@ pull:
 			put_page(skb_shinfo(skb)->frags[0].page);
 			memmove(skb_shinfo(skb)->frags,
 				skb_shinfo(skb)->frags + 1,
-				--skb_shinfo(skb)->nr_frags);
+				--skb_shinfo(skb)->nr_frags * sizeof(skb_frag_t));
 		}
 	}
 

Re: [PATCH] net: Fix a memmove bug in dev_gro_receive()

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Wed, 11 Aug 2010 12:02:10 +0000

>>Xin Xiaohui wrote:
>> I looked into the code dev_gro_receive(), found the code here:
>> if the frags[0] is pulled to 0, then the page will be released,
>> and memmove() frags left.
>> Is that right? I'm not sure if memmove do right or not, but
>> frags[0].size is never set after memove at least. what I think
>> a simple way is not to do anything if we found frags[0].size == 0.
>> The patch is as followed.
> ...
> 
> This version of the patch fixes the bug directly in memmove.
> 
> Reported-by: "Xin, Xiaohui" <xiaohui.xin@intel.com>
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

Applied thanks a lot Jarek.