Re: [PATCH net-next v3 1/3] dt-bindings: net: wiznet,w5100: convert to DT schema

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Preexisting module removal race condition: `unregister_netdev()` is called after `destroy_workqueue()`, leading to potential use-after-free.
- [High] Preexisting probe race condition: `register_netdev()` is called too early, exposing the device to userspace before hardware and structures are initialized.
--

--- Patch [1]: [PATCH net-next v3 1/3] dt-bindings: net: wiznet,w5100: convert to DT schema ---
commit 6b1d871923a8b7c229c0db289868ab706e3e28d1
Author: Arthur Crépin Leblond <[email protected]>

dt-bindings: net: wiznet,w5100: convert to DT schema
    
Convert the Wiznet w5x00 SPI Ethernet controller binding from
plain text to DT schema.

> diff --git a/Documentation/devicetree/bindings/net/wiznet,w5100.yaml b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml
> new file mode 100644
> index 0000000000000..641c9ddbde9e7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/wiznet,w5100.yaml

[ ... ]

While reviewing this dt-bindings change, I noticed a couple of pre-existing
issues in the related driver code (drivers/net/ethernet/wiznet/w5100.c)
that might be worth addressing.

drivers/net/ethernet/wiznet/w5100.c:w5100_probe() {
    ...
	err = register_netdev(ndev);
	if (err < 0)
		goto err_register;

	priv->xfer_wq = alloc_workqueue("%s", WQ_MEM_RECLAIM | WQ_PERCPU, 0,
					netdev_name(ndev));
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to call
register_netdev() before fully initializing the device?

If userspace automation brings the interface up immediately after
registration, it could trigger a packet transmission. This would call
w5100_start_tx(), which attempts to queue work to priv->xfer_wq.

Since the workqueue and other hardware structures are initialized after
registration, could this result in a NULL pointer dereference?


drivers/net/ethernet/wiznet/w5100.c:w5100_remove() {
    ...
	flush_work(&priv->setrx_work);
	flush_work(&priv->restart_work);
	destroy_workqueue(priv->xfer_wq);

	unregister_netdev(ndev);
    ...
}

[Severity: High]
This is a pre-existing issue, but does destroying the workqueue before
calling unregister_netdev() create a race condition during module removal?

The unregister_netdev() function is responsible for safely stopping the
transmission queue and tearing down device visibility. If the workqueue is
destroyed first, there is a window where the network stack can concurrently
call ndo_start_xmit (w5100_start_tx()).

Can this lead to a use-after-free when w5100_start_tx() attempts to queue
work to the destroyed workqueue?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1