Re: [PATCH net-next v7 07/11] net: pcs: xpcs: add Rockchip RK3568 platform glue driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net: pcs: use-after-free in xpcs_rk_create() during concurrent supplier unbind
- [Low] kconfig: unmet dependency warning due to PM_GENERIC_DOMAINS select without PM
--

commit 7e2d67535ce6f3173f2984680b0a77b61caaf964
Author: Coia Prant <coiaprant@gmail.com>

net: pcs: xpcs: add Rockchip RK3568 platform glue driver

This commit adds a platform glue driver for the Synopsys DesignWare XPCS
on Rockchip RK3568 SoCs. It implements a virtual MDIO bus to translate
MDIO operations into APB3 memory-mapped register accesses.

Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 59, CRU_CLKSEL_CON29)
Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part2%20V1.1-20210301.pdf (Page 2078)

> diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
> --- a/drivers/net/pcs/Kconfig
> +++ b/drivers/net/pcs/Kconfig
[ ... ]
> +config PCS_XPCS_ROCKCHIP
> +	tristate "Rockchip XPCS controller support"
> +	default ARCH_ROCKCHIP
> +	depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
> +	select GENERIC_PHY
> +	select PM_GENERIC_DOMAINS

[Severity: Low]
Will this cause a Kconfig unmet direct dependency warning during compilation
when PM is not enabled?

Because PCS_XPCS_ROCKCHIP has a COMPILE_TEST fallback in its depends on
list, it can be enabled on architectures without CONFIG_PM. Forcefully
selecting PM_GENERIC_DOMAINS (which depends on PM) without inheriting
that dependency could lead to a configuration mismatch.

Could this Kconfig entry add a dependency on PM, or only conditionally
select PM_GENERIC_DOMAINS?

> diff --git a/drivers/net/pcs/pcs-xpcs-rk.c b/drivers/net/pcs/pcs-xpcs-rk.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/net/pcs/pcs-xpcs-rk.c
[ ... ]
> +	/*
> +	 * Pin the supplier before reading its drvdata: device_link_add()
> +	 * refuses to create a managed link while the supplier is being
> +	 * unbound, so if it succeeds the drvdata cannot be freed under us.
> +	 * The link is released automatically when the consumer device is
> +	 * destroyed (DL_FLAG_AUTOREMOVE_CONSUMER), which covers all probe
> +	 * failure paths, so no explicit device_link_remove() is needed.
> +	 */
> +	if (!device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER)) {
> +		put_device(&pdev->dev);
> +		return ERR_PTR(-EPROBE_DEFER);
> +	}
> +
> +	pxpcs = platform_get_drvdata(pdev);
> +	if (!pxpcs || !pxpcs->bus) {

[Severity: High]
Is this sequence in xpcs_rk_create() safe against a concurrent supplier
unbind?

The comment assumes device_link_add() returns NULL if the supplier is
unbinding, but it actually succeeds and creates a link with a state like
DL_STATE_SUPPLIER_UNBIND or DL_STATE_DORMANT. 

If userspace concurrently unbinds the XPCS supplier device while this
consumer MAC driver is probing, reading the drvdata via
platform_get_drvdata(pdev) might yield a pointer to devres memory that
has already been freed by the supplier's remove path.

Should this code acquire the device lock on the supplier, or otherwise
verify the device is fully bound before dereferencing its drvdata?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917204624.2427486-1-coiaprant@gmail.com?part=7

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy