macvlan: Fix device ref leak when purging bc_queue

macvlan: Fix device ref leak when purging bc_queue

[Herbert Xu]

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>

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);
 }
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: macvlan: Fix device ref leak when purging bc_queue

[Joe.Ghalam]

Hi Herbert,
I agree with this change, but the same purge would be needed for the macvlan_dellink() call also. 
Thanks,
Joe

________________________________________
From: Herbert Xu <herbert@gondor.apana.org.au>
Sent: Thursday, April 20, 2017 5:55 AM
To: Ghalam, Joe
Cc: davem@davemloft.net; Wichmann, Clifford; netdev@vger.kernel.org
Subject: macvlan: Fix device ref leak when purging bc_queue

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>

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);
 }
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: macvlan: Fix device ref leak when purging bc_queue

[Herbert Xu]

On Thu, Apr 20, 2017 at 04:09:56PM +0000, Joe.Ghalam@dell.com wrote:
> I agree with this change, but the same purge would be needed for the macvlan_dellink() call also. 

I don't think that's necessary because as long as the master
device is still around it will continue to process the broadcast
queue, thus removing any reference counts held.

It's only when the queue is purged that we run into trouble.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: macvlan: Fix device ref leak when purging bc_queue

[Joe.Ghalam]

That's not true. macvlan_dellink() unregisters the queue, and macvlan_process_broadcast() will never get called. Please note that I'm not speculating. I have traced enabled on the dev_put and dev_hold, and I'm reporting a real, reproducible issue.
Her is a sequence of calls logged, when the issue happens. macvlan_process_broadcast() never happens.

Apr 19 04:35:39 OS10 kernel: e101-001-0.v257: dev_put 16 dst_destroy
Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_hold 15 dev_get_by_index
Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 16 do_ip_setsockopt
Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_hold 15 dst_alloc
Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 16 dst_destroy
Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_hold 15 macvlan_broadcast_enqueue
Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 16 macvlan_process_broadcast
Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_hold 15 macvlan_broadcast_enqueue
Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 16 macvlan_process_broadcast
Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_hold 15 macvlan_broadcast_enqueue    <---insert
Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 16 neigh_destroy
Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 15 neigh_destroy
Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 14 neigh_destroy
Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 13 dst_destroy
Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 12 dst_destroy
Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 11 __netdev_adjacent_dev_remove <--- macvlan_dellink()
Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 10 __netdev_adjacent_dev_remove <--- macvlan_dellink()
Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 9 neigh_parms_release
Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 8 neigh_parms_release
Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 7 in6_dev_finish_destroy
Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 6 rx_queue_release
Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 5 netdev_queue_release
Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 4 rollback_registered_many
Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 3 free_fib_info_rcu

________________________________________
From: Herbert Xu <herbert@gondor.apana.org.au>
Sent: Thursday, April 20, 2017 9:40 PM
To: Ghalam, Joe
Cc: davem@davemloft.net; Wichmann, Clifford; netdev@vger.kernel.org
Subject: Re: macvlan: Fix device ref leak when purging bc_queue

On Thu, Apr 20, 2017 at 04:09:56PM +0000, Joe.Ghalam@dell.com wrote:
> I agree with this change, but the same purge would be needed for the macvlan_dellink() call also.

I don't think that's necessary because as long as the master
device is still around it will continue to process the broadcast
queue, thus removing any reference counts held.

It's only when the queue is purged that we run into trouble.

Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: macvlan: Fix device ref leak when purging bc_queue

[David Miller]

Please DO NOT top-post.

Thank you.

Re: macvlan: Fix device ref leak when purging bc_queue

[Mahesh Bandewar (महेश बंडेवार)]

On Fri, Apr 21, 2017 at 7:40 AM,  <Joe.Ghalam@dell.com> wrote:
> That's not true. macvlan_dellink() unregisters the queue, and macvlan_process_broadcast() will never get called. Please note that I'm not speculating. I have traced enabled on the dev_put and dev_hold, and I'm reporting a real, reproducible issue.
> Her is a sequence of calls logged, when the issue happens. macvlan_process_broadcast() never happens.
>
> Apr 19 04:35:39 OS10 kernel: e101-001-0.v257: dev_put 16 dst_destroy
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_hold 15 dev_get_by_index
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 16 do_ip_setsockopt
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_hold 15 dst_alloc
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 16 dst_destroy
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_hold 15 macvlan_broadcast_enqueue
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 16 macvlan_process_broadcast
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_hold 15 macvlan_broadcast_enqueue
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 16 macvlan_process_broadcast
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_hold 15 macvlan_broadcast_enqueue    <---insert
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 16 neigh_destroy
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 15 neigh_destroy
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 14 neigh_destroy
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 13 dst_destroy
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 12 dst_destroy
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 11 __netdev_adjacent_dev_remove <--- macvlan_dellink()
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 10 __netdev_adjacent_dev_remove <--- macvlan_dellink()
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 9 neigh_parms_release
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 8 neigh_parms_release
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 7 in6_dev_finish_destroy
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 6 rx_queue_release
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 5 netdev_queue_release
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 4 rollback_registered_many
> Apr 19 04:35:41 OS10 kernel: e101-001-0.v257: dev_put 3 free_fib_info_rcu
>
May be the system is busy and snapshot is too small, and eventually
process_broadcast() should get called. Deleting a slave does nothing
about cancelling the work-queue so it would happen eventually.

