Re: [RFC PATCH net-next v8 04/12] net: pcs: implement Firmware node support for PCS driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The PCS provider framework lacks module reference counting, exposing consumers to Use-After-Free crashes during concurrent driver removal.
- [Medium] `fwnode_phylink_pcs_count()` silently truncates the PCS count on Device Tree errors instead of propagating them.
- [Low] Dead code and illogical condition in `fwnode_parse_pcsspec`.
--

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

net: pcs: implement Firmware node support for PCS driver

Implement the foundation of Firmware node support for PCS driver.
This implements a simple Provider API where a PCS driver can
expose multiple PCS with an xlate .get function.

> diff --git a/drivers/net/pcs/pcs.c b/drivers/net/pcs/pcs.c
> new file mode 100644
> index 0000000000000..0cc4daf7beeae
> --- /dev/null
> +++ b/drivers/net/pcs/pcs.c
[ ... ]
> +static int fwnode_parse_pcsspec(const struct fwnode_handle *fwnode,
> +				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)
> +			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;
> +}

[Severity: Low]
Is the condition (name && index < 0) needed here? 

The earlier check:

	if (name) {
		index = fwnode_property_match_string(fwnode, "pcs-names",
						     name);
		if (index < 0)
			return index;
	}

already returns early if name is present and index is less than zero, so
index will always be >= 0 when we reach this final check.

[ ... ]
> +static struct phylink_pcs *
> +fwnode_pcs_get_from_pcsspec(struct fwnode_reference_args *pcsspec)
> +{
> +	struct fwnode_pcs_provider *provider;
> +	struct phylink_pcs *pcs = ERR_PTR(-ENODEV);
> +
> +	if (!pcsspec)
> +		return ERR_PTR(-EINVAL);
> +
> +	mutex_lock(&fwnode_pcs_mutex);
> +	list_for_each_entry(provider, &fwnode_pcs_providers, link) {
> +		if (provider->fwnode == pcsspec->fwnode) {
> +			pcs = provider->get(pcsspec, provider->data);
> +			if (!IS_ERR(pcs))
> +				break;
> +		}
> +	}
> +	mutex_unlock(&fwnode_pcs_mutex);
> +
> +	return pcs;
> +}

[Severity: High]
Is there a way to prevent the provider module from being unloaded while the
consumer is actively using the PCS?

It looks like this framework doesn't capture the module owner (e.g. by passing
THIS_MODULE to fwnode_pcs_add_provider) or use try_module_get() when handing
the pointer to the consumer in fwnode_pcs_get_from_pcsspec().

If a consumer calls fwnode_pcs_get() but hasn't yet linked the PCS to its
pl->pcs_list (meaning pcs->phylink is still NULL), and the provider unbinds
during this window, phylink_release_pcs() might act as a no-op. The provider
could free the PCS memory while the consumer still holds the pointer, leading
to a use-after-free crash.

[ ... ]
> +unsigned int fwnode_phylink_pcs_count(struct fwnode_handle *fwnode)
> +{
> +	struct fwnode_reference_args out_args;
> +	int index = 0;
> +	int ret;
> +
> +	while (true) {
> +		ret = fwnode_property_get_reference_args(fwnode, "pcs-handle",
> +							 "#pcs-cells",
> +							 -1, index, &out_args);
> +		/* We expect to reach an -ENOENT error while counting */
> +		if (ret)
> +			break;
> +
> +		fwnode_handle_put(out_args.fwnode);
> +		index++;
> +	}
> +
> +	return index;
> +}

[Severity: Medium]
Does this function silently truncate the count on malformed device tree nodes?

If fwnode_property_get_reference_args() returns a critical error like -EINVAL,
the loop breaks and returns the current index. Because the function returns an
unsigned int, negative error codes aren't propagated to the caller. 

Could this mask actual configuration errors and cause the MAC driver to proceed
with an incomplete PCS configuration instead of failing the probe?

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