Re: [PATCH net-next v6 03/12] net: phylink: add phylink_release_pcs() to externally release a PCS

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unsynchronized modification of `pl->pcs_list` and TOCTOU access to `pl->pcs` in `phylink_release_pcs()` leads to data races, list corruption, and use-after-free.
- [High] `phylink_release_pcs()` silently fails to remove the PCS from `pl->pcs_list` if the interface is stopped, leading to a Use-After-Free.
- [Medium] Releasing an actively running PCS bypasses `phylink_pcs_disable()`, leaving the hardware permanently enabled.
--

commit ef373ddee33a54496ae5ebacb3521d9195428672
Author: Christian Marangi <ansuelsmth@gmail.com>

net: phylink: add phylink_release_pcs() to externally release a PCS

Add phylink_release_pcs() to externally release a PCS from a phylink
instance. This can be used to handle case when a single PCS needs to be
removed and the phylink instance needs to be refreshed.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 4d6ffda0cdd61..b45bee9273007 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -923,6 +923,55 @@ static void phylink_resolve_an_pause(struct phylink_link_state *state)
>  	}
>  }
>  
> +/**
> + * phylink_release_pcs - Removes a PCS from the phylink PCS available list
> + * @pcs: a pointer to the phylink_pcs struct to be released
> + *
> + * This function release a PCS from the phylink PCS available list if
> + * actually in use. It also refreshes the supported interfaces of the
> + * phylink instance by copying the supported interfaces from the phylink
> + * conf and merging the supported interfaces of the remaining available PCS
> + * in the list and trigger a resolve.
> + */
> +void phylink_release_pcs(struct phylink_pcs *pcs)
> +{
> +	struct phylink *pl;
> +
> +	ASSERT_RTNL();
> +
> +	pl = pcs->phylink;
> +	if (!pl)
> +		return;

[Severity: High]
Does this early return cause a use-after-free if the interface is stopped?

When the interface is brought down, phylink_stop() clears pcs->phylink = NULL
for all available PCS instances, while keeping them linked in pl->pcs_list.

If phylink_release_pcs() is called while the interface is stopped, it encounters
a NULL pointer here and returns immediately without executing
list_del(&pcs->list). The caller likely assumes the PCS was successfully
detached and frees its memory, leaving a dangling pointer in pl->pcs_list. 

When the interface is later brought up, phylink_start() will iterate over
pl->pcs_list and dereference the freed memory.

> +
> +	list_del(&pcs->list);
> +	pcs->phylink = NULL;
> +
> +	/* Check if we are removing the PCS currently
> +	 * in use by phylink. If this is the case,
> +	 * force phylink resolve to reconfigure the interface
> +	 * mode and set the phylink PCS to NULL.
> +	 */
> +	if (pl->pcs == pcs) {

[Severity: High]
Can modifying the list and checking pl->pcs outside the state_mutex race with
the workqueue?

While this function enforces the RTNL lock, pl->pcs_list is concurrently
traversed by phylink_major_config() from the phylink_resolve workqueue, which
holds only pl->state_mutex. Modifying the list outside of pl->state_mutex could
cause the workqueue to dereference LIST_POISON1.

Additionally, checking if (pl->pcs == pcs) before acquiring state_mutex creates
a Time-of-Check to Time-of-Use race. A concurrent phylink_resolve could update
pl->pcs between the lockless check and acquiring state_mutex, causing this code
to inadvertently clear a newly configured PCS.

> +		mutex_lock(&pl->state_mutex);
> +
> +		pl->force_major_config = true;
> +		pl->pcs = NULL;

[Severity: Medium]
If pl->pcs is cleared to NULL here, will the released PCS ever be cleanly
disabled?

During the subsequent reconfiguration triggered by phylink_run_resolve(),
phylink_major_config() detects a PCS change and attempts to disable the old PCS
by calling phylink_pcs_disable(pl->pcs). 

However, since pl->pcs was already cleared to NULL here, the
pcs->ops->pcs_disable() callback will never be invoked for the released PCS,
leaving it hardware-enabled.

> +
> +		mutex_unlock(&pl->state_mutex);
> +	}
> +
> +	/* Refresh supported interfaces */
> +	phy_interface_copy(pl->supported_interfaces,
> +			   pl->config->supported_interfaces);
> +	list_for_each_entry(pcs, &pl->pcs_list, list)
> +		phy_interface_or(pl->supported_interfaces,
> +				 pl->supported_interfaces,
> +				 pcs->supported_interfaces);
> +
> +	phylink_run_resolve(pl);
> +}
> +EXPORT_SYMBOL_GPL(phylink_release_pcs);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609151212.29469-1-ansuelsmth@gmail.com?part=3