Re: [PATCH] net: dpaa: Fix tbidev reference leak in dtsec_initialization()

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] In `dtsec_initialization()`
  (drivers/net/ethernet/freescale/fman/fman_dtsec.c:1506) the patch…

Pre-existing issues:
- [Medium] The dTSEC MAC has no teardown path: `mac_remove()`
  (drivers/net/ethernet/freescale/fman/mac.c:325-336) only drops the…

--

net: dpaa: Fix tbidev reference leak in dtsec_initialization()

The patch adds a put_device() on the TBI PCS mdio device at the end of
dtsec_initialization(), on the grounds that the reference taken by
of_mdio_find_device() is only dropped in dtsec_free() on the error paths.
No other change is made.

> diff --git a/drivers/net/ethernet/freescale/fman/fman_dtsec.c b/drivers/net/ethernet/freescale/fman/fman_dtsec.c
> index b8d70c0ecb6c6..4f1e3900cebdb 100644
> --- a/drivers/net/ethernet/freescale/fman/fman_dtsec.c
> +++ b/drivers/net/ethernet/freescale/fman/fman_dtsec.c
> @@ -1499,6 +1499,12 @@ int dtsec_initialization(struct mac_device *mac_dev,
>  	dev_info(mac_dev->dev, "FMan dTSEC version: 0x%08x\n",
>  		 ioread32be(&dtsec->regs->tsec_id));
>  
> +	/* The PCS device is registered as long as the FMan device, of which
> +	 * mac_dev holds a reference, so the reference taken above is not
> +	 * needed anymore.
> +	 */
> +	put_device(&dtsec->tbidev->dev);
> +

[Severity: High]
Can this put_device() lead to a use-after-free of dtsec->tbidev?

The pointer is not cleared here, and it stays in struct fman_mac and is
dereferenced for the whole lifetime of the interface, including from the
phylink state machine, which polls once per second because
dtsec->pcs.poll = true:

drivers/net/ethernet/freescale/fman/fman_dtsec.c

static void dtsec_pcs_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
				struct phylink_link_state *state)
{
	struct fman_mac *dtsec = pcs_to_dtsec(pcs);

	phylink_mii_c22_pcs_get_state(dtsec->tbidev, neg_mode, state);
}

dtsec_pcs_config() and dtsec_pcs_an_restart() use it the same way, and
dtsec_init() writes MII_TBICON through mdiodev_write(dtsec->tbidev, ...),
so the handle is the access path to the TBI registers rather than a
transient lookup.

of_mdio_find_device() documents the ownership rule this way:

drivers/net/mdio/of_mdio.c

 * If successful, returns a pointer to the mdio_device with the embedded
 * struct device refcount incremented by one, or NULL on failure.
 * The caller should call put_device() on the mdio_device after its use

Is the lifetime claim in the comment and in the commit message ("The
device stays registered for as long as the FMan device, which mac_dev
holds a reference to") accurate?  The TBI mdio_device lives on an MDIO bus
owned by a separate platform device (compatible "fsl,fman-mdio", driven by
the tristate module drivers/net/ethernet/freescale/fsl_pq_mdio.c):

drivers/net/ethernet/freescale/fsl_pq_mdio.c

static void fsl_pq_mdio_remove(struct platform_device *pdev)
{
	...
	mdiobus_unregister(bus);

	iounmap(priv->map);
	mdiobus_free(bus);
}

mdiobus_unregister() then does, for each child:

drivers/net/phy/mdio_bus_provider.c

		mdiodev->device_remove(mdiodev);
		mdiodev->device_free(mdiodev);

which is the final put once the driver's own reference is gone, and
mdio_device_release() kfree()s the object.

The reference mac_dev holds is only a struct device refcount on the FMan
platform device:

drivers/net/ethernet/freescale/fman/mac.c:mac_remove() {
	for (i = 0; i < PORT_NUM; i++)
		put_device(mac_dev->fman_port_devs[i]);
	put_device(mac_dev->fman_dev);
	...
}

Does that pin the descendant MDIO platform device or its bus in any way?
After an unbind of fsl-pq_mdio (or rmmod fsl_pq_mdio), phylink would keep
calling phylink_mii_c22_pcs_get_state(dtsec->tbidev, ...) on freed memory,
and would also walk tbidev->bus whose registers have been iounmap()ed.

Before this change the retained reference kept the object allocated, so
the symptom was a refcount leak rather than a freed dereference.

Would it be better to keep the reference and release it from a real
teardown path, the way fman_memac.c does it in memac_free() via
pcs_put()?

>  	goto _return;
>  
>  _return_fm_mac_free:

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but is it
the reason the reference is never dropped on the success path?

dtsec_free() has exactly one caller, the _return_fm_mac_free label in
dtsec_initialization():

drivers/net/ethernet/freescale/fman/fman_dtsec.c

static int dtsec_free(struct fman_mac *dtsec)
{
	free_init_resources(dtsec);

	kfree(dtsec->dtsec_drv_param);
	dtsec->dtsec_drv_param = NULL;
	if (!IS_ERR_OR_NULL(dtsec->tbidev))
		put_device(&dtsec->tbidev->dev);
	kfree(dtsec);

	return 0;
}

and mac_remove() performs no MAC-specific cleanup at all, so after a
successful probe nothing releases mac_dev->fman_mac.  On unbind the whole
fman_mac allocation, its hash tables and dtsec_drv_param leak, and the
FMan MAC/1588 interrupt callbacks registered through fman_register_intr()
stay installed pointing at the leaked dtsec whose regs mapping
(mac_dev->vaddr) has been unmapped.

fman_memac.c has the same shape, with memac_free() only reachable from the
error label in memac_initialization().

Should the fix add the missing teardown path instead, so the tbidev
reference (and the rest of the state) is released when the MAC goes away?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917110554.2148240-1-vulab%40iscas.ac.cn