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

[Jakub Kicinski <kuba@kernel.org>]

Could you make a patch series which fixes all the bugs reported
here which look real?
---
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: wan: fsl_ucc_hdlc: release HDLC device on remove

The patch makes ucc_hdlc_remove() unregister and free the HDLC netdev
before freeing the ucc_hdlc_private object, and adds dev_close() to the
generic unregister_hdlc_device() helper so ndo_stop runs while the HDLC
protocol is still attached.

Link to v1: https://lore.kernel.org/netdev/20260728171545.49247-1-mhun512@gmail.com/

> diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
> index 809f21fb93f56..4c843c94cf436 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);
> +
>  	uhdlc_memclean(priv);

[ ... ]

> diff --git a/drivers/net/wan/hdlc.c b/drivers/net/wan/hdlc.c
> index cbed10b1d862e..a38f90d3006ed 100644
> --- a/drivers/net/wan/hdlc.c
> +++ b/drivers/net/wan/hdlc.c
> @@ -263,6 +263,7 @@ EXPORT_SYMBOL(alloc_hdlcdev);
>  void unregister_hdlc_device(struct net_device *dev)
>  {
>  	rtnl_lock();
> +	dev_close(dev);
>  	detach_hdlc_protocol(dev);
>  	unregister_netdevice(dev);
>  	rtnl_unlock();

[Severity: Medium]
Should this hunk be a separate patch?  The subject is scoped to
fsl_ucc_hdlc and the Fixes tag points at c19b6d246a35, but
unregister_hdlc_device() is exported generic HDLC code with eight other
callers: c101_cleanup(), n2_destroy_card(), pc300too_remove_one(),
pci200syn_remove_one(), wanxl_pci_remove_one(), hss_remove_one(),
qmc_hdlc_remove() and farsync's fst_add_one()/fst_remove_one().

For all of those, detach_hdlc_protocol() -> hdlc_setup_dev() previously
cleared IFF_UP:

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

so netif_close_many() dropped the device and ndo_stop never ran.  After
this change every one of those drivers runs its close routine during
unregister.  Were the other users audited?  The changelog does not say,
and with Cc: stable plus a driver-only Fixes tag the core behaviour
change lands in stable as if it were an fsl_ucc_hdlc-only fix.

One call site that looks worth a sentence in the changelog is the
farsync error unwind:

drivers/net/wan/farsync.c:fst_add_one() {
	...
rx_dma_fail:
	fst_disable_intr(card);
	for (i = 0 ; i < card->nports ; i++)
		unregister_hdlc_device(card->ports[i].dev);
	...
}

Here fst_close() can now run after fst_disable_intr() and after the rx
DMA buffer has been freed.  Was that considered?

[Severity: High]
This isn't a bug introduced by this patch, but the function this hunk
newly drives can dereference a NULL protocol pointer.  uhdlc_open()
calls uhdlc_close() precisely when hdlc_open() failed:

drivers/net/wan/fsl_ucc_hdlc.c:uhdlc_open() {
	...
		rc = hdlc_open(dev);
		if (rc)
			uhdlc_close(dev);
	...
}

For a freshly probed device the only failure hdlc_open() can return is
the no-protocol case, since ucc_hdlc_probe() only calls
register_hdlc_device() and never attaches a protocol:

drivers/net/wan/hdlc.c:hdlc_open() {
	...
	if (!hdlc->proto)
		return -ENOSYS;	/* no protocol attached */
	...
}

uhdlc_close() then calls hdlc_close() unconditionally, and hdlc_close()
has no NULL check.  hdlc->carrier is initialised to 1 in hdlc_setup(),
so hdlc_proto_stop() reads hdlc->proto->stop, and the tail of
hdlc_close() dereferences hdlc->proto again:

drivers/net/wan/hdlc.c:hdlc_close() {
	...
	if (hdlc->carrier)
		hdlc_proto_stop(dev);
	...
	if (hdlc->proto->close)
		hdlc->proto->close(dev);
}

Can "ip link set hdlc0 up" before any IF_PROTO_ ioctl oops here?  Peer
drivers such as c101_open() and fst_open() call hdlc_open() first and
just propagate the error without entering their close routine.  Since
this patch makes hdlc_close() reachable from the unregister path too,
would it make sense to fix this at the same time, either by calling
hdlc_open() before the hardware is enabled or by making hdlc_close()
tolerate a NULL proto?

[Severity: High]
This is a pre-existing issue, but the new dev_close() gives it a second
trigger, and this one runs with rtnl_mutex held.  uhdlc_suspend()
disables NAPI and can then return an error without re-enabling it:

drivers/net/wan/fsl_ucc_hdlc.c:uhdlc_suspend() {
	...
	napi_disable(&priv->napi);
	...
	priv->ucc_pram_bak = kmalloc_obj(*priv->ucc_pram_bak);
	if (!priv->ucc_pram_bak)
		return -ENOMEM;
	...
}

The PM core does not call resume for a device whose suspend failed, so
the device stays running with NAPI disabled.  uhdlc_close() then calls
napi_disable() again:

drivers/net/wan/fsl_ucc_hdlc.c:uhdlc_close() {
	...
	napi_disable(&priv->napi);
	...
}

and the wait in napi_disable_locked() has no timeout or alternative exit
condition:

net/core/dev.c:napi_disable_locked() {
	...
		while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
			usleep_range(20, 200);
			val = READ_ONCE(n->state);
		}
	...
}

Those bits are only cleared by napi_enable(), which never runs on that
path.  With this patch the call chain becomes ucc_hdlc_remove() ->
unregister_hdlc_device() -> rtnl_lock(); dev_close() -> netif_close() ->
uhdlc_close(), so can an unbind or module removal after a failed suspend
spin forever while holding rtnl_mutex?  Would fixing uhdlc_suspend() to
call napi_enable() before returning -ENOMEM be preferable to leaving
this reachable from the unbind path?
-- 
pw-bot: cr