net: wan: fix FSL UCC HDLC lifecycle bugs

This issue was identified during our ongoing static-analysis research while reviewing kernel code.

The v2 review found two pre-existing FSL UCC HDLC bugs and asked that the generic HDLC behavior change be separated and audited.

Patch 1 validates that an HDLC protocol is attached before starting the UCC, avoiding a NULL protocol dereference on a premature interface open. Patch 2 allocates the suspend backup before disabling NAPI, so allocation failure leaves the running interface usable. Patch 3 closes active HDLC devices before detaching their protocol and documents the audit of every current caller, including the farsync probe unwind. Patch 4 unregisters and frees the FSL UCC HDLC netdev before its private resources.

Patch 4 depends on patch 3 to stop an active device while its protocol is still attached and before UCC and DMA resources are released.

These fixes and changelogs were prepared with an LLM and then checked against the cited source paths and call ordering.

Changes since v2:
- split the generic unregister_hdlc_device() change into its own patch;
- audit all eight in-tree HDLC hardware drivers and document farsync;
- fix the NULL protocol dereference reported in uhdlc_open();
- keep NAPI enabled when the suspend backup allocation fails;
- rebase the series onto Linux 7.3-rc1.

v2: https://lore.kernel.org/r/[email protected]
review: https://lore.kernel.org/r/[email protected]

Validation:
The series applies cleanly to net commit 784450234395. All four patches pass strict checkpatch.
No hardware runtime testing was performed.

Myeonghun Pak (4):
net: wan: fsl_ucc_hdlc: validate protocol before starting device
net: wan: fsl_ucc_hdlc: allocate suspend backup before quiescing
net: wan: hdlc: close active devices before protocol detach
net: wan: fsl_ucc_hdlc: release HDLC device on remove

 drivers/net/wan/fsl_ucc_hdlc.c | 29 ++++++++++++++++-------------
 drivers/net/wan/hdlc.c         |  1 +
 2 files changed, 17 insertions(+), 13 deletions(-)

base-commit: cee9395acd8043be0644b25c34bfa86623f2b935

2.47.1

uhdlc_open() starts the UCC, IRQ and NAPI before it calls hdlc_open(). If no HDLC protocol has been attached, hdlc_open() returns -ENOSYS. The error path then calls uhdlc_close(), which calls hdlc_close() and dereferences hdlc->proto even though it is NULL. Bringing up a freshly registered interface before an IF_PROTO ioctl can therefore trigger a NULL pointer dereference.

Call hdlc_open() before enabling the hardware. Balance a successful protocol open with hdlc_close() if requesting the IRQ then fails. This matches peer HDLC drivers and avoids running teardown for a protocol that never opened.

---
 drivers/net/wan/fsl_ucc_hdlc.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

index 809f21fb93f56..82796452e54a2 100644
@@ -34,8 +34,6 @@
 #define TDM_PPPOHT_SLIC_MAXIN
 #define RX_BD_ERRORS (R_CD_S | R_OV_S | R_CR_S | R_AB_S | R_NO_S | R_LG_S)
 
-static int uhdlc_close(struct net_device *dev);
-
 static struct ucc_tdm_info utdm_primary_info = {
 	.uf_info = {
 		.tsa = 0,

@@ -705,12 +703,18 @@ static int uhdlc_open(struct net_device *dev)
 	hdlc_device *hdlc = dev_to_hdlc(dev);
 	struct ucc_hdlc_private *priv = hdlc->priv;
 	struct ucc_tdm *utdm = priv->utdm;
-	int rc = 0;
+	int rc;
 
 	if (priv->hdlc_busy != 1) {
+		rc = hdlc_open(dev);
+		if (rc)
+			return rc;
+
 		if (request_irq(priv->ut_info->uf_info.irq,
-				ucc_hdlc_irq_handler, 0, "hdlc", priv))
+				ucc_hdlc_irq_handler, 0, "hdlc", priv)) {
+			hdlc_close(dev);
 			return -ENODEV;
+		}
 
 		cecr_subblock = ucc_fast_get_qe_cr_subblock(
 					priv->ut_info->uf_info.ucc_num);

@@ -729,13 +733,9 @@ static int uhdlc_open(struct net_device *dev)
 		napi_enable(&priv->napi);
 		netdev_reset_queue(dev);
 		netif_start_queue(dev);
-
-		rc = hdlc_open(dev);
-		if (rc)
-			uhdlc_close(dev);
 	}
 
