Patchwork vlan: Fix the ingress VLAN_FLAG_REORDER_HDR check v2

login
register
mail settings
Submitter Eric W. Biederman
Date June 2, 2011, 1:03 p.m.
Message ID <m11uzcidvq.fsf_-_@fess.ebiederm.org>
Download mbox | patch
Permalink /patch/98388/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Eric W. Biederman - June 2, 2011, 1:03 p.m.
Testing of VLAN_FLAG_REORDER_HDR does not belong in vlan_untag
but rather in vlan_do_receive.  Otherwise the vlan header
will not be properly put on the packet in the case of
vlan header accelleration.

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

Fix up the skb->pkt_type early so we don't look at the packet
after adding the vlan tag, which guarantees we don't goof
and look at the wrong field.

Use a simple if statement instead of a complicated switch
statement to decided that we need to increment rx_stats
for a multicast packet.

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   |   50 ++++++++++++++++++++++------------------------
 2 files changed, 46 insertions(+), 29 deletions(-)
Jiri Pirko - June 2, 2011, 1:15 p.m.
Thu, Jun 02, 2011 at 03:03:53PM CEST, ebiederm@xmission.com wrote:
>
>Testing of VLAN_FLAG_REORDER_HDR does not belong in vlan_untag
>but rather in vlan_do_receive.  Otherwise the vlan header
>will not be properly put on the packet in the case of
>vlan header accelleration.
>
>As we remove the check from vlan_check_reorder_header
>rename it vlan_reorder_header to keep the naming clean.
>
>Fix up the skb->pkt_type early so we don't look at the packet
>after adding the vlan tag, which guarantees we don't goof
>and look at the wrong field.
>
>Use a simple if statement instead of a complicated switch
>statement to decided that we need to increment rx_stats
>for a multicast packet.
>
>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>

Looking good to me. Thanks Eric.

Reviewed-by: Jiri Pirko <jpirko@redhat.com>

--
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
Changli Gao - June 2, 2011, 2:54 p.m.
On Thu, Jun 2, 2011 at 9:03 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> -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) {

I think an else branch maybe more readable here.

> +               /* Lifted from Gleb's VLAN code... */
> +               memmove(skb->data - ETH_HLEN,
> +                       skb->data - VLAN_ETH_HLEN, 12);
> +               skb->mac_header += VLAN_HLEN;

skb->mac_len should be adjusted too.

>        }
>        return skb;
>  }
padmanabh ratnakar - June 3, 2011, 3:34 a.m.
Doesnt __vlan_put_tag()/vlan_insert_tag() depend on skb->data pointing
to ethernet header.
Is'nt the skb->data pointing past ethernet header in netif_receive_skb().
Am I missing anything?
Regards,
Padmanabh


On Thu, Jun 2, 2011 at 6:33 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> Testing of VLAN_FLAG_REORDER_HDR does not belong in vlan_untag
> but rather in vlan_do_receive.  Otherwise the vlan header
> will not be properly put on the packet in the case of
> vlan header accelleration.
>
> As we remove the check from vlan_check_reorder_header
> rename it vlan_reorder_header to keep the naming clean.
>
> Fix up the skb->pkt_type early so we don't look at the packet
> after adding the vlan tag, which guarantees we don't goof
> and look at the wrong field.
>
> Use a simple if statement instead of a complicated switch
> statement to decided that we need to increment rx_stats
> for a multicast packet.
>
> 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   |   50 ++++++++++++++++++++++------------------------
>  2 files changed, 46 insertions(+), 29 deletions(-)
>
> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
> index 290bd8a..821f0e3 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 during 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..735c818 100644
> --- a/net/8021q/vlan_core.c
> +++ b/net/8021q/vlan_core.c
> @@ -23,6 +23,20 @@ bool vlan_do_receive(struct sk_buff **skbp)
>                return false;
>
>        skb->dev = vlan_dev;
> +       if (skb->pkt_type == 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,
> +                                       vlan_dev->dev_addr))
> +                       skb->pkt_type = PACKET_HOST;
> +       }
> +
> +       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;
>
> @@ -31,22 +45,8 @@ bool vlan_do_receive(struct sk_buff **skbp)
>        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;
> -       case PACKET_MULTICAST:
> +       if (skb->pkt_type == PACKET_MULTICAST)
>                rx_stats->rx_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,
> -                                       vlan_dev->dev_addr))
> -                       skb->pkt_type = PACKET_HOST;
> -               break;
> -       }
>        u64_stats_update_end(&rx_stats->syncp);
>
>        return true;
> @@ -89,17 +89,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 +159,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;
>
> --
> 1.7.5.1.217.g4e3aa
>
> --
> 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
>
--
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
Changli Gao - June 3, 2011, 3:59 a.m.
On Fri, Jun 3, 2011 at 11:34 AM, padmanabh ratnakar
<pratnakarlx@gmail.com> wrote:
> Doesnt __vlan_put_tag()/vlan_insert_tag() depend on skb->data pointing
> to ethernet header.
> Is'nt the skb->data pointing past ethernet header in netif_receive_skb().
> Am I missing anything?

