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

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHS8izPO2wSReuRz=k1PuXy8RAJuo5pujVMGceQVG7AvwMSVdw@mail.gmail.com>
Date: Sun, 9 Mar 2025 14:57:09 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: Stanislav Fomichev <stfomichev@...il.com>
Cc: Jakub Kicinski <kuba@...nel.org>, Stanislav Fomichev <sdf@...ichev.me>, netdev@...r.kernel.org, 
	davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com, 
	linux-kernel@...r.kernel.org, horms@...nel.org, donald.hunter@...il.com, 
	michael.chan@...adcom.com, pavan.chebbi@...adcom.com, andrew+netdev@...n.ch, 
	jdamato@...tly.com, xuanzhuo@...ux.alibaba.com, asml.silence@...il.com, 
	dw@...idwei.uk
Subject: Re: [PATCH net-next v1 3/4] net: add granular lock for the netdev
 netlink socket

On Fri, Mar 7, 2025 at 3:43 PM Stanislav Fomichev <stfomichev@...il.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