Re: [PATCH v16 1/5] phy: core: Fix use-after-free in phy_get paths

[Frank Li <Frank.li@oss.nxp.com>]

On Mon, Sep 14, 2026 at 12:37:06PM +0100, Bryan O'Donoghue wrote:
> On 11/09/2026 21:56, Frank Li wrote:
> > suggested subject:
> >
> > phy: core: use phy_provider_mutex protect between _of_phy_get and try_module_get()
>
> A pattern I try to encourage and role-model is making fixes very explicit
> and obvious - frequently I'll ask people to prefix their patches fixing
> things with Fix.

Generally, I got feedback from most maintainer is

	do something to fix ...

"do something" become more important. the most maintaniner dont like simple
said
	- fix build warning
	- simplify code
	- fix UAF
	- fix memory leask

I think it is reasonable, git log --oneline ..., if just list
	"fix build warning", which is hard to locate patch because there
are too much "fix build warning".

>
> I'll split the difference with you since now that I read this submitted
> patch title you're right, it could read better.
>
> "Fix race-condition between _of_phy_get and try_module_get()"

I think it is fine.

> > > Sashiko asked during a patch review if the existing usage pattern had a
> > > race condition; specifically in of_phy_get() if it was possible between
> > > returning from _of_phy_get() and running try_module_get() that a module
> > > might be unbound leading to use-after-free.
> > >
> > > Looking at the code this appears to be so, there is no linkage between the
> > > phy and module under a synchronisation primitive.
> > >
> > > Using the phy_provider_mutex in phy_get() will ensure there is a link between
> > > the returned phy pointer and the module_get() bumping the module reference
> > > count.
> > >
> > > Amend phy_get(), of_phy_get() and devm_of_phy_get_by_index() to fix the
> > > same usage pattern.
> > >
> > > phy_provider_unregister() must take the phy_provider_mutex so amending
> > > phy_get()/of_phy_get() to take that same mutex guarantees there is no
> > > use-after-free.
> > >
> > > Fixes: ff764963479a1 ("drivers: phy: add generic PHY framework")
> > > Cc:stable@vger.kernel.org
> > > Reviewed-by: Loic Poulain<loic.poulain@oss.qualcomm.com>
> > > Signed-off-by: Bryan O'Donoghue<bryan.odonoghue@linaro.org>
> > > ---
> > ...
> > > @@ -678,15 +677,21 @@ struct phy *of_phy_get(struct device_node *np, const char *con_id)
> > >   	if (con_id)
> > >   		index = of_property_match_string(np, "phy-names", con_id);
> > >
> > > +	mutex_lock(&phy_provider_mutex);
> > > +
> > >   	phy = _of_phy_get(np, index);
> > >   	if (IS_ERR(phy))
> > > -		return phy;
> > > +		goto out_unlock;
> > >
> > > -	if (!try_module_get(phy->ops->owner))
> > > -		return ERR_PTR(-EPROBE_DEFER);
> > > +	if (!try_module_get(phy->ops->owner)) {
> > > +		phy = ERR_PTR(-EPROBE_DEFER);
> > > +		goto out_unlock;
> > > +	}
> > why no use auto cleanup guard() for mutex lock?
>
> Technically this fix would apply to kernels predating scoped_guard() -
> kernel 6.5 v Fixes: @ ~ 3.13.

DMA engine already use cleanup. consider the same maintainer, I think it
should be fine

Frank

>
> Not really sure how far back the cherry-pick will work but I'd like to give
> the possibility.
>
> We can always sweep this file to use scoped_guard() on tip-of-tree later.
>
> ---
> bod