Is it a possible bug in dev_gro_receive()?

PATCH In review Xin Xiaohui 4 participants 9 messages Jul 30, 2010 – Aug 18, 2010

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Xin Xiaohui

author

2010-07-30 01:40 UTC #0 lore raw

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

net/core/dev.c +0 -7

index 264137f..28cdbbf 100644
@@ -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:

Signature · 1 line

1.5.4.4

Jarek Poplawski

2010-08-02 10:29 UTC #1 lore raw

Quoted 9 lines · 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?

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.

Quoted 23 lines

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

net/core/dev.c +0 -7

index 264137f..28cdbbf 100644
@@ -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:

Herbert Xu

2010-08-02 11:04 UTC #2 lore raw

Quoted 9 lines · Jarek Poplawski

Quoted 8 lines · 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.

Quoted 6 lines

Quoted 1 line
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,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Xin, Xiaohui

author

2010-08-03 02:34 UTC #3 lore raw

In reply to Jarek Poplawski

Quoted 23 lines

-----Original Message-----
From: Jarek Poplawski [mailto:[email protected]]
Sent: Monday, August 02, 2010 6:29 PM
To: Xin, Xiaohui
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: Is it a possible bug in dev_gro_receive()?
Quoted 9 lines · 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?
I think, you're right, but fixing memmove looks nicer to me:
(quoted diff) +1 -1
-	--skb_shinfo(skb)->nr_frags);
+	--skb_shinfo(skb)->nr_frags * sizeof(skb_frag_t));

Quoted 1 line

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

Quoted 26 lines

Quoted 24 lines


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

net/core/dev.c +0 -7

index 264137f..28cdbbf 100644
@@ -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:

Jarek Poplawski

2010-08-03 06:45 UTC #4 lore raw

Quoted 23 lines · Xiaohui

Quoted 23 lines
-----Original Message-----
From: Jarek Poplawski [mailto:[email protected]]
Sent: Monday, August 02, 2010 6:29 PM
To: Xin, Xiaohui
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: Is it a possible bug in dev_gro_receive()?
Quoted 9 lines · 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?
I think, you're right, but fixing memmove looks nicer to me:
(quoted diff) +1 -1
-	--skb_shinfo(skb)->nr_frags);
+	--skb_shinfo(skb)->nr_frags * sizeof(skb_frag_t));

Quoted 7 lines

Quoted 1 line
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.

Quoted 30 lines

Thanks
Xiaohui

Quoted 26 lines

Quoted 24 lines


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

net/core/dev.c +0 -7

index 264137f..28cdbbf 100644
@@ -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:

Xin, Xiaohui

author

2010-08-10 08:12 UTC #5 lore raw

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

Quoted 80 lines

-----Original Message-----
From: Jarek Poplawski [mailto:[email protected]]
Sent: Tuesday, August 03, 2010 2:45 PM
To: Xin, Xiaohui
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: Is it a possible bug in dev_gro_receive()?
Quoted 18 lines · Xiaohui
Quoted 18 lines
-----Original Message-----
From: Jarek Poplawski [mailto:[email protected]]
Sent: Monday, August 02, 2010 6:29 PM
To: Xin, Xiaohui
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: Is it a possible bug in dev_gro_receive()?
Quoted 9 lines · 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?
Quoted 8 lines
Quoted 6 lines
I think, you're right, but fixing memmove looks nicer to me:
(quoted diff) +1 -1
-	--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?
Quoted 2 lines
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
Quoted 2 lines
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.
Quoted 30 lines
Thanks
Xiaohui
Quoted 26 lines
Quoted 24 lines
---
 net/core/dev.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)
net/core/dev.c +0 -7
index 264137f..28cdbbf 100644
@@ -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:

Jarek Poplawski

2010-08-10 08:34 UTC #6 lore raw

Quoted 4 lines · 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.

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.

Quoted 84 lines

Thanks
Xiaohui
Quoted 80 lines
-----Original Message-----
From: Jarek Poplawski [mailto:[email protected]]
Sent: Tuesday, August 03, 2010 2:45 PM
To: Xin, Xiaohui
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: Is it a possible bug in dev_gro_receive()?
Quoted 18 lines · Xiaohui
Quoted 18 lines
-----Original Message-----
From: Jarek Poplawski [mailto:[email protected]]
Sent: Monday, August 02, 2010 6:29 PM
To: Xin, Xiaohui
Cc: [email protected]; [email protected]; [email protected]
Subject: Re: Is it a possible bug in dev_gro_receive()?
Quoted 9 lines · 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?
Quoted 8 lines
Quoted 6 lines
I think, you're right, but fixing memmove looks nicer to me:
(quoted diff) +1 -1
-	--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?
Quoted 2 lines
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
Quoted 2 lines
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.
Quoted 30 lines
Thanks
Xiaohui
Quoted 26 lines
Quoted 24 lines
---
 net/core/dev.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)
net/core/dev.c +0 -7
index 264137f..28cdbbf 100644
@@ -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:

Jarek Poplawski

2010-08-11 12:02 UTC #7 lore raw

PATCH 1/1 net: Fix a memmove bug in dev_gro_receive()

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

Quoted 10 lines · Jarek Poplawski

Quoted 4 lines · 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.
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...
--------------------------->

Quoted 8 lines

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" <[email protected]>

Signed-off-by Jarek Poplawski <[email protected]>

---

net/core/dev.c +1 -1

index 1ae6543..3721fbb 100644
@@ -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));
 		}
 	}

David Miller

maintainer

2010-08-18 00:37 UTC #8 lore raw

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

From: Jarek Poplawski <[email protected]>
Date: Wed, 11 Aug 2010 12:02:10 +0000

Quoted 8 lines

Quoted 8 lines
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.

Quoted 6 lines

...
This version of the patch fixes the bug directly in memmove.
Reported-by "Xin, Xiaohui" <[email protected]>
Signed-off-by Jarek Poplawski <[email protected]>

Applied thanks a lot Jarek.