* Re: [PATCH net v2] net: wan: fsl_ucc_hdlc: release HDLC device on remove
2026-08-03 13:30 [PATCH net v2] net: wan: fsl_ucc_hdlc: release HDLC device on remove Myeonghun Pak
@ 2026-08-06 2:05 ` Jakub Kicinski
0 siblings, 0 replies; 2+ messages in thread
From: Jakub Kicinski @ 2026-08-06 2:05 UTC (permalink / raw)
To: mhun512
Cc: Jakub Kicinski, qiang.zhao, horms, andrew+netdev, davem, edumazet,
pabeni, khc, netdev, linuxppc-dev, linux-kernel, stable, ae878000
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
^ permalink raw reply [flat|nested] 2+ messages in thread