Re: [PATCH net] net: wan: fsl_ucc_hdlc: release HDLC device on remove

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/[email protected]?part=1
---
--- Patch [1]: [PATCH net] net: wan: fsl_ucc_hdlc: release HDLC device on remove ---
commit 345f90c3e598219f0faf6e99ee38f97b8bd6452d
Author: Myeonghun Pak <[email protected]>

net: wan: fsl_ucc_hdlc: release HDLC device on remove

The remove path previously freed the ucc_hdlc_private object without
unregistering or freeing the HDLC netdev. This patch adds
unregister_hdlc_device() at the start of ucc_hdlc_remove() and
free_netdev() before kfree(priv).

> diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
> index 809f21fb93f5..4c843c94cf43 100644
> --- a/drivers/net/wan/fsl_ucc_hdlc.c
> +++ b/drivers/net/wan/fsl_ucc_hdlc.c
> @@ -1255,6 +1255,8 @@ static void ucc_hdlc_remove(struct platform_device *pdev)
>  {
>  	struct ucc_hdlc_private *priv = dev_get_drvdata(&pdev->dev);
>
> +	unregister_hdlc_device(priv->ndev);
> +

[Severity: High]
The commit message says this "closes an active interface through ndo_stop,
which disables NAPI, stops the UCC, and frees its IRQ".  Is ndo_stop really
reached here?

unregister_hdlc_device() detaches the protocol before unregistering:

drivers/net/wan/hdlc.c:unregister_hdlc_device() {
	rtnl_lock();
	detach_hdlc_protocol(dev);
	unregister_netdevice(dev);
	rtnl_unlock();
}

and detach_hdlc_protocol() ends with an unconditional hdlc_setup_dev(),
which assigns dev->flags rather than or-ing into it:

drivers/net/wan/hdlc.c:hdlc_setup_dev() {
	dev->flags		 = IFF_POINTOPOINT | IFF_NOARP;
	...
}

So IFF_UP is cleared while the interface is still running.  Both the
unregister path and netif_close_many() in net/core/dev.c then gate the
close on that flag:

	if (!(dev->flags & IFF_UP))
		continue;
...
	list_for_each_entry_safe(dev, tmp, head, close_list)
		if (!(dev->flags & IFF_UP))
			list_del_init(&dev->close_list);

Doesn't that mean uhdlc_close() never runs, so none of napi_disable(), the
QE_GRACEFUL_STOP_TX/QE_CLOSE_RX_BD sequence, ucc_fast_disable() or
free_irq(priv->ut_info->uf_info.irq, priv) happen on the remove path when
the link was brought up with "ip link set hdlc0 up"?

>  	uhdlc_memclean(priv);
>
>  	if (priv->utdm && priv->utdm->si_regs) {
> @@ -1266,6 +1268,7 @@ static void ucc_hdlc_remove(struct platform_device *pdev)
>  		iounmap(priv->utdm->siram);
>  		priv->utdm->siram = NULL;
>  	}
> +	free_netdev(priv->ndev);
>  	kfree(priv);

[Severity: High]
Following on from the above: if uhdlc_close() is skipped, can this sequence
free memory that the hardware and the IRQ handler are still using?

uhdlc_open() installs the handler with priv as dev_id and enables NAPI:

drivers/net/wan/fsl_ucc_hdlc.c:uhdlc_open() {
	if (request_irq(priv->ut_info->uf_info.irq,
			ucc_hdlc_irq_handler, 0, "hdlc", priv))
		return -ENODEV;
	...
	napi_enable(&priv->napi);
	...
}

With no free_irq() anywhere else in the driver, the handler stays installed
after kfree(priv), and the first thing it does is dereference dev_id:

drivers/net/wan/fsl_ucc_hdlc.c:ucc_hdlc_irq_handler() {
	struct ucc_hdlc_private *priv = (struct ucc_hdlc_private *)dev_id;
	struct net_device *dev = priv->ndev;
	...
			__napi_schedule(&priv->napi);
	...
}

Is that a use-after-free of priv, of the uccf freed by ucc_fast_free() in
uhdlc_memclean(), and of the already deleted napi_struct?

Also, since napi_disable() never ran, does free_netdev(priv->ndev) ->
netdev_napi_exit() -> __netif_napi_del_locked() trip its
WARN_ON(!test_bit(NAPI_STATE_SCHED, &napi->state)) and tear down a NAPI that
may still be scheduled or polling?

And with the UCC left enabled, can it keep DMAing into the coherent RX/TX
rings and buffers that uhdlc_memclean() already returned via
dma_free_coherent()?

Would forcing the interface down before the protocol is detached, for
example netif_close()/dev_close() under rtnl, or an explicit
napi_disable()/UCC stop/free_irq() sequence in remove, address this?