Skip to contentSkip to conversation

: Don't apply the offset of two allocations to the layer headers

PATCH In review Arnaldo Carvalho de Melo 2 participants 4 messages Mar 18, 2007

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 : Don't apply the offset of two allocations to the layer headers

Hi David,

Here is the bugger, it wasn't because {transport,network,mac}_header
is unsigned after all :-)

Please consider pulling from the usual place:

master.kernel.org:/pub/scm/linux/kernel/git/acme/net-2.6.22

Please lemme know how longer it takes to crash your workstation, I hope
not before next century 8-)

- Arnaldo

---
In pskb_expand_head and copy_skb_header (used in p?skb_copy(expand)?) the difference of two allocations were being applied to ->(transport|network|mac)_header, when this is not only not needed but causes problems, as those headers are now just offsets from ->head.

This fixes the problem with xfrm (skb_copy_checksum was the first place this bug would crash the machine, on the first packet being xfrm'ed), so reenable the offset code on 64bit arches.

---
 include/linux/skbuff.h |    4 +---
 net/core/skbuff.c      |   18 ++++++++++++++----
 2 files changed, 15 insertions(+), 7 deletions(-)
include/linux/skbuff.h +1 -3
index 673ca0c..3e4a2bd 100644
@@ -180,9 +180,7 @@ enum {
 	SKB_GSO_TCPV6 = 1 << 4,
 };
 
-/* #if BITS_PER_LONG > 32 */
-#if 0
-/* Disabled for now till the ipsec problem is fixed -acme */
+#if BITS_PER_LONG > 32
 #define NET_SKBUFF_DATA_USES_OFFSET 1
 #endif
 
net/core/skbuff.c +14 -4
index 5db345a..ea03cbf 100644
@@ -502,11 +502,12 @@ struct sk_buff *skb_clone(struct sk_buff *skb, gfp_t gfp_mask)
 
 static void copy_skb_header(struct sk_buff *new, const struct sk_buff *old)
 {
+#ifndef NET_SKBUFF_DATA_USES_OFFSET
 	/*
 	 *	Shift between the two data areas in bytes
 	 */
 	unsigned long offset = new->data - old->data;
-
+#endif
 	new->sk		= NULL;
 	new->dev	= old->dev;
 	new->priority	= old->priority;
@@ -515,9 +516,15 @@ static void copy_skb_header(struct sk_buff *new, const struct sk_buff *old)
 #ifdef CONFIG_INET
 	new->sp		= secpath_get(old->sp);
 #endif
-	new->transport_header = old->transport_header + offset;
-	new->network_header   = old->network_header + offset;
-	new->mac_header	      = old->mac_header + offset;
+	new->transport_header = old->transport_header;
+	new->network_header   = old->network_header;
+	new->mac_header	      = old->mac_header;
+#ifndef NET_SKBUFF_DATA_USES_OFFSET
+	/* {transport,network,mac}_header are relative to skb->head */
+	new->transport_header += offset;
+	new->network_header   += offset;
+	new->mac_header	      += offset;
+#endif
 	memcpy(new->cb, old->cb, sizeof(old->cb));
 	new->local_df	= old->local_df;
 	new->fclone	= SKB_FCLONE_UNAVAILABLE;
@@ -693,9 +700,12 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
 	skb->end      = data + size;
 	skb->data    += off;
 	skb->tail    += off;
+#ifndef NET_SKBUFF_DATA_USES_OFFSET
+	/* {transport,network,mac}_header are relative to skb->head */
 	skb->transport_header += off;
 	skb->network_header   += off;
 	skb->mac_header	      += off;
+#endif
 	skb->cloned   = 0;
 	skb->nohdr    = 0;
 	atomic_set(&skb_shinfo(skb)->dataref, 1);
Signature · 1 line
1.5.0.3
#1 lore raw
Quoted 15 lines · Arnaldo Carvalho de Melo

Hi David,

Here is the bugger, it wasn't because {transport,network,mac}_header
is unsigned after all :-)

Please consider pulling from the usual place:

master.kernel.org:/pub/scm/linux/kernel/git/acme/net-2.6.22

Please lemme know how longer it takes to crash your workstation, I hope
not before next century 8-)

- Arnaldo

Quoted 7 lines

---
In pskb_expand_head and copy_skb_header (used in p?skb_copy(expand)?) the difference of two allocations were being applied to ->(transport|network|mac)_header, when this is not only not needed but causes problems, as those headers are now just offsets from ->head.

