| Message ID | 20210322182145.531377-1-eric.dumazet@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | add2d73631070c951b0de81a01d1463a15cfbd47 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: set initial device refcount to 1 | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 7 maintainers not CCed: daniel@iogearbox.net cong.wang@bytedance.com andriin@fb.com ast@kernel.org ap420073@gmail.com atenart@kernel.org weiwan@google.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 6777 this patch: 6777 |
| netdev/kdoc | success | Errors and warnings before: 1 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON() WARNING: line length of 81 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 6991 this patch: 6991 |
| netdev/header_inline | success | Link |
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 >
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.
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.
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
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))