[PATCH net-next] net: set initial device refcount to 1

[PATCH net-next] net: set initial device refcount to 1

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

When adding CONFIG_PCPU_DEV_REFCNT, I forgot that the
initial net device refcount was 0.

When CONFIG_PCPU_DEV_REFCNT is not set, this means
the first dev_hold() triggers an illegal refcount
operation (addition on 0)

refcount_t: addition on 0; use-after-free.
WARNING: CPU: 0 PID: 1 at lib/refcount.c:25 refcount_warn_saturate+0x128/0x1a4

Fix is to change initial (and final) refcount to be 1.

Also add a missing kerneldoc piece, as reported by
Stephen Rothwell.

Fixes: 919067cc845f ("net: add CONFIG_PCPU_DEV_REFCNT")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Guenter Roeck <groeck@google.com>
---
 include/linux/netdevice.h | 1 +
 net/core/dev.c            | 9 ++++++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 8f003955c485b81210ed56f7e1c24080b4bb46eb..b11c2c1890b2a28ba2d02fc4466380703a12efaf 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1792,6 +1792,7 @@ enum netdev_ml_priv_type {
  *
  *	@proto_down_reason:	reason a netdev interface is held down
  *	@pcpu_refcnt:		Number of references to this device
+ *	@dev_refcnt:		Number of references to this device
  *	@todo_list:		Delayed register/unregister
  *	@link_watch_list:	XXX: need comments on this one
  *
diff --git a/net/core/dev.c b/net/core/dev.c
index be941ed754ac71d0839604bcfdd8ab67c339d27f..95c78279d900796c8a3ed0df59b168d5c5e0e309 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -10348,7 +10348,7 @@ static void netdev_wait_allrefs(struct net_device *dev)
 	rebroadcast_time = warning_time = jiffies;
 	refcnt = netdev_refcnt_read(dev);
 
-	while (refcnt != 0) {
+	while (refcnt != 1) {
 		if (time_after(jiffies, rebroadcast_time + 1 * HZ)) {
 			rtnl_lock();
 
@@ -10385,7 +10385,7 @@ static void netdev_wait_allrefs(struct net_device *dev)
 
 		refcnt = netdev_refcnt_read(dev);
 
-		if (refcnt && time_after(jiffies, warning_time + 10 * HZ)) {
+		if (refcnt != 1 && time_after(jiffies, warning_time + 10 * HZ)) {
 			pr_emerg("unregister_netdevice: waiting for %s to become free. Usage count = %d\n",
 				 dev->name, refcnt);
 			warning_time = jiffies;
@@ -10461,7 +10461,7 @@ void netdev_run_todo(void)
 		netdev_wait_allrefs(dev);
 
 		/* paranoia */
-		BUG_ON(netdev_refcnt_read(dev));
+		BUG_ON(netdev_refcnt_read(dev) != 1);
 		BUG_ON(!list_empty(&dev->ptype_all));
 		BUG_ON(!list_empty(&dev->ptype_specific));
 		WARN_ON(rcu_access_pointer(dev->ip_ptr));
@@ -10682,6 +10682,9 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 	dev->pcpu_refcnt = alloc_percpu(int);
 	if (!dev->pcpu_refcnt)
 		goto free_dev;
+	dev_hold(dev);
+#else
+	refcount_set(&dev->dev_refcnt, 1);
 #endif
 
 	if (dev_addr_init(dev))
-- 
2.31.0.291.g576ba9dcdaf-goog

Re: [PATCH net-next] net: set initial device refcount to 1

[Guenter Roeck]

On Mon, Mar 22, 2021 at 11:21 AM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> From: Eric Dumazet <edumazet@google.com>
>
> When adding CONFIG_PCPU_DEV_REFCNT, I forgot that the
> initial net device refcount was 0.
>
> When CONFIG_PCPU_DEV_REFCNT is not set, this means
> the first dev_hold() triggers an illegal refcount
> operation (addition on 0)
>
> refcount_t: addition on 0; use-after-free.
> WARNING: CPU: 0 PID: 1 at lib/refcount.c:25 refcount_warn_saturate+0x128/0x1a4
>
> Fix is to change initial (and final) refcount to be 1.
>
> Also add a missing kerneldoc piece, as reported by
> Stephen Rothwell.
>
> Fixes: 919067cc845f ("net: add CONFIG_PCPU_DEV_REFCNT")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Guenter Roeck <groeck@google.com>

The problems observed with next-20210322 are gone with this patch applied.

Tested-by: Guenter Roeck <groeck@google.com>

Thanks,
Guenter

> ---
>  include/linux/netdevice.h | 1 +
>  net/core/dev.c            | 9 ++++++---
>  2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 8f003955c485b81210ed56f7e1c24080b4bb46eb..b11c2c1890b2a28ba2d02fc4466380703a12efaf 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1792,6 +1792,7 @@ enum netdev_ml_priv_type {
>   *
>   *     @proto_down_reason:     reason a netdev interface is held down
>   *     @pcpu_refcnt:           Number of references to this device
> + *     @dev_refcnt:            Number of references to this device
>   *     @todo_list:             Delayed register/unregister
>   *     @link_watch_list:       XXX: need comments on this one
>   *
> diff --git a/net/core/dev.c b/net/core/dev.c
> index be941ed754ac71d0839604bcfdd8ab67c339d27f..95c78279d900796c8a3ed0df59b168d5c5e0e309 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -10348,7 +10348,7 @@ static void netdev_wait_allrefs(struct net_device *dev)
>         rebroadcast_time = warning_time = jiffies;
>         refcnt = netdev_refcnt_read(dev);
>
> -       while (refcnt != 0) {
> +       while (refcnt != 1) {
>                 if (time_after(jiffies, rebroadcast_time + 1 * HZ)) {
>                         rtnl_lock();
>
> @@ -10385,7 +10385,7 @@ static void netdev_wait_allrefs(struct net_device *dev)
>
>                 refcnt = netdev_refcnt_read(dev);
>
> -               if (refcnt && time_after(jiffies, warning_time + 10 * HZ)) {
> +               if (refcnt != 1 && time_after(jiffies, warning_time + 10 * HZ)) {
>                         pr_emerg("unregister_netdevice: waiting for %s to become free. Usage count = %d\n",
>                                  dev->name, refcnt);
>                         warning_time = jiffies;
> @@ -10461,7 +10461,7 @@ void netdev_run_todo(void)
>                 netdev_wait_allrefs(dev);
>
>                 /* paranoia */
> -               BUG_ON(netdev_refcnt_read(dev));
> +               BUG_ON(netdev_refcnt_read(dev) != 1);
>                 BUG_ON(!list_empty(&dev->ptype_all));
>                 BUG_ON(!list_empty(&dev->ptype_specific));
>                 WARN_ON(rcu_access_pointer(dev->ip_ptr));
> @@ -10682,6 +10682,9 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
>         dev->pcpu_refcnt = alloc_percpu(int);
>         if (!dev->pcpu_refcnt)
>                 goto free_dev;
> +       dev_hold(dev);
> +#else
> +       refcount_set(&dev->dev_refcnt, 1);
>  #endif
>
>         if (dev_addr_init(dev))
> --
> 2.31.0.291.g576ba9dcdaf-goog
>

Re: [PATCH net-next] net: set initial device refcount to 1

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 22 Mar 2021 11:21:45 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> When adding CONFIG_PCPU_DEV_REFCNT, I forgot that the
> initial net device refcount was 0.
> 
> When CONFIG_PCPU_DEV_REFCNT is not set, this means
> the first dev_hold() triggers an illegal refcount
> operation (addition on 0)
> 
> refcount_t: addition on 0; use-after-free.
> WARNING: CPU: 0 PID: 1 at lib/refcount.c:25 refcount_warn_saturate+0x128/0x1a4
> 
> Fix is to change initial (and final) refcount to be 1.
> 
> Also add a missing kerneldoc piece, as reported by
> Stephen Rothwell.
> 
> Fixes: 919067cc845f ("net: add CONFIG_PCPU_DEV_REFCNT")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Guenter Roeck <groeck@google.com>

This hunk:
> @@ -10682,6 +10682,9 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
>  	dev->pcpu_refcnt = alloc_percpu(int);
>  	if (!dev->pcpu_refcnt)
>  		goto free_dev;
> +	dev_hold(dev);
> +#else
> +	refcount_set(&dev->dev_refcnt, 1);
>  #endif
>  
>  	if (dev_addr_init(dev))
 gets rejects in net-next.  Please respin.
 

Re: [PATCH net-next] net: set initial device refcount to 1

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Mon, 22 Mar 2021 16:55:26 -0700 (PDT)

> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 22 Mar 2021 11:21:45 -0700
> 
> This hunk:
>> @@ -10682,6 +10682,9 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
>>  	dev->pcpu_refcnt = alloc_percpu(int);
>>  	if (!dev->pcpu_refcnt)
>>  		goto free_dev;
>> +	dev_hold(dev);
>> +#else
>> +	refcount_set(&dev->dev_refcnt, 1);
>>  #endif
>>  
>>  	if (dev_addr_init(dev))
>  gets rejects in net-next.  Please respin.
>  

My bad, I was trying to apply it to 'net' mistakedly.

All good now, thanks.

Re: [PATCH net-next] net: set initial device refcount to 1

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

On Mon, 22 Mar 2021 11:21:45 -0700 you wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> When adding CONFIG_PCPU_DEV_REFCNT, I forgot that the
> initial net device refcount was 0.
> 
> When CONFIG_PCPU_DEV_REFCNT is not set, this means
> the first dev_hold() triggers an illegal refcount
> operation (addition on 0)
> 
> [...]

Here is the summary with links:
  - [net-next] net: set initial device refcount to 1
    https://git.kernel.org/netdev/net-next/c/add2d7363107

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html