Re: [PATCH net 04/10] net: pse-pd: do not reach a freed PHY from the notification worker

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The comment this patch adds to pse_send_ntf_worker()
  (drivers/net/pse-pd/pse_core.c, lines 1022-1028) says: "A pse_control…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  phy_device_remove() (drivers/net/phy/phy_device.c:1155) calls…

--

net: pse-pd: do not reach a freed PHY from the notification worker

The notification worker used to take a reference on the pse_control. It
now looks up the pse_control under pse_list_mutex and uses
attached_phydev under that mutex, so a removed PHY cannot be reached. If
a second PHY takes a pse_control for a shared PI, attached_phydev is
cleared and a warning is printed.

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index 65a41c517b359..336f00d82b5f8 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c

[ ... ]

> @@ -1020,13 +1019,21 @@ static void pse_send_ntf_worker(struct work_struct *work)
>  		struct net_device *netdev;
>  		struct pse_control *psec;
>  
> -		psec = pse_control_find_by_id(pcdev, ntf.id);
> +		/* A pse_control is only on the list while the PHY it was
> +		 * handed to holds it: the PHY's pse_control_put() unlinks
> +		 * and frees it under pse_list_mutex before the PHY itself
> +		 * can be freed. Holding the mutex across the lookup and the
> +		 * attached_phydev dereference therefore keeps the PHY alive,
> +		 * which a reference on the pse_control alone would not.
> +		 */

[Severity: Low]
This comment says a pse_control is only on the list while the PHY it was
handed to holds it, and that the PHY's pse_control_put() unlinks and frees
it. Does that hold when two PHYs share a PI?

In that case pse_control_get_internal() returns the existing psec after
kref_get(). The unlink and free only happen on the last put:

drivers/net/pse-pd/pse_core.c:__pse_control_release() {
    ...
	list_del(&psec->list);
	kfree(psec);
}

So the first PHY's phy_device_remove()->pse_control_put() only drops the
refcount. The psec stays on pcdev->pse_control_head after that PHY can be
freed.

The worker is still safe today, but only because
pse_control_get_internal() clears psec->attached_phydev when a different
PHY takes the psec. The comment gives list membership as the reason and
does not mention that dependency.

Could the comment say that it relies on attached_phydev being cleared in
the shared-PI case? Later patches in the series leave this comment
unchanged.

[ ... ]

> @@ -1417,6 +1424,19 @@ pse_control_get_internal(struct pse_controller_dev *pcdev, unsigned int index,
>  
>  	list_for_each_entry(psec, &pcdev->pse_control_head, list) {
>  		if (psec->id == index) {
> +			/* attached_phydev can only name one PHY. If a second
> +			 * one shares the PI, either may be removed first and
> +			 * leave it pointing at a freed phy_device, so stop
> +			 * reporting events for this PI instead.
> +			 */
> +			if (psec->attached_phydev &&
> +			    psec->attached_phydev != phydev) {
> +				dev_warn(pcdev->dev,
> +					 "PI %u shared by several PHYs, not reporting its events\n",
> +					 index);
> +				psec->attached_phydev = NULL;
> +			}
> +
>  			kref_get(&psec->refcnt);
>  			return psec;
>  		}

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. This
patch relies on the pse_control_put() in phy_device_remove(). Does that
call leave phydev->psec dangling?

drivers/net/phy/phy_device.c:phy_device_remove() {
	unregister_mii_timestamper(phydev->mii_ts);
	pse_control_put(phydev->psec);

	device_del(&phydev->mdio.dev);
    ...
}

With the usual one PHY per PI, this put drops the last reference and
__pse_control_release() does list_del() and kfree(psec). phydev->psec is
never set to NULL.

The phy_device can outlive phy_device_remove(), because
phy_attach_direct() holds get_device() on it. The MAC<->PHY device_link
is DL_FLAG_PM_RUNTIME | DL_FLAG_STATELESS, so the MAC is not unbound.
phy_remove() also does not detach the netdev. After the MDIO bus is
unregistered or unbound under an attached MAC, netdev->phydev still
points at a phy_device whose psec has been freed.

ETHTOOL_MSG_PSE_GET does not need admin permissions, and it reaches:

pse_prepare_data()
  pse_get_pse_attributes()
    pse_ethtool_get_status(phydev->psec, ...)

net/ethtool/pse-pd.c:pse_get_pse_attributes() {
    ...
	if (!phydev->psec) {
		NL_SET_ERR_MSG(extack, "No PSE is attached");
		return -EOPNOTSUPP;
	}
    ...
	return pse_ethtool_get_status(phydev->psec, extack, &data->status);
}

Can this dereference the freed pse_control? The PSE SET handlers in
net/ethtool/pse-pd.c appear to follow the same pattern.

There may also be a race during removal. pse_control_put() in
phy_device_remove() runs without rtnl, but the ethtool handlers read
phydev->psec under rtnl. Could a concurrent PSE_GET or PSE_SET use the
psec while it is being freed?

Would clearing phydev->psec after the put, synchronized with the
rtnl-protected readers, address both cases? The pse_control_put() call in
phy_device_remove() dates back to commit 5e82147de1cbd7 in 2022. No later
patch in this series touches phy_device_remove() or the ethtool PSE
handlers.

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