Re: [PATCH net-next v6 02/12] net: phylink: introduce internal phylink PCS handling

[sashiko-bot@kernel.org]

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

New issues:
- [High] phylink_stop() permanently deletes PCS-supported interfaces from the phylink mask.
- [High] Inactive PCSs attached to the phylink instance can spuriously trigger link changes.
- [Medium] PCS selection logic inconsistently applies pcs_interfaces, causing validation and configuration mismatches.
- [Low] Mutual exclusion check for PCS methods is placed in a validation loop, causing excessive log spam.

Pre-existing issues:
- [High] Missing cleanup of `pcs->phylink` for legacy `.mac_select_pcs` code paths leads to Use-After-Free.
- [High] Missing `IS_ERR` check on `mac_select_pcs` return value in `phylink_inband_caps`.
--

commit 0ed3c25ac9713ad466d414b45a844576296e0982
Author: Christian Marangi <ansuelsmth@gmail.com>
Subject: net: phylink: introduce internal phylink PCS handling
Summary: This patch introduces internal PCS handling for phylink as an alternative to the MAC .mac_select_pcs callback. It allows MAC drivers to provide an array of available PCSs during creation, which phylink manages as a linked list to dynamically select and validate the appropriate PCS.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 4d59c0dd78dbf..4d6ffda0cdd61 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -518,22 +523,59 @@ static void phylink_validate_mask_caps(unsigned long *supported,
>  	linkmode_and(state->advertising, state->advertising, mask);
>  }
>  
> +static int phylink_validate_pcs_interface(struct phylink_pcs *pcs,
> +					  phy_interface_t interface)
> +{
> +	/* If PCS define an empty supported_interfaces value, assume
> +	 * all interface are supported.
> +	 */
> +	if (phy_interface_empty(pcs->supported_interfaces))
> +		return 0;
> +
> +	/* Ensure that this PCS supports the interface mode */
> +	if (!test_bit(interface, pcs->supported_interfaces))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static int phylink_validate_mac_and_pcs(struct phylink *pl,
>  					unsigned long *supported,
>  					struct phylink_link_state *state)
>  {
> -	struct phylink_pcs *pcs = NULL;
>  	unsigned long capabilities;
> +	struct phylink_pcs *pcs;
> +	bool pcs_found = false;
>  	int ret;
>  
>  	/* Get the PCS for this interface mode */
>  	if (pl->mac_ops->mac_select_pcs) {
> +		/* Make sure either PCS internal validation or .mac_select_pcs
> +		 * is used. Return error if both are defined.
> +		 */
> +		if (!list_empty(&pl->pcs_list)) {
> +			phylink_err(pl, "either phylink_pcs_add() or .mac_select_pcs must be used\n");
> +			return -EINVAL;
> +		}

[Severity: Low]
Is this the best place for this structural check? Since
phylink_validate_mac_and_pcs() is called iteratively for every supported
interface mode during mask validation, won't a misconfigured driver
trigger this error message dozens of times? Could this check be moved to a
one-time initialization path like phylink_create()?

> +
>  		pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
>  		if (IS_ERR(pcs))
>  			return PTR_ERR(pcs);
> +
> +		pcs_found = !!pcs;
> +	} else {
> +		/* Check every assigned PCS and search for one that supports
> +		 * the interface.
> +		 */
> +		list_for_each_entry(pcs, &pl->pcs_list, list) {
> +			if (!phylink_validate_pcs_interface(pcs, state->interface)) {
> +				pcs_found = true;
> +				break;
> +			}
> +		}
>  	}

[Severity: Medium]
Does this validation logic properly account for the pcs_interfaces mask?
In phylink_major_config(), the PCS list is only searched if the interface
requires a dedicated PCS via test_bit(state->interface,
pl->config->pcs_interfaces). Since this loop unconditionally checks the
PCS list, could it incorrectly constrain validation by the PCS capabilities
even if the MAC handles the interface directly?

[ ... ]
> @@ -965,12 +1004,22 @@ static unsigned int phylink_inband_caps(struct phylink *pl,
>  					 phy_interface_t interface)
>  {
>  	struct phylink_pcs *pcs;
> +	bool pcs_found = false;
>  
> -	if (!pl->mac_ops->mac_select_pcs)
> -		return 0;
> +	if (pl->mac_ops->mac_select_pcs) {
> +		pcs = pl->mac_ops->mac_select_pcs(pl->config,
> +						  interface);
> +		pcs_found = !!pcs;

[Severity: High]
This is a pre-existing issue, but does this correctly handle error
pointers returned by mac_select_pcs()? If it returns an ERR_PTR on
failure, !!pcs evaluates to true, causing phylink_pcs_inband_caps() to
dereference the error pointer later in this function. Could we add an
IS_ERR() check here?

> +	} else {
> +		list_for_each_entry(pcs, &pl->pcs_list, list) {
> +			if (!phylink_validate_pcs_interface(pcs, interface)) {
> +				pcs_found = true;
> +				break;
> +			}
> +		}
> +	}

[Severity: Medium]
Similar to phylink_validate_mac_and_pcs(), this iterates the PCS list
without checking if the interface requires a PCS via
pl->config->pcs_interfaces. Is it possible for this to return inaccurate
inband capabilities if a MAC supports an interface directly without using
an available PCS?

[ ... ]
> @@ -1953,10 +2081,16 @@ EXPORT_SYMBOL_GPL(phylink_create);
>   */
>  void phylink_destroy(struct phylink *pl)
>  {
> +	struct phylink_pcs *pcs, *tmp;
> +
>  	sfp_bus_del_upstream(pl->sfp_bus);
>  	if (pl->link_gpio)
>  		gpiod_put(pl->link_gpio);
>  
> +	/* Remove every PCS from phylink PCS list */
> +	list_for_each_entry_safe(pcs, tmp, &pl->pcs_list, list)
> +		list_del(&pcs->list);
> +
>  	cancel_work_sync(&pl->resolve);
>  	kfree(pl);
>  }

[Severity: High]
This is a pre-existing issue, but is it possible for a legacy PCS to
access freed memory here? For MAC drivers using the legacy
.mac_select_pcs callback, phylink_major_config() sets pcs->phylink = pl,
but that pointer is never cleared during teardown. If the PCS driver
outlives the phylink instance and triggers phylink_pcs_change(), it would
dereference the freed phylink pointer.

[ ... ]
> @@ -2437,6 +2571,7 @@ static irqreturn_t phylink_link_handler(int irq, void *data)
>   */
>  void phylink_start(struct phylink *pl)
>  {
> +	struct phylink_pcs *pcs;
>  	bool poll = false;
>  
>  	ASSERT_RTNL();
> @@ -2463,6 +2598,10 @@ void phylink_start(struct phylink *pl)
>  
>  	pl->pcs_state = PCS_STATE_STARTED;
>  
> +	/* link available PCS to phylink struct */
> +	list_for_each_entry(pcs, &pl->pcs_list, list)
> +		pcs->phylink = pl;
> +

[Severity: High]
Does attaching all available PCSs to the phylink instance create a risk
of spurious link changes? Since all PCSs in the list receive the phylink
pointer, any of them can call phylink_pcs_change(). Because
phylink_pcs_change() does not verify if the calling PCS is the actively
selected one (pl->pcs), couldn't an interrupt from an inactive PCS
(like a loss of signal) falsely trigger a link state change and disrupt
the active connection?

[ ... ]
> @@ -2507,6 +2646,8 @@ EXPORT_SYMBOL_GPL(phylink_start);
>   */
>  void phylink_stop(struct phylink *pl)
>  {
> +	struct phylink_pcs *pcs;
> +
>  	ASSERT_RTNL();
>  
>  	if (pl->sfp_bus)
> @@ -2524,6 +2665,14 @@ void phylink_stop(struct phylink *pl)
>  	pl->pcs_state = PCS_STATE_DOWN;
>  
>  	phylink_pcs_disable(pl->pcs);
> +
> +	/* Drop link between phylink and PCS */
> +	list_for_each_entry(pcs, &pl->pcs_list, list)
> +		pcs->phylink = NULL;
> +
> +	/* Restore original supported interfaces */
> +	phy_interface_copy(pl->supported_interfaces,
> +			   pl->config->supported_interfaces);
>  }

[Severity: High]
Will unconditionally clearing the supported interfaces permanently
break PCS capabilities on a link down/up cycle? During phylink_create(),
pl->supported_interfaces aggregates capabilities from all attached PCSs.
When phylink_stop() resets this to the base MAC capabilities, the PCS
capabilities are lost because phylink_start() does not reconstruct the
aggregated mask. Could this prevent the link from coming back up in modes
that rely on a PCS after the interface is bounced?

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