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

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

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

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 takes the channel bound from
  of_get_child_count(dev->of_node).
- [Low] The patch removes the per-SoC channel limit
  (rcar_canfd_hw_info.max_channels).
- [Low] In rcar_canfd_probe(), the int returned by of_get_child_count()
  is stored in `u8 max_channels` before the `> RCANFD_NUM_CHANNELS`…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is in the
  error-path loop the patch edits.
- [High] This problem wasn't introduced by this patch, but it is in the
  loops the patch edits.

--

can: rcar_canfd: Derive max_channels from the device tree

This commit removes the hardcoded per-SoC max_channels from struct
rcar_canfd_hw_info. Probe now computes it with of_get_child_count() on
the controller node and stores it 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 (local)

[ ... ]

[Severity: Low]

This isn't a bug for binding-valid DTs, but of_get_child_count() returns
an int, and the result is stored in the u8 max_channels before the range
check. With 256 or more children the comparison sees the value modulo 256
(256 becomes 0, 260 becomes 4), so the check would not reject such a node.

The binding only allows "^channel[0-7]$" children and sets
unevaluatedProperties: false. The truncated value is also still bounded
by RCANFD_NUM_CHANNELS before it is used as an index bound, so nothing is
accessed out of bounds.

Would it be cleaner to keep the result in an int until after the
comparison?

[Severity: Low]

With max_channels gone from struct rcar_canfd_hw_info, this check against
RCANFD_NUM_CHANNELS (8) is the only limit left. It is the family-wide
maximum, not the channel count of the specific SoC.

Suppose a DT declared channel nodes the IP doesn't have, for example
channel2..channel7 on a 2-channel Gen3, RZ/G2L or RZ/T2H part. Then
rcar_canfd_reset_controller(), rcar_canfd_configure_controller() and
rcar_canfd_global_init() would program and poll the CCTR/CSTS,
fcbase[ch].cfdcfg, FIFO and AFL registers of channels that don't exist.
Before this patch those nodes were never looked up.

The binding rejects these layouts per compatible ("^channel[2-7]$": false
and similar), so this only affects DTs that violate the schema.

Separately, this -EINVAL path prints nothing. Could it report the reason,
for example with dev_err_probe()?

[Severity: Medium]

of_get_child_count() returns how many child nodes there are, not the
highest channel index plus one. Can channels be missed when the DT
numbering has a gap?

renesas,rcar-canfd.yaml matches "^channel[0-7]$" and only requires
channel0 and channel1, so a gap looks valid. Take a Gen4 DT with
channel0, channel1 and channel4, or a board that removes channel2 with
/delete-node/. Either way the child count is 3.

The loop above then only looks up channel0 to channel2, so channel4's bit
is never set in channels_mask. Every later for_each_set_bit() loop is
also capped by gpriv->max_channels. As a result channel4 is never reset
or configured, never registered as a netdev and never serviced by the IRQ
handlers. No warning is printed.

Before this patch the bound was info->max_channels (8 on Gen4, 6 on
RZ/G3E), so such a channel was found and probed. Does that fit with the
"No functional change" statement in the commit message?

All in-tree SoC dtsi files appear to use contiguous numbering, so only
out-of-tree DTBs or overlays would be affected.

[ ... ]

[Severity: High]

This is a pre-existing issue and was not introduced by this patch, but
since this unwind loop is being changed: if register_candev() fails,
could the channel's net_device be freed twice?

rcar_canfd_channel_probe() publishes the pointer before it registers the
device:

drivers/net/can/rcar/rcar_canfd.c:rcar_canfd_channel_probe() {
    ...
	gpriv->ch[priv->channel] = priv;
	err = register_candev(ndev);
	if (err) {
		dev_err(dev, "register_candev() failed: %pe\n", ERR_PTR(err));
		goto fail_candev;
	}
    ...
fail_candev:
	netif_napi_del(&priv->napi);
fail:
	free_candev(ndev);
	return err;
}

When rcar_canfd_probe() reaches fail_channel, gpriv->ch[ch] still points
into the freed ndev. This loop also calls rcar_canfd_channel_remove() for
the channel that failed, and that function only checks for NULL:

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

So priv->ndev is read from freed memory, unregister_candev() is called on
a netdev that was never registered, and free_candev() runs a second time.

Would it work to set gpriv->ch[ch] only after register_candev() succeeds,
or to clear it on the failure path?

 fail_mode:
 	rcar_canfd_global_deinit(gpriv, false);

[ ... ]

[Severity: High]

This is also a pre-existing issue and not introduced by this patch. Can
the IRQ handlers reach an already freed channel while this loop runs?

Each pass frees a channel through
rcar_canfd_channel_remove()->free_candev(). gpriv->ch[ch] is never
cleared, and the global interrupts are only disabled later, in
rcar_canfd_global_deinit().

The shared handlers walk every bit in channels_mask and dereference
gpriv->ch[ch]:

rcar_canfd_channel_interrupt()
    for_each_set_bit(ch, &gpriv->channels_mask, gpriv->max_channels) {
        rcar_canfd_handle_channel_err(gpriv, ch);
        rcar_canfd_handle_channel_tx(gpriv, ch);
    }

Take two channels. After ch0 has been freed, ch1 keeps running until
unregister_candev() stops it. Any RX, TX or global interrupt from ch1 in
that window runs one of the shared handlers:

- rcar_canfd_global_interrupt() or rcar_canfd_channel_interrupt()
- rcar_canfd_global_receive_fifo_interrupt() or
  rcar_canfd_global_err_interrupt() on RZ/G2L-style parts

That handler then reads ch0's priv->ndev and priv->base from freed
memory.

In rcar_canfd_handle_channel_err(), the stale priv->can.state
(CAN_STATE_STOPPED after close) passes this check:

	if (unlikely(priv->can.state != CAN_STATE_ERROR_ACTIVE &&
		     priv->can.state != CAN_STATE_BUS_OFF))
		rcar_canfd_state_change(ndev, txerr, rxerr);

so rcar_canfd_state_change() runs on the freed ndev.

On multi_channel_irqs parts, the per-channel devm IRQs use dev_id = priv.
They stay registered after free_candev(), and there is no
synchronize_irq(), so a handler already running on another CPU can also
see the freed priv. The fail_channel path in rcar_canfd_probe() has the
same problem.

Would it help to disable the global interrupts and synchronize or free
the IRQs before any channel is freed?

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