Re: [Bridge] RE: [VLAN] Re: [PATCH/RFC] Let {ip, arp}tables "see" bridged VLAN tagged{I,AR}P packets

Re: [Bridge] RE: [VLAN] Re: [PATCH/RFC] Let {ip, arp}tables "see" bridged VLAN tagged{I,AR}P packets

[Tommy Christensen]

On Tue, 2003-10-07 at 11:06, Christian Darnell wrote:

> Hi Ben and all others, 
> 
> Just to clarify for other who hasn't been a part of this discussion before. 
> 
> ---- 8< ----
> When trying to grab a packet with pcap when using VLAN the beginning of the
> packet is corrupt an the VLAN TCI bits are missing. This is only a problem
> when sniffing on incoming traffic not outgoing.
> 
> 00 60 08 50 00 60 08 50 26 2a 00 60 08 6a b4 53 xx xx xx xx 08 00 45 00
> ^^^^^^^^^^^                                     ^^^^^^^^^^^^
> Where does these bytes come from?               Bytes missing (VLAN header)?
> 
> The correct MAC addresses here are:
> 00 60 08 50 26 2a and 00 60 08 6a b4 53
> ---- 8< ----

This is because the VLAN code is mangling shared data.
You need to do something like this:


--- linux-2.4/net/8021q/vlan_dev.c.org	2003-02-25 15:23:09.000000000
+0100
+++ linux-2.4/net/8021q/vlan_dev.c	2003-10-07 16:01:29.000000000 +0200
@@ -75,7 +75,12 @@
 static inline struct sk_buff *vlan_check_reorder_header(struct sk_buff
*skb)
 {
 	if (VLAN_DEV_INFO(skb->dev)->flags & 1) {
-		skb = skb_share_check(skb, GFP_ATOMIC);
+		if (skb_shared(skb) || skb_cloned(skb)) {
+			struct sk_buff *nskb;
+			nskb = skb_copy(skb, GFP_ATOMIC);
+			kfree_skb(skb);
+			skb = nskb;
+		}
 		if (skb) {
 			/* Lifted from Gleb's VLAN code... */
 			memmove(skb->data - ETH_HLEN,


Christian, could you try this out?


Regarding sharing, the following should be applied as well.
The VLAN code is handed shared sk_buff's, but doesn't handle them
as such.


--- linux-2.4/net/8021q/vlan.c.org	2003-02-25 15:23:09.000000000 +0100
+++ linux-2.4/net/8021q/vlan.c	2003-10-07 16:02:52.000000000 +0200
@@ -67,7 +67,7 @@
 	type: __constant_htons(ETH_P_8021Q),
 	dev:  NULL,
 	func: vlan_skb_recv, /* VLAN receive method */
-	data: (void *)(-1),  /* Set here '(void *)1' when this code can SHARE
SKBs */
+	data: NULL,          /* Set here '(void *)1' when this code can SHARE
SKBs */
 	next: NULL
 };
 

I guess this is a special case of "off-by-one" ;-)

-Tommy

RE: [Bridge] RE: [VLAN] Re: [PATCH/RFC] Let {ip, arp}tables "see" bridged VLAN tagged{I,AR}P packets

[Christian Darnell]

-----Original Message-----
>From: Tommy Christensen [mailto:tommy.christensen@tpack.net]
>Sent: Wednesday, October 08, 2003 10:09 AM
>To: Christian Darnell
>Cc: 'Linux 802.1Q VLAN'; Bart De Schuymer; netdev@oss.sgi.com; bridge
>Subject: Re: [Bridge] RE: [VLAN] Re: [PATCH/RFC] Let {ip, arp}tables
>"see" bridged VLAN tagged{I,AR}P packets
>
>
>
>This is because the VLAN code is mangling shared data.
>You need to do something like this:
>
>
>--- linux-2.4/net/8021q/vlan_dev.c.org	2003-02-25 15:23:09.000000000
>+0100
>+++ linux-2.4/net/8021q/vlan_dev.c	2003-10-07 16:01:29.000000000 +0200
>@@ -75,7 +75,12 @@
> static inline struct sk_buff *vlan_check_reorder_header(struct sk_buff
>*skb)
> {
> 	if (VLAN_DEV_INFO(skb->dev)->flags & 1) {
>-		skb = skb_share_check(skb, GFP_ATOMIC);
>+		if (skb_shared(skb) || skb_cloned(skb)) {
>+			struct sk_buff *nskb;
>+			nskb = skb_copy(skb, GFP_ATOMIC);
>+			kfree_skb(skb);
>+			skb = nskb;
>+		}
> 		if (skb) {
> 			/* Lifted from Gleb's VLAN code... */
> 			memmove(skb->data - ETH_HLEN,
>
>
>Christian, could you try this out?


Thanks Tommy! I tried this (on kernel 2.4.22) and it works great!


Best Regards, 

Christian Darnell

Re: [Bridge] RE: [VLAN] Re: [PATCH/RFC] Let {ip, arp}tables "see" bridged VLAN tagged{I,AR}P packets

[David S. Miller]

On 08 Oct 2003 10:09:25 +0200
Tommy Christensen <tommy.christensen@tpack.net> wrote:

> This is because the VLAN code is mangling shared data.
> You need to do something like this:

This fix is definitely needed and correct, patch applied
thanks.

Re: [Bridge] RE: [VLAN] Re: [PATCH/RFC] Let {ip, arp}tables "see" bridged VLAN tagged{I,AR}P packets

[David S. Miller]

On Wed, 08 Oct 2003 09:34:20 -0700
Ben Greear <greearb@candelatech.com> wrote:

> So, what good is skb_share_check then?
> Maybe we should have a skb_share_or_cloned_check() ?

What input handlers are supposed to do is first:

	skb = skb_share_check(...);


then look at packet contents etc., then if they need to write
to the header do something like skb_cow().

The best example, as usual, is ipv4 input.  Look at how ip_rcv()
makes sure it can safely get at the packet header parts it needs
to parse, then look at ip_forward and how it cows the IPV4 header
so it can modify the TTL field.

Re: [Bridge] RE: [VLAN] Re: [PATCH/RFC] Let {ip, arp}tables "see" bridged VLAN tagged{I,AR}P packets

[Ben Greear]

Christian Darnell wrote:
> -----Original Message-----
> 
>>From: Tommy Christensen [mailto:tommy.christensen@tpack.net]
>>Sent: Wednesday, October 08, 2003 10:09 AM
>>To: Christian Darnell
>>Cc: 'Linux 802.1Q VLAN'; Bart De Schuymer; netdev@oss.sgi.com; bridge
>>Subject: Re: [Bridge] RE: [VLAN] Re: [PATCH/RFC] Let {ip, arp}tables
>>"see" bridged VLAN tagged{I,AR}P packets
>>
>>
>>
>>This is because the VLAN code is mangling shared data.
>>You need to do something like this:
>>
>>
>>--- linux-2.4/net/8021q/vlan_dev.c.org	2003-02-25 15:23:09.000000000
>>+0100
>>+++ linux-2.4/net/8021q/vlan_dev.c	2003-10-07 16:01:29.000000000 +0200
>>@@ -75,7 +75,12 @@
>>static inline struct sk_buff *vlan_check_reorder_header(struct sk_buff
>>*skb)
>>{
>>	if (VLAN_DEV_INFO(skb->dev)->flags & 1) {
>>-		skb = skb_share_check(skb, GFP_ATOMIC);
>>+		if (skb_shared(skb) || skb_cloned(skb)) {
>>+			struct sk_buff *nskb;
>>+			nskb = skb_copy(skb, GFP_ATOMIC);
>>+			kfree_skb(skb);
>>+			skb = nskb;
>>+		}

Thanks for catching that!

So, what good is skb_share_check then?
Maybe we should have a skb_share_or_cloned_check() ?

Ben


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