[PATCH] qed: fix division by zero in qed_init_wfq_param when all vports configured

[PATCH] qed: fix division by zero in qed_init_wfq_param when all vports configured

[Evgenii Burenchev]

In qed_init_wfq_param(), variable non_requested_count can become zero
when all vports already have configured flag set. This happens when
num_vports equals the number of configured vports.

The function then calculates left_rate_per_vp = total_left_rate /
non_requested_count, which causes division by zero.

Add a check for non_requested_count == 0 before the division. Return
error when no unconfigured vports exist to distribute bandwidth.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Evgenii Burenchev <evg28bur@yandex.ru>
---
 drivers/net/ethernet/qlogic/qed/qed_dev.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/qlogic/qed/qed_dev.c b/drivers/net/ethernet/qlogic/qed/qed_dev.c
index 42c6dcfb1f0f..b287e04c8adc 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_dev.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_dev.c
@@ -5105,6 +5105,13 @@ static int qed_init_wfq_param(struct qed_hwfn *p_hwfn,
 
 	total_left_rate	= min_pf_rate - total_req_min_rate;
 
+	if (non_requested_count == 0) {
+		DP_VERBOSE(p_hwfn, NETIF_MSG_LINK,
+			   "All %d vports are already configured for WFQ, no unconfigured vports to distribute remaining bandwidth\n",
+			   num_vports);
+		return -EINVAL;
+	}
+
 	left_rate_per_vp = total_left_rate / non_requested_count;
 	if (left_rate_per_vp <  min_pf_rate / QED_WFQ_UNIT) {
 		DP_VERBOSE(p_hwfn, NETIF_MSG_LINK,
-- 
2.43.0

Re: [PATCH] qed: fix division by zero in qed_init_wfq_param when all vports configured

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
qed: fix division by zero in qed_init_wfq_param when all vports configured

This patch adds a guard for non_requested_count == 0 in qed_init_wfq_param()
to prevent a division-by-zero when every vport already has the configured
flag set, returning -EINVAL in that case.

> diff --git a/drivers/net/ethernet/qlogic/qed/qed_dev.c b/drivers/net/ethernet/qlogic/qed/qed_dev.c
> index 42c6dcfb1f0f..b287e04c8adc 100644
> --- a/drivers/net/ethernet/qlogic/qed/qed_dev.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_dev.c
> @@ -5105,6 +5105,13 @@ static int qed_init_wfq_param(struct qed_hwfn *p_hwfn,
>  
>  	total_left_rate	= min_pf_rate - total_req_min_rate;
>  
> +	if (non_requested_count == 0) {
> +		DP_VERBOSE(p_hwfn, NETIF_MSG_LINK,
> +			   "All %d vports are already configured for WFQ, no unconfigured vports to distribute remaining bandwidth\n",
> +			   num_vports);
> +		return -EINVAL;
> +	}
> +
>  	left_rate_per_vp = total_left_rate / non_requested_count;

Is returning -EINVAL here the right behavior when non_requested_count
reaches zero, or does it turn a division-by-zero into a functional
regression in at least two reachable paths?

Path 1: qed_configure_vport_wfq() configuring the final vport.

Looking at the counting logic in qed_init_wfq_param():

    for (i = 0; i < num_vports; i++) {
        u32 tmp_speed;

        if ((i != vport_id) &&
            p_hwfn->qm_info.wfq_data[i].configured) {
            req_count++;
            ...
        }
    }

    /* Include current vport data as well */
    req_count++;
    total_req_min_rate += req_rate;
    non_requested_count = num_vports - req_count;

When num_vports - 1 vports are already configured and the user is
explicitly configuring the last remaining one, the loop counts
num_vports - 1 configured vports (skipping vport_id), then the
unconditional req_count++ for the current vport makes req_count ==
num_vports, so non_requested_count == 0.  With this patch the call
through qed_configure_vport_wfq() -> __qed_configure_vport_wfq() ->
qed_init_wfq_param() now rejects that operation with -EINVAL.

Path 2: __qed_configure_vp_wfq_on_link_change() with all vports
configured.

    for (i = 0; i < p_hwfn->qm_info.num_vports; i++) {
        ...
        if (!p_hwfn->qm_info.wfq_data[i].configured)
            continue;

        rate = p_hwfn->qm_info.wfq_data[i].min_speed;
        use_wfq = true;

        rc = qed_init_wfq_param(p_hwfn, i, rate, min_pf_rate);
        if (rc) {
            DP_NOTICE(p_hwfn,
                      "WFQ validation failed while configuring min rate\n");
            break;
        }
    }

    if (!rc && use_wfq)
        qed_configure_wfq_for_all_vports(p_hwfn, p_ptt, min_pf_rate);
    else
        qed_disable_wfq_for_all_vports(p_hwfn, p_ptt, min_pf_rate);

If every vport has been configured, each iteration re-validates via
qed_init_wfq_param() and now hits the new -EINVAL return, so the loop
breaks, rc is non-zero, and the else branch calls
qed_disable_wfq_for_all_vports() on every link-state change.

Would it be more appropriate to skip just the division and the
subsequent percent-of-min_pf_rate check when non_requested_count is
zero, and then fall through to record wfq_data[vport_id] and return 0
(the final distribution loop is then naturally a no-op because every
vport hits the configured continue)?

One additional question on the commit metadata: the same function was
previously patched for an adjacent num_vports < 2 division-by-zero in
commit 1a9dc5610ef8 ("qed/qed_dev: guard against a possible division by
zero"), which carried a Fixes: bcd197c81f63 ("qed: Add vport WFQ
configuration APIs") tag.  Since this patch closes a second
division-by-zero path in the same function with the same root cause,
would it be worth adding a matching Fixes: bcd197c81f63 tag so stable
backport tooling picks it up?
-- 
pw-bot: cr