-	return rc;
+	return 0;
 }
 
 static void uhdlc_memclean(struct ucc_hdlc_private *priv)

2.47.1

uhdlc_suspend() detaches the netdev and disables NAPI before allocating the parameter RAM backup. If that allocation fails, suspend returns -ENOMEM with the interface still running but NAPI disabled. The PM core does not call resume after a failed suspend, so a later close attempts to disable NAPI again and can wait indefinitely.

Allocate the backup before changing the runtime state. An allocation failure then leaves the interface attached and NAPI enabled.

---
 drivers/net/wan/fsl_ucc_hdlc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index 82796452e54a2..596f4ef053636 100644
@@ -888,6 +888,10 @@ static int uhdlc_suspend(struct device *dev)
 	if (!netif_running(priv->ndev))
 		return 0;
 
+	priv->ucc_pram_bak = kmalloc_obj(*priv->ucc_pram_bak);
+	if (!priv->ucc_pram_bak)
+		return -ENOMEM;
+
 	netif_device_detach(priv->ndev);
 	napi_disable(&priv->napi);
 

@@ -897,10 +901,6 @@ static int uhdlc_suspend(struct device *dev)
 	priv->gumr = ioread32be(&uf_regs->gumr);
 	priv->guemr = ioread8(&uf_regs->guemr);
 
-	priv->ucc_pram_bak = kmalloc_obj(*priv->ucc_pram_bak);
-	if (!priv->ucc_pram_bak)
-		return -ENOMEM;
-
 	/* backup HDLC parameter */
 	memcpy_fromio(priv->ucc_pram_bak, priv->ucc_pram,
 		      sizeof(struct ucc_hdlc_param));

2.47.1

Commit ff3516442768 ("WAN: HDLC: Detach protocol before unregistering device") moved protocol detach ahead of netdev unregister so detach could still use its state. However, detach_hdlc_protocol() calls hdlc_setup_dev(), which clears IFF_UP. unregister_netdevice() then sees an already-down device and skips ndo_stop, leaving an active HDLC device running while its driver releases resources.

Close the device under RTNL while its protocol is still attached, then keep the existing detach-before-unregister order. This runs the hardware stop callback and the protocol close callback before their state is released.

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. The farsync probe unwind can disable interrupts and free its RX DMA buffer first, but fst_close() does not use that buffer or require interrupts; the card is also in FST_RESET, so fst_closeport() does not access the port hardware.

---
 drivers/net/wan/hdlc.c | 1 +
 1 file changed, 1 insertion(+)

index cbed10b1d862e..a38f90d3006ed 100644
@@ -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();

2.47.1

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.

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);
}

drivers/net/wan/hdlc.c:hdlc_rcv() {
	...
	BUG_ON(!hdlc->proto->netif_rx);
	return hdlc->proto->netif_rx(skb);
}

net/core/dev.c:unregister_netdevice_many_notify() {
	...
	flush_all_backlogs();

	synchronize_net();
	...
}

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);
	...
}

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;
}

This is an AI-generated review.

ucc_hdlc_probe() registers an HDLC netdev whose private pointer refers to the separately allocated ucc_hdlc_private object. The remove path frees that object and its resources without unregistering or freeing the netdev. The registered device is left with a dangling private pointer.

Unregister the HDLC device before releasing the UCC and DMA resources so an active interface is stopped first. Free the netdev before releasing its private object.

This patch depends on the preceding "net: wan: hdlc: close active devices before protocol detach" fix (patch 3 of this series). Without that fix, protocol detach clears IFF_UP before unregister can invoke ndo_stop, so an active interface would not be stopped before its resources are freed.

---
 drivers/net/wan/fsl_ucc_hdlc.c | 3 +++
 1 file changed, 3 insertions(+)

index 596f4ef053636..371150efc1a65 100644
@@ -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);
 
 	if (priv->utdm && priv->utdm->si_regs) {

@@ -1266,6 +1268,7 @@ static void ucc_hdlc_remove(struct platform_device *pdev)
 		iounmap(priv->utdm->siram);
 		priv->utdm->siram = NULL;
 	}
+	free_netdev(priv->ndev);
 	kfree(priv);
 
 	dev_info(&pdev->dev, "UCC based hdlc module removed\n");

2.47.1