[PATCH] net: wan: fsl_ucc_hdlc: release resources on remove

[PATCH] net: wan: fsl_ucc_hdlc: release resources on remove

[Guangshuo Li]

ucc_hdlc_probe() allocates and registers an HDLC netdev and, when the
TDM interface is enabled, separately allocates struct ucc_tdm. The
probe failure paths release these resources, but the remove path does
not.

As a result, the registered HDLC netdev can outlive its private data
and retain a dangling private pointer. The separately allocated TDM
data is also leaked when a TDM-mode device is removed.

Unregister the HDLC device before releasing the UCC and DMA resources,
free the netdev after the hardware resources have been cleaned up, and
free the TDM data before releasing the private data.

This issue was found by manual code inspection.

Fixes: c19b6d246a35 ("drivers/net: support hdlc function for QE-UCC")
Cc: stable@vger.kernel.org
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/net/wan/fsl_ucc_hdlc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
index 809f21fb93f5..ffc8b989e33a 100644
--- a/drivers/net/wan/fsl_ucc_hdlc.c
+++ b/drivers/net/wan/fsl_ucc_hdlc.c
@@ -1254,6 +1254,7 @@ static int ucc_hdlc_probe(struct platform_device *pdev)
 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);
 
@@ -1266,6 +1267,8 @@ static void ucc_hdlc_remove(struct platform_device *pdev)
 		iounmap(priv->utdm->siram);
 		priv->utdm->siram = NULL;
 	}
+	free_netdev(priv->ndev);
+	kfree(priv->utdm);
 	kfree(priv);
 
 	dev_info(&pdev->dev, "UCC based hdlc module removed\n");
-- 
2.43.0

Re: [PATCH] net: wan: fsl_ucc_hdlc: release resources on remove

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 1 (1)

- [High] Teardown ordering defect in the new code: ucc_hdlc_remove()
  calls unregister_hdlc_device(priv->ndev) as its first action.
- [Low] Coding style in the new code: the added
  unregister_hdlc_device(priv->ndev) is inserted directly after the…

Pre-existing issues:
- [Medium] Probe error-unwind asymmetry: the labels reached after
  uhdlc_init() has already succeeded ('free_dev:' at…
- [Low] ucc_hdlc_probe() creates a virq mapping with
  'ut_info->uf_info.irq = irq_of_parse_and_map(np, 0);'…

--

net: wan: fsl_ucc_hdlc: release resources on remove

The patch makes ucc_hdlc_remove() unregister the HDLC netdev, free the
netdev after the hardware teardown, and free the separately allocated
struct ucc_tdm before freeing the private data.  A few questions about the
ordering and about the surrounding teardown paths below.

> diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
> index 809f21fb93f56..ffc8b989e33a7 100644
> --- a/drivers/net/wan/fsl_ucc_hdlc.c
> +++ b/drivers/net/wan/fsl_ucc_hdlc.c
> @@ -1254,6 +1254,7 @@ static int ucc_hdlc_probe(struct platform_device *pdev)
>  static void ucc_hdlc_remove(struct platform_device *pdev)
>  {
>  	struct ucc_hdlc_private *priv = dev_get_drvdata(&pdev->dev);
> +	unregister_hdlc_device(priv->ndev);

[Severity: High]
Can uhdlc_close() be skipped here when the interface is administratively
up at unbind time?

unregister_hdlc_device() in drivers/net/wan/hdlc.c detaches the protocol
before unregistering the netdevice:

void unregister_hdlc_device(struct net_device *dev)
{
	rtnl_lock();
	detach_hdlc_protocol(dev);
	unregister_netdevice(dev);
	rtnl_unlock();
}

detach_hdlc_protocol() ends with hdlc_setup_dev(), which assigns
dev->flags directly:

	dev->flags		 = IFF_POINTOPOINT | IFF_NOARP;

That clears IFF_UP.  netif_close_many() in net/core/dev.c then drops the
device from the close list:

	list_for_each_entry_safe(dev, tmp, head, close_list)
		if (!(dev->flags & IFF_UP))
			list_del_init(&dev->close_list);

so ndo_stop (uhdlc_close()) is not called during unregistration.  Since
uhdlc_close() is the only place that does:

	napi_disable(&priv->napi);
	...
	free_irq(priv->ut_info->uf_info.irq, priv);

does that leave ucc_hdlc_irq_handler() still installed on the virq, with
dev_id == priv, while the code below frees the DMA rings, the netdev and
priv?  The handler starts with:

	struct ucc_hdlc_private *priv = (struct ucc_hdlc_private *)dev_id;
	struct net_device *dev = priv->ndev;

Would it be safer to close the device through the networking core (while
IFF_UP and hdlc->proto are still intact) before calling
unregister_hdlc_device()?  A related side effect is that the
request_irq()/free_irq() pair stays unbalanced, so a re-bind would get
-EBUSY from request_irq() in uhdlc_open() on the same non-shared virq.

Reproducer would be: attach a protocol via SIOCWANDEV, ip link set hdlc0
up, then unbind the platform device via sysfs.

[Severity: Low]
This isn't a bug, but the new call sits directly after the declaration of
priv with no blank line between the declaration and the first statement,
which checkpatch.pl flags as "Missing a blank line after declarations".

>  
>  	uhdlc_memclean(priv);
>  
> @@ -1266,6 +1267,8 @@ static void ucc_hdlc_remove(struct platform_device *pdev)
>  		iounmap(priv->utdm->siram);
>  		priv->utdm->siram = NULL;
>  	}
> +	free_netdev(priv->ndev);
> +	kfree(priv->utdm);
>  	kfree(priv);

[Severity: Medium]
This isn't a problem introduced by this patch, but while the two teardown
sites are being brought into agreement: the probe error labels reached
after uhdlc_init() has already succeeded never call uhdlc_memclean():

free_dev:
	free_netdev(dev);
undo_uhdlc_init:
	if (utdm)
		iounmap(utdm->siram);
unmap_si_regs:
	if (utdm)
		iounmap(utdm->si_regs);
free_utdm:
	if (uhdlc_priv->tsa)
		kfree(utdm);
free_uhdlc_priv:
	kfree(uhdlc_priv);

If alloc_hdlcdev() returns NULL or register_hdlc_device() fails, does the
kfree(uhdlc_priv) drop the only handles to what uhdlc_init() allocated,
i.e. the two coherent BD rings, the coherent RX/TX data buffer, the QE
MURAM parameter area plus the riptr/tiptr MURAM allocations, the
ucc_fast_private from ucc_fast_init(), and the rx_skbuff/tx_skbuff
arrays?

The commit message says:

    The probe failure paths release these resources, but the remove path
    does not.

Is that accurate only for the netdev and the ucc_tdm struct?  The label
name undo_uhdlc_init suggests it undoes uhdlc_init(), while the code
under it does not call uhdlc_memclean().

[Severity: Low]
This is a pre-existing asymmetry rather than something added here, but
ucc_hdlc_probe() creates the virq mapping with:

	ut_info->uf_info.irq = irq_of_parse_and_map(np, 0);

and neither the probe error labels nor ucc_hdlc_remove() call
irq_dispose_mapping().  Should the mapping acquired at probe time be
disposed in the remove path being completed here, given that
request_irq()/free_irq() are balanced by uhdlc_open()/uhdlc_close() but
the mapping itself is not?

>  
>  	dev_info(&pdev->dev, "UCC based hdlc module removed\n");

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916065136.2926089-1-lgs201920130244%40gmail.com