Re: [PATCH net v3 3/5] net: phy: set PHY_READY after LED setup

[Xuanqiang Luo]

在 2026/8/19 14:02, Xuanqiang Luo 写道:
> From: Xuanqiang Luo <luoxuanqiang@xxxxxxxxxx>
>
> phy_probe() sets PHY_READY before calling of_phy_leds(). If LED setup
> fails, the error path releases the initialized resources while the PHY
> state remains READY even though probing failed.
>
> Set PHY_READY only after LED setup succeeds.
>
> Fixes: 01e5b728e9e4 ("net: phy: Add a binding for PHY LEDs")
> Signed-off-by: Xuanqiang Luo <luoxuanqiang@xxxxxxxxxx>
I noticed that Sashiko found two new issues:

> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index 538e9ca44d093..8fd6fbf378f0a 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -3799,9 +3799,6 @@ static int phy_probe(struct device *dev)
> >                   phydev->supported);
> >      }
> >  
> > -    /* Set the state to READY by default */
> > -    phydev->state = PHY_READY;
> > -
> >      /* Register the PHY LED triggers */
> >      if (!phydev->is_on_sfp_module)
> >          phy_led_triggers_register(phydev);
>    
> [Severity: Critical]
> This is a pre-existing issue, but by ignoring the return value of
> phy_led_triggers_register() here, could we trigger a double-free later?
>
> In drivers/net/phy/phy_led_triggers.c, if phy_led_triggers_register() fails,
> it frees phy->phy_led_triggers but leaves it as a dangling pointer and sets
> phy_num_led_triggers = 0. Since the return value is ignored here, phy_probe()
> continues execution.
> 	
> If of_phy_leds() then fails, the code jumps to out_unreg_led_triggers and
> calls phy_led_triggers_unregister(). Inside that function, the loop is
> skipped because phy_num_led_triggers is 0, but it unconditionally calls
> kfree(phy->phy_led_triggers). This results in a double-free of the dangling
> pointer.	

Although this part was not introduced by the current patch, I think it
still falls within the scope of probe failure handling, so I plan to add
a separate patch in the next revision to fix it.

> > @@ -3815,6 +3812,9 @@ static int phy_probe(struct device *dev)
> >  			goto out_unreg_led_triggers;
> >  	}
> >  
> > +	/* Set the state to READY by default */
> > +	phydev->state = PHY_READY;
> > +
>
> [Severity: High]
> Does moving this lockless state assignment to the end of phy_probe() create
> a race condition with concurrent MAC driver attachments?
>
> The driver core sets dev->driver before invoking phy_probe(). While
> phy_probe() is still running and potentially blocked in of_phy_leds(), a
> concurrent network driver probe could find this PHY and call
> phy_attach_direct().
>
> Because dev->driver is already set, phy_attach_direct() proceeds without
> acquiring the PHY's device lock and sets the state to PHY_READY. The MAC
> driver then immediately calls phy_start(), which acquires phydev->lock and
> transitions the state from PHY_READY to PHY_UP.
>
> If this lockless assignment completes after the MAC driver has already called
> phy_start(), won't it silently clobber the PHY_UP state back to PHY_READY and
> stall the PHY state machine?
This appears to be a pre-existing issue, since phy_attach_direct() and
phy_probe() are not serialized.

This patch does widen the race window, but phydev->state should still
be set to PHY_READY before phy_probe() returns.

Thanks,
Xuanqiang