* [PATCH] net: dpaa2: Fix device reference count leak in MAC endpoint handling
@ 2025-07-15 12:00 Ma Ke
2025-07-15 16:51 ` Simon Horman
0 siblings, 1 reply; 2+ messages in thread
From: Ma Ke @ 2025-07-15 12:00 UTC (permalink / raw)
To: ioana.ciornei, andrew+netdev, davem, edumazet, kuba, pabeni
Cc: netdev, linux-kernel, akpm, Ma Ke, stable
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>
---
drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 15 ++++++++++++---
.../net/ethernet/freescale/dpaa2/dpaa2-switch.c | 15 ++++++++++++---
2 files changed, 25 insertions(+), 6 deletions(-)
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;
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
index 147a93bf9fa9..6bf1c164129a 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-switch.c
@@ -1448,12 +1448,20 @@ static int dpaa2_switch_port_connect_mac(struct ethsw_port_priv *port_priv)
if (PTR_ERR(dpmac_dev) == -EPROBE_DEFER)
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(*mac), GFP_KERNEL);
- if (!mac)
+ if (!mac) {
+ put_device(&dpmac_dev->dev);
return -ENOMEM;
+ }
mac->mc_dev = dpmac_dev;
mac->mc_io = port_priv->ethsw_data->mc_io;
@@ -1461,7 +1469,7 @@ static int dpaa2_switch_port_connect_mac(struct ethsw_port_priv *port_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);
@@ -1481,6 +1489,8 @@ static int dpaa2_switch_port_connect_mac(struct ethsw_port_priv *port_priv)
err_close_mac:
dpaa2_mac_close(mac);
+err_put_device:
+ put_device(&dpmac_dev->dev);
err_free_mac:
kfree(mac);
return err;
--
2.25.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* 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
end of thread, other threads:[~2025-07-15 16:51 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).