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

Message-ID: <CAMArcTUx5cK2kh2M8BirtQRG5Qt+ArwZ_a=xwi_bTHyKJ7E+og@mail.gmail.com>
Date: Wed, 7 May 2025 13:55:44 +0900
From: Taehee Yoo <ap420073@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, pabeni@...hat.com, edumazet@...gle.com, 
	andrew+netdev@...n.ch, horms@...nel.org, almasrymina@...gle.com, 
	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 Wed, May 7, 2025 at 11:55 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Tue,  6 May 2025 14:08:58 +0000 Taehee Yoo wrote:
> > +     mutex_lock(&binding->priv->lock);
> >       xa_for_each(&binding->bound_rxqs, xa_idx, bound_rxq) {
> >               if (bound_rxq == rxq) {
> >                       xa_erase(&binding->bound_rxqs, xa_idx);
> > +                     if (xa_empty(&binding->bound_rxqs))
> > +                             binding->dev = NULL;
> >                       break;
> >               }
> >       }
> > +     mutex_unlock(&binding->priv->lock);
>
> Why do we need to lock the socket around the while loop?
> binding->bound_rxqs have its own lock, and add/del are also
> protected by the netdev instance lock. The only thing we
> must lock is the write to binding->dev I think ?

I intended to protect both binding->bound_rxq and binding->dev.
But you're right, xarray API internally acquires a lock.
Only binding->dev is protected by socket lock here.

>
> Would it be cleaner to move that write and locking to a helper
> which would live in netdev-genl.c?

You mean that the socket lock is not required to cover whole loop
because bound_rxq is safe itself.
So, it acquires a socket lock only for setting binding->dev to NULL,
right? It makes sense to me.
Making a helper in netdev-genl.c would be good, I will make it.

>
> Similarly could we move:
>
>         if (binding->list.next)
>                 list_del(&binding->list);
>
> from net_devmem_unbind_dmabuf() to its callers?
> The asymmetry of list_add() being directly in netdev_nl_bind_rx_doit()
> not net_devmem_bind_dmabuf(), and list_del() being in
> net_devmem_unbind_dmabuf() always confuses me.

I agree with you. I will change it in the next version, too.

>
> >+      mutex_lock(&priv->lock);
> >+      binding = net_devmem_bind_dmabuf(netdev, dmabuf_fd, priv, info->extack);
>
> We shouldn't have to lock the net_devmem_bind_dmabuf(), we have the
> instance lock so the device can't go away, and we haven't listed
> the binding on the socket, yet. Locking around list_add() should
> be enough?

I agree with it.
If binding is not listed, it doesn't have to be protected by lock.
As you mentioned, I will try doing just locking around list_add()
in the netdev_nl_bind_rx_doit().

Thanks a lot!
Taehee Yoo