net: deliver skbs on inactive slaves to exact matches

PATCH In review John Fastabend 4 participants 6 messages Jun 3, 2010 – Jun 14, 2010

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

John Fastabend

author

2010-06-03 19:32 UTC #0 lore raw

PATCH v2 1/1 net: deliver skbs on inactive slaves to exact matches

Currently, the accelerated receive path for VLAN's will drop packets if the real device is an inactive slave and is not one of the special pkts tested for in skb_bond_should_drop(). This behavior is different then the non-accelerated path and for pkts over a bonded vlan.

For example,

vlanx -> bond0 -> ethx

will be dropped in the vlan path and not delivered to any packet handlers at all. However,

bond0 -> vlanx -> ethx

and

bond0 -> ethx

will be delivered to handlers that match the exact dev, because the VLAN path checks the real_dev which is not a slave and netif_recv_skb() doesn't drop frames but only delivers them to exact matches.

This patch adds a sk_buff flag which is used for tagging skbs that would previously been dropped and allows the skb to continue to skb_netif_recv(). Here we add logic to check for the deliver_no_wcard flag and if it is set only deliver to handlers that match exactly. This makes both paths above consistent and gives pkt handlers a way to identify skbs that come from inactive slaves. Without this patch in some configurations skbs will be delivered to handlers with exact matches and in others be dropped out right in the vlan path.

I have tested the following 4 configurations in failover modes and load balancing modes.

# bond0 -> ethx

# vlanx -> bond0 -> ethx

# bond0 -> vlanx -> ethx

# bond0 -> ethx
            |
  vlanx -> --

Signed-off-by John Fastabend <[email protected]>

---

 include/linux/skbuff.h |    5 ++++-
 net/8021q/vlan_core.c  |    4 ++--
 net/core/dev.c         |   17 ++++++++++++++---
 3 files changed, 20 insertions(+), 6 deletions(-)

include/linux/skbuff.h +4 -1

