Handle shared SKBs in VLAN receive code

PATCH In review Tommy Christensen 3 participants 9 messages Oct 10, 2003 – Oct 13, 2003

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Tommy Christensen

author

2003-10-10 23:38 UTC #0 lore raw

PATCH 1/1 Handle shared SKBs in VLAN receive code

As noted by Jonas Munsin, the vlan patch that went into 2.6.0-test7 did a kfree() rather than a kfree_skb(). Hmm.

Instead of just fixing this up, I suggest we do things the right way:

The VLAN code have long been claiming to handle shared SKBs, without actually doing so. I have now added the call to skb_share_check().

This enables us to simply do a skb_unshare() when mangling the ethernet header.

Patch is against linux-2.6.0-test7 (applies to 2.4.23-pre7 as well).

-Tommy

net/8021q/vlan_dev.c +14 -9

diff -ru linux-2.6.0-test7/net/8021q/vlan_dev.c linux-2.6/net/8021q/vlan_dev.c
@@ -74,11 +74,7 @@
 static inline struct sk_buff *vlan_check_reorder_header(struct sk_buff *skb)
 {
 	if (VLAN_DEV_INFO(skb->dev)->flags & 1) {
-		if (skb_shared(skb) || skb_cloned(skb)) {
-			struct sk_buff *nskb = skb_copy(skb, GFP_ATOMIC);
-			kfree(skb);
-			skb = nskb;
-		}
+		skb = skb_unshare(skb, GFP_ATOMIC);
 		if (skb) {
 			/* Lifted from Gleb's VLAN code... */
 			memmove(skb->data - ETH_HLEN,

@@ -121,6 +117,7 @@
 	struct net_device_stats *stats;
 	unsigned short vlan_TCI;
 	unsigned short proto;
+	struct net_device *vlan_dev;
 
 	/* vlan_TCI = ntohs(get_unaligned(&vhdr->h_vlan_TCI)); */
 	vlan_TCI = ntohs(vhdr->h_vlan_TCI);

@@ -144,8 +141,8 @@
 	 */
 
 	spin_lock_bh(&vlan_group_lock);
-	skb->dev = __find_vlan_dev(dev, vid);
-	if (!skb->dev) {
+	vlan_dev = __find_vlan_dev(dev, vid);
+	if (!vlan_dev) {
 		spin_unlock_bh(&vlan_group_lock);
 
 #ifdef VLAN_DEBUG

@@ -156,10 +153,18 @@
 		return -1;
 	}
 
-	skb->dev->last_rx = jiffies;
+	vlan_dev->last_rx = jiffies;
+	stats = vlan_dev_get_stats(vlan_dev);
+
+	skb = skb_share_check(skb, GFP_ATOMIC);
+	if (!skb) {
+		spin_unlock_bh(&vlan_group_lock);
+		stats->rx_dropped++;
+		return NET_RX_DROP;
+	}
+	skb->dev = vlan_dev;
 
 	/* Bump the rx counters for the VLAN device. */
-	stats = vlan_dev_get_stats(skb->dev);
 	stats->rx_packets++;
 	stats->rx_bytes += skb->len;

Ben Greear

2003-10-10 23:52 UTC #1 lore raw

Quoted 22 lines · Tommy Christensen

As noted by Jonas Munsin, the vlan patch that went into 2.6.0-test7 did a kfree() rather than a kfree_skb(). Hmm.
Instead of just fixing this up, I suggest we do things the right way:
The VLAN code have long been claiming to handle shared SKBs, without actually doing so. I have now added the call to skb_share_check().
This enables us to simply do a skb_unshare() when mangling the ethernet header.
Patch is against linux-2.6.0-test7 (applies to 2.4.23-pre7 as well).
-Tommy
------------------------------------------------------------------------
diff -ru linux-2.6.0-test7/net/8021q/vlan_dev.c linux-2.6/net/8021q/vlan_dev.c

net/8021q/vlan_dev.c line 77

Quoted 9 lines

net/8021q/vlan_dev.c +1 -5

@@ -74,11 +74,7 @@
 static inline struct sk_buff *vlan_check_reorder_header(struct sk_buff *skb)
 {
 	if (VLAN_DEV_INFO(skb->dev)->flags & 1) {
-		if (skb_shared(skb) || skb_cloned(skb)) {
-			struct sk_buff *nskb = skb_copy(skb, GFP_ATOMIC);
-			kfree(skb);
-			skb = nskb;
-		}
+		skb = skb_unshare(skb, GFP_ATOMIC);

On 2.4.22, at least, skb_unshare only checks skb_cloned(), so do we also need to check skb_shared()? Or was that check in the old patch above not needed?

Ben

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Tommy Christensen

author

2003-10-11 00:09 UTC #2 lore raw

Quoted 11 lines · Ben Greear

net/8021q/vlan_dev.c line 77
Quoted 6 lines
net/8021q/vlan_dev.c +1 -5
-        if (skb_shared(skb) || skb_cloned(skb)) {
-            struct sk_buff *nskb = skb_copy(skb, GFP_ATOMIC);
-            kfree(skb);
-            skb = nskb;
-        }
+        skb = skb_unshare(skb, GFP_ATOMIC);
On 2.4.22, at least, skb_unshare only checks skb_cloned(), so do we also need to check skb_shared()? Or was that check in the old patch above not needed?

skb_shared() is now taken care of by skb_share_check(), called earlier in the receive routine.

Admittedly, the naming is not very helpful. skb_share_check() handles shared sk_buff's, while skb_unshare() handles cloned sk_buff's. Well ...

-Tommy

David S. Miller

2003-10-11 19:03 UTC #4 lore raw

In reply to Ben Greear

Quoted 4 lines · On Sat, 11 Oct 2003 12:03:52 -0700 Ben Greear <[email protected]> wrote:

Maybe we should have a method called: skb_get_me_an_skb_that_I_can_modify(skb);
Then the subtle differences between sharing, cloning, etc can be handled by the skb code internally...

We can't create this routine, because it depends upon context.

The current code is correct and handles shared SKBs properly, so I don't think it's worth playing around with this much more.

Ben Greear

2003-10-11 19:03 UTC #3 lore raw

In reply to Tommy Christensen

Quoted 11 lines · Tommy Christensen

Quoted 9 lines · Ben Greear

Quoted 6 lines

net/8021q/vlan_dev.c +1 -5

-        if (skb_shared(skb) || skb_cloned(skb)) {
-            struct sk_buff *nskb = skb_copy(skb, GFP_ATOMIC);
-            kfree(skb);
-            skb = nskb;
-        }
+        skb = skb_unshare(skb, GFP_ATOMIC);

Quoted 7 lines

Quoted 3 lines
On 2.4.22, at least, skb_unshare only checks skb_cloned(), so do we also need to check skb_shared()? Or was that check in the old patch above not needed?
skb_shared() is now taken care of by skb_share_check(), called earlier in the receive routine.

Maybe we should have a method called: skb_get_me_an_skb_that_I_can_modify(skb);

Then the subtle differences between sharing, cloning, etc can be handled by the skb code internally...

Ben

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

David S. Miller

2003-10-11 19:54 UTC #6 lore raw

In reply to Ben Greear

Quoted 4 lines · On Sat, 11 Oct 2003 12:55:23 -0700 Ben Greear <[email protected]> wrote:

I have read the sk_buf.h file repeatedly trying to get this all straight in my head, and I think I'm still missing things. Is there any other documentation around that describes in detail exactly the things you must do to handle shared skbs in all contexts?

No there isn't, sorry.

If you just want to read the IP address in a packet, or anything else in general you have two options:

1) if (pskb_may_pull(skb, len))

	goto drop;

After this call, it is guarenteed you may look at the all bytes up to 'len' from the start of the SKB using skb->data et al.

2) Use skb_peek_bits().

Ben Greear

2003-10-11 19:55 UTC #5 lore raw

In reply to David S. Miller

Quoted 11 lines · David S. Miller

Quoted 4 lines · On Sat, 11 Oct 2003 12:03:52 -0700 Ben Greear <[email protected]> wrote:
Maybe we should have a method called: skb_get_me_an_skb_that_I_can_modify(skb);
Then the subtle differences between sharing, cloning, etc can be handled by the skb code internally...
We can't create this routine, because it depends upon context.

I have read the sk_buf.h file repeatedly trying to get this all straight in my head, and I think I'm still missing things. Is there any other documentation around that describes in detail exactly the things you must do to handle shared skbs in all contexts?

I have a question on linearize as well. Assuming I want to read the IP address in a packet. Do I have to linearize before I start looking at offsets in the skb?

Thanks,
Ben

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Tommy Christensen

author

2003-10-12 10:00 UTC #7 lore raw

In reply to David S. Miller

Quoted 2 lines · David S. Miller

The current code is correct and handles shared SKBs properly, so I don't think it's worth playing around with this much more.

No, it is still not right.

Bear with me, but this is how I understand it:

- The sk_buff *structure* is read-only on a shared SKB. skb_share_check(skb) breaks the sharing, thus allowing updates of skb->dev, skb->len etc.
But the SKB still points at the same data buffer.

- The *data* buffer is read-only on a cloned SKB. skb_cow() or skb_copy() or ... gives us a writeable buffer.

The tcpdump problem was caused by violating the second rule, this is now fixed.

But the code still doesn't follow the first rule, which is what this patch fixes.

-Tommy

David S. Miller

2003-10-13 17:19 UTC #8 lore raw

Quoted 9 lines · On Sun, 12 Oct 2003 12:00:09 +0200 Tommy Christensen…

Bear with me, but this is how I understand it:
- The sk_buff *structure* is read-only on a shared SKB. skb_share_check(skb) breaks the sharing, thus allowing updates of skb->dev, skb->len etc.
But the SKB still points at the same data buffer.
- The *data* buffer is read-only on a cloned SKB. skb_cow() or skb_copy() or ... gives us a writeable buffer.

skb_copy() makes both the struct sk_buff and the data it points to unique and yours alone to modify however you wish.