Yes, you are right. skb->data should be adjusted before feeding to
vlan_insert_tag(), or we make vlan_insert_tag() rely on
skb->mac_header instead.
David Miller - June 5, 2011, 9:14 p.m.
From: Changli Gao <xiaosuo@gmail.com>
Date: Fri, 3 Jun 2011 11:59:26 +0800

> On Fri, Jun 3, 2011 at 11:34 AM, padmanabh ratnakar
> <pratnakarlx@gmail.com> wrote:
>> Doesnt __vlan_put_tag()/vlan_insert_tag() depend on skb->data pointing
>> to ethernet header.
>> Is'nt the skb->data pointing past ethernet header in netif_receive_skb().
>> Am I missing anything?
> 
> Yes, you are right. skb->data should be adjusted before feeding to
> vlan_insert_tag(), or we make vlan_insert_tag() rely on
> skb->mac_header instead.

I told you guys this is not simple stuff :-)

Even here we have 3 or 4 people who study this code all the time, and
are still having trouble sorting out all of these details.

--
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
Jiri Pirko - June 8, 2011, 4:28 p.m.
Thu, Jun 02, 2011 at 03:03:53PM CEST, ebiederm@xmission.com wrote:
>
>Testing of VLAN_FLAG_REORDER_HDR does not belong in vlan_untag
>but rather in vlan_do_receive.  Otherwise the vlan header
>will not be properly put on the packet in the case of
>vlan header accelleration.
>
>As we remove the check from vlan_check_reorder_header
>rename it vlan_reorder_header to keep the naming clean.
>
>Fix up the skb->pkt_type early so we don't look at the packet
>after adding the vlan tag, which guarantees we don't goof
>and look at the wrong field.
>
>Use a simple if statement instead of a complicated switch
>statement to decided that we need to increment rx_stats
>for a multicast packet.
>
>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.

Why we can't remove this right away?
--
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
Changli Gao - June 8, 2011, 11:08 p.m.
On Thu, Jun 9, 2011 at 12:28 AM, Jiri Pirko <jpirko@redhat.com> wrote:
>
> Why we can't remove this right away?
>

I think the reason is this patch is a bug fix for the stable kernel,
so we should not introduce new features or remove old features.
Jiri Pirko - June 9, 2011, 6:01 a.m.
Thu, Jun 09, 2011 at 01:08:13AM CEST, xiaosuo@gmail.com wrote:
>On Thu, Jun 9, 2011 at 12:28 AM, Jiri Pirko <jpirko@redhat.com> wrote:
>>
>> Why we can't remove this right away?
>>
>
>I think the reason is this patch is a bug fix for the stable kernel,
>so we should not introduce new features or remove old features.

But for net-next if should be ok, right?

What's the reason for possibility of removing the reorder flag? Why
would user want do it?

Thanks

Jirka
>
>-- 
>Regards,
>Changli Gao(xiaosuo@gmail.com)
--
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..821f0e3 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 during 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..735c818 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -23,6 +23,20 @@  bool vlan_do_receive(struct sk_buff **skbp)
 		return false;
 
 	skb->dev = vlan_dev;
+	if (skb->pkt_type == 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,
+					vlan_dev->dev_addr))
+			skb->pkt_type = PACKET_HOST;
+	}
+
+	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;
 
@@ -31,22 +45,8 @@  bool vlan_do_receive(struct sk_buff **skbp)
 	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;
-	case PACKET_MULTICAST:
+	if (skb->pkt_type == PACKET_MULTICAST)
 		rx_stats->rx_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,
-					vlan_dev->dev_addr))
-			skb->pkt_type = PACKET_HOST;
-		break;
-	}
 	u64_stats_update_end(&rx_stats->syncp);
 
 	return true;
@@ -89,17 +89,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 +159,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;