Re: [PATCH net-next v6 5/5] net: phy: release phydev->psec from phy_device_remove() again

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:

Critical: 0 · High: 2 (1) · Medium: 2 · Low: 0

- [High] The commit claims that after this change phydev->psec "can no
  longer outlive the PSE controller", but the same freed-array sink…
- [High] phy_device_remove() now drops phydev->psec and releases
  pse_phy_lock() *before* device_del(), i.e. before the phy leaves the…
- [Medium] In-series regression fixed by a later patch of the same
  series, with no attribution.
- [Medium] Memory leak of pse_control on the error path of
  phy_device_register() if device_add() fails.

Pre-existing issues:
- [High] phylib clears only the forward link on detach (phydev->psec =
  NULL in phy_pse_detach_one() and in the newly added block in…

--

net: phy: release phydev->psec from phy_device_remove() again

This moves the final pse_control_put() of phydev->psec out of
phy_device_release() and back into phy_device_remove(), where it now runs
under pse_phy_lock() just before device_del().  The stated reason is that a
device_del()'d but still pinned phy is invisible to the PSE_UNREGISTERED
bus walk, so a deferred put could outlive the PSE controller.

Link: https://lore.kernel.org/netdev/20260703071025.100797-1-pabeni@xxxxxxxxxx/

A few questions below.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index e8d894bbfa7aa..bb120045c406e 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -223,19 +223,8 @@ static void phy_mdio_device_free(struct mdio_device *mdiodev)
>  
>  static void phy_device_release(struct device *dev)
>  {
> -	struct phy_device *phydev = to_phy_device(dev);
> -
> -	/* bus_for_each_dev() holds get_device() across each iteration
> -	 * step, deferring this release callback until any in-flight PSE
> -	 * notifier walk has advanced past this phy. pse_control_put()
> -	 * takes pse_list_mutex, so this path must run in sleepable
> -	 * context.
> -	 */
> -	might_sleep();
> -	pse_control_put(phydev->psec);
> -
>  	fwnode_handle_put(dev->fwnode);
> -	kfree(phydev);
> +	kfree(to_phy_device(dev));
>  }

[Severity: Medium]
With the put gone from the release callback, phy_device_remove() is
the only place phylib ever drops phydev->psec.  Do the error paths of
phy_device_register() still reach it?

As far as I can tell they do not: if device_add() fails,
phy_device_register() logs "failed to add", jumps to out:, asserts the
reset and calls mdiobus_unregister_device(), then returns the error to
the caller, which typically just phy_device_free()s the phy.
phy_device_remove() is never invoked, and phy_device_release() is now
only fwnode_handle_put() plus kfree().

The window matters because device_add() puts the device on the
mdio_bus_type klist in bus_add_device() before dpm_sysfs_add() and the
remaining steps, any of which can still fail and roll back.  While the
phy sits on that klist a concurrent PSE_REGISTERED walk
(bus_for_each_dev(&mdio_bus_type, ..., phy_pse_attach_one) under
pse_phy_lock()) can see it, and phy_try_attach_pse() will set
phydev->psec via of_pse_control_get() because psec is still NULL.  On
the failure return nothing puts that reference: the struct pse_control
stays on pcdev->pse_control_head with attached_phydev pointing at
memory that is about to be kfree()d, and the try_module_get(pcdev->owner)
taken in pse_control_get_internal() is never dropped, pinning the PSE
driver module for the lifetime of the system.  Keeping the put in
phy_device_release() happened to cover this case for free.

This is a cold path, but could the same locked put be factored into a
small helper (say phy_pse_detach(phydev)) and called from both
phy_device_remove() and the out: label of phy_device_register()?  Or
do you consider a put in the release callback still warranted as a
backstop for phys that never reach phy_device_remove() at all?

Related question: if phy_device_register() itself attaches the psec
before any of its remaining failure points, the leak would not even
need a concurrent notifier walk -- can you confirm the ordering there?

[Severity: Medium]
Is the use-after-free described in the changelog reachable in any released
tree?  In the baseline this series applies to, phy_device_remove() already
contained:

	unregister_mii_timestamper(phydev->mii_ts);
	pse_control_put(phydev->psec);

and phy_device_release() had no PSE put at all.  The deferral being undone
here was added earlier in this same submission by "net: phy: own
phydev->psec via PSE notifier and remove fwnode_mdio hook", whose own
changelog says the put "moves from phy_device_remove() to
phy_device_release()".

Should this hunk simply be folded into that patch instead of appearing as a
separate commit?  As posted, the two intermediate commits of the series
carry the live use-after-free on pcdev->pi[] that this commit removes, so
bisecting an unrelated phy or PSE problem into that window hits it.

The two sibling patches are referenced by quoted title only, with no SHA,
no Fixes: tag and no statement that they belong to the same series.  Could
that be made explicit either way, so a maintainer or stable backporter can
tell whether a Fixes:/backport is needed here?  If the commits do stay
separate, the introducing patch's changelog is left permanently documenting
a deferral rationale that no longer matches the tree.

> @@ -1265,6 +1254,16 @@ EXPORT_SYMBOL(phy_device_register);
>  void phy_device_remove(struct phy_device *phydev)
>  {
>  	unregister_mii_timestamper(phydev->mii_ts);
> +
> +	/* Detach synchronously, before the phy leaves the bus, so the put cannot
> +	 * outlive the PSE controller (an off-bus but still-pinned phy is missed by
> +	 * the PSE_UNREGISTERED walk). pse_phy_lock() serialises against that walk.
> +	 */
> +	pse_phy_lock();
> +	pse_control_put(phydev->psec);
> +	phydev->psec = NULL;
> +	pse_phy_unlock();
> +
>  	device_del(&phydev->mdio.dev);

[Severity: High]
Can a concurrent PSE_REGISTERED walk re-attach a psec to this phy after the
put has already run?

The lock is dropped here, and the phy only leaves the mdio_bus_type klist
much later, inside device_del() -> bus_remove_device():

drivers/base/bus.c:bus_remove_device() {
	...
	if (klist_node_attached(&dev->p->knode_bus))
		klist_del(&dev->p->knode_bus);
	...
}

bus_for_each_dev() does not skip devices whose device_del() is in progress,
so throughout kill_device(), BUS_NOTIFY_DEL_DEVICE, dpm_sysfs_remove() and
the sysfs teardown the phy is still reachable from:

phy_pse_notifier_event() {
	case PSE_REGISTERED:
		pse_phy_lock();
		bus_for_each_dev(&mdio_bus_type, NULL, NULL,
				 phy_pse_attach_one);
		pse_phy_unlock();
}

phy_pse_attach_one() -> phy_try_attach_pse() then sees dev->type ==
&mdio_bus_phy_type, of_node still set, and psec == NULL because this hunk
just cleared it:

	if (phydev->psec)
		return;

	psec = of_pse_control_get(np, phydev);
	...
	phydev->psec = psec;

Which path releases that new reference?  phy_pse_detach_one() can no longer
see the phy once it is off the klist, the put in phy_device_remove() has
already run, and phy_device_release() is now just fwnode_handle_put() plus
kfree().  That looks like a leaked struct pse_control left on
pcdev->pse_control_head with attached_phydev pointing at freed memory, plus
the try_module_get(pcdev->owner) reference taken in
pse_control_get_internal(), which would pin the PSE driver module for good.

The changelog says "whichever runs first clears phydev->psec, the other sees
NULL".  Does that hold for PSE_REGISTERED, which creates a reference rather
than clearing one?

Would placing the locked put after device_del(), still synchronously in
phy_device_remove(), close this?  Then either the put wins and the walk
cannot see the off-klist phy, or the walk wins and the put releases whatever
it attached.

[Severity: High]
The changelog states the put "can no longer outlive the PSE controller".  Is
that accurate given the attach side?

pse_controller_unregister() fires the notifier, frees pcdev->pi[], and only
then removes the controller from the list:

drivers/net/pse-pd/pse_core.c:pse_controller_unregister() {
	blocking_notifier_call_chain(&pse_controller_notifier,
				     PSE_UNREGISTERED, pcdev);

	pse_flush_pw_ds(pcdev);
	pse_release_pis(pcdev);
	...
	mutex_lock(&pse_list_mutex);
	list_del(&pcdev->list);
	mutex_unlock(&pse_list_mutex);
}

pse_release_pis() does kfree(pcdev->pi) without clearing pcdev->pi or
pcdev->nr_lines.  pse_phy_lock() is held only for the duration of the
notifier walk inside phy_pse_notifier_event(), which ends before
pse_release_pis() runs, so between the end of the walk and that list_del the
controller is still resolvable:

phy_device_register() -> phy_try_attach_pse() -> of_pse_control_get()

of_pse_match_pi() then reads pcdev->pi[i].np out of the freed array, and
pse_control_get_internal() reads and writes it:

	pcdev->pi[index].admin_state_enabled = ret;
	psec->ps = devm_regulator_get_exclusive(pcdev->dev,
						rdev_get_name(pcdev->pi[index].rdev));

The resulting psec lands in phydev->psec, and its eventual put reaches the
same freed-array dereference the changelog says is eliminated:

drivers/net/pse-pd/pse_core.c:__pse_control_release() {
	if (psec->pcdev->pi[psec->id].admin_state_enabled)
		regulator_disable(psec->ps);
}

The documented contract in include/linux/pse-pd/pse.h says subscribers "must
not acquire new references for it" after PSE_UNREGISTERED, but nothing on
the subscriber side can enforce that while the controller stays on
pse_controller_list.

Would making the controller unresolvable before pse_release_pis() (list_del,
or an unregistering flag tested under pse_list_mutex), and having
pse_release_pis() NULL out pcdev->pi and zero nr_lines, be needed for the
claim to hold?

[Severity: High]
This isn't a bug introduced by this patch, but only the forward link is
cleared here; nothing in phylib ever invalidates the reverse link
psec->attached_phydev.  pse_core sets it once, and only when it allocates a
fresh psec:

drivers/net/pse-pd/pse_core.c:pse_control_get_internal() {
	psec->attached_phydev = phydev;
}

An existing psec at the same PI index is merely kref_get()'d and the new
phydev pointer discarded, so the back-pointer stays bound to whichever phy
attached first and survives that phy's detach.  It is dereferenced here:

drivers/net/pse-pd/pse_core.c:pse_control_get_netdev() {
	ASSERT_RTNL();

	if (!psec || !psec->attached_phydev)
		return NULL;

	return psec->attached_phydev->attached_dev;
}

The NULL check cannot detect a freed phy.  pse_send_ntf_worker() takes a
transient reference and then blocks:

	psec = pse_control_find_by_id(pcdev, ntf.id);
	rtnl_lock();
	netdev = pse_control_get_netdev(psec);

If the phy removal completes in that window (this put drops phylib's
reference while the psec stays alive on the worker's kref, then
phy_mdio_device_free() -> put_device() -> phy_device_release() -> kfree()),
does the worker dereference the freed phy_device and hand a garbage
net_device pointer to ethnl_pse_send_ntf()?

Same window existed with the put in phy_device_release() and in the
baseline, so this is not a new problem, but should phylib also clear
psec->attached_phydev on detach?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906153102.959217-1-github%40szelinsky.de