| Message ID | 1319904819.2586.45.camel@edumazet-laptop |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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);
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