Skip to contentSkip to conversation

vxlan: handle skb_clone failure

PATCH Applied Stephen Hemminger 4 participants 13 messages Jun 10, 2013 – Jun 11, 2013 1 message not in the archive

Subsystems (2)

v2 of 3 Applied Latest is v3 →

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

#1 lore raw

PATCH v2 1/3 vxlan: only migrate dynamic FDB entries

Only migrate dynamic forwarding table entries, don't modify static entries. If packet received from incorrect source IP address assume it is an imposter and drop it.

---
Should go to -stable as well.

drivers/net/vxlan.c +13 -5
@@ -603,9 +603,10 @@ skip:
 
 /* Watch incoming packets to learn mapping between Ethernet address
  * and Tunnel endpoint.
+ * Return true if packet is bogus and should be droppped.
  */
-static void vxlan_snoop(struct net_device *dev,
-			__be32 src_ip, const u8 *src_mac)
+static bool vxlan_snoop(struct net_device *dev,
+		       __be32 src_ip, const u8 *src_mac)
 {
 	struct vxlan_dev *vxlan = netdev_priv(dev);
 	struct vxlan_fdb *f;
@@ -614,7 +615,11 @@ static void vxlan_snoop(struct net_devic
 	f = vxlan_find_mac(vxlan, src_mac);
 	if (likely(f)) {
 		if (likely(f->remote.remote_ip == src_ip))
-			return;
+			return false;
+
+		/* Don't migrate static entries, drop packets */
+		if (!(f->flags & NTF_SELF))
+			return true;
 
 		if (net_ratelimit())
 			netdev_info(dev,
@@ -634,6 +639,8 @@ static void vxlan_snoop(struct net_devic
 				       0, NTF_SELF);
 		spin_unlock(&vxlan->hash_lock);
 	}
+
+	return false;
 }
 
 
@@ -766,8 +773,9 @@ static int vxlan_udp_encap_recv(struct s
 			       vxlan->dev->dev_addr) == 0)
 		goto drop;
 
-	if (vxlan->flags & VXLAN_F_LEARN)
-		vxlan_snoop(skb->dev, oip->saddr, eth_hdr(skb)->h_source);
+	if ((vxlan->flags & VXLAN_F_LEARN) &&
+	    vxlan_snoop(skb->dev, oip->saddr, eth_hdr(skb)->h_source))
+		goto drop;
 
 	__skb_tunnel_rx(skb, vxlan->dev);
 	skb_reset_network_header(skb);
#2 lore raw

Re: [PATCH net v2 01/03] vxlan: only migrate dynamic FDB entries

Hello.

Quoted 12 lines · Stephen Hemminger

Only migrate dynamic forwarding table entries, don't modify static entries. If packet received from incorrect source IP address assume it is an imposter and drop it.

---
Should go to -stable as well.

drivers/net/vxlan.c skip: · line 609

Quoted 9 lines
drivers/net/vxlan.c +3 -2
@@ -603,9 +603,10 @@ skip:
 
  /* Watch incoming packets to learn mapping between Ethernet address
   * and Tunnel endpoint.
+ * Return true if packet is bogus and should be droppped.
   */
-static void vxlan_snoop(struct net_device *dev,
-			__be32 src_ip, const u8 *src_mac)
+static bool vxlan_snoop(struct net_device *dev,
+		       __be32 src_ip, const u8 *src_mac)

Not clear why you changed the indentation of the second line...

WBR, Sergei

#3 lore raw

Re: [PATCH net v2 01/03] vxlan: only migrate dynamic FDB entries

Quoted 28 lines · On Tue, 11 Jun 2013 00:27:02 +0400 Sergei Shtylyov…

Hello.

Quoted 12 lines · Stephen Hemminger

Only migrate dynamic forwarding table entries, don't modify static entries. If packet received from incorrect source IP address assume it is an imposter and drop it.

---
Should go to -stable as well.

drivers/net/vxlan.c skip: · line 609

Quoted 9 lines
drivers/net/vxlan.c +3 -2
@@ -603,9 +603,10 @@ skip:
 
  /* Watch incoming packets to learn mapping between Ethernet address
   * and Tunnel endpoint.
+ * Return true if packet is bogus and should be droppped.
   */
-static void vxlan_snoop(struct net_device *dev,
-			__be32 src_ip, const u8 *src_mac)
+static bool vxlan_snoop(struct net_device *dev,
+		       __be32 src_ip, const u8 *src_mac)

Not clear why you changed the indentation of the second line...

Quoted 2 lines

WBR, Sergei

It was int, now bool, and second line didn't get reindented. Now dave will make resend the whole series...

maintainer
#4 lore raw

Re: [PATCH net v2 01/03] vxlan: only migrate dynamic FDB entries

From: Stephen Hemminger <[email protected]>
Date: Mon, 10 Jun 2013 13:32:01 -0700

Quoted 1 line

Now dave will make resend the whole series...

Attention to detail generally tends to show how much the submitter cares about the quality of their work :-)

#5 lore raw

Re: [PATCH net v2 01/03] vxlan: only migrate dynamic FDB entries

In reply to Stephen Hemminger

