Patchwork vlan: Fix the b0rked ingress VLAN_FLAG_REORDER_HDR check.

login
register
mail settings
Submitter Eric W. Biederman
Date May 24, 2011, 12:11 a.m.
Message ID <m1oc2tyn20.fsf_-_@fess.ebiederm.org>
Download mbox | patch
Permalink /patch/97110/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Eric W. Biederman - May 24, 2011, 12:11 a.m.
Testing of VLAN_FLAG_REORDER_HDR does not belong in vlan_untag but
rather in vlan_do_receive.  In attempting to test VLAN_FLAG_REORDER_HDR
we are attempting to deference vlan private network device members a non
vlan device, which is just wrong.  Move the test into vlan_do_receive so
that the vlan header will not be properly put on the packet in the case
of vlan header acceleration.

As we remove the check from vlan_check_reorder_header rename it
vlan_reorder_header to keep the naming clean.

Hopefully at somepoint we will just declare the case where
VLAN_FLAG_REORDER_HDR is cleared as unsupported and remove the code.
Until then this keeps it working correctly.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 include/linux/if_vlan.h |   25 ++++++++++++++++++++++---
 net/8021q/vlan_core.c   |   25 ++++++++++++++-----------
 2 files changed, 36 insertions(+), 14 deletions(-)
David Miller - May 24, 2011, 4:54 a.m.
From: ebiederm@xmission.com (Eric W. Biederman)
Date: Mon, 23 May 2011 17:11:51 -0700

> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
> index 41495dc..c08dae7 100644
> --- a/net/8021q/vlan_core.c
> +++ b/net/8021q/vlan_core.c
> @@ -23,6 +23,11 @@ bool vlan_do_receive(struct sk_buff **skbp)
>  		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;
>  

Below we do a eth_hdr(skb) based check on the ethernet address in the
PACKET_OTHERHOST case.

Won't this VLAN tag insertion change skb->mac_header and thus screw up
that test?

Touching this code requires surgical precision and long audits, because
there are so many subtble dependencies all over the place like this.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric W. Biederman - May 24, 2011, 6:18 a.m.
David Miller <davem@davemloft.net> writes:

> From: ebiederm@xmission.com (Eric W. Biederman)
> Date: Mon, 23 May 2011 17:11:51 -0700
>
>> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>> index 41495dc..c08dae7 100644
>> --- a/net/8021q/vlan_core.c
>> +++ b/net/8021q/vlan_core.c
>> @@ -23,6 +23,11 @@ bool vlan_do_receive(struct sk_buff **skbp)
>>  		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;
>>  
>
> Below we do a eth_hdr(skb) based check on the ethernet address in the
> PACKET_OTHERHOST case.
>
> Won't this VLAN tag insertion change skb->mac_header and thus screw up
> that test?

Yes the vlan tag insertion does in fact change the skb->mac_header
index.  However we also move the location of the data as well, so
I fail to see any reason to be concerned about the PACKET_OTHERHOST
case.  Things were moved around and we updated the appropriate references.

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.

> Touching this code requires surgical precision and long audits, because
> there are so many subtble dependencies all over the place like this.

Certainly.

I think you will find that I have applied great precision and restraint
in this case. 

Eric


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - May 24, 2011, 6:24 a.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 290bd8a..26f3c92 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -220,7 +220,7 @@  static inline int vlan_hwaccel_receive_skb(struct sk_buff *skb,
 }
 
 /**
- * __vlan_put_tag - regular VLAN tag inserting
+ * vlan_insert_tag - regular VLAN tag inserting
  * @skb: skbuff to tag
  * @vlan_tci: VLAN TCI to insert
  *
@@ -229,8 +229,10 @@  static inline int vlan_hwaccel_receive_skb(struct sk_buff *skb,
  *
  * Following the skb_unshare() example, in case of error, the calling function
  * doesn't have to worry about freeing the original skb.
+ *
+ * Does not change skb->protocol so this function can be used for receive.
  */
-static inline struct sk_buff *__vlan_put_tag(struct sk_buff *skb, u16 vlan_tci)
+static inline struct sk_buff *vlan_insert_tag(struct sk_buff *skb, u16 vlan_tci)
 {
 	struct vlan_ethhdr *veth;
 
@@ -250,8 +252,25 @@  static inline struct sk_buff *__vlan_put_tag(struct sk_buff *skb, u16 vlan_tci)
 	/* now, the TCI */
 	veth->h_vlan_TCI = htons(vlan_tci);
 
-	skb->protocol = htons(ETH_P_8021Q);
+	return skb;
+}
 
+/**
+ * __vlan_put_tag - regular VLAN tag inserting
+ * @skb: skbuff to tag
+ * @vlan_tci: VLAN TCI to insert
+ *
+ * Inserts the VLAN tag into @skb as part of the payload
+ * Returns a VLAN tagged skb. If a new skb is created, @skb is freed.
+ *
+ * Following the skb_unshare() example, in case of error, the calling function
+ * doesn't have to worry about freeing the original skb.
+ */
+static inline struct sk_buff *__vlan_put_tag(struct sk_buff *skb, u16 vlan_tci)
+{
+	skb = vlan_insert_tag(skb, vlan_tci);
+	if (skb)
+		skb->protocol = htons(ETH_P_8021Q);
 	return skb;
 }
 
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 41495dc..c08dae7 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -23,6 +23,11 @@  bool vlan_do_receive(struct sk_buff **skbp)
 		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;
 
@@ -89,17 +94,15 @@  gro_result_t vlan_gro_frags(struct napi_struct *napi, struct vlan_group *grp,
 }
 EXPORT_SYMBOL(vlan_gro_frags);
 
-static struct sk_buff *vlan_check_reorder_header(struct sk_buff *skb)
+static struct sk_buff *vlan_reorder_header(struct sk_buff *skb)
 {
-	if (vlan_dev_info(skb->dev)->flags & VLAN_FLAG_REORDER_HDR) {
-		if (skb_cow(skb, skb_headroom(skb)) < 0)
-			skb = NULL;
-		if (skb) {
-			/* Lifted from Gleb's VLAN code... */
-			memmove(skb->data - ETH_HLEN,
-				skb->data - VLAN_ETH_HLEN, 12);
-			skb->mac_header += VLAN_HLEN;
-		}
+	if (skb_cow(skb, skb_headroom(skb)) < 0)
+		skb = NULL;
+	if (skb) {
+		/* Lifted from Gleb's VLAN code... */
+		memmove(skb->data - ETH_HLEN,
+			skb->data - VLAN_ETH_HLEN, 12);
+		skb->mac_header += VLAN_HLEN;
 	}
 	return skb;
 }
@@ -161,7 +164,7 @@  struct sk_buff *vlan_untag(struct sk_buff *skb)
 	skb_pull_rcsum(skb, VLAN_HLEN);
 	vlan_set_encap_proto(skb, vhdr);
 
-	skb = vlan_check_reorder_header(skb);
+	skb = vlan_reorder_header(skb);
 	if (unlikely(!skb))
 		goto err_free;