Re: [PATCH] vlan: Fix the b0rked ingress VLAN_FLAG_REORDER_HDR check.

[ebiederm@xmission.com (Eric W. Biederman)]

David Miller <davem@davemloft.net> writes:

> From: ebiederm@xmission.com (Eric W. Biederman)
> Date: Mon, 23 May 2011 23:18:02 -0700
>
>> Feel free to read through the code, to convince yourself it is correct.
>> In addition the code is untouched from the vlan header insertion for
>> emulation of vlan header acceleration in dev_hard_start_xmit() which
>> presumably has been working for quite awhile.
>
> I'm not keeping code there that does eth_hdr(skb)->foo when there
> can be either a vlan_hdr(skb) or a eth_hdr(skb) there.
>
> That's just asking for trouble.

How so?  eth_hdr(skb) is a proper subset of vlan_hdr(skb)?

vlan_insert_tag() moves the ethernet header to make space for the vlan
header after it, but before the rest of the data.  With the appropriate
fixups applied to the skb->mac_pointer.

I can see not leaving a vlan_hdr(skb) reference, but beyond that I'm
not seeing the problem.

Certainly we need to do the insert before we update the statics or
we will get rx_bytes wrong.

Would you be happier if the pkt_type check was moved earlier in the
function like say:

diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index c08dae7..7571637 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -22,21 +22,6 @@ bool vlan_do_receive(struct sk_buff **skbp)
 	if (unlikely(!skb))
 		return false;
 
-	skb->dev = vlan_dev;
-	if (unlikely(!(vlan_dev_info(vlan_dev)->flags & VLAN_FLAG_REORDER_HDR))) {
-		skb = *skbp = vlan_insert_tag(skb, skb->vlan_tci);
-		if (!skb)
-			return false;
-	}
-	skb->priority = vlan_get_ingress_priority(vlan_dev, skb->vlan_tci);
-	skb->vlan_tci = 0;
-
-	rx_stats = this_cpu_ptr(vlan_dev_info(vlan_dev)->vlan_pcpu_stats);
-
-	u64_stats_update_begin(&rx_stats->syncp);
-	rx_stats->rx_packets++;
-	rx_stats->rx_bytes += skb->len;
-
 	switch (skb->pkt_type) {
 	case PACKET_BROADCAST:
 		break;
@@ -52,6 +37,21 @@ bool vlan_do_receive(struct sk_buff **skbp)
 			skb->pkt_type = PACKET_HOST;
 		break;
 	}
+
+	skb->dev = vlan_dev;
+	if (unlikely(!(vlan_dev_info(vlan_dev)->flags & VLAN_FLAG_REORDER_HDR))) {
+		skb = *skbp = vlan_insert_tag(skb, skb->vlan_tci);
+		if (!skb)
+			return false;
+	}
+	skb->priority = vlan_get_ingress_priority(vlan_dev, skb->vlan_tci);
+	skb->vlan_tci = 0;
+
+	rx_stats = this_cpu_ptr(vlan_dev_info(vlan_dev)->vlan_pcpu_stats);
+
+	u64_stats_update_begin(&rx_stats->syncp);
+	rx_stats->rx_packets++;
+	rx_stats->rx_bytes += skb->len;
 	u64_stats_update_end(&rx_stats->syncp);
 
 	return true;

Eric