From: Qingfang Deng <qingfang.deng@siflower.com.cn>

A network restart test on a router led to an out-of-memory condition,
which was traced to a memory leak in the PHY LED trigger code.

The root cause is misuse of the devm API. The registration function
(phy_led_triggers_register) is called from phy_attach_direct, not
phy_probe, and the unregister function (phy_led_triggers_unregister)
is called from phy_detach, not phy_remove. This means the register and
unregister functions can be called multiple times for the same PHY
device, but devm-allocated memory is not freed until the driver is
unbound.

This also prevents kmemleak from detecting the leak, as the devm API
internally stores the allocated pointer.

Fix this by replacing devm_kzalloc/devm_kcalloc with standard
kzalloc/kcalloc, and add the corresponding kfree calls in the unregister
path.

Fixes: 3928ee6485a3 ("net: phy: leds: Add support for "link" trigger")
Fixes: 2e0bc452f472 ("net: phy: leds: add support for led triggers on phy link state change")
Signed-off-by: Hao Guan <hao.guan@siflower.com.cn>
Signed-off-by: Qingfang Deng <qingfang.deng@siflower.com.cn>
---
 drivers/net/phy/phy_led_triggers.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/net/phy/phy_led_triggers.c b/drivers/net/phy/phy_led_triggers.c
index bd3c9554f6ac..60893691d4c3 100644
--- a/drivers/net/phy/phy_led_triggers.c
+++ b/drivers/net/phy/phy_led_triggers.c
@@ -93,9 +93,8 @@ int phy_led_triggers_register(struct phy_device *phy)
 	if (!phy->phy_num_led_triggers)
 		return 0;
 
-	phy->led_link_trigger = devm_kzalloc(&phy->mdio.dev,
-					     sizeof(*phy->led_link_trigger),
-					     GFP_KERNEL);
+	phy->led_link_trigger = kzalloc(sizeof(*phy->led_link_trigger),
+					GFP_KERNEL);
 	if (!phy->led_link_trigger) {
 		err = -ENOMEM;
 		goto out_clear;
@@ -105,10 +104,9 @@ int phy_led_triggers_register(struct phy_device *phy)
 	if (err)
 		goto out_free_link;
 
-	phy->phy_led_triggers = devm_kcalloc(&phy->mdio.dev,
-					    phy->phy_num_led_triggers,
-					    sizeof(struct phy_led_trigger),
-					    GFP_KERNEL);
+	phy->phy_led_triggers = kcalloc(phy->phy_num_led_triggers,
+					sizeof(struct phy_led_trigger),
+					GFP_KERNEL);
 	if (!phy->phy_led_triggers) {
 		err = -ENOMEM;
 		goto out_unreg_link;
@@ -129,11 +127,11 @@ int phy_led_triggers_register(struct phy_device *phy)
 out_unreg:
 	while (i--)
 		phy_led_trigger_unregister(&phy->phy_led_triggers[i]);
-	devm_kfree(&phy->mdio.dev, phy->phy_led_triggers);
+	kfree(phy->phy_led_triggers);
 out_unreg_link:
 	phy_led_trigger_unregister(phy->led_link_trigger);
 out_free_link:
-	devm_kfree(&phy->mdio.dev, phy->led_link_trigger);
+	kfree(phy->led_link_trigger);
 	phy->led_link_trigger = NULL;
 out_clear:
 	phy->phy_num_led_triggers = 0;
@@ -147,8 +145,13 @@ void phy_led_triggers_unregister(struct phy_device *phy)
 
 	for (i = 0; i < phy->phy_num_led_triggers; i++)
 		phy_led_trigger_unregister(&phy->phy_led_triggers[i]);
+	kfree(phy->phy_led_triggers);
+	phy->phy_led_triggers = NULL;
 
-	if (phy->led_link_trigger)
+	if (phy->led_link_trigger) {
 		phy_led_trigger_unregister(phy->led_link_trigger);
+		kfree(phy->led_link_trigger);
+		phy->led_link_trigger = NULL;
+	}
 }
 EXPORT_SYMBOL_GPL(phy_led_triggers_unregister);
-- 
2.43.0

On Thu, Apr 17, 2025 at 11:25:56AM +0800, Qingfang Deng wrote:
> From: Qingfang Deng <qingfang.deng@siflower.com.cn>
> 
> A network restart test on a router led to an out-of-memory condition,
> which was traced to a memory leak in the PHY LED trigger code.
> 
> The root cause is misuse of the devm API. The registration function
> (phy_led_triggers_register) is called from phy_attach_direct, not
> phy_probe, and the unregister function (phy_led_triggers_unregister)
> is called from phy_detach, not phy_remove. This means the register and
> unregister functions can be called multiple times for the same PHY
> device, but devm-allocated memory is not freed until the driver is
> unbound.
> 
> This also prevents kmemleak from detecting the leak, as the devm API
> internally stores the allocated pointer.
> 
> Fix this by replacing devm_kzalloc/devm_kcalloc with standard
> kzalloc/kcalloc, and add the corresponding kfree calls in the unregister
> path.
> 
> Fixes: 3928ee6485a3 ("net: phy: leds: Add support for "link" trigger")
> Fixes: 2e0bc452f472 ("net: phy: leds: add support for led triggers on phy link state change")
> Signed-off-by: Hao Guan <hao.guan@siflower.com.cn>
> Signed-off-by: Qingfang Deng <qingfang.deng@siflower.com.cn>

Thanks for the fix. I agree with Maxime, this looks correct.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

The use of devm_free() should trigger any reviewer to take a closer
look because it generally means something is wrong.

    Andrew

On Thu, 17 Apr 2025 11:25:56 +0800
Qingfang Deng <dqfext@gmail.com> wrote:

> From: Qingfang Deng <qingfang.deng@siflower.com.cn>
> 
> A network restart test on a router led to an out-of-memory condition,
> which was traced to a memory leak in the PHY LED trigger code.
> 
> The root cause is misuse of the devm API. The registration function
> (phy_led_triggers_register) is called from phy_attach_direct, not
> phy_probe, and the unregister function (phy_led_triggers_unregister)
> is called from phy_detach, not phy_remove. This means the register and
> unregister functions can be called multiple times for the same PHY
> device, but devm-allocated memory is not freed until the driver is
> unbound.

Are there historical reasons for the triggers not to be registered at
probe time ? I agree with your analysis otherwise.

Maxime

Hi Maxime,

On Thu, Apr 17, 2025 at 3:38 PM Maxime Chevallier
<maxime.chevallier@bootlin.com> wrote:
>
> On Thu, 17 Apr 2025 11:25:56 +0800
> Qingfang Deng <dqfext@gmail.com> wrote:
>
> > From: Qingfang Deng <qingfang.deng@siflower.com.cn>
> >
> > A network restart test on a router led to an out-of-memory condition,
> > which was traced to a memory leak in the PHY LED trigger code.
> >
> > The root cause is misuse of the devm API. The registration function
> > (phy_led_triggers_register) is called from phy_attach_direct, not
> > phy_probe, and the unregister function (phy_led_triggers_unregister)
> > is called from phy_detach, not phy_remove. This means the register and
> > unregister functions can be called multiple times for the same PHY
> > device, but devm-allocated memory is not freed until the driver is
> > unbound.
>
> Are there historical reasons for the triggers not to be registered at
> probe time ? I agree with your analysis otherwise.

I'm not sure exactly, but both register and unregister are called
under a condition:

-  if (!phydev->is_on_sfp_module)

Which may not be available at probe time.


>
> Maxime