Patchwork [v2] vlan: allow nested vlan_do_receive()

login
register
mail settings
Submitter Eric Dumazet
Date Oct. 29, 2011, 4:13 p.m.
Message ID <1319904819.2586.45.camel@edumazet-laptop>
Download mbox | patch
Permalink /patch/122542/
State Accepted
Delegated to: David Miller
Headers show

Comments

Eric Dumazet - Oct. 29, 2011, 4:13 p.m.
commit 2425717b27eb (net: allow vlan traffic to be received under bond)
broke ARP processing on vlan on top of bonding.

       +-------+
eth0 --| bond0 |---bond0.103
eth1 --|       |
       +-------+

52870.115435: skb_gro_reset_offset <-napi_gro_receive
52870.115435: dev_gro_receive <-napi_gro_receive
52870.115435: napi_skb_finish <-napi_gro_receive
52870.115435: netif_receive_skb <-napi_skb_finish
52870.115435: get_rps_cpu <-netif_receive_skb
52870.115435: __netif_receive_skb <-netif_receive_skb
52870.115436: vlan_do_receive <-__netif_receive_skb
52870.115436: bond_handle_frame <-__netif_receive_skb
52870.115436: vlan_do_receive <-__netif_receive_skb
52870.115436: arp_rcv <-__netif_receive_skb
52870.115436: kfree_skb <-arp_rcv

Packet is dropped in arp_rcv() because its pkt_type was set to
PACKET_OTHERHOST in the first vlan_do_receive() call, since no eth0.103
exists.

We really need to change pkt_type only if no more rx_handler is about to
be called for the packet.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
V2 : change the vlan_do_receive() added argument to be a boolean

 include/linux/if_vlan.h |    6 +++---
 net/8021q/vlan_core.c   |    7 +++++--
 net/core/dev.c          |    4 ++--
 3 files changed, 10 insertions(+), 7 deletions(-)



