Re: [PATCH net-next v10 06/11] net: Document PCS subsystem

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Race condition between `phylink_release_pcs` and `phylink_create`/`phylink_destroy` leading to list corruption and use-after-free.
--

--- Patch [6]: [PATCH net-next v10 06/11] net: Document PCS subsystem ---
commit 71f5e420c5ee13d83f4218aff0877aa80d862ffc
Author: Christian Marangi <ansuelsmth@gmail.com>

net: Document PCS subsystem

This commit adds extensive documentation for the new PCS subsystem and its
fwnode implementation using a producer/consumer API. It also updates the
sfp-phylink migration guide.

> diff --git a/Documentation/networking/pcs.rst b/Documentation/networking/pcs.rst
> new file mode 100644
> index 0000000000000..e16d8b5bf6612
> --- /dev/null
> +++ b/Documentation/networking/pcs.rst

[ ... ]

> +You must call :c:func:`fwnode_pcs_del_provider` from your remove function and
> +release the PCS from any phylink instance under RTNL lock with
> +:c:func:`phylink_release_pcs`::
> +
> +   fwnode_pcs_del_provider(dev_fwnode(&pdev->dev));
> +
> +	rtnl_lock();
> +
> +	for (i = 0; i < data->num_port; i++) {
> +		struct pcs_port *port = &priv->ports[i];
> +
> +		phylink_release_pcs(&port->pcs);

[Severity: High]
Does this documentation example encourage a teardown pattern that can cause a
use-after-free and list corruption?

If a PCS provider module is removed concurrently with a MAC driver probe (which
calls phylink_create()) or remove (which calls phylink_destroy()), and the PCS
driver calls phylink_release_pcs() as shown here, they can race.

For instance, phylink_release_pcs() accesses pcs->phylink and takes
pl->state_mutex. If phylink_destroy() runs concurrently, it could free pl
just before mutex_lock(), causing a use-after-free.

Also, phylink_create() iterates pl->pcs_list without holding pl->state_mutex.
A concurrent phylink_release_pcs() will modify the list, causing list
corruption. Similarly, phylink_destroy() modifies the list and clears
pcs->phylink without holding pl->state_mutex.

Because fw_devlink does not natively parse pcs-handle to enforce ordering,
the driver core does not prevent these modules from being unloaded
simultaneously.

Is there a safer teardown sequence that should be documented here instead?

> +	}
> +
> +	rtnl_unlock();
> +

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