[PATCH] fix long-standing bug in 2.6/2.4 skb_copy/skb_copy_expand

[PATCH] fix long-standing bug in 2.6/2.4 skb_copy/skb_copy_expand

[Stuffed Crust]

[-- Attachment #1.1: Type: text/plain, Size: 1937 bytes --]

Signed-off-by: Solomon Peachy <pizza@shaftnet.org>

This patch tweaks the skb_copy_bits() call in skb_copy() and 
skb_copy_expand().  In the sace of skb_copy():

if (skb_copy_bits(skb, -headerlen, n->head, headerlen + skb->len))

Basically, this call assumes that n->head+headerlen == n->data.

This is, fortunately, generally true.  But if the alloc_skb function 
allocates extra head room (ie calls skb_reserve() on the skb before it 
passes it to the callee, this doesn't quite work.  Instead, it should be 
rewritten as:

if (skb_copy_bits(skb, -headerlen, n->data-headerlen, headerlen + skb->len))

Rewriting it this way works; n->data-headerlen is equal to n->data 
before the skb_reserve() call.  This seems MoreCorrect(tm), as it makes 
no assumptions about the state of the skb passed into it.  (n->data just 
so happens to equal n->head too)

skb_copy_expand() has the same problem as well, and has a similar fix.

This patch is against 2.6.12-rc4, though it should apply cleanly to any 
2.4/2.6 kernel.

...

The history behind this is a little sordid -- We were trying to
implement a "poor man's zerocopy" transmit path for a braindead USB
wireless controller.  It needed a descriptor packet prepended to the
frame contents, but couldn't handle it in a separate USB packet -- so
we'd have to do a realloc on the skb to give us the headroom we eneded. 
memcpy()s on the very underpowered target were expensive, so we tried
modifying skb_alloc to always ensure there would be enough headroom for
the descriptor (allocating extra, and then skb_reserve()ing it).  It was
a crude hack, but it gained us a few much-needed percentage points of
throughput.  That is once we fixed skb_copy()..

Anyway, please consider this patch for inclusion. 

 - Solomon
-- 
Solomon Peachy        				 ICQ: 1318344
Melbourne, FL 					 JID: pitha@myjabber.net
Quidquid latine dictum sit, altum viditur

[-- Attachment #1.2: skb_copy_fixes.diff --]
[-- Type: text/plain, Size: 704 bytes --]

--- /linux/net/core/skbuff.c	2005-05-08 09:57:37.000000000 -0400
+++ skbuff.c	2005-05-08 10:27:17.000000000 -0400
@@ -486,7 +486,7 @@
 	n->csum	     = skb->csum;
 	n->ip_summed = skb->ip_summed;
 
-	if (skb_copy_bits(skb, -headerlen, n->head, headerlen + skb->len))
+	if (skb_copy_bits(skb, -headerlen, n->data-headerlen, headerlen + skb->len))
 		BUG();
 
 	copy_skb_header(n, skb);
@@ -680,7 +680,7 @@
 		head_copy_off = newheadroom - head_copy_len;
 
 	/* Copy the linear header and data. */
-	if (skb_copy_bits(skb, -head_copy_len, n->head + head_copy_off,
+	if (skb_copy_bits(skb, -head_copy_len, n->data-newheadroom + head_copy_off,
 			  skb->len + head_copy_len))
 		BUG();
 

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] fix long-standing bug in 2.6/2.4 skb_copy/skb_copy_expand

[Herbert Xu]

Stuffed Crust <pizza@shaftnet.org> wrote:
> 
> This is, fortunately, generally true.  But if the alloc_skb function 
> allocates extra head room (ie calls skb_reserve() on the skb before it 
> passes it to the callee, this doesn't quite work.  Instead, it should be 
> rewritten as:

As far as I know the alloc_skb funciton in the kernel tree doesn't do
that so your patch is not necessary unless we decide to change the way
alloc_skb works.  If that's what you want then please provide a patch
to alloc_skb and a rationale as to why we should do that.

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: [PATCH] fix long-standing bug in 2.6/2.4 skb_copy/skb_copy_expand

[Stuffed Crust]

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

On Mon, May 09, 2005 at 01:04:34PM +1000, Herbert Xu wrote:
> > This is, fortunately, generally true.  But if the alloc_skb function 
> > allocates extra head room (ie calls skb_reserve() on the skb before it 
> > passes it to the callee, this doesn't quite work.  Instead, it should be 
> > rewritten as:
> 
> As far as I know the alloc_skb funciton in the kernel tree doesn't do
> that so your patch is not necessary unless we decide to change the way
> alloc_skb works.  If that's what you want then please provide a patch
> to alloc_skb and a rationale as to why we should do that.

It does not, and I have no intention of submitting a patch to change it. 
As I said in my original message, it was a crude hack which has since
been relegated to the great bitbucket of the sky.  All that's left is
that "bugfix" patch.

I've performed my due-diligence in airing it to the powers that be, so 
I'll go way now.

 - Solomon
-- 
Solomon Peachy        				 ICQ: 1318344
Melbourne, FL 					 JID: pitha@myjabber.net
Quidquid latine dictum sit, altum viditur

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] fix long-standing bug in 2.6/2.4 skb_copy/skb_copy_expand

[David S. Miller]

On Sun, 8 May 2005 10:32:59 -0400
Stuffed Crust <pizza@shaftnet.org> wrote:

> But if the alloc_skb function 
> allocates extra head room (ie calls skb_reserve() on the skb before it 
> passes it to the callee, this doesn't quite work.

alloc_skb() may call skb_reserve() in your tree, but it doesn't in anyone
else's.