Re: [PATCH net-next 17/37] can: rcar_canfd: Derive max_channels from the device tree

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (2) · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] rcar_canfd_probe() now sets max_channels from
  of_get_child_count(dev->of_node), which is the number of child nodes.
- [Low] The patch removes the per-SoC max_channels from struct
  rcar_canfd_hw_info.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the probe
  error path has a use-after-free and double free.
- [High] This problem wasn't introduced by this patch, but the
  controller-wide IRQ handlers (rcar_canfd_global_interrupt,…

--

can: rcar_canfd: Derive max_channels from the device tree

This commit removes the per-SoC max_channels from struct
rcar_canfd_hw_info. Probe now gets the channel count from
of_get_child_count(), and the value is stored in struct
rcar_canfd_global. All for_each_set_bit() loops over channels_mask now
use gpriv->max_channels as their bound.

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

> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index 74c3c3f20991f..41c3951189490 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c

[ ... ]

> @@ -2145,14 +2139,18 @@ static int rcar_canfd_probe(struct platform_device *pdev)
>  	bool fdmode = true;			/* CAN FD only mode - default */
>  	char name[9] = "channelX";
>  	u32 ch, fcan_freq;
> +	u8 max_channels;
>  	int i;
>
>  	info = of_device_get_match_data(dev);
> +	max_channels = of_get_child_count(dev->of_node);
> +	if (max_channels > RCANFD_NUM_CHANNELS)
> +		return -EINVAL;

[Severity: Low]
With max_channels gone from struct rcar_canfd_hw_info, the only limit
left here is RCANFD_NUM_CHANNELS (8). That is the IP-wide maximum, not
the number of channels a given SoC implements.

Suppose a DT enabled channel2 on a 2-channel part (R-Car Gen3, RZ/G2L,
RZ/G3S, RZ/T2H). Would rcar_canfd_reset_controller() and
rcar_canfd_global_init() then access CCTR/CSTS/RFCC/CFCC/AFL registers
for a channel that does not exist? Probe would then fail after the
CSTS poll times out, with a "reset controller failed" message.

The binding already rejects such DTs for every compatible (for example
"^channel[2-7]$": false). This only affects DTs that fail schema
validation. Is it intended that the driver no longer has a per-SoC
limit?

>
>  	if (of_property_read_bool(dev->of_node, "renesas,no-can-fd"))
>  		fdmode = false;			/* Classical CAN only mode */
>
> -	for (i = 0; i < info->max_channels; ++i) {
> +	for (i = 0; i < max_channels; ++i) {
>  		name[7] = '0' + i;
>  		of_child = of_get_available_child_by_name(dev->of_node, name);

[Severity: Medium]
Does this loop still find every channel node when the channel numbers
have gaps?

of_get_child_count() returns the number of child nodes. The number in
the "channelN" name, however, is the hardware channel index. It becomes
the bit in channels_mask and the ch used for RCANFD_CCTR(ch),
RCANFD_CSTS(ch), fcbase[ch], the RX FIFO index and the AFL rules.

renesas,rcar-canfd.yaml requires only channel0 and channel1 and allows
any subset of channel[2-7].

Take R-Car V4H as an example. It uses rcar_gen4_hw_info, which had
max_channels = 8 before this patch. With only channel0, channel1 and
channel5 in the DT, max_channels becomes 3. This loop never looks up
channel5, so bit 5 is never set in channels_mask. Every later
for_each_set_bit() is also capped at 3.

Hardware channel 5 is then never put into reset mode, its FIFOs and AFL
rule are never configured, and no candev is registered for it. Probe
still succeeds and nothing is logged. Before this patch, channel5 was
found.

All in-tree dtsi files list their channel nodes without gaps. Setting
status = "disabled" keeps the node, so the count stays the same. Only
DTs that omit a middle channel node or remove it with /delete-node/ are
affected. This code is unchanged at the end of the series.

The commit message says "No functional change". Should this loop be
bounded by the highest channel index, or by a per-SoC limit, rather
than by the number of nodes?

[ ... ]

> @@ -2300,7 +2299,7 @@ static int rcar_canfd_probe(struct platform_device *pdev)
>  	if (err)
>  		goto fail_mode;
>
> -	for_each_set_bit(ch, &gpriv->channels_mask, info->max_channels) {
> +	for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels) {
>  		err = rcar_canfd_channel_probe(gpriv, ch, fcan_freq,
>  					       transceivers[ch]);
>  		if (err)
> @@ -2314,7 +2313,7 @@ static int rcar_canfd_probe(struct platform_device *pdev)
>  	return 0;
>
>  fail_channel:
> -	for_each_set_bit(ch, &gpriv->channels_mask, info->max_channels)
> +	for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels)
>  		rcar_canfd_channel_remove(gpriv, ch);

[Severity: High]
This isn't a bug introduced by this patch, but can this cleanup loop
free the failing channel's netdev twice?

rcar_canfd_channel_probe() sets the channel pointer before it calls
register_candev():

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

priv is netdev_priv(ndev). After free_candev(), gpriv->ch[ch] still
holds a non-NULL pointer to freed memory. The fail_channel loop then
visits the same channel, and rcar_canfd_channel_remove() does:

	struct rcar_canfd_channel *priv = gpriv->ch[ch];

	if (priv) {
		unregister_candev(priv->ndev);
		netif_napi_del(&priv->napi);
		free_candev(priv->ndev);
	}

This calls unregister_candev() through freed memory on a netdev that
was never registered. A second free_candev() follows.

The ordering goes back to commit c5048a7b2c23 ("can: rcar_canfd:
rcar_canfd_channel_probe(): register the CAN device when fully ready"),
which moved the gpriv->ch[] assignment ahead of register_candev(). It
is still present at the end of this series.

Would clearing gpriv->ch[priv->channel] on the failure path, or setting
it only after register_candev() succeeds, avoid this?

[ ... ]

> @@ -2327,7 +2326,7 @@ static void rcar_canfd_remove(struct platform_device *pdev)
>  	struct rcar_canfd_global *gpriv = platform_get_drvdata(pdev);
>  	u32 ch;
>
> -	for_each_set_bit(ch, &gpriv->channels_mask, gpriv->info->max_channels) {
> +	for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels) {
>  		rcar_canfd_disable_channel_interrupts(gpriv->ch[ch]);
>  		rcar_canfd_channel_remove(gpriv, ch);
>  	}

[Severity: High]
This is a pre-existing issue, but can the global IRQ handlers
dereference a freed channel while this loop runs?

Four handlers walk every bit in channels_mask: rcar_canfd_global_interrupt(),
rcar_canfd_global_err_interrupt(),
rcar_canfd_global_receive_fifo_interrupt(), and the shared Gen3 handler
rcar_canfd_channel_interrupt(). They use gpriv->ch[ch] with no NULL
check, lock or RCU:

rcar_canfd_handle_global_err() {
	struct rcar_canfd_channel *priv = gpriv->ch[ch];
	struct net_device *ndev = priv->ndev;
	...
	gerfl = rcar_canfd_read(priv->base, RCANFD_GERFL);
	...
}

In this loop, channel 0 is unregistered and freed while channel 1 is
still running.

rcar_canfd_stop() disables only that channel's CCTR interrupts and its
own FIFOs. The global interrupts stay enabled until
rcar_canfd_global_deinit() runs after the loop. The devm IRQs are
released only after remove() returns.

Suppose a frame arrives on channel 1 in that window. The handler starts
at ch = 0 and reads priv->ndev and priv->base from the freed channel 0
priv, in hardirq context.

rcar_canfd_probe() has a similar window. rcar_canfd_global_init()
enables the global interrupts before rcar_canfd_channel_probe() fills
gpriv->ch[]. Once can0 is registered and brought up, traffic can raise
the global IRQ while gpriv->ch[1] is still NULL. The fail_channel path
also frees channels before the global interrupts are disabled.

This patch only changes the loop bound to gpriv->max_channels. The race
is still present at the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009134323.64064-2-mkl%40pengutronix.de