Re: [PATCH net 15/20] can: rcar_canfd: change the initializing flow for clocks and resets

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] If `rcar_canfd_global_init()` fails, `rcar_canfd_probe()` incorrectly calls `rcar_canfd_global_deinit()`, causing a kernel panic from accessing registers with disabled clocks, followed by a double-disable of clocks and double-assertion of resets.
- [High] If `register_candev()` fails during channel probe, the network device is freed, but the global context retains a dangling pointer to it, which is subsequently dereferenced and double-freed during the driver probe error teardown.
--

--- Patch [15]: [PATCH net 15/20] can: rcar_canfd: change the initializing flow for clocks and resets ---
commit 4a859a221c1b4bd83a846c702809583b08a6af18
Author: Tu Nguyen <tu.nguyen.xg@renesas.com>

can: rcar_canfd: change the initializing flow for clocks and resets

This patch updates the initialization order for clocks and resets to match
the hardware manual, ensuring clocks are supplied before resets are
deasserted. It also updates the teardown path in rcar_canfd_global_deinit()
to mirror this new init ordering.

Link: https://patch.msgid.link/20260625135216.130450-1-biju.das.jz@bp.renesas.com

> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
[ ... ]
> @@ -2068,14 +2068,14 @@ static int rcar_canfd_global_init(struct rcar_canfd_global *gpriv)
>  
>  fail_mode:
>  	rcar_canfd_disable_global_interrupts(gpriv);
[ ... ]
> +fail_ram_clk:
> +	clk_disable_unprepare(gpriv->clk_ram);
> +fail_clk:
> +	clk_disable_unprepare(gpriv->clkp);
>  	return err;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does the probe error path
cause a panic and double-disable of clocks here?

When rcar_canfd_global_init() fails, it correctly cleans up its state and
disables the clocks. However, in rcar_canfd_probe(), the failure jumps to
the fail_mode label:

rcar_canfd_probe() {
    ...
    err = rcar_canfd_global_init(gpriv);
    if (err)
        goto fail_mode;
    ...
fail_mode:
    rcar_canfd_global_deinit(gpriv, false);
    ...
}

Since the clocks were already disabled by the initialization failure path,
calling rcar_canfd_global_deinit() will access hardware registers with
disabled clocks via rcar_canfd_disable_global_interrupts(). Can this trigger
a precise or imprecise external abort? It also looks like it will redundantly
assert the resets and disable the clocks a second time.

[Severity: High]
This is also a pre-existing issue, but could this probe error cleanup sequence
trigger a double-free on a channel probe failure?

In rcar_canfd_channel_probe(), the device pointer is saved to the global
array before registration:

rcar_canfd_channel_probe() {
    ...
    gpriv->ch[priv->channel] = priv;
    err = register_candev(ndev);
    if (err) {
        ...
        goto fail_candev;
    }
    ...
fail:
    free_candev(ndev);
}

If register_candev() fails, the memory is freed but the dangling pointer
remains in the global array. Back in rcar_canfd_probe(), the error is caught
and rcar_canfd_channel_remove() is called for all channels in the mask. Does
the cleanup loop find this dangling pointer and try to operate on the
already-freed network device, causing a use-after-free and double-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729102802.505168-1-mkl@pengutronix.de?part=15