diff mbox

[v3] vlan: Fix the ingress VLAN_FLAG_REORDER_HDR check

Message ID 20110610083539.GB17568@minipsycho.redhat.com
State Superseded, archived
Delegated to: David Miller
Headers show

Commit Message

Jiri Pirko June 10, 2011, 8:35 a.m. UTC 
Please review. I'm not sure if there wouldn't be necessary to control
for rx accelerated skbs. Smoke tested.

Subject: [patch net-2.6 v3] vlan: Fix the ingress VLAN_FLAG_REORDER_HDR check

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.

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.

Use vlan_unreorder_header to revert actions previously done by
vlan_untag->vlan_reorder_header in case VLAN_FLAG_REORDER_HDR is not set

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
Signed-off-by: Jiri Pirko <jpirko@redhat.com>
---
 net/8021q/vlan_core.c |   75 ++++++++++++++++++++++++++++--------------------
 1 files changed, 44 insertions(+), 31 deletions(-)

Comments

Changli Gao June 10, 2011, 9:26 a.m. UTC | #1 
On Fri, Jun 10, 2011 at 4:35 PM, Jiri Pirko <jpirko@redhat.com> wrote:
> +
> +/* Should be used only to revert vlan_reorder_header action */
> +static struct sk_buff *vlan_unreorder_header(struct sk_buff *skb)
> +{
> +       unsigned char *mac_header;
> +       struct vlan_ethhdr *veth;
> +
> +       if (skb_cow(skb, skb_headroom(skb)) < 0)
> +               return NULL;

I think we need to make sure if there is enough headroom for this
header expansion.

> +       skb->mac_header -= VLAN_HLEN;

skb->mac_len isn't adjusted. You forgot to move the headers resetting
after vlan_untag().
Jiri Pirko June 10, 2011, 9:34 a.m. UTC | #2 
Fri, Jun 10, 2011 at 11:26:06AM CEST, xiaosuo@gmail.com wrote:
>On Fri, Jun 10, 2011 at 4:35 PM, Jiri Pirko <jpirko@redhat.com> wrote:
>> +
>> +/* Should be used only to revert vlan_reorder_header action */
>> +static struct sk_buff *vlan_unreorder_header(struct sk_buff *skb)
>> +{
>> +       unsigned char *mac_header;
>> +       struct vlan_ethhdr *veth;
>> +
>> +       if (skb_cow(skb, skb_headroom(skb)) < 0)
>> +               return NULL;
>
>I think we need to make sure if there is enough headroom for this
>header expansion.

Well the header expansion was previously there so there should be place
there in all cases, or am I wrong?

>
>> +       skb->mac_header -= VLAN_HLEN;
>
>skb->mac_len isn't adjusted. You forgot to move the headers resetting
>after vlan_untag().

that is correct, I'll move that. I'll also add reset after unreorder.

Thanks Changli.

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
Changli Gao June 10, 2011, 9:49 a.m. UTC | #3 
On Fri, Jun 10, 2011 at 5:34 PM, Jiri Pirko <jpirko@redhat.com> wrote:
> Fri, Jun 10, 2011 at 11:26:06AM CEST, xiaosuo@gmail.com wrote:
>>On Fri, Jun 10, 2011 at 4:35 PM, Jiri Pirko <jpirko@redhat.com> wrote:
>>> +
>>> +/* Should be used only to revert vlan_reorder_header action */
>>> +static struct sk_buff *vlan_unreorder_header(struct sk_buff *skb)
>>> +{
>>> +       unsigned char *mac_header;
>>> +       struct vlan_ethhdr *veth;
>>> +
>>> +       if (skb_cow(skb, skb_headroom(skb)) < 0)
>>> +               return NULL;
>>
>>I think we need to make sure if there is enough headroom for this
>>header expansion.
>
> Well the header expansion was previously there so there should be place
> there in all cases, or am I wrong?

For hw-accel-vlan-rx, is the headroom for this header expansion
reserved? I don't think so. Thanks.
Jiri Pirko June 10, 2011, 10:35 a.m. UTC | #4 
Fri, Jun 10, 2011 at 11:49:27AM CEST, xiaosuo@gmail.com wrote:
>On Fri, Jun 10, 2011 at 5:34 PM, Jiri Pirko <jpirko@redhat.com> wrote:
>> Fri, Jun 10, 2011 at 11:26:06AM CEST, xiaosuo@gmail.com wrote:
>>>On Fri, Jun 10, 2011 at 4:35 PM, Jiri Pirko <jpirko@redhat.com> wrote:
>>>> +
>>>> +/* Should be used only to revert vlan_reorder_header action */
>>>> +static struct sk_buff *vlan_unreorder_header(struct sk_buff *skb)
>>>> +{
>>>> +       unsigned char *mac_header;
>>>> +       struct vlan_ethhdr *veth;
>>>> +
>>>> +       if (skb_cow(skb, skb_headroom(skb)) < 0)
>>>> +               return NULL;
>>>
>>>I think we need to make sure if there is enough headroom for this
>>>header expansion.
>>
>> Well the header expansion was previously there so there should be place
>> there in all cases, or am I wrong?
>
>For hw-accel-vlan-rx, is the headroom for this header expansion
>reserved? I don't think so. Thanks.

But this wasn't done for hw-accel-vlan-rx previously right? So why don't
check for hw-accel-vlan-rx and don do unreorder in that case?
>
>-- 
>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
Changli Gao June 10, 2011, 11:20 a.m. UTC | #5 
On Fri, Jun 10, 2011 at 6:35 PM, Jiri Pirko <jpirko@redhat.com> wrote:
> Fri, Jun 10, 2011 at 11:49:27AM CEST, xiaosuo@gmail.com wrote:
>>
>>For hw-accel-vlan-rx, is the headroom for this header expansion
>>reserved? I don't think so. Thanks.
>
> But this wasn't done for hw-accel-vlan-rx previously right? So why don't
> check for hw-accel-vlan-rx and don do unreorder in that case?
>

Yes, it wasn't done before. We want to make the behaviors of
hw-accel-vlan-rx and non-hw-accel-vlan-rx are the same. Is it your
original goal of your vlan-untag patch?
Jiri Pirko June 10, 2011, 12:12 p.m. UTC | #6 
Fri, Jun 10, 2011 at 01:20:29PM CEST, xiaosuo@gmail.com wrote:
>On Fri, Jun 10, 2011 at 6:35 PM, Jiri Pirko <jpirko@redhat.com> wrote:
>> Fri, Jun 10, 2011 at 11:49:27AM CEST, xiaosuo@gmail.com wrote:
>>>
>>>For hw-accel-vlan-rx, is the headroom for this header expansion
>>>reserved? I don't think so. Thanks.
>>
>> But this wasn't done for hw-accel-vlan-rx previously right? So why don't
>> check for hw-accel-vlan-rx and don do unreorder in that case?
>>
>
>Yes, it wasn't done before. We want to make the behaviors of
>hw-accel-vlan-rx and non-hw-accel-vlan-rx are the same. Is it your
>original goal of your vlan-untag patch?

You are correct. I just thought that since disabling reordering will be
most likely removed in near future, non-hw-accel-vlan-rx reorder disable
would not be needed.

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

Patch

diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 41495dc..d71cc81 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -4,6 +4,33 @@ 
 #include <linux/netpoll.h>
 #include "vlan.h"
 
+static struct sk_buff *vlan_reorder_header(struct sk_buff *skb)
+{
+	if (skb_cow(skb, skb_headroom(skb)) < 0)
+		return NULL;
+	/* Lifted from Gleb's VLAN code... */
+	memmove(skb->data - ETH_HLEN, skb->data - VLAN_ETH_HLEN, 12);
+	skb->mac_header += VLAN_HLEN;
+	return skb;
+}
+
+/* Should be used only to revert vlan_reorder_header action */
+static struct sk_buff *vlan_unreorder_header(struct sk_buff *skb)
+{
+	unsigned char *mac_header;
+	struct vlan_ethhdr *veth;
+
+	if (skb_cow(skb, skb_headroom(skb)) < 0)
+		return NULL;
+	skb->mac_header -= VLAN_HLEN;
+	mac_header = skb_mac_header(skb);
+	memmove(mac_header, mac_header + VLAN_HLEN, 12);
+	veth = (struct vlan_ethhdr *) mac_header;
+	veth->h_vlan_proto = htons(ETH_P_8021Q);
+	veth->h_vlan_TCI = htons(skb->vlan_tci);
+	return skb;
+}
+
 bool vlan_do_receive(struct sk_buff **skbp)
 {
 	struct sk_buff *skb = *skbp;
@@ -23,6 +50,21 @@  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 (!(vlan_dev_info(vlan_dev)->flags & VLAN_FLAG_REORDER_HDR)) {
+		skb = *skbp = vlan_unreorder_header(skb);
+		if (!skb)
+			return false;
+	}
+
 	skb->priority = vlan_get_ingress_priority(vlan_dev, skb->vlan_tci);
 	skb->vlan_tci = 0;
 
@@ -31,22 +73,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,21 +117,6 @@  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)
-{
-	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;
-		}
-	}
-	return skb;
-}
-
 static void vlan_set_encap_proto(struct sk_buff *skb, struct vlan_hdr *vhdr)
 {
 	__be16 proto;
@@ -161,7 +174,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;