[PATCH] Handle shared SKBs in VLAN receive code

[PATCH] Handle shared SKBs in VLAN receive code

[Tommy Christensen]

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

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

[-- Attachment #2: vlan_dev.c.patch --]
[-- Type: text/plain, Size: 1552 bytes --]

diff -ru linux-2.6.0-test7/net/8021q/vlan_dev.c linux-2.6/net/8021q/vlan_dev.c
--- linux-2.6.0-test7/net/8021q/vlan_dev.c	Wed Oct  8 21:24:44 2003
+++ linux-2.6/net/8021q/vlan_dev.c	Sat Oct 11 00:45:28 2003
@@ -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;
 

Re: [PATCH] Handle shared SKBs in VLAN receive code

[Ben Greear]

Tommy Christensen wrote:
> 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
> --- linux-2.6.0-test7/net/8021q/vlan_dev.c	Wed Oct  8 21:24:44 2003
> +++ linux-2.6/net/8021q/vlan_dev.c	Sat Oct 11 00:45:28 2003
> @@ -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

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] Handle shared SKBs in VLAN receive code

[Tommy Christensen]

Ben Greear wrote:

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

Re: [PATCH] Handle shared SKBs in VLAN receive code

[David S. Miller]

On Sat, 11 Oct 2003 12:03:52 -0700
Ben Greear <greearb@candelatech.com> 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.

Re: [PATCH] Handle shared SKBs in VLAN receive code

[Ben Greear]

Tommy Christensen wrote:
> Ben Greear wrote:
> 
>>> -        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.

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

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] Handle shared SKBs in VLAN receive code

[David S. Miller]

On Sat, 11 Oct 2003 12:55:23 -0700
Ben Greear <greearb@candelatech.com> 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().

Re: [PATCH] Handle shared SKBs in VLAN receive code

[Ben Greear]

David S. Miller wrote:
> On Sat, 11 Oct 2003 12:03:52 -0700
> Ben Greear <greearb@candelatech.com> 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

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] Handle shared SKBs in VLAN receive code

[Tommy Christensen]

David S. Miller wrote:

> 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

Re: [PATCH] Handle shared SKBs in VLAN receive code

[David S. Miller]

On Sun, 12 Oct 2003 12:00:09 +0200
Tommy Christensen <tommy.christensen@tpack.net> wrote:

> 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.