index bf243fc..f89e7fd 100644
@@ -380,7 +380,10 @@ struct sk_buff {
 	kmemcheck_bitfield_begin(flags2);
 	__u16			queue_mapping:16;
 #ifdef CONFIG_IPV6_NDISC_NODETYPE
-	__u8			ndisc_nodetype:2;
+	__u8			ndisc_nodetype:2,
+				deliver_no_wcard:1;
+#else
+	__u8			deliver_no_wcard:1;
 #endif
 	kmemcheck_bitfield_end(flags2);

net/8021q/vlan_core.c +2 -2

index bd537fc..50f58f5 100644
@@ -12,7 +12,7 @@ int __vlan_hwaccel_rx(struct sk_buff *skb, struct vlan_group *grp,
 		return NET_RX_DROP;
 
 	if (skb_bond_should_drop(skb, ACCESS_ONCE(skb->dev->master)))
-		goto drop;
+		skb->deliver_no_wcard = 1;
 
 	skb->skb_iif = skb->dev->ifindex;
 	__vlan_hwaccel_put_tag(skb, vlan_tci);

@@ -84,7 +84,7 @@ vlan_gro_common(struct napi_struct *napi, struct vlan_group *grp,
 	struct sk_buff *p;
 
 	if (skb_bond_should_drop(skb, ACCESS_ONCE(skb->dev->master)))
-		goto drop;
+		skb->deliver_no_wcard = 1;
 
 	skb->skb_iif = skb->dev->ifindex;
 	__vlan_hwaccel_put_tag(skb, vlan_tci);

net/core/dev.c +14 -3

index ccba781..9c356fd 100644
@@ -2821,13 +2821,24 @@ static int __netif_receive_skb(struct sk_buff *skb)
 	if (!skb->skb_iif)
 		skb->skb_iif = skb->dev->ifindex;
 
+	/*
+	 * bonding note: skbs received on inactive slaves should only
+	 * be delivered to pkt handlers that are exact matches.  Also
+	 * the deliver_no_wcard flag will be set.  If packet handlers
+	 * are sensitive to duplicate packets these skbs will need to
+	 * be dropped at the handler.  The vlan accel path may have
+	 * already set the deliver_no_wcard flag.
+	 */
 	null_or_orig = NULL;
 	orig_dev = skb->dev;
 	master = ACCESS_ONCE(orig_dev->master);
-	if (master) {
-		if (skb_bond_should_drop(skb, master))
+	if (skb->deliver_no_wcard)
+		null_or_orig = orig_dev;
+	else if (master) {
+		if (skb_bond_should_drop(skb, master)) {
+			skb->deliver_no_wcard = 1;
 			null_or_orig = orig_dev; /* deliver only exact match */
-		else
+		} else
 			skb->dev = master;
 	}

David Miller

maintainer

2010-06-11 05:31 UTC #1 lore raw

From: John Fastabend <[email protected]>
Date: Thu, 03 Jun 2010 12:30:11 -0700

Quoted 5 lines

Currently, the accelerated receive path for VLAN's will drop packets if the real device is an inactive slave and is not one of the special pkts tested for in skb_bond_should_drop(). This behavior is different then the non-accelerated path and for pkts over a bonded vlan.

...

Quoted 10 lines

This patch adds a sk_buff flag which is used for tagging skbs that would previously been dropped and allows the skb to continue to skb_netif_recv(). Here we add logic to check for the deliver_no_wcard flag and if it is set only deliver to handlers that match exactly. This makes both paths above consistent and gives pkt handlers a way to identify skbs that come from inactive slaves. Without this patch in some configurations skbs will be delivered to handlers with exact matches and in others be dropped out right in the vlan path.

...

Quoted 1 line

Signed-off-by John Fastabend <[email protected]>

I can't figure out a better way to fix this myself so I've applied your patch, thanks John!

Michael S. Tsirkin

2010-06-14 13:26 UTC #2 lore raw

In reply to John Fastabend

Quoted 42 lines · John Fastabend

Currently, the accelerated receive path for VLAN's will drop packets if the real device is an inactive slave and is not one of the special pkts tested for in skb_bond_should_drop(). This behavior is different then the non-accelerated path and for pkts over a bonded vlan.
For example,
vlanx -> bond0 -> ethx
will be dropped in the vlan path and not delivered to any packet handlers at all. However,
bond0 -> vlanx -> ethx
and
bond0 -> ethx
will be delivered to handlers that match the exact dev, because the VLAN path checks the real_dev which is not a slave and netif_recv_skb() doesn't drop frames but only delivers them to exact matches.
This patch adds a sk_buff flag which is used for tagging skbs that would previously been dropped and allows the skb to continue to skb_netif_recv(). Here we add logic to check for the deliver_no_wcard flag and if it is set only deliver to handlers that match exactly. This makes both paths above consistent and gives pkt handlers a way to identify skbs that come from inactive slaves. Without this patch in some configurations skbs will be delivered to handlers with exact matches and in others be dropped out right in the vlan path.
I have tested the following 4 configurations in failover modes and load balancing modes.
# bond0 -> ethx
# vlanx -> bond0 -> ethx

Quoted 7 lines

# bond0 -> vlanx -> ethx
# bond0 -> ethx
            |
  vlanx -> --
Signed-off-by John Fastabend <[email protected]>

I am using qemu with both tap and slirp (userspace) networking. This works fine under 2.6.35-rc2 but breaks under 2.6.35-rc3:
ssh over slirp stops working sometimes right away
and sometimes after a bit of use, connection times out.

Git bisect gave me this commit:
597a264b1a9c7e36d1728f677c66c5c1f7e3b837.

Reverting 597a264b1a9c7e36d1728f677c66c5c1f7e3b837 fixes the issue for me.

I'm short for time now so didn't debug this further. I opened a bugzilla to track this issue:
https://bugzilla.kernel.org/show_bug.cgi?id=16204

Signature · 1 line

MST

Eric Dumazet

Acked-by

2010-06-14 13:42 UTC #3 lore raw

From: John Fastabend <[email protected]>

Quoted 59 lines · Michael S. Tsirkin

Quoted 42 lines · John Fastabend
Currently, the accelerated receive path for VLAN's will drop packets if the real device is an inactive slave and is not one of the special pkts tested for in skb_bond_should_drop(). This behavior is different then the non-accelerated path and for pkts over a bonded vlan.
For example,
vlanx -> bond0 -> ethx
will be dropped in the vlan path and not delivered to any packet handlers at all. However,
bond0 -> vlanx -> ethx
and
bond0 -> ethx
will be delivered to handlers that match the exact dev, because the VLAN path checks the real_dev which is not a slave and netif_recv_skb() doesn't drop frames but only delivers them to exact matches.
This patch adds a sk_buff flag which is used for tagging skbs that would previously been dropped and allows the skb to continue to skb_netif_recv(). Here we add logic to check for the deliver_no_wcard flag and if it is set only deliver to handlers that match exactly. This makes both paths above consistent and gives pkt handlers a way to identify skbs that come from inactive slaves. Without this patch in some configurations skbs will be delivered to handlers with exact matches and in others be dropped out right in the vlan path.
I have tested the following 4 configurations in failover modes and load balancing modes.
# bond0 -> ethx
# vlanx -> bond0 -> ethx
Quoted 7 lines
# bond0 -> vlanx -> ethx
# bond0 -> ethx
            |
  vlanx -> --
Signed-off-by John Fastabend <[email protected]>
I am using qemu with both tap and slirp (userspace) networking. This works fine under 2.6.35-rc2 but breaks under 2.6.35-rc3:
ssh over slirp stops working sometimes right away
and sometimes after a bit of use, connection times out.
Git bisect gave me this commit:
597a264b1a9c7e36d1728f677c66c5c1f7e3b837.

Quoted 7 lines

Reverting 597a264b1a9c7e36d1728f677c66c5c1f7e3b837 fixes the issue for me.
I'm short for time now so didn't debug this further. I opened a bugzilla to track this issue:
https://bugzilla.kernel.org/show_bug.cgi?id=16204

A fix is already there, and bug is already opened multiple times.

http://lkml.org/lkml/2010/6/13/155

[PATCH] net: fix deliver_no_wcard regression on loopback device

deliver_no_wcard is not being set in skb_copy_header. In the skb_cloned case it is not being cleared and may cause the skb to be dropped when the loopback device pushes it back up the stack.

Signed-off-by John Fastabend <[email protected]>

Acked-by Eric Dumazet <[email protected]>

---

net/core/skbuff.c +1 -0

index 9f07e74..bcf2fa3 100644
@@ -532,6 +532,7 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old)
 	new->ip_summed		= old->ip_summed;
 	skb_copy_queue_mapping(new, old);
 	new->priority		= old->priority;
+	new->deliver_no_wcard	= old->deliver_no_wcard;
 #if defined(CONFIG_IP_VS) || defined(CONFIG_IP_VS_MODULE)
 	new->ipvs_property	= old->ipvs_property;
 #endif

Eric Dumazet

Tested-by Markus Trippelsdorf

2010-06-14 13:38 UTC #4 lore raw

Quoted 9 lines · Eric Dumazet

[PATCH] net: fix deliver_no_wcard regression on loopback device
deliver_no_wcard is not being set in skb_copy_header. In the skb_cloned case it is not being cleared and may cause the skb to be dropped when the loopback device pushes it back up the stack.
Signed-off-by John Fastabend <[email protected]>
Acked-by Eric Dumazet <[email protected]>

Oh I forgot :

Tested-by Markus Trippelsdorf <[email protected]>

Quoted 14 lines

---

net/core/skbuff.c +1 -0

index 9f07e74..bcf2fa3 100644
@@ -532,6 +532,7 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old)
 	new->ip_summed		= old->ip_summed;
 	skb_copy_queue_mapping(new, old);
 	new->priority		= old->priority;
+	new->deliver_no_wcard	= old->deliver_no_wcard;
 #if defined(CONFIG_IP_VS) || defined(CONFIG_IP_VS_MODULE)
 	new->ipvs_property	= old->ipvs_property;
 #endif

Michael S. Tsirkin

2010-06-14 14:15 UTC #5 lore raw

Quoted 9 lines · Eric Dumazet

Quoted 7 lines · Eric Dumazet
[PATCH] net: fix deliver_no_wcard regression on loopback device
deliver_no_wcard is not being set in skb_copy_header. In the skb_cloned case it is not being cleared and may cause the skb to be dropped when the loopback device pushes it back up the stack.

Quoted 6 lines

Quoted 2 lines
Signed-off-by John Fastabend <[email protected]>
Acked-by Eric Dumazet <[email protected]>
Oh I forgot :
Tested-by Markus Trippelsdorf <[email protected]>

Grr. Could have saved myself a bit of time if I guessed it's related.

Quoted 15 lines

Quoted 14 lines

---

net/core/skbuff.c +1 -0

index 9f07e74..bcf2fa3 100644
@@ -532,6 +532,7 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old)
 	new->ip_summed		= old->ip_summed;
 	skb_copy_queue_mapping(new, old);
 	new->priority		= old->priority;
+	new->deliver_no_wcard	= old->deliver_no_wcard;
 #if defined(CONFIG_IP_VS) || defined(CONFIG_IP_VS_MODULE)
 	new->ipvs_property	= old->ipvs_property;
 #endif