This fixes the problem with xfrm (skb_copy_checksum was the first place this

Argh, skb_checksum_help! lemme fix this comment before its too late...

Quoted 78 lines

bug would crash the machine, on the first packet being xfrm'ed), so reenable the offset code on 64bit arches.

---
 include/linux/skbuff.h |    4 +---
 net/core/skbuff.c      |   18 ++++++++++++++----
 2 files changed, 15 insertions(+), 7 deletions(-)
include/linux/skbuff.h +1 -3
index 673ca0c..3e4a2bd 100644
@@ -180,9 +180,7 @@ enum {
        SKB_GSO_TCPV6 = 1 << 4,
 };
 
-/* #if BITS_PER_LONG > 32 */
-#if 0
-/* Disabled for now till the ipsec problem is fixed -acme */
+#if BITS_PER_LONG > 32
 #define NET_SKBUFF_DATA_USES_OFFSET 1
 #endif
 
net/core/skbuff.c +14 -5
index 5db345a..ea03cbf 100644
@@ -502,11 +502,12 @@ struct sk_buff *skb_clone(struct sk_buff *skb, gfp_t gfp_mask)
 
 static void copy_skb_header(struct sk_buff *new, const struct sk_buff *old)
 {
+#ifndef NET_SKBUFF_DATA_USES_OFFSET
        /*
         *      Shift between the two data areas in bytes
         */
        unsigned long offset = new->data - old->data;
-
+#endif
        new->sk         = NULL;
        new->dev        = old->dev;
        new->priority   = old->priority;
@@ -515,9 +516,15 @@ static void copy_skb_header(struct sk_buff *new, const struct sk_buff *old)
 #ifdef CONFIG_INET
        new->sp         = secpath_get(old->sp);
 #endif
-       new->transport_header = old->transport_header + offset;
-       new->network_header   = old->network_header + offset;
-       new->mac_header       = old->mac_header + offset;
+       new->transport_header = old->transport_header;
+       new->network_header   = old->network_header;
+       new->mac_header       = old->mac_header;
+#ifndef NET_SKBUFF_DATA_USES_OFFSET
+       /* {transport,network,mac}_header are relative to skb->head */
+       new->transport_header += offset;
+       new->network_header   += offset;
+       new->mac_header       += offset;
+#endif
        memcpy(new->cb, old->cb, sizeof(old->cb));
        new->local_df   = old->local_df;
        new->fclone     = SKB_FCLONE_UNAVAILABLE;
@@ -693,9 +700,12 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
        skb->end      = data + size;
        skb->data    += off;
        skb->tail    += off;
+#ifndef NET_SKBUFF_DATA_USES_OFFSET
+       /* {transport,network,mac}_header are relative to skb->head */
        skb->transport_header += off;
        skb->network_header   += off;
        skb->mac_header       += off;
+#endif
        skb->cloned   = 0;
        skb->nohdr    = 0;
        atomic_set(&skb_shinfo(skb)->dataref, 1);
--

1.5.0.3

#2 lore raw
Quoted 22 lines · Arnaldo Carvalho de Melo
Quoted 21 lines · Arnaldo Carvalho de Melo

Hi David,

Here is the bugger, it wasn't because {transport,network,mac}_header
is unsigned after all :-)

Please consider pulling from the usual place:

master.kernel.org:/pub/scm/linux/kernel/git/acme/net-2.6.22

Please lemme know how longer it takes to crash your workstation, I hope
not before next century 8-)

- Arnaldo

---
In pskb_expand_head and copy_skb_header (used in p?skb_copy(expand)?) the difference of two allocations were being applied to ->(transport|network|mac)_header, when this is not only not needed but causes problems, as those headers are now just offsets from ->head.

Quoted 3 lines
Quoted 1 line

This fixes the problem with xfrm (skb_copy_checksum was the first place this

Argh, skb_checksum_help! lemme fix this comment before its too late...

Done!

- Arnaldo

maintainer
#3 lore raw
In reply to Arnaldo Carvalho de Melo

From: Arnaldo Carvalho de Melo <[email protected]>
Date: Sun, 18 Mar 2007 00:12:24 -0300

Quoted 7 lines

Here is the bugger, it wasn't because {transport,network,mac}_header
is unsigned after all :-)

Please consider pulling from the usual place:

master.kernel.org:/pub/scm/linux/kernel/git/acme/net-2.6.22

Pulled and pushed out, thanks a lot!

Quoted 2 lines

Please lemme know how longer it takes to crash your workstation, I hope
not before next century 8-)

I'll play with it after I figure out another bug I'm working on first :)