Re: [PATCH net-next v3 3/3] w5100: detect carrier state using link status bit

[sashiko-bot@kernel.org]

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

New issues:
- [High] Using `devm_request_threaded_irq` for the link interrupt causes a use-after-free on module removal because the network device is manually freed before devres cleanup.
- [High] W5500 interface becomes permanently stuck with the carrier off if the cable is plugged in late and no optional link GPIO is configured.
- [High] Link detection is broken for W5100 and W5200 hardware when a link GPIO is used, as the driver always incorrectly reports the link as 'up' on every interrupt edge.
- [Medium] `w5100_get_link()` does not handle negative error codes from `w5100_read()`, causing SPI read failures to be misinterpreted as a valid link state.

Pre-existing issues:
- [High] Early device registration in `w5100_probe()` exposes uninitialized driver state and hardware to userspace.
- [High] `unregister_netdev()` is called after destroying the workqueue and freeing interrupts, allowing active network operations to use destroyed resources.
- [High] The probe error path unregisters and frees the netdev without canceling pending asynchronous works, leading to a Use-After-Free.
--

--- Patch [3]: [PATCH net-next v3 3/3] w5100: detect carrier state using link status bit ---
commit 6726aad63717b1d64239168658010e6e77a6636c
Author: Arthur Crépin Leblond <arthur@marmottus.net>

w5100: detect carrier state using link status bit

Detect the link status on the w5500 using the Link Status bit from the
PHY Configuration register.

Also add an optional GPIO line that can be wired to the LINKLED pin of
the w5500 as an interrupt to detect link status changes and read the
register.

On chip variants other than the w5500, which lack this register, the
link is always reported as up.

Commit dacf281771a9 ("w5100: remove unused gpio link detection")
dropped the link_gpio/link_irq handling on the grounds that no
devicetree user passed a "link" GPIO at the time and that it used the
old gpio interface.

This isn't a plain revert of that removal. It uses the new managed GPIO
descriptors so that the resources are released automatically on driver
removal or probe failure.

> diff --git a/drivers/net/ethernet/wiznet/w5100.c b/drivers/net/ethernet/wiznet/w5100.c
> index 53d8dc642fbd6..6707a6f273954 100644
> --- a/drivers/net/ethernet/wiznet/w5100.c
> +++ b/drivers/net/ethernet/wiznet/w5100.c

[ ... ]

> @@ -414,6 +418,16 @@ static void w5100_get_drvinfo(struct net_device *ndev,
>  		sizeof(info->bus_info));
>  }
>  
> +static u32 w5100_get_link(struct net_device *ndev)
> +{
> +	struct w5100_priv *priv = netdev_priv(ndev);
> +
> +	if (priv->ops->chip_id == W5500)
> +		return w5100_read(priv, W5500_PHYCFGR) & PHYCFGR_LNK;

[Severity: Medium]
Does this correctly handle negative error codes from w5100_read()?

w5100_read() delegates to SPI read functions which can return negative error
codes (e.g., -EIO or -ENOMEM) upon transfer failure. Because negative numbers
are represented in two's complement, an error code like -EIO (-5, which is
0xFFFFFFFB) will have the lowest bit set.

Could this cause the driver to incorrectly interpret an SPI read failure as a
valid "link up" state?

> +
> +	return 1;
> +}
> +

[ ... ]

