netdev - Re: [PATCH net v2] net: devmem: fix kernel panic when socket close after module unload

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAHS8izPgzJUe8t73K6=pG2g--7MYD_M0Nn_ZJhEMCFSKELENRw@mail.gmail.com>
Date: Thu, 8 May 2025 13:45:55 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: Taehee Yoo <ap420073@...il.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com, 
	edumazet@...gle.com, andrew+netdev@...n.ch, horms@...nel.org, sdf@...ichev.me, 
	netdev@...r.kernel.org, asml.silence@...il.com, dw@...idwei.uk, 
	skhawaja@...gle.com, willemb@...gle.com, jdamato@...tly.com
Subject: Re: [PATCH net v2] net: devmem: fix kernel panic when socket close
 after module unload

On Thu, May 8, 2025 at 3:00 AM Taehee Yoo <ap420073@...il.com> wrote:
> >
> > Instead of this, if you need to protect concurrent access to
> > binding->dev, either:
> >
> > 1. create new spin_lock, binding->dev_lock, which protects access to
> > binding->dev, or
>
> Would a mutex be sufficient, or is a spinlock required?
>

AFAICT a mutex would be fine.

> > 2. Use rcu protection to lockelessly set binding->dev, or
> > 3. Use some cmpxchg logic to locklessly set/query binding->dev and
> > detect if it got modified in a racing thread.
> >
> > But please no multiplexing priv->lock to protect both priv->bindings
> > and binding->dev.
>
> Okay, I think introducing a new lock for protecting a binding would
> be enough. Not only protect binding->dev, all members of a binding.
> So, binding->lock would be more fit as a name.
>
> All members except a dev are not required to be protected so far.
> Because they are initialized when binding is initialized, and they are
> not changed in live.
> So, binding is alive, members are valid except for a dev.
> However, introducing binding->lock makes the code obvious, and it will
> not confuse us someday we want to protect other members of a binding.
>

Thanks, yes AFAICT we don't really need to synchronize access to any
other members of the binding in this patch, it's just binding->dev
that can be NULL'd while another thread is reading it. Maybe for now
we can have binding->lock protect binding->dev and in the future if
more sync it's needed, binding->lock can be re-used. A comment above
binding->lock can explain what it protects and doesn't.

> >
> > >                         break;
> > >                 }
> > >         }
> > > +       mutex_unlock(&binding->priv->lock);
> > >  }
> > >
> > >  static const struct memory_provider_ops dmabuf_devmem_ops = {
> > > diff --git a/net/core/devmem.h b/net/core/devmem.h
> > > index 7fc158d52729..afd6320b2c9b 100644
> > > --- a/net/core/devmem.h
> > > +++ b/net/core/devmem.h
> > > @@ -11,6 +11,7 @@
> > >  #define _NET_DEVMEM_H
> > >
> > >  #include <net/netmem.h>
> > > +#include <net/netdev_netlink.h>
> > >
> > >  struct netlink_ext_ack;
> > >
> > > @@ -20,6 +21,7 @@ struct net_devmem_dmabuf_binding {
> > >         struct sg_table *sgt;
> > >         struct net_device *dev;
> > >         struct gen_pool *chunk_pool;
> > > +       struct netdev_nl_sock *priv;
> > >
> > >         /* The user holds a ref (via the netlink API) for as long as they want
> > >          * the binding to remain alive. Each page pool using this binding holds
> > > @@ -63,6 +65,7 @@ struct dmabuf_genpool_chunk_owner {
> > >  void __net_devmem_dmabuf_binding_free(struct net_devmem_dmabuf_binding *binding);
> > >  struct net_devmem_dmabuf_binding *
> > >  net_devmem_bind_dmabuf(struct net_device *dev, unsigned int dmabuf_fd,
> > > +                      struct netdev_nl_sock *priv,
> > >                        struct netlink_ext_ack *extack);
> > >  void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding);
> > >  int net_devmem_bind_dmabuf_to_queue(struct net_device *dev, u32 rxq_idx,
> > > diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> > > index 230743bdbb14..b8bb73574276 100644
> > > --- a/net/core/netdev-genl.c
> > > +++ b/net/core/netdev-genl.c
> > > @@ -859,13 +859,11 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
> > >                 goto err_genlmsg_free;
> > >         }
> > >
> > > -       mutex_lock(&priv->lock);
> > > -
> > >         err = 0;
> > >         netdev = netdev_get_by_index_lock(genl_info_net(info), ifindex);
> > >         if (!netdev) {
> > >                 err = -ENODEV;
> > > -               goto err_unlock_sock;
> > > +               goto err_genlmsg_free;
> > >         }
> > >         if (!netif_device_present(netdev))
> > >                 err = -ENODEV;
> > > @@ -877,10 +875,11 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
> > >                 goto err_unlock;
> > >         }
> > >
> > > -       binding = net_devmem_bind_dmabuf(netdev, dmabuf_fd, info->extack);
> > > +       mutex_lock(&priv->lock);
> > > +       binding = net_devmem_bind_dmabuf(netdev, dmabuf_fd, priv, info->extack);
> >
> > I'm not so sure about this as well. priv->lock should protect access
> > to priv->bindings only. I'm not sure why it's locked before
> > net_devmem_bind_dmabuf? Can it be locked around the access to
> > priv->bindings only?
>
> As you mentioned, this lock is unnecessary to protect
> netdev_devmem_bind_dmabuf().
> It is required to protect only priv->bindings.
> So,  I will move priv->lock to lock only around priv->bindings
> in the next patch.
>

Thanks!

-- 
Thanks,
Mina