Re: [PATCH net-next v18 03/12] net: pcs: implement Firmware node support for PCS driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] net: pcs: fwnode_pcs_get_from_pcsspec() masks xlate errors and bypasses probe deferral
- [High] net: pcs: Use-After-Free during teardown due to missing module refcounting
- [Medium] net: pcs: fwnode_phylink_pcs_parse() ignores unprobed PCS providers and shifts array indices
- [Medium] net: pcs: fwnode_phylink_pcs_count() truncates array count on malformed DT nodes
--

commit 5776ecc6cd389824c086b9dad29a383f13c19ae9
Author: Christian Marangi <ansuelsmth@gmail.com>

net: pcs: implement Firmware node support for PCS driver
    
This commit implements the foundation of Firmware node support for PCS drivers.
It introduces a simple Provider API where a PCS driver can expose multiple PCS
using a .fwnode_xlate function. Consumers can then access the PCS via
fwnode_pcs_get().

> diff --git a/drivers/net/pcs/pcs.c b/drivers/net/pcs/pcs.c
> index 0000000000000..39550d9a12a2a
> --- /dev/null
> +++ b/drivers/net/pcs/pcs.c
> @@ -0,0 +1,245 @@
[ ... ]
> +static struct phylink_pcs *
> +fwnode_pcs_get_from_pcsspec(struct fwnode_reference_args *pcsspec)
> +{
> +	struct fwnode_pcs_provider *provider;
> +	struct phylink_pcs *pcs = NULL;
> +
> +	if (!pcsspec)
> +		return ERR_PTR(-EINVAL);
> +
> +	mutex_lock(&fwnode_pcs_mutex);
> +	list_for_each_entry(provider, &fwnode_pcs_providers, link) {
> +		pcs = __fwnode_pcs_get_from_pcsspec_provider(pcsspec, provider);
> +		if (!IS_ERR(pcs))
> +			break;
> +	}

[Severity: High]
In fwnode_pcs_get_from_pcsspec(), if a matching provider's fwnode_xlate()
callback returns an error pointer like -EPROBE_DEFER, !IS_ERR(pcs) evaluates
to false. Does this cause the loop to continue to the next provider (which
will mismatch on fwnode and return -EINVAL), overwriting the valid error
returned by the matching provider?

> +	mutex_unlock(&fwnode_pcs_mutex);
> +
> +	return !IS_ERR_OR_NULL(pcs) ? pcs : ERR_PTR(-ENODEV);

[Severity: High]
Does this unconditionally clobber any remaining error with -ENODEV? This
appears to completely break probe deferral because -EPROBE_DEFER is lost,
causing consumers to misinterpret the failure as the provider missing.

> +}
[ ... ]
> +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", 0, index,
> +							 &out_args);
> +		/* We expect to reach an -ENOENT error while counting */
> +		if (ret)
> +			break;

[Severity: Medium]
If fwnode_property_get_reference_args() encounters a malformed node in the
middle of the list, it returns a parsing error like -EINVAL. Since the loop
fails to distinguish between the expected end of list (-ENOENT) and a parsing
error, does this blindly break and silently hide malformed DT entries?

Furthermore, because fwnode_phylink_pcs_count() returns an unsigned int, it
cannot propagate negative error codes to the caller. Does this silently
truncate the array length, causing the system to ignore the malformed node
and all subsequent valid nodes without failing the probe or warning?

> +
> +		fwnode_handle_put(out_args.fwnode);
> +		index++;
> +	}
> +
> +	return index;
> +}
[ ... ]
> +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)) {
> +			/* Exit early if no PCS remain.*/
> +			if (PTR_ERR(pcs) == -ENOENT)
> +				break;
> +
> +			/*
> +			 * Ignore -ENODEV error for PCS that still
> +			 * needs to probe.
> +			 */
> +			if (PTR_ERR(pcs) == -ENODEV)
> +				continue;

[Severity: Medium]
When fwnode_pcs_get() returns -ENODEV for a PCS provider that hasn't probed
yet, the code explicitly ignores -ENODEV and executes a continue statement,
skipping the advancement of the found index.

Will any subsequently found valid PCS be placed at a lower index in the
available_pcs array than its actual index in the hardware description? Could
this squashing effect decouple the returned PCS pointers from their physical
layout indices, breaking MAC drivers that rely on array position to map
specific PCS channels to hardware lanes?

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

I also have a question about the teardown ordering introduced in the related
commit "net: phylink: support PCS provider release".

[Severity: High]
The fwnode_pcs_get() API does not appear to take a module reference
(try_module_get()) or create a device link to pin the provider in memory.

While the series attempts to mitigate this for phylink via a notifier, the
teardown ordering in phylink_destroy() and the error path of phylink_create()
unregisters the fwnode_pcs_notify_list notifier BEFORE it clears pl->pcs_list:

    if (pl->fwnode_pcs_nb.notifier_call)
        unregister_fwnode_pcs_notifier(&pl->fwnode_pcs_nb);

    cancel_work_sync(&pl->resolve);
    mutex_lock(&pl->state_mutex);

    list_for_each_entry_safe(pcs, tmp, &pl->pcs_list, list) {
        ...

If the PCS provider device is unbound concurrently (e.g., via sysfs) after the
notifier is unregistered but before phylink_destroy() clears the list, the
provider's devres callbacks would immediately free the PCS memory.

Does this create a use-after-free when phylink_destroy() then iterates over the
freed pcs pointers, modifying pcs->phylink and pcs->list?

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