Re: [PATCH net-next v7 06/12] net: Document PCS subsystem

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Error paths in `phylink_create()` free the `phylink` struct without unregistering the PCS notifier.
- [High] The PCS notifier is registered before the `phylink` instance is fully initialized, allowing concurrent notifications to dereference a NULL pointer.
- [Medium] Always-false unsigned comparison in `fwnode_parse_pcsspec()` masks actual error returns.
- [Medium] `fwnode_phylink_pcs_parse()` fails completely if `num_pcs` exceeds the actual number of entries in the device tree.
--

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

net: Document PCS subsystem
    
Add extensive documentation of the new PCS subsystem and the fwnode
implementation with producer/consumer API.

> diff --git a/drivers/net/pcs/pcs.c b/drivers/net/pcs/pcs.c
> new file mode 100644
> index 0000000000000..713b2ec22c97e
> --- /dev/null
> +++ b/drivers/net/pcs/pcs.c
> @@ -0,0 +1,257 @@

[ ... ]

> +static int fwnode_parse_pcsspec(const struct fwnode_handle *fwnode,
> +				unsigned int index, const char *name,
> +				struct fwnode_reference_args *out_args)
> +{
> +	int ret;
> +
> +	if (!fwnode)
> +		return -EINVAL;
> +
> +	if (name) {
> +		index = fwnode_property_match_string(fwnode, "pcs-names",
> +						     name);
> +		if (index < 0)

[Severity: Medium]
Since index is declared as an unsigned int, will this check for < 0 always be
false, potentially masking error returns from fwnode_property_match_string()?

> +			return index;
> +	}
> +
> +	ret = fwnode_property_get_reference_args(fwnode, "pcs-handle",
> +						 "#pcs-cells",
> +						 -1, index, out_args);
> +	if (ret || (name && index < 0))
> +		return ret;
> +
> +	return 0;
> +}

[ ... ]

> +int fwnode_phylink_pcs_parse(struct fwnode_handle *fwnode,
> +			     struct phylink_pcs **available_pcs,
> +			     unsigned int num_pcs)
> +{
> +	unsigned int i, found = 0;
> +
> +	if (!available_pcs)
> +		return -EINVAL;
> +
> +	if (!fwnode_property_present(fwnode, "pcs-handle"))
> +		return -ENODEV;
> +
> +	for (i = 0; i < num_pcs; i++) {
> +		struct phylink_pcs *pcs;
> +
> +		pcs = fwnode_pcs_get(fwnode, i);
> +		if (IS_ERR(pcs)) {
> +			/*
> +			 * Ignore -ENODEV error for PCS that still
> +			 * needs to probe.
> +			 */
> +			if (PTR_ERR(pcs) == -ENODEV)
> +				continue;
> +
> +			return PTR_ERR(pcs);

[Severity: Medium]
If num_pcs exceeds the actual number of entries in the device tree array,
fwnode_pcs_get() will return -ENOENT for the out-of-bounds index.

Will returning the error here cause the entire function to abort instead of
breaking gracefully and returning the valid entries found so far?

> +		}
> +
> +		available_pcs[found] = pcs;
> +		found++;
> +	}
> +
> +	return found;
> +}
> +EXPORT_SYMBOL_GPL(fwnode_phylink_pcs_parse);

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 087ac63f9193d..0734c98498a99 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c

[ ... ]

> @@ -1867,6 +2093,33 @@ struct phylink *phylink_create(struct phylink_config *config,

[ ... ]

> +	/* Register notifier for late PCS attach */
> +	if (!phy_interface_empty(config->pcs_interfaces)) {
> +		pl->fwnode_pcs_nb.notifier_call = pcs_provider_notify;
> +		register_fwnode_pcs_notifier(&pl->fwnode_pcs_nb);
> +	}
>  
>  	pl->config = config;

[Severity: High]
If a PCS provider is registered concurrently right after the notifier is added,
pcs_provider_notify() could execute before pl->config is initialized here.

Will the callback attempt to dereference a NULL pl->config->dev and crash?

[Severity: High]
If an error occurs later in phylink_create() (for example, if
phylink_parse_mode(), phylink_parse_fixedlink(), or phylink_register_sfp()
fail), the function calls kfree(pl) and returns.

Does this error path leave the notifier registered to freed memory, leading
to a use-after-free regression when the next PCS provider is added?

>  	if (config->type == PHYLINK_NETDEV) {

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