Re: [PATCH net-next v7 09/12] net: phylink: add .pcs_link_down PCS OP

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Data Race and Use-After-Free in `phylink_destroy()` due to incorrect cancellation order
- [Medium] Missing Link Teardown Callback (pcs_link_down) on PCS release
- [Low] Missing Kernel-Doc for `pcs_link_down`
--

commit 33892cef9bd3c637bb8d5d91bf19b186872ff8bf
Author: Christian Marangi <ansuelsmth@gmail.com>

net: phylink: add .pcs_link_down PCS OP

Permit for PCS driver to define specific operation to tear down the link
between the MAC and the PCS. This might be needed for some PCS that reset
counter or require special reset to correctly work if the link needs to be
restored later.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 0734c98498a99..1bedac517d213 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -1736,6 +1742,9 @@ static void phylink_link_down(struct phylink *pl)
>  
>  	pl->mac_ops->mac_link_down(pl->config, pl->act_link_an_mode,
>  				   pl->cur_interface);
> +
> +	phylink_pcs_link_down(pl->pcs);

[Severity: Medium]
Will this callback be missed if a PCS is released?

In phylink_release_pcs(), pl->pcs is disabled and set to NULL while the link
is logically UP:

	if (pl->pcs == pcs) {
		phylink_pcs_disable(pl->pcs);
		pl->force_major_config = true;
		pl->pcs = NULL;
	}

When this triggers phylink_resolve(), which attempts to bring the link down
using phylink_link_down(), pl->pcs is already NULL. The released PCS never
receives its .pcs_link_down() callback, which might break link lifecycle
symmetry and fail to reset internal counters.

> +
>  	phylink_info(pl, "Link is Down\n");
>  }
>  
> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index 15e6b1a39dfe2..eb6c6ca34147f 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
> @@ -528,6 +528,7 @@ struct phylink_pcs {
>   * @pcs_an_restart: restart 802.3z BaseX autonegotiation.
>   * @pcs_link_up: program the PCS for the resolved link configuration
>   *               (where necessary).
> + * @pcs_link_down: tear down link between MAC and PCS.

[Severity: Low]
Is there a missing kernel-doc entry for the function signature of
pcs_link_down?

The @pcs_link_down callback is added here, but the detailed function signature
appears to be missing from the corresponding "#if 0" block in
include/linux/phylink.h (for kernel-doc purposes only), where other PCS
callbacks like pcs_link_up() are fully documented.

>   * @pcs_disable_eee: optional notification to PCS that EEE has been disabled
>   *		     at the MAC.
>   * @pcs_enable_eee: optional notification to PCS that EEE will be enabled at
[ ... ]


[Severity: High]
Does the incorrect cancellation order in phylink_destroy() cause a data race
and use-after-free?

In phylink_destroy(), pl->pcs_list is modified without holding
pl->state_mutex:

	/* Remove every PCS from phylink PCS list */
	list_for_each_entry_safe(pcs, tmp, &pl->pcs_list, list)
		list_del(&pcs->list);

	cancel_work_sync(&pl->resolve);

If the background work pl->resolve is currently running, the worker thread
executes phylink_major_config() and iterates over pl->pcs_list while holding
pl->state_mutex. Because phylink_destroy() modifies the list concurrently
without synchronization and before canceling the work, the worker thread can
traverse corrupted list pointers, resulting in a use-after-free. Should
cancel_work_sync(&pl->resolve) be called before modifying the list?

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