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: <20250506195526.2ab7c15b@kernel.org>
Date: Tue, 6 May 2025 19:55:26 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Taehee Yoo <ap420073@...il.com>
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 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?