The change that Herbert proposed is correct. When packets are enqueued
for processing later a dev reference is taken and it's removed when
it's processed when it gets scheduled. The backlog is per port so it
makes sense to remove reference(s) before purging the queue prior to
deleting the port.

> ________________________________________
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Sent: Thursday, April 20, 2017 9:40 PM
> To: Ghalam, Joe
> Cc: davem@davemloft.net; Wichmann, Clifford; netdev@vger.kernel.org
> Subject: Re: macvlan: Fix device ref leak when purging bc_queue
>
> On Thu, Apr 20, 2017 at 04:09:56PM +0000, Joe.Ghalam@dell.com wrote:
>> I agree with this change, but the same purge would be needed for the macvlan_dellink() call also.
>
> I don't think that's necessary because as long as the master
> device is still around it will continue to process the broadcast
> queue, thus removing any reference counts held.
>
> It's only when the queue is purged that we run into trouble.
>
> Cheers,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: macvlan: Fix device ref leak when purging bc_queue

[Joe.Ghalam]

________________________________________
> From: Mahesh Bandewar (महेश बंडेवार) <maheshb@google.com>
> Sent: Friday, April 21, 2017 12:23 PM
> To: Ghalam, Joe
> Cc: herbert@gondor.apana.org.au; David Miller; Wichmann, Clifford; linux-netdev
> Subject: Re: macvlan: Fix device ref leak when purging bc_queue

> May be the system is busy and snapshot is too small, and eventually
> process_broadcast() should get called. Deleting a slave does nothing
> about cancelling the work-queue so it would happen eventually.

> The change that Herbert proposed is correct. When packets are enqueued
> for processing later a dev reference is taken and it's removed when
> it's processed when it gets scheduled. The backlog is per port so it
> makes sense to remove reference(s) before purging the queue prior to
> deleting the port.

I only included the snapshot of the logs that's relevant. The system in question has been left in that state for hours, without ever seeing process_broadcast() being called. And, yes I did check the cpu load, and the system was running at around 20% load. So, I don't think that's the case. I would suggest to take closer look at the code in mtacvlan_dellink(), where it performs unlink and unregister:

void macvlan_dellink(struct net_device *dev, struct list_head *head)
{
	struct macvlan_dev *vlan = netdev_priv(dev);
	list_del_rcu(&vlan->list);
	unregister_netdevice_queue(dev, head);
	netdev_upper_dev_unlink(vlan->lowerdev, dev);
}

As I stated in my reply to Herbert initially, the code change he suggested is correct and needed, but not enough. We have tested with his code change and observed the same behavior. I can guarantee you that the code change to macvlan_port_destroy() has no effect on this issue, since the function macvlan_port_destroy () is not even called during the operation. 

