Re: [PATCH] w5100: restore GPIO-based link detection

[sashiko-bot@kernel.org]

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

New issues:
- [High] The link_gpio descriptor is released via gpiod_put() before unregister_netdev() is called, allowing concurrent access to the freed descriptor.
- [Medium] Calling gpiod_get_value() instead of gpiod_get_value_cansleep() in the interrupt handler causes a WARN_ON if the GPIO is on a sleepable controller.

Pre-existing issues:
- [High] register_netdev() is called before fully initializing driver resources, exposing an uninitialized device to userspace and causing a NULL pointer dereference.
- [High] Workqueues are destroyed before unregister_netdev(), allowing work items to be queued to a destroyed workqueue.
--

--- Patch [1]: [PATCH] w5100: restore GPIO-based link detection ---
commit cb90c7abab96f3cb827517717f36ffb68ab9ffc0
Author: Arthur Crépin Leblond <arthur@marmottus.net>

w5100: restore GPIO-based link detection

This patch restores GPIO-based link detection for the w5100 driver, adding
back link_gpio handling that was previously removed.

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

[ ... ]

> @@ -616,6 +629,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 (gpiod_get_value(priv->link_gpio) != 0) {

[Severity: Medium]
Since the link GPIO could be provided by a sleepable controller like an I2C
expander, should this be gpiod_get_value_cansleep() instead?

Calling the non-sleeping gpiod_get_value() in w5100_detect_link() can trigger
a WARN_ON stack trace if the underlying GPIO controller requires sleeping.

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

[ ... ]

> @@ -803,13 +850,40 @@ int w5100_probe(struct device *dev, const struct w5100_ops *ops,
>  	if (err)
>  		goto err_hw;

[Severity: High]
This is a pre-existing issue, but is it safe to expose the device to
userspace before its internal resources are fully allocated?

Looking at w5100_probe(), register_netdev() is called before priv->xfer_wq
is allocated:

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));
        ...
}

If a userspace daemon like NetworkManager immediately configures the interface
and attempts to send a packet, ndo_start_xmit will execute and queue work
onto a NULL xfer_wq pointer, causing a dereference crash.

> +	if (priv->link_gpio) {
> +		char *link_name = devm_kasprintf(dev, GFP_KERNEL, "%s-link",

[ ... ]

> @@ -822,6 +896,10 @@ void w5100_remove(struct device *dev)
>  
>  	w5100_hw_reset(priv);
>  	free_irq(priv->irq, ndev);
> +	if (priv->link_gpio) {
> +		free_irq(priv->link_irq, ndev);
> +		gpiod_put(priv->link_gpio);
> +	}

[Severity: High]
Does this prematurely free the GPIO descriptor?

Since unregister_netdev() hasn't been called yet, the network device is still
active in w5100_remove(). A concurrent ethtool command or interface up event
could call w5100_get_link() or w5100_open(), accessing the freed link_gpio
descriptor and causing a use-after-free.

>  	flush_work(&priv->setrx_work);
>  	flush_work(&priv->restart_work);

[Severity: High]
This is a pre-existing issue, but does this destroy the workqueue while the
network device is still fully active?

Looking further down in the w5100_remove() function:

void w5100_remove(struct device *dev)
{
        ...
        flush_work(&priv->restart_work);
        destroy_workqueue(priv->xfer_wq);

        unregister_netdev(ndev);
        free_netdev(ndev);
}

Since unregister_netdev() is called after destroy_workqueue(), a concurrent
packet transmission could trigger ndo_start_xmit and queue work onto the
already destroyed xfer_wq, resulting in a use-after-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804-wiznet-link-gpio-v1-1-b626fd4f7ccb@marmottus.net?part=1