Re: [PATCH net-next v6 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] Unprotected list modification and iteration in phylink PCS list handling causes race conditions.
- [Medium] `fwnode_pcs_get_from_pcsspec()` incorrectly returns `-ENODEV` instead of `-EPROBE_DEFER` when a provider is not found.
- [Low] `fwnode_parse_pcsspec()` passes a negative error code as an unsigned index, masking the original error and causing out-of-bounds lookups.
--

commit 436ed2d38da7824de31cb0cb85a006f98cbdeb4b
Author: Christian Marangi <ansuelsmth@gmail.com>

net: pcs: implement Firmware node support for PCS driver

This commit implements the foundation for firmware node support in PCS drivers.
It introduces a simple provider API where a PCS driver can expose multiple PCS
instances with a get function. It also adds a generic parsing function for MAC
drivers to declare PCS in DT or ACPI.

> diff --git a/drivers/net/pcs/pcs.c b/drivers/net/pcs/pcs.c
> new file mode 100644
> index 0000000000000..72f56f55d198d
> --- /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 -ENOENT;
> +
> +	if (name)
> +		index = fwnode_property_match_string(fwnode, "pcs-names",
> +						     name);
> +
> +	ret = fwnode_property_get_reference_args(fwnode, "pcs-handle",
> +						 "#pcs-cells",
> +						 -1, index, out_args);

[Severity: Low]
If name is provided but fwnode_property_match_string() fails, it returns a
negative error code. Are we passing this negative value directly into
fwnode_property_get_reference_args(), which expects an unsigned int?

This could wrap around to a large positive integer, causing an out-of-bounds
fetch attempt that internally fails with -ENOENT. The original error code
would therefore be masked by -ENOENT.

Should this function check if (index < 0) return index; before proceeding?

> +	if (ret || (name && index < 0))
> +		return ret;
> +
> +	return 0;
> +}
> +
> +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);

[Severity: Medium]
As stated in the kernel doc for fwnode_pcs_get(), it must return
-EPROBE_DEFER if the PCS is not present in the global providers list.
Is it intentional that fwnode_pcs_get_from_pcsspec() defaults to returning
-ENODEV here when a provider is not found?

This could break the standard kernel probe deferral mechanism for the consumer
driver, preventing it from deferring its probe to wait for the PCS driver to
load if fw_devlink is disabled or bypassed.

> +
> +	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;
> +}

Regarding the integration of this feature with the phylink subsystem, is there a
risk of unprotected list modification and iteration causing a race condition?

[Severity: High]
The pl->pcs_list appears to be protected by mismatched locks. In
phylink_release_pcs() and pcs_provider_notify(), list modifications are
performed under rtnl_lock() but without holding pl->state_mutex:

drivers/net/phy/phylink.c:phylink_release_pcs() {
    ...
    list_del(&pcs->list);
    ...
}

Concurrently, the phylink_resolve() workqueue executes
phylink_major_config(), which iterates over pl->pcs_list using
list_for_each_entry() while holding pl->state_mutex but without holding
rtnl_lock():

drivers/net/phy/phylink.c:phylink_major_config() {
    ...
    list_for_each_entry(pcs, &pl->pcs_list, list) {
    ...
}

Since list modification and iteration are not synchronized by a common lock,
could this lead to linked-list corruption, torn reads, and use-after-free
crashes if the workqueue accesses a PCS that is being removed?

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