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

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()` incorrec=
tly calls `rcar_canfd_global_deinit()`, causing a kernel panic from accessi=
ng registers with disabled clocks, followed by a double-disable of clocks a=
nd double-assertion of resets.
- [High] If `register_candev()` fails during channel probe, the network dev=
ice is freed, but the global context retains a dangling pointer to it, whic=
h is subsequently dereferenced and double-freed during the driver probe err=
or teardown.
--

--- Patch [15]: [PATCH net 15/20] can: rcar_canfd: change the initializing =
flow for clocks and resets ---
commit 4a859a221c1b4bd83a846c702809583b08a6af18
Author: Tu Nguyen <[email protected]>

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/[email protected]=
as.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_can=
fd_global *gpriv)
> =20
>  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 =3D 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 redundant=
ly
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 seque=
nce
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] =3D priv;
    err =3D 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?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260729102802.5051=
[email protected]?part=3D15