Re: [PATCH net v3 3/4] net: wan: hdlc: close active devices before protocol detach

[Paolo Abeni]

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/20260806020541.2011936-2-kuba@xxxxxxxxxx

> 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.