> @@ -616,6 +630,24 @@ static irqreturn_t w5100_interrupt(int irq, void *ndev_instance)
>  	return IRQ_HANDLED;
>  }
>  
> +static irqreturn_t w5100_detect_link(int irq, void *ndev_instance)
> +{
> +	struct net_device *ndev = ndev_instance;
> +	struct w5100_priv *priv = netdev_priv(ndev);
> +
> +	if (netif_running(ndev)) {
> +		if (w5100_get_link(ndev)) {

[Severity: High]
For non-W5500 chips (like W5100 or W5200), w5100_get_link() unconditionally
returns 1.

Since the new link GPIO interrupt is configured to trigger on both rising
and falling edges, does this mean any link state change (even a cable unplug)
will be incorrectly interpreted as "link is up" on older chips because the
actual GPIO value is never read?

> +			netif_info(priv, link, ndev, "link is up\n");
> +			netif_carrier_on(ndev);
> +		} else {
> +			netif_info(priv, link, ndev, "link is down\n");
> +			netif_carrier_off(ndev);
> +		}
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +

[ ... ]

> @@ -659,6 +691,12 @@ static int w5100_open(struct net_device *ndev)
>  	w5100_hw_start(priv);
>  	napi_enable(&priv->napi);
>  	netif_start_queue(ndev);
> +
> +	if (w5100_get_link(ndev))
> +		netif_carrier_on(ndev);
> +	else
> +		netif_carrier_off(ndev);

[Severity: High]
If the cable is disconnected at open time, this explicitly turns the carrier
off.

If a board does not define the optional link GPIO in its devicetree, the
driver skips setting up the link IRQ. Since there is no fallback polling
timer implemented when the interrupt is unavailable, is there any mechanism
to detect when a cable is plugged in later, or does the interface remain
permanently stuck with the carrier off?

> +
>  	return 0;
>  }
>  

[ ... ]

> @@ -751,6 +790,13 @@ int w5100_probe(struct device *dev, const struct w5100_ops *ops,
>  	priv->ndev = ndev;
>  	priv->ops = ops;
>  	priv->irq = irq;
> +	priv->link_gpio = devm_gpiod_get_optional(dev, "link", GPIOD_IN);
> +	if (IS_ERR(priv->link_gpio)) {
> +		err = dev_err_probe(dev, PTR_ERR(priv->link_gpio),
> +				    "failed to get link GPIO\n");
> +		priv->link_gpio = NULL;
> +		goto err_register;
> +	}

[Severity: High]
This is a pre-existing issue, but does registering the netdev this early
expose uninitialized state to userspace?

Later in w5100_probe(), register_netdev() is called before critical
initialization steps like allocating priv->xfer_wq, executing INIT_WORK(),
and performing a hardware reset. 

If userspace attempts to use the device immediately after registration (e.g.,
via udev bringing the interface UP), could ndo_open() and transmit paths
execute on uninitialized hardware or attempt to queue work to a NULL
priv->xfer_wq?

[ ... ]

> @@ -803,8 +849,29 @@ int w5100_probe(struct device *dev, const struct w5100_ops *ops,
>  	if (err)
>  		goto err_hw;
>  
> +	if (priv->link_gpio) {
> +		int link_irq = gpiod_to_irq(priv->link_gpio);
> +
> +		if (link_irq < 0) {
> +			err = dev_err_probe(dev, link_irq,
> +					    "No corresponding irq for link gpio\n");
> +			goto err_gpio;
> +		}
> +
> +		err = devm_request_threaded_irq(dev, link_irq, NULL,
> +						w5100_detect_link,
> +						IRQF_TRIGGER_RISING |
> +						IRQF_TRIGGER_FALLING |
> +						IRQF_ONESHOT,
> +						"w5100-link", priv->ndev);

[Severity: High]
Can this devm-managed interrupt trigger a use-after-free on module removal?

The devm cleanup runs after the driver's w5100_remove() callback returns.
However, w5100_remove() calls free_netdev(ndev) to synchronously free the
network device before returning. 

If a link change triggers the interrupt during this window, wouldn't the
active threaded IRQ handler w5100_detect_link() execute and dereference the
already-freed ndev pointer?

> +		if (err < 0)
> +			goto err_gpio;
> +	}
> +
>  	return 0;
>  
> +err_gpio:
> +	free_irq(priv->irq, ndev);
>  err_hw:
>  	destroy_workqueue(priv->xfer_wq);

[Severity: High]
This is a pre-existing issue, but is it safe to destroy the workqueue before
unregistering the netdev?

By calling destroy_workqueue() before unregister_netdev(), the network
device is still registered and active while its workqueue is being destroyed.
Could a concurrent packet transmission attempt to queue work to the destroyed
workqueue?

>  err_wq:
>  	unregister_netdev(ndev);

[Severity: High]
This is a pre-existing issue, but does this error path leak pending
asynchronous work or cause a use-after-free?

If an error occurs after register_netdev() succeeds, the error path
unregisters and frees the netdev. Because the device was visible to userspace,
actions like ndo_set_rx_mode might have already scheduled priv->setrx_work
onto the global system workqueue.

Since cancel_work_sync() is never called for these global works before
free_netdev() is invoked, could the pending work execute later and
dereference freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806-wiznet-link-gpio-v3-0-532d4a143805@marmottus.net?part=3