Here is the forced stack trace that I caused to show the removal call:
Apr 20 06:23:40 OS10 kernel:  [<ffffffff810d312c>] __netdev_adjacent_dev_remove+0x3c/0x1a0
Apr 20 06:23:40 OS10 kernel:  [<ffffffff81bb6e87>] __netdev_adjacent_dev_unlink_lists+0x67/0x69
Apr 20 06:23:40 OS10 kernel:  [<ffffffff810d32a0>] __netdev_adjacent_dev_unlink+0x82/0x40
Apr 20 06:23:40 OS10 kernel:  [<ffffffff811d31e0>] netdev_upper_dev_unlink+0x10/0x20
Apr 20 06:23:40 OS10 kernel:  [<ffffffff8180e770>] macvlan_dellink+0x50/0x130
Apr 20 06:23:40 OS10 kernel:  [<ffffffff81a2ca27>] rtnl_dellink+0xb7/0x120
Apr 20 06:23:40 OS10 kernel:  [<ffffffff81a609ab>] ? __netlink_ns_capable+0x3b/0x40
Apr 20 06:23:40 OS10 kernel:  [<ffffffff81a2a6c5>] rtnetlink_rcv_msg+0x95/0x250
Apr 20 06:23:40 OS10 kernel:  [<ffffffff811c1499>] ? zone_statistics+0x89/0xa0
Apr 20 06:23:40 OS10 kernel:  [<ffffffff81a0a9de>] ? __alloc_skb+0x7e/0x2a0
Apr 20 06:23:40 OS10 kernel:  [<ffffffff81a2a630>] ? rtnetlink_rcv+0x30/0x30
Apr 20 06:23:40 OS10 kernel:  [<ffffffff81a64f59>] netlink_rcv_skb+0xa9/0xc0
Apr 20 06:23:40 OS10 kernel:  [<ffffffff81a2a628>] rtnetlink_rcv+0x28/0x30
Apr 20 06:23:40 OS10 kernel:  [<ffffffff81a64603>] netlink_unicast+0xf3/0x200
Apr 20 06:23:40 OS10 kernel:  [<ffffffff81a64a1e>] netlink_sendmsg+0x30e/0x680
Apr 20 06:23:40 OS10 kernel:  [<ffffffff81a014fb>] sock_sendmsg+0x8b/0xc0
Apr 20 06:23:40 OS10 kernel:  [<ffffffff81a011ee>] ? move_addr_to_kernel.part.18+0x1e/0x60
Apr 20 06:23:40 OS10 kernel:  [<ffffffff81a01ff1>] ? move_addr_to_kernel+0x21/0x30
Apr 20 06:23:40 OS10 kernel:  [<ffffffff81a018f6>] ___sys_sendmsg+0x376/0x390
Apr 20 06:23:40 OS10 kernel:  [<ffffffff81a0019f>] ? sock_destroy_inode+0x2f/0x40
Apr 20 06:23:40 OS10 kernel:  [<ffffffff810a161c>] ? __do_page_fault+0x20c/0x560
Apr 20 06:23:40 OS10 kernel:  [<ffffffff812279ad>] ? dput+0xad/0x180
Apr 20 06:23:40 OS10 kernel:  [<ffffffff81230a74>] ? mntput+0x24/0x40
Apr 20 06:23:40 OS10 kernel:  [<ffffffff81212a50>] ? __fput+0x190/0x220
Apr 20 06:23:40 OS10 kernel:  [<ffffffff81a026b2>] __sys_sendmsg+0x42/0x80
Apr 20 06:23:40 OS10 kernel:  [<ffffffff81a02702>] SyS_sendmsg+0x12/0x20
Apr 20 06:23:40 OS10 kernel:  [<ffffffff81bc86cd>] system_call_fast_compare_end+0x10/0x15

Re: macvlan: Fix device ref leak when purging bc_queue

[Herbert Xu]

On Fri, Apr 21, 2017 at 02:40:50PM +0000, Joe.Ghalam@dell.com wrote:
> That's not true. macvlan_dellink() unregisters the queue, and macvlan_process_broadcast() will never get called. Please note that I'm not speculating. I have traced enabled on the dev_put and dev_hold, and I'm reporting a real, reproducible issue.

The only thing that can stop macvlan_process_broadcast from getting
called is macvlan_port_destroy.  Nothing else can stop the work
queue, unless of course the work queue mechanism itself is broken.

So if you're sure macvlan_port_destroy is never even called in
your case, then you'll need to start debugging the kernel work
queue mechanism to see why macvlan_process_broadcast is not getting
called.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: macvlan: Fix device ref leak when purging bc_queue

[Joe.Ghalam]

> The only thing that can stop macvlan_process_broadcast from getting
> called is macvlan_port_destroy.  Nothing else can stop the work
> queue, unless of course the work queue mechanism itself is broken.

> So if you're sure macvlan_port_destroy is never even called in
> your case, then you'll need to start debugging the kernel work
> queue mechanism to see why macvlan_process_broadcast is not getting
> called.

I will get your changes reloaded and re-tested without any other debug tools. Hopefully, we'll see success. I will let you know if I see any issues. 
Btw, is your fix committed already? if not, do you know when and where it would be committed?

Thanks,
Joe
 

Re: macvlan: Fix device ref leak when purging bc_queue

[David Miller]

From: <Joe.Ghalam@dell.com>
Date: Mon, 24 Apr 2017 15:01:24 +0000

>> The only thing that can stop macvlan_process_broadcast from getting
>> called is macvlan_port_destroy.  Nothing else can stop the work
>> queue, unless of course the work queue mechanism itself is broken.
> 
>> So if you're sure macvlan_port_destroy is never even called in
>> your case, then you'll need to start debugging the kernel work
>> queue mechanism to see why macvlan_process_broadcast is not getting
>> called.
> 
> I will get your changes reloaded and re-tested without any other debug tools. Hopefully, we'll see success. I will let you know if I see any issues. 
> Btw, is your fix committed already? if not, do you know when and where it would be committed?

I'm waiting for this discussion to settle down before I apply the
patch.

Re: macvlan: Fix device ref leak when purging bc_queue

[Joe.Ghalam]

> I'm waiting for this discussion to settle down before I apply the patch.

Thanks David. I will get some answers soon, and hopefully the change is a good one.

Re: macvlan: Fix device ref leak when purging bc_queue

[David Miller]

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.

Re: macvlan: Fix device ref leak when purging bc_queue

[Joe.Ghalam]

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