macvlan: Fix device ref leak when purging bc_queue

Submitted by Herbert Xu on April 20, 2017, 12:55 p.m.

Details

Message ID 20170420125512.GA9113@gondor.apana.org.au
State Accepted
Delegated to: David Miller
Headers show

Commit Message

Herbert Xu April 20, 2017, 12:55 p.m.
When a parent macvlan device is destroyed we end up purging its
broadcast queue without dropping the device reference count on
the packet source device.  This causes the source device to linger.

This patch drops that reference count.

Fixes: 260916dfb48c ("macvlan: Fix potential use-after free for...")
Reported-by: Joe Ghalam <Joe.Ghalam@dell.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Comments

David Miller April 25, 2017, 2:42 p.m.
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu, 20 Apr 2017 20:55:12 +0800

> When a parent macvlan device is destroyed we end up purging its
> broadcast queue without dropping the device reference count on
> the packet source device.  This causes the source device to linger.
> 
> This patch drops that reference count.
> 
> Fixes: 260916dfb48c ("macvlan: Fix potential use-after free for...")
> Reported-by: Joe Ghalam <Joe.Ghalam@dell.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied and queued up for -stable, thanks Herbert.
Joe.Ghalam@dell.com April 25, 2017, 3:19 p.m.
From: David Miller <davem@davemloft.net>
Sent: Tuesday, April 25, 2017 7:42 AM
To: herbert@gondor.apana.org.au
Cc: Ghalam, Joe; Wichmann, Clifford; netdev@vger.kernel.org
Subject: Re: macvlan: Fix device ref leak when purging bc_queue

> Applied and queued up for -stable, thanks Herbert.

Herbert and David,
Glad to report that we had 10 iterations of the test that was showing 100% failure with Herbert's changes, and we have not seen any failures. The fix is good.
Thanks for the help.
Joe

Patch hide | download patch | download mbox

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 9261722..b34eaaa 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -1139,6 +1139,7 @@  static int macvlan_port_create(struct net_device *dev)
 static void macvlan_port_destroy(struct net_device *dev)
 {
 	struct macvlan_port *port = macvlan_port_get_rtnl(dev);
+	struct sk_buff *skb;
 
 	dev->priv_flags &= ~IFF_MACVLAN_PORT;
 	netdev_rx_handler_unregister(dev);
@@ -1147,7 +1148,15 @@  static void macvlan_port_destroy(struct net_device *dev)
 	 * but we need to cancel it and purge left skbs if any.
 	 */
 	cancel_work_sync(&port->bc_work);
-	__skb_queue_purge(&port->bc_queue);
+
+	while ((skb = __skb_dequeue(&port->bc_queue))) {
+		const struct macvlan_dev *src = MACVLAN_SKB_CB(skb)->src;
+
+		if (src)
+			dev_put(src->dev);
+
+		kfree_skb(skb);
+	}
 
 	kfree(port);
 }