[PATCH 1/5] [VLAN]: Unclassified vlan packet

[PATCH 1/5] [VLAN]: Unclassified vlan packet

[Joonwoo Park]

To be polite to the PACKET,
Don't kill the unclassified & hardware accelerated vlan packets if netdev
is in promiscuous, set packet type with PACKET_OTHERHOST. 
Put the vlan tag into skb->cb for all hardware accelerated vlan packets.

Signed-off-by: Joonwoo Park <joonwpark81@gmail.com>
---
 include/linux/if_vlan.h |   56 +++++++++++++++++++++++++++++-----------------
 1 files changed, 35 insertions(+), 21 deletions(-)

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 79504b2..e400141 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -62,6 +62,9 @@ struct vlan_ethhdr {
 
 #include <linux/skbuff.h>
 
+static inline struct sk_buff *__vlan_hwaccel_put_tag(struct sk_buff *skb,
+						     unsigned short tag);
+
 static inline struct vlan_ethhdr *vlan_eth_hdr(const struct sk_buff *skb)
 {
 	return (struct vlan_ethhdr *)skb_mac_header(skb);
@@ -175,14 +178,19 @@ static inline int __vlan_hwaccel_rx(struct sk_buff *skb,
 				    unsigned short vlan_tag, int polling)
 {
 	struct net_device_stats *stats;
+	struct net_device *vlan_dev;
 
 	if (skb_bond_should_drop(skb)) {
 		dev_kfree_skb_any(skb);
 		return NET_RX_DROP;
 	}
 
-	skb->dev = vlan_group_get_device(grp, vlan_tag & VLAN_VID_MASK);
-	if (skb->dev == NULL) {
+	vlan_dev = vlan_group_get_device(grp, vlan_tag & VLAN_VID_MASK);
+	if (vlan_dev)
+		skb->dev = vlan_dev;
+	else if (skb->dev->flags & IFF_PROMISC)
+		skb->pkt_type = PACKET_OTHERHOST;
+	else {
 		dev_kfree_skb_any(skb);
 
 		/* Not NET_RX_DROP, this is not being dropped
@@ -191,31 +199,37 @@ static inline int __vlan_hwaccel_rx(struct sk_buff *skb,
 		return 0;
 	}
 
+	/* Deliever vlan_tag to PACKET */
+	__vlan_hwaccel_put_tag(skb, vlan_tag);
+
 	skb->dev->last_rx = jiffies;
 
 	stats = &skb->dev->stats;
 	stats->rx_packets++;
 	stats->rx_bytes += skb->len;
 
-	skb->priority = vlan_get_ingress_priority(skb->dev, vlan_tag);
-	switch (skb->pkt_type) {
-	case PACKET_BROADCAST:
-		break;
-
-	case PACKET_MULTICAST:
-		stats->multicast++;
-		break;
-
-	case PACKET_OTHERHOST:
-		/* Our lower layer thinks this is not local, let's make sure.
-		 * This allows the VLAN to have a different MAC than the underlying
-		 * device, and still route correctly.
-		 */
-		if (!compare_ether_addr(eth_hdr(skb)->h_dest,
-				       	skb->dev->dev_addr))
-			skb->pkt_type = PACKET_HOST;
-		break;
-	};
+	if (vlan_dev) {
+		skb->priority = vlan_get_ingress_priority(skb->dev, vlan_tag);
+		switch (skb->pkt_type) {
+		case PACKET_BROADCAST:
+			break;
+
+		case PACKET_MULTICAST:
+			stats->multicast++;
+			break;
+
+		case PACKET_OTHERHOST:
+			/* Our lower layer thinks this is not local, let's
+			 * make sure.
+			 * This allows the VLAN to have a different MAC than
+			 * the underlying device, and still route correctly.
+			 */
+			if (!compare_ether_addr(eth_hdr(skb)->h_dest,
+							skb->dev->dev_addr))
+				skb->pkt_type = PACKET_HOST;
+			break;
+		};
+	}
 
 	return (polling ? netif_receive_skb(skb) : netif_rx(skb));
 }
-- 
1.5.4.3

Re: [PATCH 1/5] [VLAN]: Unclassified vlan packet

[Patrick McHardy]

Joonwoo Park wrote:
> To be polite to the PACKET,
> Don't kill the unclassified & hardware accelerated vlan packets if netdev
> is in promiscuous, set packet type with PACKET_OTHERHOST. 
> Put the vlan tag into skb->cb for all hardware accelerated vlan packets.

Conceptually I think this patch goes in the right direction,
one question remaining is when to invalidate the VLAN tag again.

The only solution I could come up with is invalidating it in
netif_receive_skb() when the receiving device is not a VLAN
device and additionally invalidating it in all callers of
dev_queue_xmit except VLAN itself, but I would really prefer
something less error prone without touching netif_receive_skb().

BTW, I already have a patch queued to move the VLAN tag from
skb->cb to a seperate skb member to fix the the conflict with
qdiscs (this should also allow to use vlan accel through virtual
network devices later on). So please don't resend, I'll integrate
the patch on top of this change once we find a good spot for
invalidation.

> 
> Signed-off-by: Joonwoo Park <joonwpark81@gmail.com>
> ---
>  include/linux/if_vlan.h |   56 +++++++++++++++++++++++++++++-----------------
>  1 files changed, 35 insertions(+), 21 deletions(-)
> 
> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
> index 79504b2..e400141 100644
> --- a/include/linux/if_vlan.h
> +++ b/include/linux/if_vlan.h
> @@ -62,6 +62,9 @@ struct vlan_ethhdr {
>  
>  #include <linux/skbuff.h>
>  
> +static inline struct sk_buff *__vlan_hwaccel_put_tag(struct sk_buff *skb,
> +						     unsigned short tag);
> +
>  static inline struct vlan_ethhdr *vlan_eth_hdr(const struct sk_buff *skb)
>  {
>  	return (struct vlan_ethhdr *)skb_mac_header(skb);
> @@ -175,14 +178,19 @@ static inline int __vlan_hwaccel_rx(struct sk_buff *skb,
>  				    unsigned short vlan_tag, int polling)
>  {
>  	struct net_device_stats *stats;
> +	struct net_device *vlan_dev;
>  
>  	if (skb_bond_should_drop(skb)) {
>  		dev_kfree_skb_any(skb);
>  		return NET_RX_DROP;
>  	}
>  
> -	skb->dev = vlan_group_get_device(grp, vlan_tag & VLAN_VID_MASK);
> -	if (skb->dev == NULL) {
> +	vlan_dev = vlan_group_get_device(grp, vlan_tag & VLAN_VID_MASK);
> +	if (vlan_dev)
> +		skb->dev = vlan_dev;
> +	else if (skb->dev->flags & IFF_PROMISC)
> +		skb->pkt_type = PACKET_OTHERHOST;
> +	else {
>  		dev_kfree_skb_any(skb);
>  
>  		/* Not NET_RX_DROP, this is not being dropped
> @@ -191,31 +199,37 @@ static inline int __vlan_hwaccel_rx(struct sk_buff *skb,
>  		return 0;
>  	}
>  
> +	/* Deliever vlan_tag to PACKET */
> +	__vlan_hwaccel_put_tag(skb, vlan_tag);
> +
>  	skb->dev->last_rx = jiffies;
>  
>  	stats = &skb->dev->stats;
>  	stats->rx_packets++;
>  	stats->rx_bytes += skb->len;
>  
> -	skb->priority = vlan_get_ingress_priority(skb->dev, vlan_tag);
> -	switch (skb->pkt_type) {
> -	case PACKET_BROADCAST:
> -		break;
> -
> -	case PACKET_MULTICAST:
> -		stats->multicast++;
> -		break;
> -
> -	case PACKET_OTHERHOST:
> -		/* Our lower layer thinks this is not local, let's make sure.
> -		 * This allows the VLAN to have a different MAC than the underlying
> -		 * device, and still route correctly.
> -		 */
> -		if (!compare_ether_addr(eth_hdr(skb)->h_dest,
> -				       	skb->dev->dev_addr))
> -			skb->pkt_type = PACKET_HOST;
> -		break;
> -	};
> +	if (vlan_dev) {
> +		skb->priority = vlan_get_ingress_priority(skb->dev, vlan_tag);
> +		switch (skb->pkt_type) {
> +		case PACKET_BROADCAST:
> +			break;
> +
> +		case PACKET_MULTICAST:
> +			stats->multicast++;
> +			break;
> +
> +		case PACKET_OTHERHOST:
> +			/* Our lower layer thinks this is not local, let's
> +			 * make sure.
> +			 * This allows the VLAN to have a different MAC than
> +			 * the underlying device, and still route correctly.
> +			 */
> +			if (!compare_ether_addr(eth_hdr(skb)->h_dest,
> +							skb->dev->dev_addr))
> +				skb->pkt_type = PACKET_HOST;
> +			break;
> +		};
> +	}
>  
>  	return (polling ? netif_receive_skb(skb) : netif_rx(skb));
>  }

Re: [PATCH 1/5] [VLAN]: Unclassified vlan packet

[Joonwoo Park]

2008/5/26 Patrick McHardy <kaber@trash.net>:
> Joonwoo Park wrote:
>>
>> To be polite to the PACKET,
>> Don't kill the unclassified & hardware accelerated vlan packets if netdev
>> is in promiscuous, set packet type with PACKET_OTHERHOST. Put the vlan tag
>> into skb->cb for all hardware accelerated vlan packets.
>
> Conceptually I think this patch goes in the right direction,
> one question remaining is when to invalidate the VLAN tag again.
>
> The only solution I could come up with is invalidating it in
> netif_receive_skb() when the receiving device is not a VLAN
> device and additionally invalidating it in all callers of
> dev_queue_xmit except VLAN itself, but I would really prefer
> something less error prone without touching netif_receive_skb().
>
> BTW, I already have a patch queued to move the VLAN tag from
> skb->cb to a seperate skb member to fix the the conflict with
> qdiscs (this should also allow to use vlan accel through virtual
> network devices later on). So please don't resend, I'll integrate
> the patch on top of this change once we find a good spot for
> invalidation.
>

Thanks Patrick for reviewing.
I'll be looking forward to seeing it on the list.

Thanks,
Joonwoo

Re: [PATCH 1/5] [VLAN]: Unclassified vlan packet

[Patrick McHardy]

Joonwoo Park wrote:
> 2008/5/26 Patrick McHardy <kaber@trash.net>:
>> Joonwoo Park wrote:
>>> To be polite to the PACKET,
>>> Don't kill the unclassified & hardware accelerated vlan packets if netdev
>>> is in promiscuous, set packet type with PACKET_OTHERHOST. Put the vlan tag
>>> into skb->cb for all hardware accelerated vlan packets.
>> Conceptually I think this patch goes in the right direction,
>> one question remaining is when to invalidate the VLAN tag again.
>>
>> The only solution I could come up with is invalidating it in
>> netif_receive_skb() when the receiving device is not a VLAN
>> device and additionally invalidating it in all callers of
>> dev_queue_xmit except VLAN itself, but I would really prefer
>> something less error prone without touching netif_receive_skb().
>>
>> BTW, I already have a patch queued to move the VLAN tag from
>> skb->cb to a seperate skb member to fix the the conflict with
>> qdiscs (this should also allow to use vlan accel through virtual
>> network devices later on). So please don't resend, I'll integrate
>> the patch on top of this change once we find a good spot for
>> invalidation.
>>
> 
> Thanks Patrick for reviewing.
> I'll be looking forward to seeing it on the list.


Well, we still need to find a good spot for invalidation.
Suggestions welcome :)

Re: [PATCH 1/5] [VLAN]: Unclassified vlan packet

[Joonwoo Park]

2008/5/26 Patrick McHardy <kaber@trash.net>:
>>> BTW, I already have a patch queued to move the VLAN tag from
>>> skb->cb to a seperate skb member to fix the the conflict with
>>> qdiscs (this should also allow to use vlan accel through virtual
>>> network devices later on). So please don't resend, I'll integrate
>>> the patch on top of this change once we find a good spot for
>>> invalidation.
>>>

Was that patch applied to certain tree?
If it was, may I know what tree is it?

>
> Well, we still need to find a good spot for invalidation.
> Suggestions welcome :)
>

huh, also I'll try to find it! :-)

Thanks,
Joonwoo

Re: [PATCH 1/5] [VLAN]: Unclassified vlan packet

[Patrick McHardy]

Joonwoo Park wrote:
> 2008/5/26 Patrick McHardy <kaber@trash.net>:
>>>> BTW, I already have a patch queued to move the VLAN tag from
>>>> skb->cb to a seperate skb member to fix the the conflict with
>>>> qdiscs (this should also allow to use vlan accel through virtual
>>>> network devices later on). So please don't resend, I'll integrate
>>>> the patch on top of this change once we find a good spot for
>>>> invalidation.
>>>>
> 
> Was that patch applied to certain tree?
> If it was, may I know what tree is it?

I'm uploading it to kernel.org now, should appear within
the next 30 minutes at:

git://git.kernel.org/pub/scm/linux/kernel/git/kaber/vlan-next-2.6.git

Re: [PATCH 1/5] [VLAN]: Unclassified vlan packet

[Joonwoo Park]

2008/5/27 Patrick McHardy <kaber@trash.net>:
> Joonwoo Park wrote:
>>
>> 2008/5/26 Patrick McHardy <kaber@trash.net>:
>>>>>
>>>>> BTW, I already have a patch queued to move the VLAN tag from
>>>>> skb->cb to a seperate skb member to fix the the conflict with
>>>>> qdiscs (this should also allow to use vlan accel through virtual
>>>>> network devices later on). So please don't resend, I'll integrate
>>>>> the patch on top of this change once we find a good spot for
>>>>> invalidation.
>>>>>
>>
>> Was that patch applied to certain tree?
>> If it was, may I know what tree is it?
>
> I'm uploading it to kernel.org now, should appear within
> the next 30 minutes at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/kaber/vlan-next-2.6.git
>

Thanks a lot Patrick!

Joonwoo

Re: [PATCH 1/5] [VLAN]: Unclassified vlan packet

[Joonwoo Park]

2008/5/26 Patrick McHardy <kaber@trash.net>:
> Joonwoo Park wrote:
>>
>> 2008/5/26 Patrick McHardy <kaber@trash.net>:
>>>
>>> Joonwoo Park wrote:
>>>>
>>>> To be polite to the PACKET,
>>>> Don't kill the unclassified & hardware accelerated vlan packets if
>>>> netdev
>>>> is in promiscuous, set packet type with PACKET_OTHERHOST. Put the vlan
>>>> tag
>>>> into skb->cb for all hardware accelerated vlan packets.
>>>
>>> Conceptually I think this patch goes in the right direction,
>>> one question remaining is when to invalidate the VLAN tag again.
>>>
>>> The only solution I could come up with is invalidating it in
>>> netif_receive_skb() when the receiving device is not a VLAN
>>> device and additionally invalidating it in all callers of
>>> dev_queue_xmit except VLAN itself, but I would really prefer
>>> something less error prone without touching netif_receive_skb().
>>>
>>> BTW, I already have a patch queued to move the VLAN tag from
>>> skb->cb to a seperate skb member to fix the the conflict with
>>> qdiscs (this should also allow to use vlan accel through virtual
>>> network devices later on). So please don't resend, I'll integrate
>>> the patch on top of this change once we find a good spot for
>>> invalidation.
>>>
>>
>> Thanks Patrick for reviewing.
>> I'll be looking forward to seeing it on the list.
>
>
> Well, we still need to find a good spot for invalidation.
> Suggestions welcome :)
>

Patrick,
Do you mind inserting a new flag to indicate vlan_tag is just for
af_packet or not into sk_buff?
I think if flag exists, we can pass vlan_tag to af_packet with just
modifications of vlan_get_tag(), vlan_put_tag() and
vlan_tag_present(),
without patching for invalidation.

Thanks,
Joonwoo

Re: [PATCH 1/5] [VLAN]: Unclassified vlan packet

[Patrick McHardy]

Joonwoo Park wrote:
> Patrick,
> Do you mind inserting a new flag to indicate vlan_tag is just for
> af_packet or not into sk_buff?
> I think if flag exists, we can pass vlan_tag to af_packet with just
> modifications of vlan_get_tag(), vlan_put_tag() and
> vlan_tag_present(),
> without patching for invalidation.

I've considered this, but that would mean that we report
the vlan tag for all devices layered on top, which seems
wrong.

For now I think I'm going to temporarily do something like
that so I can continue integrating the remaining patches
and fix it properly later.

Re: [PATCH 1/5] [VLAN]: Unclassified vlan packet

[Patrick McHardy]

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

Patrick McHardy wrote:
> Joonwoo Park wrote:
>> Patrick,
>> Do you mind inserting a new flag to indicate vlan_tag is just for
>> af_packet or not into sk_buff?
>> I think if flag exists, we can pass vlan_tag to af_packet with just
>> modifications of vlan_get_tag(), vlan_put_tag() and
>> vlan_tag_present(),
>> without patching for invalidation.
> 
> I've considered this, but that would mean that we report
> the vlan tag for all devices layered on top, which seems
> wrong.
> 
> For now I think I'm going to temporarily do something like
> that so I can continue integrating the remaining patches
> and fix it properly later.


The entire idea didn't work out too well. Your original patch
passed packets for unknown VLANs to netif_receive_skb again,
but since they don't have a VLAN header, they are treated like
a normal non-VLAN packet and might end up in a protocol handler
instead of getting dropped. It would also have left one
inconsistency, for locally configured VLANs the packet is still
not visible on the lower device.

This patch (on top of my local tree with other changes, I'll
post the entire series soon) takes a different approach.
__vlan_hwaccel_rx() is supposed to behave similar to
netif_receive_skb(), so the easiest fix is to make it behave
more like netif_receive_skb() and deliver packets to ptype_all
handlers manually.


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 3016 bytes --]

commit 5110748e684267ca3d381f47ab972d47ddf178c6
Author: Patrick McHardy <kaber@trash.net>
Date:   Sat Jul 5 21:32:29 2008 +0200

    vlan: deliver packets received with VLAN acceleration to network taps
    
    When VLAN header stripping is used, packets currently bypass packet
    sockets (and other network taps) completely. For locally existing
    VLANs, they appear directly on the VLAN device, for unknown VLANs
    they are silently dropped.
    
    Add a new function netif_nit_deliver() to deliver incoming packets
    to all network interface taps and use it in __vlan_hwaccel_rx() to
    make VLAN packets visible on the underlying device.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 0e1e4c1..ef95e7f 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1144,6 +1144,7 @@ extern int		netif_rx(struct sk_buff *skb);
 extern int		netif_rx_ni(struct sk_buff *skb);
 #define HAVE_NETIF_RECEIVE_SKB 1
 extern int		netif_receive_skb(struct sk_buff *skb);
+extern void		netif_nit_deliver(struct sk_buff *skb);
 extern int		dev_valid_name(const char *name);
 extern int		dev_ioctl(struct net *net, unsigned int cmd, void __user *);
 extern int		dev_ethtool(struct net *net, struct ifreq *);
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 69a09df..77d2fa4 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -14,6 +14,9 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
 		return NET_RX_DROP;
 	}
 
+	skb->vlan_tag = vlan_tag;
+	netif_nit_deliver(skb);
+
 	skb->dev = vlan_group_get_device(grp, vlan_tag & VLAN_VID_MASK);
 	if (skb->dev == NULL) {
 		dev_kfree_skb_any(skb);
@@ -23,6 +26,7 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
 		 */
 		return 0;
 	}
+	skb->vlan_tag = 0;
 
 	skb->dev->last_rx = jiffies;
 
diff --git a/net/core/dev.c b/net/core/dev.c
index fca23a3..6d9bbf1 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2030,6 +2030,38 @@ out:
 }
 #endif
 
+/*
+ * 	netif_tap_deliver - deliver received packets to network taps
+ * 	@skb: buffer
+ *
+ * 	This function is used to deliver incoming packets to network
+ * 	taps. It should be used when the normal netif_receive_skb path
+ * 	is bypassed, for example because of VLAN acceleration.
+ */
+void netif_nit_deliver(struct sk_buff *skb)
+{
+	struct packet_type *ptype, *pt_prev = NULL;
+
+	if (list_empty(&ptype_all))
+		return;
+
+	skb_reset_network_header(skb);
+	skb_reset_transport_header(skb);
+	skb->mac_len = skb->network_header - skb->mac_header;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(ptype, &ptype_all, list) {
+		if (!ptype->dev || ptype->dev == skb->dev) {
+			if (pt_prev)
+				deliver_skb(skb, pt_prev, skb->dev);
+			pt_prev = ptype;
+		}
+	}
+	if (pt_prev)
+		pt_prev->func(skb, skb->dev, pt_prev, skb->dev);
+	rcu_read_unlock();
+}
+
 /**
  *	netif_receive_skb - process receive buffer from network
  *	@skb: buffer to process