Re: [PATCH net-next v01 3/4] Fixes: Ensure secure concurrent access to adaptive_rx_coal.

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · 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).

- [High] The fix does not close the race its commit message describes.
- [Medium] The commit message describes a bug that cannot happen in this
  tree, and the code change does not touch that behaviour.
- [Low] The commit subject and body are malformed.

--

Fixes: Ensure secure concurrent access to adaptive_rx_coal.

This patch wraps the write of nic_dev->adaptive_rx_coal in
init_intr_coal_param() in WRITE_ONCE(), and the read in hinic3_net_dim()
in READ_ONCE(). The commit message says this fixes a race where an
already-scheduled DIM work item overwrites static coalescing settings
after adaptive RX coalescing is disabled via ethtool.

> Fixes: Ensure secure concurrent access to adaptive_rx_coal.

[Severity: Low]
This isn't a bug, but the subject starts with "Fixes:". Tools and
maintainers read that as a trailer pointing at an earlier commit. The
subject also lacks the "net: hinic3:" prefix and ends with a period.

> As noted in the subsequent review.

The body points to a review the reader can't see. It then copies a
reviewer's question word for word, but never says what the patch changes
or why. There is also no real Fixes: <sha> ("...") trailer.

Would something like "net: hinic3: annotate adaptive_rx_coal accesses
with READ_ONCE/WRITE_ONCE" describe the change more accurately?

> When disabling adaptive RX coalescing via ethtool, does the driver
> cancel any pending Dynamic Interrupt Moderation (DIM) work items?

[Severity: Medium]
Can this sequence happen in the current driver? hinic3_ethtool_ops has no
.set_coalesce or .get_coalesce callback:

static const struct ethtool_ops hinic3_ethtool_ops = {
	.supported_coalesce_params      = ETHTOOL_COALESCE_USECS |
					  ETHTOOL_COALESCE_PKT_RATE_RX_USECS,
	.get_link_ksettings             = hinic3_get_link_ksettings,
	.get_drvinfo                    = hinic3_get_drvinfo,
	.get_msglevel                   = hinic3_get_msglevel,
	.set_msglevel                   = hinic3_set_msglevel,
	.get_link                       = ethtool_op_get_link,
};

The only writer of adaptive_rx_coal runs once at probe time, before
register_netdev():

hinic3_nic_probe()
  hinic3_sw_init()
    hinic3_alloc_txrxqs()
      hinic3_init_intr_coalesce()
        init_intr_coal_param()
          WRITE_ONCE(nic_dev->adaptive_rx_coal, 1);

The only reader is hinic3_net_dim(). The value never changes after probe,
so the READ_ONCE()/WRITE_ONCE() annotations don't change behaviour.
Nothing later in the series adds set_coalesce support either.

Could the commit message describe this as an annotation-only change,
rather than a fix for a user-visible coalescing bug? As written, it may
mislead maintainers and stable backporters.

> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_irq.c b/drivers/net/ethernet/huawei/hinic3/hinic3_irq.c
> index e7d6c2033b451..b4e58d4c8ba85 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_irq.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_irq.c
> @@ -20,7 +20,7 @@ static void hinic3_net_dim(struct hinic3_nic_dev *nic_dev,
>  	struct dim_sample sample = {};
>  
>  	if (!test_bit(HINIC3_INTF_UP, &nic_dev->flags) ||
> -	    !nic_dev->adaptive_rx_coal)
> +	    !READ_ONCE(nic_dev->adaptive_rx_coal))
>  		return;
>  
>  	dim_update_sample(irq_cfg->total_events, rxq->rxq_stats.packets,

[Severity: High]
Does this close the race the commit message describes? Nothing cancels,
flushes or disables rxq->dim.work when the flag changes. The work handler
also never re-checks adaptive_rx_coal before it programs the hardware:

hinic3_rx_dim_work() {
	...
	cur_moder = net_dim_get_rx_moderation(dim->mode, dim->profile_ix);

	hinic3_update_queue_coal(rxq->netdev, rxq->q_id,
				 cur_moder.usec, cur_moder.pkts);
	...
}

That goes through
hinic3_update_queue_coal()->hinic3_set_interrupt_moder()->hinic3_set_interrupt_cfg().
It writes the DIM-derived coalesc_timer_cfg and pending_limit to the
hardware, then updates rxq->last_coalesc_timer_cfg and last_pending_limit.

The check here also happens separately from the schedule_work() inside
net_dim(), so this is a check-then-act sequence:

CPU0 (NAPI)                          CPU1 (disable path, if one existed)
hinic3_net_dim()
  READ_ONCE(adaptive_rx_coal) == 1
                                     clears adaptive_rx_coal
                                     programs static coalescing values
  net_dim()
    schedule_work(&dim->work)

hinic3_rx_dim_work()
  hinic3_update_queue_coal()   <- overwrites CPU1's static values

Work queued before the flag was cleared would still run as well.

READ_ONCE()/WRITE_ONCE() only stop the compiler from tearing or caching
the access. They don't order anything against the work item, and they
can't stop it from running.

Would the fix need one of these instead?

- The worker re-checks the flag under a lock (or with proper ordering)
  shared with the configuration path.
- The disable path clears the flag, quiesces NAPI, and cancels or
  disables each rxq->dim.work before it programs the static values.

Right now the only disable_work_sync() calls on dim.work are in the IRQ
teardown paths. Nothing later in the series adds this synchronization.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1793946318.git.gongfan1%40huawei.com