* Re: [PATCH] net: dpaa2: Fix device reference count leak in MAC endpoint handling
2025-07-15 12:00 [PATCH] net: dpaa2: Fix device reference count leak in MAC endpoint handling Ma Ke
@ 2025-07-15 16:51 ` Simon Horman
0 siblings, 0 replies; 2+ messages in thread
From: Simon Horman @ 2025-07-15 16:51 UTC (permalink / raw)
To: Ma Ke
Cc: ioana.ciornei, andrew+netdev, davem, edumazet, kuba, pabeni,
netdev, linux-kernel, akpm, stable
On Tue, Jul 15, 2025 at 08:00:56PM +0800, Ma Ke wrote:
> The fsl_mc_get_endpoint() function uses device_find_child() for
> localization, which implicitly calls get_device() to increment the
> device's reference count before returning the pointer. However, the
> caller dpaa2_switch_port_connect_mac() and dpaa2_eth_connect_mac()
> fails to properly release this reference in multiple scenarios. We
> should call put_device() to decrement reference count properly.
>
> As comment of device_find_child() says, 'NOTE: you will need to drop
> the reference with put_device() after use'.
>
> Found by code review.
>
> Cc: stable@vger.kernel.org
> Fixes: 719479230893 ("dpaa2-eth: add MAC/PHY support through phylink")
> Fixes: 84cba72956fd ("dpaa2-switch: integrate the MAC endpoint support")
> Signed-off-by: Ma Ke <make24@iscas.ac.cn>
As a fix for Networking code this should be targeted at the net tree.
That can be done like this:
Please also make sure that the patch compiles against the main branch
of the net tree. That doesn't appear to be the case with this patch.
> ---
> drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 15 ++++++++++++---
> .../net/ethernet/freescale/dpaa2/dpaa2-switch.c | 15 ++++++++++++---
As this updates two drivers I have a week preference for two patches.
One with the prefix dpaa2-eth and the other with the prefix dpaa2-switch.
Subject: [PATCH net v2 1/2] dpaa2-eth: ...
Subject: [PATCH net v2 2/2] dpaa2-switch: ...
> 2 files changed, 25 insertions(+), 6 deletions(-)
I looked over fsl_mc_get_endpoint. I see that that it calls
device_find_child() indirectly via fsl_mc_device_lookup(). And I agree that
leaking references is a concern.
But if so, is it not also a concern that fsl_mc_get_endpoint()
can make two successful calls to to_fsl_mc_bus(). That is, the
reference count may be taken twice.
So I wonder if something like the following is appropriate.
(Compile tested only).
diff --git a/drivers/bus/fsl-mc/fsl-mc-bus.c b/drivers/bus/fsl-mc/fsl-mc-bus.c
index 7671bd158545..c1c0a4759c7e 100644
--- a/drivers/bus/fsl-mc/fsl-mc-bus.c
+++ b/drivers/bus/fsl-mc/fsl-mc-bus.c
@@ -943,6 +943,7 @@ struct fsl_mc_device *fsl_mc_get_endpoint(struct fsl_mc_device *mc_dev,
struct fsl_mc_obj_desc endpoint_desc = {{ 0 }};
struct dprc_endpoint endpoint1 = {{ 0 }};
struct dprc_endpoint endpoint2 = {{ 0 }};
+ struct fsl_mc_bus *mc_bus;
int state, err;
mc_bus_dev = to_fsl_mc_device(mc_dev->dev.parent);
@@ -966,6 +967,8 @@ struct fsl_mc_device *fsl_mc_get_endpoint(struct fsl_mc_device *mc_dev,
strcpy(endpoint_desc.type, endpoint2.type);
endpoint_desc.id = endpoint2.id;
endpoint = fsl_mc_device_lookup(&endpoint_desc, mc_bus_dev);
+ if (endpoint)
+ return endpoint;
/*
* We know that the device has an endpoint because we verified by
@@ -973,17 +976,13 @@ struct fsl_mc_device *fsl_mc_get_endpoint(struct fsl_mc_device *mc_dev,
* yet discovered by the fsl-mc bus, thus the lookup returned NULL.
* Force a rescan of the devices in this container and retry the lookup.
*/
- if (!endpoint) {
- struct fsl_mc_bus *mc_bus = to_fsl_mc_bus(mc_bus_dev);
-
- if (mutex_trylock(&mc_bus->scan_mutex)) {
- err = dprc_scan_objects(mc_bus_dev, true);
- mutex_unlock(&mc_bus->scan_mutex);
- }
-
- if (err < 0)
- return ERR_PTR(err);
+ mc_bus = to_fsl_mc_bus(mc_bus_dev);
+ if (mutex_trylock(&mc_bus->scan_mutex)) {
+ err = dprc_scan_objects(mc_bus_dev, true);
+ mutex_unlock(&mc_bus->scan_mutex);
}
+ if (err < 0)
+ return ERR_PTR(err);
endpoint = fsl_mc_device_lookup(&endpoint_desc, mc_bus_dev);
/*
>
> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> index b82f121cadad..f1543039a5b6 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> @@ -4666,12 +4666,19 @@ static int dpaa2_eth_connect_mac(struct dpaa2_eth_priv *priv)
> return PTR_ERR(dpmac_dev);
> }
>
> - if (IS_ERR(dpmac_dev) || dpmac_dev->dev.type != &fsl_mc_bus_dpmac_type)
> + if (IS_ERR(dpmac_dev))
> return 0;
>
> + if (dpmac_dev->dev.type != &fsl_mc_bus_dpmac_type) {
> + put_device(&dpmac_dev->dev);
> + return 0;
> + }
> +
> mac = kzalloc(sizeof(struct dpaa2_mac), GFP_KERNEL);
> - if (!mac)
> + if (!mac) {
> + put_device(&dpmac_dev->dev);
> return -ENOMEM;
> + }
>
> mac->mc_dev = dpmac_dev;
> mac->mc_io = priv->mc_io;
> @@ -4679,7 +4686,7 @@ static int dpaa2_eth_connect_mac(struct dpaa2_eth_priv *priv)
>
> err = dpaa2_mac_open(mac);
> if (err)
> - goto err_free_mac;
> + goto err_put_device;
>
> if (dpaa2_mac_is_type_phy(mac)) {
> err = dpaa2_mac_connect(mac);
> @@ -4703,6 +4710,8 @@ static int dpaa2_eth_connect_mac(struct dpaa2_eth_priv *priv)
>
> err_close_mac:
> dpaa2_mac_close(mac);
> +err_put_device:
> + put_device(&dpmac_dev->dev);
> err_free_mac:
> kfree(mac);
> return err;
I think it would be best to construct the lader of unwind labels
such that they release resources in the reverse order to which
they were taken. This allows the ladder to be used consistently
to release the reources for all cases where that is needed.
E.g. (compile tested only!)
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
index b82f121cadad..2f553336b02f 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
@@ -4666,12 +4666,20 @@ static int dpaa2_eth_connect_mac(struct dpaa2_eth_priv *priv)
return PTR_ERR(dpmac_dev);
}
- if (IS_ERR(dpmac_dev) || dpmac_dev->dev.type != &fsl_mc_bus_dpmac_type)
+
+ if (IS_ERR(dpmac_dev))
return 0;
+ if (dpmac_dev->dev.type != &fsl_mc_bus_dpmac_type) {
+ err = 0;
+ goto out_put_device;
+ }
+
mac = kzalloc(sizeof(struct dpaa2_mac), GFP_KERNEL);
- if (!mac)
- return -ENOMEM;
+ if (!mac) {
+ err = -ENOMEM;
+ goto out_put_device;
+ }
mac->mc_dev = dpmac_dev;
mac->mc_io = priv->mc_io;
@@ -4705,6 +4713,8 @@ static int dpaa2_eth_connect_mac(struct dpaa2_eth_priv *priv)
dpaa2_mac_close(mac);
err_free_mac:
kfree(mac);
+out_put_device:
+ put_device(&dpmac_dev->dev);
return err;
}
...
--
pw-bot: changes-requested
^ permalink raw reply related [flat|nested] 2+ messages in thread