Messages in this thread | | | From | Eric Dumazet <> | Date | Thu, 27 Jan 2022 18:09:13 -0800 | Subject | Re: [PATCH net] net: dev: Detect dev_hold() after netdev_wait_allrefs() |
| |
On Thu, Jan 27, 2022 at 5:43 PM Jann Horn <jannh@google.com> wrote: > > I've run into a bug where dev_hold() was being called after > netdev_wait_allrefs(). But at that point, the device is already going > away, and dev_hold() can't stop that anymore. > > To make such problems easier to diagnose in the future: > > - For CONFIG_PCPU_DEV_REFCNT builds: Recheck in free_netdev() whether > the net refcount has been elevated. If this is detected, WARN() and > leak the object (to prevent worse consequences from a > use-after-free). > - For builds without CONFIG_PCPU_DEV_REFCNT: Set the refcount to zero. > This signals to the generic refcount infrastructure that any attempt > to increment the refcount later is a bug. > > Signed-off-by: Jann Horn <jannh@google.com> > --- > net/core/dev.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/net/core/dev.c b/net/core/dev.c > index 1baab07820f6..f7916c0d226d 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -9949,8 +9949,18 @@ void netdev_run_todo(void) > > netdev_wait_allrefs(dev); > > + /* Drop the netdev refcount (which should be 1 at this point) > + * to zero. If we're using the generic refcount code, this will > + * tell it that any dev_hold() after this point is a bug. > + */ > +#ifdef CONFIG_PCPU_DEV_REFCNT > + this_cpu_dec(*dev->pcpu_refcnt); > + BUG_ON(netdev_refcnt_read(dev) != 0); > +#else > + BUG_ON(!refcount_dec_and_test(&dev->dev_refcnt)); > +#endif > + > /* paranoia */ > - 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)); > @@ -10293,6 +10303,12 @@ void free_netdev(struct net_device *dev) > free_percpu(dev->xdp_bulkq); > dev->xdp_bulkq = NULL; > > + /* Recheck in case someone called dev_hold() between > + * netdev_wait_allrefs() and here. > + */
At this point, dev->pcpu_refcnt per-cpu data has been freed already (CONFIG_PCPU_DEV_REFCNT=y)
So this should probably crash, or at least UAF ?
> + if (WARN_ON(netdev_refcnt_read(dev) != 0)) > + return; /* leak memory, otherwise we might get UAF */ > + > /* Compatibility with error handling in drivers */ > if (dev->reg_state == NETREG_UNINITIALIZED) { > netdev_freemem(dev); > > base-commit: 23a46422c56144939c091c76cf389aa863ce9c18 > -- > 2.35.0.rc0.227.g00780c9af4-goog >
| |