--
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 - Oct. 29, 2011, 4:28 p.m.
Sat, Oct 29, 2011 at 06:13:39PM CEST, eric.dumazet@gmail.com wrote:
>commit 2425717b27eb (net: allow vlan traffic to be received under bond)
>broke ARP processing on vlan on top of bonding.
>
>       +-------+
>eth0 --| bond0 |---bond0.103
>eth1 --|       |
>       +-------+
>
>52870.115435: skb_gro_reset_offset <-napi_gro_receive
>52870.115435: dev_gro_receive <-napi_gro_receive
>52870.115435: napi_skb_finish <-napi_gro_receive
>52870.115435: netif_receive_skb <-napi_skb_finish
>52870.115435: get_rps_cpu <-netif_receive_skb
>52870.115435: __netif_receive_skb <-netif_receive_skb
>52870.115436: vlan_do_receive <-__netif_receive_skb
>52870.115436: bond_handle_frame <-__netif_receive_skb
>52870.115436: vlan_do_receive <-__netif_receive_skb
>52870.115436: arp_rcv <-__netif_receive_skb
>52870.115436: kfree_skb <-arp_rcv
>
>Packet is dropped in arp_rcv() because its pkt_type was set to
>PACKET_OTHERHOST in the first vlan_do_receive() call, since no eth0.103
>exists.
>
>We really need to change pkt_type only if no more rx_handler is about to
>be called for the packet.
>
>Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>---
>V2 : change the vlan_do_receive() added argument to be a boolean
>
> include/linux/if_vlan.h |    6 +++---
> net/8021q/vlan_core.c   |    7 +++++--
> net/core/dev.c          |    4 ++--
> 3 files changed, 10 insertions(+), 7 deletions(-)
>
>diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
>index 44da482..12d5543 100644
>--- a/include/linux/if_vlan.h
>+++ b/include/linux/if_vlan.h
>@@ -106,7 +106,7 @@ extern struct net_device *__vlan_find_dev_deep(struct net_device *real_dev,
> extern struct net_device *vlan_dev_real_dev(const struct net_device *dev);
> extern u16 vlan_dev_vlan_id(const struct net_device *dev);
> 
>-extern bool vlan_do_receive(struct sk_buff **skb);
>+extern bool vlan_do_receive(struct sk_buff **skb, bool last_handler);
> extern struct sk_buff *vlan_untag(struct sk_buff *skb);
> 
> #else
>@@ -128,9 +128,9 @@ static inline u16 vlan_dev_vlan_id(const struct net_device *dev)
> 	return 0;
> }
> 
>-static inline bool vlan_do_receive(struct sk_buff **skb)
>+static inline bool vlan_do_receive(struct sk_buff **skb, bool last_handler)
> {
>-	if ((*skb)->vlan_tci & VLAN_VID_MASK)
>+	if (((*skb)->vlan_tci & VLAN_VID_MASK) && last_handler)
> 		(*skb)->pkt_type = PACKET_OTHERHOST;
> 	return false;
> }
>diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>index f1f2f7b..163397f 100644
>--- a/net/8021q/vlan_core.c
>+++ b/net/8021q/vlan_core.c
>@@ -4,7 +4,7 @@
> #include <linux/netpoll.h>
> #include "vlan.h"
> 
>-bool vlan_do_receive(struct sk_buff **skbp)
>+bool vlan_do_receive(struct sk_buff **skbp, bool last_handler)
> {
> 	struct sk_buff *skb = *skbp;
> 	u16 vlan_id = skb->vlan_tci & VLAN_VID_MASK;
>@@ -13,7 +13,10 @@ bool vlan_do_receive(struct sk_buff **skbp)
> 
> 	vlan_dev = vlan_find_dev(skb->dev, vlan_id);
> 	if (!vlan_dev) {
>-		if (vlan_id)
>+		/* Only the last call to vlan_do_receive() should change
>+		 * pkt_type to PACKET_OTHERHOST
>+		 */
>+		if (vlan_id && last_handler)
> 			skb->pkt_type = PACKET_OTHERHOST;
> 		return false;
> 	}
>diff --git a/net/core/dev.c b/net/core/dev.c
>index edcf019..6ba50a1 100644
>--- a/net/core/dev.c
>+++ b/net/core/dev.c
>@@ -3283,18 +3283,18 @@ another_round:
> ncls:
> #endif
> 
>+	rx_handler = rcu_dereference(skb->dev->rx_handler);
> 	if (vlan_tx_tag_present(skb)) {
> 		if (pt_prev) {
> 			ret = deliver_skb(skb, pt_prev, orig_dev);
> 			pt_prev = NULL;
> 		}
>-		if (vlan_do_receive(&skb))
>+		if (vlan_do_receive(&skb, !rx_handler))

This I had on mind as well. Looks nicer. I have one another thought how
to resolve this. I will try it and let you know by tomorrow.

Jirka

> 			goto another_round;
> 		else if (unlikely(!skb))
> 			goto out;
> 	}
> 
>-	rx_handler = rcu_dereference(skb->dev->rx_handler);
> 	if (rx_handler) {
> 		if (pt_prev) {
> 			ret = deliver_skb(skb, pt_prev, orig_dev);
>
>
--
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 - Oct. 30, 2011, 8:38 a.m.
Sat, Oct 29, 2011 at 06:28:40PM CEST, jpirko@redhat.com wrote:
>Sat, Oct 29, 2011 at 06:13:39PM CEST, eric.dumazet@gmail.com wrote:
>>commit 2425717b27eb (net: allow vlan traffic to be received under bond)
>>broke ARP processing on vlan on top of bonding.
>>
>>       +-------+
>>eth0 --| bond0 |---bond0.103
>>eth1 --|       |
>>       +-------+
>>
>>52870.115435: skb_gro_reset_offset <-napi_gro_receive
>>52870.115435: dev_gro_receive <-napi_gro_receive
>>52870.115435: napi_skb_finish <-napi_gro_receive
>>52870.115435: netif_receive_skb <-napi_skb_finish
>>52870.115435: get_rps_cpu <-netif_receive_skb
>>52870.115435: __netif_receive_skb <-netif_receive_skb
>>52870.115436: vlan_do_receive <-__netif_receive_skb
>>52870.115436: bond_handle_frame <-__netif_receive_skb
>>52870.115436: vlan_do_receive <-__netif_receive_skb
>>52870.115436: arp_rcv <-__netif_receive_skb
>>52870.115436: kfree_skb <-arp_rcv
>>
>>Packet is dropped in arp_rcv() because its pkt_type was set to
>>PACKET_OTHERHOST in the first vlan_do_receive() call, since no eth0.103
>>exists.
>>
>>We really need to change pkt_type only if no more rx_handler is about to
>>be called for the packet.
>>
>>Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>>---
>>V2 : change the vlan_do_receive() added argument to be a boolean
>>
>> include/linux/if_vlan.h |    6 +++---
>> net/8021q/vlan_core.c   |    7 +++++--
>> net/core/dev.c          |    4 ++--
>> 3 files changed, 10 insertions(+), 7 deletions(-)
>>
>>diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
>>index 44da482..12d5543 100644
>>--- a/include/linux/if_vlan.h
>>+++ b/include/linux/if_vlan.h
>>@@ -106,7 +106,7 @@ extern struct net_device *__vlan_find_dev_deep(struct net_device *real_dev,
>> extern struct net_device *vlan_dev_real_dev(const struct net_device *dev);
>> extern u16 vlan_dev_vlan_id(const struct net_device *dev);
>> 
>>-extern bool vlan_do_receive(struct sk_buff **skb);
>>+extern bool vlan_do_receive(struct sk_buff **skb, bool last_handler);
>> extern struct sk_buff *vlan_untag(struct sk_buff *skb);
>> 
>> #else
>>@@ -128,9 +128,9 @@ static inline u16 vlan_dev_vlan_id(const struct net_device *dev)
>> 	return 0;
>> }
>> 
>>-static inline bool vlan_do_receive(struct sk_buff **skb)
>>+static inline bool vlan_do_receive(struct sk_buff **skb, bool last_handler)
>> {
>>-	if ((*skb)->vlan_tci & VLAN_VID_MASK)
>>+	if (((*skb)->vlan_tci & VLAN_VID_MASK) && last_handler)
>> 		(*skb)->pkt_type = PACKET_OTHERHOST;
>> 	return false;
>> }
>>diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
>>index f1f2f7b..163397f 100644
>>--- a/net/8021q/vlan_core.c
>>+++ b/net/8021q/vlan_core.c
>>@@ -4,7 +4,7 @@
>> #include <linux/netpoll.h>
>> #include "vlan.h"
>> 
>>-bool vlan_do_receive(struct sk_buff **skbp)
>>+bool vlan_do_receive(struct sk_buff **skbp, bool last_handler)
>> {
>> 	struct sk_buff *skb = *skbp;
>> 	u16 vlan_id = skb->vlan_tci & VLAN_VID_MASK;
>>@@ -13,7 +13,10 @@ bool vlan_do_receive(struct sk_buff **skbp)
>> 
>> 	vlan_dev = vlan_find_dev(skb->dev, vlan_id);
>> 	if (!vlan_dev) {
>>-		if (vlan_id)
>>+		/* Only the last call to vlan_do_receive() should change
>>+		 * pkt_type to PACKET_OTHERHOST
>>+		 */
>>+		if (vlan_id && last_handler)
>> 			skb->pkt_type = PACKET_OTHERHOST;
>> 		return false;
>> 	}
>>diff --git a/net/core/dev.c b/net/core/dev.c
>>index edcf019..6ba50a1 100644
>>--- a/net/core/dev.c
>>+++ b/net/core/dev.c
>>@@ -3283,18 +3283,18 @@ another_round:
>> ncls:
>> #endif
>> 
>>+	rx_handler = rcu_dereference(skb->dev->rx_handler);
>> 	if (vlan_tx_tag_present(skb)) {
>> 		if (pt_prev) {
>> 			ret = deliver_skb(skb, pt_prev, orig_dev);
>> 			pt_prev = NULL;
>> 		}
>>-		if (vlan_do_receive(&skb))
>>+		if (vlan_do_receive(&skb, !rx_handler))
>
>This I had on mind as well. Looks nicer. I have one another thought how
>to resolve this. I will try it and let you know by tomorrow.

Okay that would not work.

So I'm okay with this patch 


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


>
>Jirka
>
>> 			goto another_round;
>> 		else if (unlikely(!skb))
>> 			goto out;
>> 	}
>> 
>>-	rx_handler = rcu_dereference(skb->dev->rx_handler);
>> 	if (rx_handler) {
>> 		if (pt_prev) {
>> 			ret = deliver_skb(skb, pt_prev, orig_dev);
>>
>>
--
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 - Oct. 30, 2011, 8:44 a.m.
From: Jiri Pirko <jpirko@redhat.com>
Date: Sun, 30 Oct 2011 09:38:12 +0100

> Sat, Oct 29, 2011 at 06:28:40PM CEST, jpirko@redhat.com wrote:
>>Sat, Oct 29, 2011 at 06:13:39PM CEST, eric.dumazet@gmail.com wrote:
>>>commit 2425717b27eb (net: allow vlan traffic to be received under bond)
>>>broke ARP processing on vlan on top of bonding.
 ...
>>>Packet is dropped in arp_rcv() because its pkt_type was set to
>>>PACKET_OTHERHOST in the first vlan_do_receive() call, since no eth0.103
>>>exists.
>>>
>>>We really need to change pkt_type only if no more rx_handler is about to
>>>be called for the packet.
>>>
>>>Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>>>---
>>>V2 : change the vlan_do_receive() added argument to be a boolean
 ...
> Reviewed-by: Jiri Pirko <jpirko@redhat.com>

Applied, thanks everyone.
--
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 Dumazet - Oct. 30, 2011, 8:44 a.m.
Le dimanche 30 octobre 2011 à 09:38 +0100, Jiri Pirko a écrit :


> So I'm okay with this patch 
> 
> 
> Reviewed-by: Jiri Pirko <jpirko@redhat.com>
> 

Thanks a lot for this review


--
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 44da482..12d5543 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -106,7 +106,7 @@  extern struct net_device *__vlan_find_dev_deep(struct net_device *real_dev,
 extern struct net_device *vlan_dev_real_dev(const struct net_device *dev);
 extern u16 vlan_dev_vlan_id(const struct net_device *dev);
 
-extern bool vlan_do_receive(struct sk_buff **skb);
+extern bool vlan_do_receive(struct sk_buff **skb, bool last_handler);
 extern struct sk_buff *vlan_untag(struct sk_buff *skb);
 
 #else
@@ -128,9 +128,9 @@  static inline u16 vlan_dev_vlan_id(const struct net_device *dev)
 	return 0;
 }
 
-static inline bool vlan_do_receive(struct sk_buff **skb)
+static inline bool vlan_do_receive(struct sk_buff **skb, bool last_handler)
 {
-	if ((*skb)->vlan_tci & VLAN_VID_MASK)
+	if (((*skb)->vlan_tci & VLAN_VID_MASK) && last_handler)
 		(*skb)->pkt_type = PACKET_OTHERHOST;
 	return false;
 }
diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index f1f2f7b..163397f 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -4,7 +4,7 @@ 
 #include <linux/netpoll.h>
 #include "vlan.h"
 
-bool vlan_do_receive(struct sk_buff **skbp)
+bool vlan_do_receive(struct sk_buff **skbp, bool last_handler)
 {
 	struct sk_buff *skb = *skbp;
 	u16 vlan_id = skb->vlan_tci & VLAN_VID_MASK;
@@ -13,7 +13,10 @@  bool vlan_do_receive(struct sk_buff **skbp)
 
 	vlan_dev = vlan_find_dev(skb->dev, vlan_id);
 	if (!vlan_dev) {
-		if (vlan_id)
+		/* Only the last call to vlan_do_receive() should change
+		 * pkt_type to PACKET_OTHERHOST
+		 */
+		if (vlan_id && last_handler)
 			skb->pkt_type = PACKET_OTHERHOST;
 		return false;
 	}
diff --git a/net/core/dev.c b/net/core/dev.c
index edcf019..6ba50a1 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3283,18 +3283,18 @@  another_round:
 ncls:
 #endif
 
+	rx_handler = rcu_dereference(skb->dev->rx_handler);
 	if (vlan_tx_tag_present(skb)) {
 		if (pt_prev) {
 			ret = deliver_skb(skb, pt_prev, orig_dev);
 			pt_prev = NULL;
 		}
-		if (vlan_do_receive(&skb))
+		if (vlan_do_receive(&skb, !rx_handler))
 			goto another_round;
 		else if (unlikely(!skb))
 			goto out;
 	}
 
-	rx_handler = rcu_dereference(skb->dev->rx_handler);
 	if (rx_handler) {
 		if (pt_prev) {
 			ret = deliver_skb(skb, pt_prev, orig_dev);