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/[email protected]/ > 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
