[PATCH net-next v1 1/2] netpoll: fix use after free

[PATCH net-next v1 1/2] netpoll: fix use after free

[David Decotigny]

After a bonding master reclaims the netpoll info struct, slaves could
still hold a pointer to the reclaimed data. This patch fixes it: as
soon as netpoll_async_cleanup is called for a slave (eg. when
un-enslaved), we make sure that this slave doesn't point to the data.

The style of this patch is not nice, but it represents the simplest
fix. The function is restructured by the next patch anyway ("netpoll:
avoid reference leaks").

Tested:
  One way to reproduce the panic is with netconsole: with the script
  below run in a loop without this patch. Same procedure with this
  patch can run without panic.
    CONFIG_NETCONSOLE=m
    CONFIG_NETCONSOLE_DYNAMIC=y
  Then run in a loop (dual port NIC makes it easier to crash):
    ifconfig eth0 192.168.42.4 up
    ifconfig eth1 192.168.56.4 up
    sleep 10
    modprobe -r netconsole
    modprobe -r bonding
    modprobe netconsole
    modprobe bonding mode=4
    echo +bond0 > /sys/class/net/bonding_masters
    ifconfig bond0 192.168.56.3 up
    mkdir /sys/kernel/config/netconsole/blah
    echo 0 > /sys/kernel/config/netconsole/blah/enabled
    echo bond0 > /sys/kernel/config/netconsole/blah/dev_name
    echo 192.168.56.42 > /sys/kernel/config/netconsole/blah/remote_ip
    echo 1 > /sys/kernel/config/netconsole/blah/enabled
    # npinfo refcnt ->1
    ifenslave bond0 eth1
    # npinfo refcnt ->2
    ifenslave bond0 eth0
    # (this should be optional, preventing ndo_cleanup_nepoll below)
    # npinfo refcnt ->3
    sleep 3
    ifenslave -d bond0 eth1
    # npinfo refcnt ->2, eth1 keeps ptr to npinfo reclaimed later => garbage
    sleep 1
    echo -bond0 > /sys/class/net/bonding_masters
    # netpoll_cleanup(bond0) + dec(refcnt)
    # (should be optional: becomes -> 1 (aka. != 0)
    #                      ==> do not call ndo_cleanup_nepoll)
    # try to increase chance of writing garbage onto npinfo:
    ls -lR / 2>&1 | tail -30
    echo +bond0 > /sys/class/net/bonding_masters
    ifconfig bond0 192.168.56.3 up
    ifenslave bond0 eth1
    # dev_open() called --> netpoll_rx_disable() + npinfo is garbage
    #    -> down(&ni->dev_lock);

Signed-off-by: David Decotigny <decot@googlers.com>
---
 net/core/netpoll.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index e33937f..907fb5e 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -822,7 +822,8 @@ void __netpoll_cleanup(struct netpoll *np)
 
 		RCU_INIT_POINTER(np->dev->npinfo, NULL);
 		call_rcu_bh(&npinfo->rcu, rcu_cleanup_netpoll_info);
-	}
+	} else
+		RCU_INIT_POINTER(np->dev->npinfo, NULL);
 }
 EXPORT_SYMBOL_GPL(__netpoll_cleanup);
 
-- 
2.0.0.526.g5318336

[PATCH net-next v1 2/2] netpoll: avoid reference leaks

[David Decotigny]

This ensures that the ndo_netpoll_cleanup callback is called for every
device that provides one. Otherwise there is a risk of reference leak
with bonding for example, which depends on this callback to cleanup
the slaves' references to netpoll info.

Tested:
  see patch "netpoll: fix use after free"

Signed-off-by: David Decotigny <decot@googlers.com>
---
 net/core/netpoll.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 907fb5e..1e10d5d 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -802,6 +802,7 @@ static void rcu_cleanup_netpoll_info(struct rcu_head *rcu_head)
 void __netpoll_cleanup(struct netpoll *np)
 {
 	struct netpoll_info *npinfo;
+	const struct net_device_ops *ops;
 
 	/* rtnl_dereference would be preferable here but
 	 * rcu_cleanup_netpoll path can put us in here safely without
@@ -813,17 +814,17 @@ void __netpoll_cleanup(struct netpoll *np)
 
 	synchronize_srcu(&netpoll_srcu);
 
-	if (atomic_dec_and_test(&npinfo->refcnt)) {
-		const struct net_device_ops *ops;
+	ops = np->dev->netdev_ops;
+	if (ops->ndo_netpoll_cleanup)
+		ops->ndo_netpoll_cleanup(np->dev);
 
-		ops = np->dev->netdev_ops;
-		if (ops->ndo_netpoll_cleanup)
-			ops->ndo_netpoll_cleanup(np->dev);
+	/* before dropping ref count, make sure this device does not
+	 * reference npinfo anymore
+	 */
+	RCU_INIT_POINTER(np->dev->npinfo, NULL);
 
-		RCU_INIT_POINTER(np->dev->npinfo, NULL);
+	if (atomic_dec_and_test(&npinfo->refcnt))
 		call_rcu_bh(&npinfo->rcu, rcu_cleanup_netpoll_info);
-	} else
-		RCU_INIT_POINTER(np->dev->npinfo, NULL);
 }
 EXPORT_SYMBOL_GPL(__netpoll_cleanup);
 
