Re: [PATCH net-next v1 3/4] net: add granular lock for the netdev netlink socket

Mina Almasry
2025-03-09T21:57:24Z

On Fri, Mar 7, 2025 at 3:43 PM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> On 03/07, Jakub Kicinski wrote:
> > On Fri, 7 Mar 2025 07:57:24 -0800 Stanislav Fomichev wrote:
> > > diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> > > index a219be90c739..8acdeeae24e7 100644
> > > --- a/net/core/netdev-genl.c
> > > +++ b/net/core/netdev-genl.c
> > > @@ -859,6 +859,7 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
> > > goto err_genlmsg_free;
> > > }
> > >
> > > + mutex_lock(&priv->lock);
> > > rtnl_lock();
> > >
> > > netdev = __dev_get_by_index(genl_info_net(info), ifindex);
> > > @@ -925,6 +926,7 @@ int netdev_nl_bind_rx_doit(struct sk_buff *skb, struct genl_info *info)
> > > net_devmem_unbind_dmabuf(binding);
> > > err_unlock:
> > > rtnl_unlock();
> > > + mutex_unlock(&priv->lock);
> > > err_genlmsg_free:
> > > nlmsg_free(rsp);
> > > return err;
> >
> > I think you're missing an unlock before successful return here no?
>
> Yes, thanks! :-( I have tested some of this code with Mina's latest TX + my
> loopback mode, but it doesn't have any RX tests.. Will try to hack
> something together to run RX bind before I repost.

Is the existing RX test not working for you?

Also running `./ncdevmem` manually on a driver you have that supports
devmem will test the binding patch.

I wonder if we can change list_head to xarray, which manages its own
locking, instead of list_head plus manual locking. Just an idea, I
don't have a strong preference here. It may be annoying that xarray do
lookups by an index, so we have to store the index somewhere. But if
all we do here is add to the xarray and later loop over it to unbind
elements, we don't need to store the indexes anywhere.

--
Thanks,
Mina