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/ --- net: wan: hdlc: close active devices before protocol detach
This adds a dev_close() call to unregister_hdlc_device() so that an up HDLC device runs its ndo_stop and protocol close callbacks before detach_hdlc_protocol() clears IFF_UP via hdlc_setup_dev(). The detach-before-unregister order introduced by ff3516442768 is kept. Link: https://lore.kernel.org/r/[email protected] > 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); [Severity: High] This isn't a bug introduced by this patch, but the new dev_close() makes the remove path call into ixp4xx_hss's ndo_stop, and hss_hdlc_close() sleeps inside a spin_lock_irqsave() section: drivers/net/wan/ixp4xx_hss.c:hss_hdlc_close() { spin_lock_irqsave(&npe_lock, flags); ports_open--; qmgr_disable_irq(port->rxq); netif_stop_queue(dev); napi_disable(&port->napi); ... free_irq(...); spin_unlock_irqrestore(&npe_lock, flags); } napi_disable() reaches napi_disable_locked(), which begins with might_sleep() and loops on usleep_range(20, 200); free_irq() also calls might_sleep() and takes desc->request_mutex. Can that produce a "sleeping function called from invalid context" splat here, and if the scheduler is actually entered, can the CPU leave npe_lock held while hss_hdlc_dcd_irq() -> hss_hdlc_set_carrier() spins on the same lock? The same path is already reachable via "ip link set hss0 down", but the commit message lists ixp4xx_hss among the audited-safe users: Audit all current users: c101, n2, pc300too, pci200syn, wanxl, ixp4xx_hss, fsl_qmc_hdlc and farsync unregister before releasing the resources used by their close callbacks. Should the audit note this, given the remove path now runs hss_hdlc_close() where it previously did not? > detach_hdlc_protocol(dev); [Severity: High] Does dev_close() close the window this relies on for the RX path? detach_hdlc_protocol() sets hdlc->proto = NULL and kfree()s hdlc->state while the netdev is still registered, so hdlc_rcv() can still be reached from NET_RX softirq for frames already queued on a per-CPU input_pkt_queue: drivers/net/wan/hdlc.c:hdlc_rcv() { ... BUG_ON(!hdlc->proto->netif_rx); return hdlc->proto->netif_rx(skb); } The only barrier that drops backlogged skbs for the device runs later, inside unregister_netdevice(): net/core/dev.c:unregister_netdevice_many_notify() { ... flush_all_backlogs(); synchronize_net(); ... } __dev_close_many() does not touch the RX backlog, so a frame handed to netif_rx() before ndo_stop and still sitting in another CPU's queue can reach hdlc_rcv() after hdlc->proto became NULL. The "if (!(dev->priv_flags & IFF_WAN_HDLC))" test in hdlc_rcv() does not filter it out, since hdlc_setup_dev() re-sets priv_flags to IFF_WAN_HDLC. Can that hit the BUG_ON above, or use the freed hdlc->state inside fr_rx()/cisco_rx()/ppp_rx() if proto was already read? [Severity: Medium] This is a pre-existing issue and not introduced by this patch, but the return value of detach_hdlc_protocol() is dropped while unregister_netdevice() runs unconditionally. detach_hdlc_protocol() returns early when a NETDEV_PRE_TYPE_CHANGE notifier refuses the type change, skipping both release points: drivers/net/wan/hdlc.c:detach_hdlc_protocol() { ... if (err) { netdev_err(dev, "Refused to change device type\n"); return err; } ... module_put(hdlc->proto->module); ... kfree(hdlc->state); ... } The refusal is reachable: hdlc_raw_eth's raw_eth_ioctl() calls ether_setup(), so dev->type becomes ARPHRD_ETHER and a VLAN can be stacked on it, after which net/8021q/vlan.c:vlan_device_event() { case NETDEV_PRE_TYPE_CHANGE: /* Forbid underlaying device to change its type. */ if (vlan_uses_dev(dev)) return NOTIFY_BAD; } returns NOTIFY_BAD. lapbether_device_event() refuses the same way. Does this leak the kmalloc'ed hdlc->state and the hdlc_raw_eth module reference, given every caller then calls free_netdev() and loses the only pointer to that state? > unregister_netdevice(dev); > rtnl_unlock(); -- This is an AI-generated review.
