Re: [PATCH net-next v01 4/4] Fixes: Ensure max_qps is always greater than or equal to num_qps

[netdev-bot+sashiko@kernel.org]

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

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

- [High] In hinic3_try_to_enable_rss()
  (drivers/net/ethernet/huawei/hinic3/hinic3_rss.c:309-314), the patch…
- [Medium] The subject 'Ensure max_qps is always greater than or equal to
  num_qps' describes an invariant that already held in the baseline…
- [Medium] The commit message justifies the change with an out-of-bounds
  read through netdev_nl_stats_by_netdev() -> netdev_stat_queue_sum() ->…
- [Low] The subject uses 'Fixes:' as a prefix instead of a subsystem
  prefix such as 'net: hinic3:', and there is no `Fixes: <12+ char sha>…

--

Fixes: Ensure max_qps is always greater than or equal to num_qps

This patch changes hinic3_try_to_enable_rss() in two ways. First, max_qps
is now forced to 1 only when the function reports max_qps <= 1 or lacks
RSS support, and stays at hinic3_func_max_qnum() when RSS resource
allocation or hardware configuration fails. Second, every error path now
sets q_params.num_qps to 1. The patch also reflows the signatures of
hinic3_alloc_channel_resources() and hinic3_free_channel_resources() and
renames their trxq_params argument to q_params.

> Fixes: Ensure max_qps is always greater than or equal to num_qps

[Severity: Medium]
Is this subject accurate? Before this patch, hinic3_try_to_enable_rss()
already kept num_qps <= max_qps on every exit. The error label did:

err_reset_q_params:
	clear_bit(HINIC3_RSS_ENABLE, &nic_dev->flags);
	nic_dev->q_params.num_qps = nic_dev->max_qps;

The success path goes through decide_num_qps():

	nic_dev->q_params.num_qps = min(dev_cpus, nic_dev->max_qps);

At open, hinic3_config_num_qps() only lowers num_qps further.

The problem the body describes is a different one: the per-queue arrays
sized by max_qps can be smaller than netdev->real_num_*_queues.

The message also leaves out the behaviour changes this patch makes:

- On multi-queue hardware without HINIC3_NIC_F_RSS, max_qps changes from
  hinic3_func_max_qnum() to 1. That shrinks txqs, rxqs, intr_coalesce and
  qps_msix_entries, and lowers the hinic3_get_stats64() loop bounds.
- When alloc_rss_resource() or hinic3_set_hw_rss_parameters() fails,
  max_qps now stays at hinic3_func_max_qnum() instead of 1.
- err_reset_q_params now hard-codes q_params.num_qps = 1.

Could the message describe these changes, especially the one on the
non-RSS path?

> As noted in the subsequent review.
>
> After that, hinic3_sw_init()->hinic3_try_to_enable_rss() can shrink
> max_qps to 1 when alloc_rss_resource() or hinic3_set_hw_rss_parameters()
> fails:

[ ... ]

> Would this read past the end of the rxqs and txqs slab objects and return
> those values to user space? NETDEV_CMD_QSTATS_GET does not require
> GENL_ADMIN_PERM, so any local user can issue it.
> The same applies to nic_dev->txqs[i] in hinic3_get_queue_stats_tx().
> Before this patch hinic3 had no stat_ops, and hinic3_get_stats64() bounds
> its loops by max_qps.

[Severity: Medium]
Does the code path described here exist in this tree? There is no
hinic3_get_queue_stats_rx(), hinic3_get_queue_stats_tx() or stat_ops
anywhere under drivers/net/ethernet/huawei/hinic3/. The only per-queue
stats consumer is hinic3_get_stats64(), and it is bounded by max_qps:

	for (i = 0; i < nic_dev->max_qps; i++) {
		txq = &nic_dev->txqs[i];

"Before this patch hinic3 had no stat_ops" looks like it refers to a
different, unmerged patch. If so, should this change be folded into the
patch that adds stat_ops? As a standalone fix, its rationale depends on
code that isn't in the tree.

The message also opens with "As noted in the subsequent review." and
pastes the reviewer's question ("Would this read past the end...?")
verbatim. Could it say what was wrong and how this patch fixes it
instead?

> This patch also solve some defects reported by checkpatch.

[Severity: Low]
This isn't a bug, but the subject uses "Fixes:" as its prefix instead of
a subsystem prefix such as "net: hinic3:". There is also no
Fixes: <sha> ("subject") trailer, so backport tooling can't tell which
commit this fixes.

The checkpatch cleanup in hinic3_netdev_ops.c only reflows the signatures
of hinic3_alloc_channel_resources() and hinic3_free_channel_resources()
and renames trxq_params to q_params. It has nothing to do with max_qps.
Could it go in a separate patch?

> diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_rss.c b/drivers/net/ethernet/huawei/hinic3/hinic3_rss.c
> index 25db74d8c7ddb..4aa725339e604 100644
> --- a/drivers/net/ethernet/huawei/hinic3/hinic3_rss.c
> +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_rss.c
> @@ -308,14 +308,14 @@ void hinic3_try_to_enable_rss(struct net_device *netdev)
>  
>  	nic_dev->max_qps = hinic3_func_max_qnum(hwdev);
>  	if (nic_dev->max_qps <= 1 ||
> -	    !hinic3_test_support(nic_dev, HINIC3_NIC_F_RSS))
> +	    !hinic3_test_support(nic_dev, HINIC3_NIC_F_RSS)) {
> +		nic_dev->max_qps = 1;

[Severity: High]
Doesn't this recreate the mismatch the commit message says it closes,
and now on the normal probe path? Take a function whose firmware reports
max_sqs > 1 but doesn't advertise HINIC3_NIC_F_RSS:

hinic3_nic_probe()
    max_qps = hinic3_func_max_qnum(hwdev);
    netdev = alloc_etherdev_mq(sizeof(*nic_dev), max_qps);
    -> real_num_{tx,rx}_queues start at func_max_qnum
hinic3_sw_init()
    hinic3_try_to_enable_rss()
        nic_dev->max_qps = 1;

hinic3_alloc_txqs() and hinic3_alloc_rxqs() then size nic_dev->txqs[]
and nic_dev->rxqs[] by max_qps:

    u16 q_id, num_txqs = nic_dev->max_qps;
    ...
    u16 num_rxqs = nic_dev->max_qps;

hinic3_init_intr_coalesce() and the qps_msix_entries allocation also use
max_qps, so each of these arrays ends up with one entry.

Only hinic3_vport_up() lowers real_num_*_queues, and only on a
successful open:

	err = netif_set_real_num_queues(netdev, nic_dev->q_params.num_qps,
					nic_dev->q_params.num_qps);

Before this patch, this branch left max_qps at func_max_qnum, so the
arrays matched the netdev queue count.

The stat_ops consumer the commit message describes walks
real_num_*_queues:

netdev_nl_stats_by_netdev()->netdev_stat_queue_sum()->
    hinic3_get_queue_stats_rx()/hinic3_get_queue_stats_tx()

Once that is added, wouldn't nic_dev->rxqs[i] and nic_dev->txqs[i] read
past the one-entry slab objects on every non-RSS multi-queue function?
Any local user could trigger this via NETDEV_CMD_QSTATS_GET, which
doesn't require GENL_ADMIN_PERM.

At this revision the read can't happen yet:

- hinic3_xmit_frame() is bounded by q_params.num_qps.
- hinic3_get_stats64() is bounded by max_qps.
- There are no per-queue ethtool or stat_ops callbacks.

The bug class the message claims to fix is still there, though, and it
now hits the common no-RSS probe path rather than rare allocation or
firmware failures.

Is the new assignment needed at all? hinic3_config_num_qps() already
forces num_qps to 1 at open when RSS is off:

	if (!test_bit(HINIC3_RSS_ENABLE, &nic_dev->flags))
		q_params->num_qps = 1;

On this path, the only effect of the assignment seems to be shrinking
the arrays.

Would it be more consistent to keep max_qps = hinic3_func_max_qnum() in
every branch and set only q_params.num_qps to 1? Other options would be
to size the netdev from the same value as the arrays, or to call
netif_set_real_num_queues() during probe.

> +		goto err_reset_q_params;
> +	}

[ ... ]

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