-- 
2.0.0.526.g5318336

Re: [PATCH net-next v1 2/2] netpoll: avoid reference leaks

[David Miller]

From: David Decotigny <decot@googlers.com>
Date: Mon, 30 Jun 2014 16:50:10 -0700

> This ensures that the ndo_netpoll_cleanup callback is called for every
> device that provides one. Otherwise there is a risk of reference leak
> with bonding for example, which depends on this callback to cleanup
> the slaves' references to netpoll info.
> 
> Tested:
>   see patch "netpoll: fix use after free"
> 
> Signed-off-by: David Decotigny <decot@googlers.com>

I definitely don't understand this.

Why would we call the cleanup function of an object before it's
reference count hits zero?  It is exactly the act of reaching a
zero refcount which should trigger invoking the cleanup callback.

If a refcount is being released in another location without checking
if it hits zero and invoking the cleanup if so, _THAT_ is the bug.

Re: [PATCH net-next v1 2/2] netpoll: avoid reference leaks

[David Decotigny]

Hi,

Thanks for the feedback. This patch results from manual inspection of
the code. I agree my commit description is abusive: in the case of
bonding, I think everything is fine, there should be no ref leak,
cleanup paths seem clean.

My point was to make things more predictable: ndo_netpoll_cleanup
called anyways to acknowledge actual loss of a ref to npinfo,
irrespective of whether it's the last ref or not. Without this patch,
calling ndo_netpoll_cleanup would depend on some timing behavior, hard
to predict, and users of the API have better be careful to reclaim the
refs manually anyways: as a consequence, not sure this callback is
actually required in its current inception.

On Mon, Jul 7, 2014 at 7:35 PM, David Miller <davem@davemloft.net> wrote:
> From: David Decotigny <decot@googlers.com>
> Date: Mon, 30 Jun 2014 16:50:10 -0700
>
>> This ensures that the ndo_netpoll_cleanup callback is called for every
>> device that provides one. Otherwise there is a risk of reference leak
>> with bonding for example, which depends on this callback to cleanup
>> the slaves' references to netpoll info.
>>
>> Tested:
>>   see patch "netpoll: fix use after free"
>>
>> Signed-off-by: David Decotigny <decot@googlers.com>
>
> I definitely don't understand this.
>
> Why would we call the cleanup function of an object before it's
> reference count hits zero?  It is exactly the act of reaching a
> zero refcount which should trigger invoking the cleanup callback.
>
> If a refcount is being released in another location without checking
> if it hits zero and invoking the cleanup if so, _THAT_ is the bug.

Re: [PATCH net-next v1 2/2] netpoll: avoid reference leaks

[David Miller]

From: David Decotigny <decot@googlers.com>
Date: Tue, 8 Jul 2014 12:35:14 -0700

> Thanks for the feedback. This patch results from manual inspection of
> the code. I agree my commit description is abusive: in the case of
> bonding, I think everything is fine, there should be no ref leak,
> cleanup paths seem clean.
> 
> My point was to make things more predictable: ndo_netpoll_cleanup
> called anyways to acknowledge actual loss of a ref to npinfo,
> irrespective of whether it's the last ref or not. Without this patch,
> calling ndo_netpoll_cleanup would depend on some timing behavior, hard
> to predict, and users of the API have better be careful to reclaim the
> refs manually anyways: as a consequence, not sure this callback is
> actually required in its current inception.

You've increased my confusion rather than decreased it.

You fail to address the core issue in my feedback:

	Whoever drops the refcount to zero must be the one to invoke
	the cleanup function.

Please address this concisely, and directly.

Re: [PATCH net-next v1 2/2] netpoll: avoid reference leaks

[David Decotigny]

In that case, that's what the original code does: dropping this patch 2/2.

Patch 1/2 "netpoll: fix use after free" is still needed to prevent
panics, though.

On Tue, Jul 8, 2014 at 2:17 PM, David Miller <davem@davemloft.net> wrote:
> From: David Decotigny <decot@googlers.com>
> Date: Tue, 8 Jul 2014 12:35:14 -0700
>
>> Thanks for the feedback. This patch results from manual inspection of
>> the code. I agree my commit description is abusive: in the case of
>> bonding, I think everything is fine, there should be no ref leak,
>> cleanup paths seem clean.
>>
>> My point was to make things more predictable: ndo_netpoll_cleanup
>> called anyways to acknowledge actual loss of a ref to npinfo,
>> irrespective of whether it's the last ref or not. Without this patch,
>> calling ndo_netpoll_cleanup would depend on some timing behavior, hard
>> to predict, and users of the API have better be careful to reclaim the
>> refs manually anyways: as a consequence, not sure this callback is
>> actually required in its current inception.
>
> You've increased my confusion rather than decreased it.
>
> You fail to address the core issue in my feedback:
>
>         Whoever drops the refcount to zero must be the one to invoke
>         the cleanup function.
>
> Please address this concisely, and directly.

Re: [PATCH net-next v1 2/2] netpoll: avoid reference leaks

[David Miller]

From: David Decotigny <decot@googlers.com>
Date: Tue, 8 Jul 2014 14:50:24 -0700

> In that case, that's what the original code does: dropping this patch 2/2.
> 
> Patch 1/2 "netpoll: fix use after free" is still needed to prevent
> panics, though.

Please resubmit it then.