Re: [PATCH v8 2/2] can: dev: can_tdc_fill_info(): don't read automatic TDCV if interface is down

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · 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] The commit message calls this change "defensive hardening
  based on code inspection" and has no Fixes: tag.
- [Medium] netif_running() is used as a stand-in for "the controller is
  clocked", but in xilinx_can it can be true while the clocks are gated.
- [Low] The modified can_tdc_get_size() prototype in
  drivers/net/can/dev/netlink.c (lines 723-725) has continuation lines…

--

can: dev: can_tdc_fill_info(): don't read automatic TDCV if interface is down

This commit makes can_tdc_fill_info() call do_get_auto_tdcv() only when
netif_running() is true. Drivers such as xilinx_can then no longer read
controller registers while the interface is down. can_tdc_get_size() and
can_data_bittiming_get_size() now take the net_device, so IFLA_CAN_TDCV
space is reserved only under the same condition.

> This is a defensive hardening based on code inspection; the automatic
> TDCV path has not been triggered on hardware.

[Severity: Medium]
Is "defensive hardening" an accurate description here? The path this patch
closes looks reachable by an unprivileged user on current mainline and
stable kernels.

xcan_probe() ends with pm_runtime_put(). The controller then stays
runtime-suspended, with bus_clk and can_clk disabled, until xcan_open()
takes a reference. xcan_get_auto_tdcv() reads a register without taking a
runtime PM reference:

drivers/net/can/xilinx_can.c:xcan_get_auto_tdcv() {
    ...
	*tdcv = FIELD_GET(XCAN_SR_TDCV_MASK, priv->read_reg(priv, XCAN_SR_OFFSET));
    ...
}

xcan_get_berr_counter(), by comparison, wraps its reads in
pm_runtime_get_sync()/pm_runtime_put().

TDC_AUTO also gets turned on while the interface is down.
can_dbt_changelink() accepts data bittiming only when the device is not
IFF_UP. It calls can_calc_tdco(), which sets TDC_AUTO whenever the data BRP
is 1 or 2. Take the usual setup sequence:

  ip link set canX type can bitrate ... dbitrate ... fd on
  ip -d link show canX

Before this patch, that sequence reaches the following path with the
clocks gated:

  rtnl_getlink()->can_fill_info()->can_tdc_fill_info()->xcan_get_auto_tdcv()

The callback was added in commit 1010a8fa96086d ("can: xilinx_can: add
Transmitter Delay Compensation (TDC) feature support"). Should this patch
carry a Fixes: tag for that commit, plus Cc: stable?

The companion patch "can: dev: can_fill_info(): don't wake up controller if
interface is down" fixes the same kind of access, and it has both tags.

The changelog note says:

> Split out of the berr-counter patch so that the stable backport only
> carries the hardware-tested change.

Without a Fixes: tag, the stable tooling cannot tell that kernels
containing 1010a8fa96086d also need this change.

> diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> index b958c04d53db6..613beee9e9362 100644
> --- a/drivers/net/can/dev/netlink.c
> +++ b/drivers/net/can/dev/netlink.c
> @@ -720,8 +720,9 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
>  	return 0;
>  }
>  
> -static size_t can_tdc_get_size(struct data_bittiming_params *dbt_params,
> -			       u32 tdc_flags)
> +static size_t can_tdc_get_size(const struct net_device *dev,
> +				 struct data_bittiming_params *dbt_params,
> +			     u32 tdc_flags)

[Severity: Low]
This isn't a bug, but the continuation lines no longer line up with the
open parenthesis. The dbt_params line is two columns to the right of it,
and the tdc_flags line is two columns to the left. checkpatch --strict will
report "Alignment should match open parenthesis".

can_data_bittiming_get_size(), later in the same patch, is aligned
correctly.

>  {
>  	bool tdc_manual = tdc_flags & CAN_CTRLMODE_TDC_MANUAL_MASK;
>  	size_t size;
> @@ -742,7 +743,8 @@ static size_t can_tdc_get_size(struct data_bittiming_params *dbt_params,
>  	}
>  
>  	if (tdc_flags) {
> -		if (tdc_manual || dbt_params->do_get_auto_tdcv)
> +		if (tdc_manual ||
> +		    (dbt_params->do_get_auto_tdcv && netif_running(dev)))
>  			size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCV */
>  		size += nla_total_size(sizeof(u32));		/* IFLA_CAN_TDCO */
>  		if (dbt_params->tdc_const->tdcf_max)

[ ... ]

> @@ -910,7 +913,7 @@ static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev,
>  		if (tdc_manual) {
>  			tdcv = tdc->tdcv;
>  			err = 0;
> -		} else if (dbt_params->do_get_auto_tdcv) {
> +		} else if (dbt_params->do_get_auto_tdcv && netif_running(dev)) {
>  			err = dbt_params->do_get_auto_tdcv(dev, &tdcv);
>  		}

[Severity: Medium]
Can netif_running() be true while the xilinx_can clocks are still gated?
If so, xcan_get_auto_tdcv() can still read XCAN_SR_OFFSET from unclocked
hardware.

xcan_suspend() stops and detaches a running interface, then calls
pm_runtime_force_suspend(). That runs xcan_runtime_suspend(), which
disables bus_clk and can_clk. __LINK_STATE_START stays set:

drivers/net/can/xilinx_can.c:xcan_suspend() {
    ...
	if (netif_running(ndev)) {
		netif_stop_queue(ndev);
		netif_device_detach(ndev);
		xcan_chip_stop(ndev);
	}

	return pm_runtime_force_suspend(dev);
}

If pm_runtime_force_resume() fails during resume, xcan_resume() returns
early. This can happen, for example, when clk_prepare_enable() fails in
xcan_runtime_resume():

drivers/net/can/xilinx_can.c:xcan_resume() {
    ...
	ret = pm_runtime_force_resume(dev);
	if (ret) {
		dev_err(dev, "pm_runtime_force_resume failed on resume\n");
		return ret;
	}
    ...
}

At that point the clocks are off, the netdev is still IFF_UP and detached,
and netif_running() still returns true. An unprivileged
"ip -d link show canX" then passes the new check in can_tdc_fill_info()
and reads XCAN_SR_OFFSET in xcan_get_auto_tdcv().

rcar_canfd looks similar. rcar_canfd_suspend() calls
rcar_canfd_global_deinit(), which disables clkp and asserts reset. If
rcar_canfd_global_init() then fails in rcar_canfd_resume(),
rcar_canfd_get_auto_tdcv() reads cfdsts from a block that is unclocked or
held in reset.

Would it be more robust to take a runtime PM reference in
xcan_get_auto_tdcv(), as xcan_get_berr_counter() does?

Another option is to gate on netif_running(dev) && netif_device_present(dev).
Both drivers call netif_device_attach() only after the hardware is back up.

With either approach, can_tdc_get_size() needs the same condition so the
size estimate stays consistent with can_tdc_fill_info().

>  		if (!err && nla_put_u32(skb, IFLA_CAN_TDC_TDCV, tdcv))

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027065012.191348-1-chunyuzhiqiang%40yinhe.ht