From: Stephen Hemminger <[email protected]>
Date: Mon, 10 Jun 2013 13:13:17 -0700

Quoted 8 lines

Only migrate dynamic forwarding table entries, don't modify static entries. If packet received from incorrect source IP address assume it is an imposter and drop it.

---
Should go to -stable as well.

Applied and queued up for -stable.

#6 lore raw

PATCH v2 2/3 vxlan: handle skb_clone failure

In reply to a message not in the archive

skb_clone can fail if out of memory. Just skip the fanout.

---
The multi-destination was added in 3.10, does not need to goto stable

drivers/net/vxlan.c +5 -3
@@ -1198,9 +1198,11 @@ static netdev_tx_t vxlan_xmit(struct sk_
 		struct sk_buff *skb1;
 
 		skb1 = skb_clone(skb, GFP_ATOMIC);
-		rc1 = vxlan_xmit_one(skb1, dev, rdst, did_rsc);
-		if (rc == NETDEV_TX_OK)
-			rc = rc1;
+		if (skb1) {
+			rc1 = vxlan_xmit_one(skb1, dev, rdst, did_rsc);
+			if (rc == NETDEV_TX_OK)
+				rc = rc1;
+		}
 	}
 
 	rc1 = vxlan_xmit_one(skb, dev, rdst0, did_rsc);
#8 lore raw

PATCH v2 3/3 vxlan: fix crash on module removal

In reply to a message not in the archive

If vxlan is removed with active vxlan's it would crash because rtnl_link_unregister (which calls vxlan_dellink), was invoked before unregister_pernet_device (which calls vxlan_stop).

---
Should goto stable

drivers/net/vxlan.c +2 -1
@@ -1785,8 +1785,9 @@ late_initcall(vxlan_init_module);
 
 static void __exit vxlan_cleanup_module(void)
 {
-	rtnl_link_unregister(&vxlan_link_ops);
 	unregister_pernet_device(&vxlan_net_ops);
+	rtnl_link_unregister(&vxlan_link_ops);
+	destroy_workqueue(vxlan_wq);
 	rcu_barrier();
 }
 module_exit(vxlan_cleanup_module);
#9 lore raw

Re: [PATCH net v2 03/03] [PATCH] vxlan: fix crash on module removal

From: Stephen Hemminger <[email protected]>
Date: Mon, 10 Jun 2013 13:13:15 -0700

Quoted 7 lines

If vxlan is removed with active vxlan's it would crash because rtnl_link_unregister (which calls vxlan_dellink), was invoked before unregister_pernet_device (which calls vxlan_stop).

---
Should goto stable

Applied and queued up for -stable.

Thanks.

maintainer
#10 lore raw

Re: [PATCH net v2 03/03] [PATCH] vxlan: fix crash on module removal

From: David Miller <[email protected]>
Date: Mon, 10 Jun 2013 14:01:14 -0700 (PDT)

Quoted 7 lines

From: Stephen Hemminger <[email protected]>
Date: Mon, 10 Jun 2013 13:13:15 -0700

Quoted 4 lines

If vxlan is removed with active vxlan's it would crash because rtnl_link_unregister (which calls vxlan_dellink), was invoked before unregister_pernet_device (which calls vxlan_stop).

Quoted 5 lines
Quoted 3 lines

---
Should goto stable

Applied and queued up for -stable.

Actually, this doesn't even compile.

+ destroy_workqueue(vxlan_wq);

vxlan_wq doesn't exist in the 'net' tree.

It doesn't exist in net-next either.

It gets added as part of your vxlan-next series which you posted at the same time.

I'm tossing both vxlan patch sets, you'll need to resubmit them properly.

I'm tossing both becuase I can tell the -next one won't apply properly on top of this series up when you fix it up.

maintainer
#11 lore raw

Re: [PATCH net v2 03/03] [PATCH] vxlan: fix crash on module removal

From: David Miller <[email protected]>
Date: Mon, 10 Jun 2013 14:14:21 -0700 (PDT)

Quoted 1 line

Actually, this doesn't even compile.

One last thing, no compile testing means no functional testing was done on these patches in the environment in which they were meant to be applied.

#12 lore raw

Re: [PATCH net v2 03/03] [PATCH] vxlan: fix crash on module removal

The issue was trying to split them into net and net-next series. If I sent one series, would you be able to handle that?

maintainer
#13 lore raw

Re: [PATCH net v2 03/03] [PATCH] vxlan: fix crash on module removal

From: Stephen Hemminger <[email protected]>
Date: Mon, 10 Jun 2013 14:48:14 -0700

Quoted 2 lines

The issue was trying to split them into net and net-next series. If I sent one series, would you be able to handle that?

Everyone who wants to do this, does it like this:

1) You give me the "net" series with the bug fixes.

2) You give me the "net-next" series, and you tell me "this series depends upon the bug fixes going into 'net' from series X, please merge that into 'net-next' before applying this series.

And if you're really testing this stuff at all, that's how you're developing and testing it too. You put together the net patch set, you _build_ it and you _test_ it. Then you make a branch on net-next, pull your 'net' stuff into it, and the build your 'net-next' series on that branch. Then you _build_ it and you _test_ it before even thinking about posting it here.