Re: [PATCH v14 05/12] net: phylink: support PCS provider release

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

New issues:
- [Medium] Stale linkmode capabilities when a PCS provider is dynamically deleted.
- [Medium] Data race on the `pcs->phylink` pointer could lead to a NULL pointer dereference.

Pre-existing issues:
- [High] Unchecked error pointer dereference from `mac_select_pcs` in `phylink_inband_caps`.
--

--- Patch [5]: [PATCH v14 05/12] net: phylink: support PCS provider release ---
commit 33b352668c4f60d5ab772e19eb7823565b80a8ab
Author: Christian Marangi <[email protected]>

net: phylink: support PCS provider release
    
Add support for release of fwnode PCS from a PCS provider.
This works by creating a global notifier for the PCS provider and
making each phylink instance that makes use of fwnode subscribe to
this notifier.

The PCS notifier will emit the event FWNODE_PCS_PROVIDER_DEL every time
a new PCS provider is released.

[ ... ]

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 82eae20daed00..967cc919a3656 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c

[ ... ]

> @@ -998,6 +1010,9 @@ static unsigned int phylink_inband_caps(struct phylink *pl,
>  {
>  	struct phylink_pcs *pcs;
>  	bool pcs_found = false;
> +	int ret = 0;
> +
> +	mutex_lock(&pl->state_mutex);
>  
>  	if (pl->mac_ops->mac_select_pcs) {
>  		pcs = pl->mac_ops->mac_select_pcs(pl->config,
> @@ -1013,9 +1028,14 @@ static unsigned int phylink_inband_caps(struct phylink *pl,
>  	}
>  
>  	if (!pcs_found)
> -		return 0;
> +		goto exit;
> +
> +	ret = phylink_pcs_inband_caps(pcs, interface);

[Severity: High]
This is a pre-existing issue, but can mac_select_pcs() return an error pointer
here?

If so, the !!pcs check earlier in the function evaluates to true for error
pointers. Is it possible that phylink_pcs_inband_caps() is then passed an
error pointer, which it unconditionally dereferences?

> +
> +exit:
> +	mutex_unlock(&pl->state_mutex);
>  
> -	return phylink_pcs_inband_caps(pcs, interface);
> +	return ret;
>  }

[ ... ]

> @@ -1937,11 +1962,89 @@ static int phylink_fill_available_pcs(struct phylink *pl,
>  	}
>  
>  out:
> +	mutex_unlock(&pl->state_mutex);
> +
>  	kfree(pcss);
>  
>  	return ret;
>  }
>  
> +static void phylink_del_pcs(struct phylink *pl, struct phylink_pcs *pcs)
> +{
> +	lockdep_assert_held(&pl->state_mutex);
> +
> +	list_del(&pcs->list);
> +	pcs->phylink = NULL;

[Severity: Medium]
Is it possible for a lockless reader like phylink_pcs_change() to access
pcs->phylink concurrently when a provider is removed?

If phylink_pcs_change() reads pcs->phylink without READ_ONCE(), and we do a
plain store here without WRITE_ONCE(), could compiler optimizations lead to a
NULL pointer dereference?

> +
> +	/*
> +	 * Check if we are removing the PCS currently
> +	 * in use by this phylink instance. If this is the case,
> +	 * tear down the link, force phylink resolve to reconfigure the

[ ... ]

> +static int pcs_provider_notify(struct notifier_block *self,
> +			       unsigned long val, void *data)
> +{
> +	struct phylink *pl = container_of(self, struct phylink, fwnode_pcs_nb);

[ ... ]

> +	/* 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);

[Severity: Medium]
When a PCS provider is dynamically deleted and we update
pl->supported_interfaces here, do we also need to recalculate the global
pl->supported and pl->link_config.advertising linkmode masks?

Without calling phylink_validate() to update these masks, is it possible
they retain the stale capabilities of the removed PCS, causing ethtool to
report incorrect capabilities?

> +
> +	mutex_unlock(&pl->state_mutex);
> +
> +	rtnl_unlock();
> +
> +	phylink_run_resolve(pl);
> +
> +	return NOTIFY_OK;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5