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

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

[Christian Darnell]

>-----Original Message-----
>From: Ben Greear [mailto:greearb@candelatech.com]
>Sent: Monday, October 06, 2003 10:24 PM
>To: Bart De Schuymer
>Cc: netdev@oss.sgi.com; vlan@wanfear.com; bridge
>Subject: [VLAN] Re: [PATCH/RFC] Let {ip,arp}tables "see" bridged VLAN
>tagged{I,AR}P packets
>
>
>Bart De Schuymer wrote:
>> - add some code in vlan_dev.c::vlan_dev_hard_start_xmit():
>> skb->protocol = __constant_htons(ETH_P_8021Q);
>> skb->mac.raw -= VLAN_HLEN;
>> skb->nh.raw -= VLAN_HLEN;
>
>I wonder if this is what was messing up the tcpdump packet capture
>as well?

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


I added those lines (see above) to vlan_dev.c but it didn't solve the
problem with tcpdump (pcap) incoming packets. vlan_dev_hard_start_xmit()
tells me that this is only outgoing packets right? For outgoing packets
tcpdump works (and Ethereal for Windows sees the packet correctly on the
wire) so the problem doesn't occur when sending packets. I added following
line to vlan_dev.c:

vlan_dev.c:: int vlan_skb_recv(struct sk_buff *skb, struct net_device *dev,
struct packet_type* ptype):

vlan_TCI = ntohs(vhdr->h_vlan_TCI);

+ /* Print the 802.1p priority */
+printk("VLAN Prio: %hx\n", vlan_TCI >> 13);

vid = (vlan_TCI & VLAN_VID_MASK);

With the code above I get the 802.1p correctly. Does anyone know how and
when pcap grabs the packet (does pcap grab the packet before or after
vlan_skb_recv is called)?

When I'm sniffing with pcap I sniff the Ethernet interface, so the VLAN
stuff isn't really involved when pcap grab the packet or have I
misunderstand how it works?


Best Regards, 

Christian Darnell
_______________________________________________
VLAN mailing list  -  VLAN@wanfear.com
http://www.WANfear.com/mailman/listinfo/vlan
VLAN Page:  http://scry.wanfear.com/~greear/vlan.html

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