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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Dereferencing the uninitialized pointer `pl` in `phylink_create()` causes a kernel panic.
- [High] Error paths in `phylink_create()` fail to unlink PCS objects and unregister notifiers, leading to Use-After-Free and list corruption.
- [Medium] `phylink_validate_mac_and_pcs()` falsely reports an interface as supported when a required PCS is not found.
- [Low] Lockless data race on `pl->pcs` between process context and interrupt context.
--

commit 5bb4efcfcad2f214630e951f75ab343ec05780b5
Author: Christian Marangi <ansuelsmth@gmail.com>
Subject: net: phylink: introduce internal phylink PCS handling

This commit introduces internal handling of PCS for phylink, moving the
selection logic of the PCS entirely to phylink. MAC drivers can now provide
a callback to fill the available PCS into an array which is then parsed
into a linked list.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 4d59c0dd78dbf..cb07184ce82f3 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -531,9 +553,24 @@ static int phylink_validate_mac_and_pcs(struct phylink *pl,
>  		pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
>  		if (IS_ERR(pcs))
>  			return PTR_ERR(pcs);
> +
> +		pcs_found = !!pcs;
> +	/*
> +	 * Find a PCS in available PCS list for the requested interface.
> +	 *
> +	 * Skip searching if the MAC doesn't require a dedicated PCS for
> +	 * the requested interface.
> +	 */
> +	} else if (test_bit(state->interface, pl->config->pcs_interfaces)) {
> +		list_for_each_entry(pcs, &pl->pcs_list, list) {
> +			if (!phylink_validate_pcs_interface(pcs, state->interface)) {
> +				pcs_found = true;
> +				break;
> +			}
> +		}
>  	}
>  
> -	if (pcs) {
> +	if (pcs_found) {

[Severity: Medium]
If the MAC explicitly requires a dedicated PCS for the requested interface
mode but the loop doesn't find a compatible one, pcs_found remains false.
Because the code then skips the pcs_found block and validates only the MAC
capabilities, it seems it could return 0 (success) instead of an error like
-EINVAL.

Does this unintentionally mark the interface as supported despite missing a
required hardware component?

[ ... ]

> @@ -1295,11 +1365,13 @@ static void phylink_major_config(struct phylink *pl, bool restart,
>  	if (pcs_changed) {
>  		phylink_pcs_disable(pl->pcs);
>  
> -		if (pl->pcs)
> -			pl->pcs->phylink = NULL;
> +		if (pl->mac_ops->mac_select_pcs) {
> +			if (pl->pcs)
> +				pl->pcs->phylink = NULL;
>  
> -		if (pcs)
> -			pcs->phylink = pl;
> +			if (pcs)
> +				pcs->phylink = pl;
> +		}
>  
>  		pl->pcs = pcs;

[Severity: Low]
Is it possible for the compiler to optimize this assignment inappropriately?
Since phylink_pcs_change() reads this pointer locklessly from interrupt
context, should we use WRITE_ONCE() here to prevent a potential data race?

[ ... ]

> @@ -1865,6 +1976,16 @@ struct phylink *phylink_create(struct phylink_config *config,
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> +	/*
> +	 * Make sure either PCS internal validation or .mac_select_pcs
> +	 * is used. Return error if both are defined.
> +	 */
> +	if (config->num_possible_pcs && pl->mac_ops->mac_select_pcs) {

[Severity: Critical]
Does this dereference an uninitialized pointer? The local variable pl has not
been allocated yet. Should this use the function argument
mac_ops->mac_select_pcs instead of pl->mac_ops->mac_select_pcs?

> +		dev_err(config->dev,
> +			"phylink: error: either phylink_config .num_possible_pcs or .mac_select_pcs must be used\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>  	pl = kzalloc_obj(*pl);
>  	if (!pl)
>  		return ERR_PTR(-ENOMEM);
> @@ -1872,10 +1993,28 @@ struct phylink *phylink_create(struct phylink_config *config,
>  	mutex_init(&pl->phydev_mutex);
>  	mutex_init(&pl->state_mutex);
>  	INIT_WORK(&pl->resolve, phylink_resolve);
> +	INIT_LIST_HEAD(&pl->pcs_list);
> +
> +	/* Fill the PCS list with available PCS from phylink config */
> +	ret = phylink_fill_available_pcs(pl, config);
> +	if (ret < 0) {
> +		kfree(pl);
> +		return ERR_PTR(ret);
> +	}
> +
> +	/* Link available PCS to phylink */
> +	list_for_each_entry(pcs, &pl->pcs_list, list)
> +		pcs->phylink = pl;
>  
>  	phy_interface_copy(pl->supported_interfaces,
>  			   config->supported_interfaces);
>  
> +	/* Update supported interfaces */
> +	list_for_each_entry(pcs, &pl->pcs_list, list)
> +		phy_interface_or(pl->supported_interfaces,
> +				 pl->supported_interfaces,
> +				 pcs->supported_interfaces);
> +
>  	pl->config = config;
>  	if (config->type == PHYLINK_NETDEV) {
>  		pl->netdev = to_net_dev(config->dev);

[Severity: High]
If the configuration validation fails later in phylink_create() (for example,
if config->type is invalid or phylink_parse_mode() fails), the error paths
call kfree(pl) and return.

Does this leave the previously populated PCS objects with dangling phylink
pointers and corrupted list nodes? If a MAC driver defers probe and retries,
could this result in list corruption or a use-after-free when accessing the
dangling pcs->phylink pointer?

[ ... ]

> @@ -2413,8 +2562,15 @@ void phylink_pcs_change(struct phylink_pcs *pcs, bool up)
>  {
>  	struct phylink *pl = pcs->phylink;
>  
> -	if (pl)
> -		phylink_link_changed(pl, up, "pcs");
> +	/*
> +	 * Ignore PCS link state change if the PCS is not
> +	 * attached to a phylink instance or the phylink
> +	 * instance is not currently using this PCS.
> +	 */
> +	if (!pl || pl->pcs != pcs)
> +		return;

[Severity: Low]
Since pl->pcs is updated in process context without WRITE_ONCE(), and we read
it locklessly here in interrupt context, should this use READ_ONCE(pl->pcs) to
avoid a data race? Even if it's benign, the compiler might perform unexpected
optimizations.

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