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

[Jakub Kicinski <kuba@kernel.org>]

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 ?

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

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.

>+	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?