Re: [PATCH net-next v11 4/5] net: dsa: motorcomm: Add LED support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Lockless Time-of-Check to Time-of-Use (TOCTOU) race when accessing `dp->user` and `dp->conduit` leads to garbage pointers and Use-After-Free (UAF).
- [Medium] Flawed register state restoration in `yt921x_led_setup()` error path clobbers concurrent modifications and conflates valid register states with sentinels.
--

commit 47253868ab0bfc21ecc3dfb700f5599e47b8967c
Author: David Yang <mmyangfl@gmail.com>
Date:   Mon Aug 31 04:38:57 2026 +0000

    net: dsa: motorcomm: Add LED support

    This patch adds LED support for the Motorcomm YT9215 ethernet switch chip,
    allowing each port to declare up to 3 LEDs using the standard LEDs
    structure. Currently, only parallel mode and strict 1:1 mapping are
    supported.

> diff --git a/drivers/net/dsa/motorcomm/leds.c b/drivers/net/dsa/motorcomm/leds.c
> new file mode 100644
> index 0000000000000..7d3c0f68d8ffb
> --- /dev/null
> +++ b/drivers/net/dsa/motorcomm/leds.c
[ ... ]
> +static struct device * __maybe_unused
> +yt921x_cled_hw_control_get_device(struct led_classdev *led_cdev)
> +{
> +	struct yt921x_led *led = to_yt921x_led(led_cdev);
> +	struct yt921x_port *pp = to_yt921x_port(led);
> +	struct yt921x_priv *priv = to_yt921x_priv(pp);
> +	struct dsa_port *dp;
> +
> +	dp = dsa_to_port(&priv->ds, pp->index);
> +	if (!dp)
> +		return NULL;
> +
> +	if (dsa_port_is_user(dp))
> +		return !dp->user ? NULL : &dp->user->dev;
> +	if (dsa_port_is_cpu(dp))
> +		return !dp->conduit ? NULL : &dp->conduit->dev;

[Severity: High]
Can accessing dp->user and dp->conduit twice without READ_ONCE() or lock
protection lead to a lockless Time-of-Check to Time-of-Use (TOCTOU) race?

If dp->user is concurrently set to NULL by the DSA core during teardown,
the compiler can emit two separate loads. The second load could evaluate
as NULL, yielding an invalid offset pointer (&NULL->dev) and potentially
causing a kernel crash or Use-After-Free.

During the teardown window, dsa_port_teardown() destroys and frees the user
netdev before dsa_switch_teardown() invokes yt921x_dsa_teardown() to
unregister the LEDs.

Would it be safer to read these pointers into local variables once or use
READ_ONCE()?

[ ... ]
> +static int
> +yt921x_led_setup(struct yt921x_priv *priv, int port,
> +		 struct fwnode_handle *fwnode)
> +{
[ ... ]
> +	led2_val = U32_MAX;
> +	res = yt921x_reg_read(priv, YT921X_LED2_PORTn(port), &led2_val);
> +	if (res)
> +		goto revoke_led2;
> +	mask = YT921X_LED2_PORT_FORCEn_M(group);

[Severity: Medium]
Does using U32_MAX as a sentinel here prevent restoration if the hardware
register was legitimately set to 0xFFFFFFFF (all mask bits set)?

[ ... ]
> +revoke:
> +	mutex_lock(&priv->reg_lock);
> +revoke_led2:
> +	if (led2_val != U32_MAX) {
> +		ret = yt921x_reg_write(priv, YT921X_LED2_PORTn(port), led2_val);

[Severity: Medium]
When restoring the register state here, the lock priv->reg_lock was
previously released prior to calling devm_led_classdev_register_ext().

Does the unconditional write-back of led2_val overwrite the entire register,
potentially destroying concurrent sysfs changes made to sibling LED groups
on the same port during the unlocked window?

Could we use yt921x_reg_update_bits() instead of a full register write to
only restore the specific mask bits modified by the failed initialization?

> +		if (ret)
> +			dev_err(dev,
> +				"Failed to restore %s for LED %02d:%02u: %d\n",
> +				"LED2_PORT", port, group, ret);
> +	}
[ ... ]

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