Re: [PATCH net-next v2 10/14] iavf: use new opcodes to request more than 16 queues

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] iavf: early return in iavf_parse_vf_resource_msg() skips VSI initialization causing panic
- [High] core: strict type mismatch in min() macro causing build failure
- [High] iavf: division by zero in iavf_fill_rss_lut() from unbounded qregion_width
- [High] iavf: RMW data race on adapter->flags leading to lost state updates
- [High] iavf: early success return from iavf_parse_vf_resource_msg causes BUG_ON panic
- [Medium] iavf: auto-requesting queues on every reset destroys user configuration
- [Medium] iavf: ignored ENOMEM from iavf_parse_vf_resource_msg leaves inconsistent RSS state
--

commit 30797d207471017781d2b1737aa86bb6b0ea80c5
Author: Ahmed Zaki <ahmed.zaki@intel.com>

iavf: use new opcodes to request more than 16 queues

This commit introduces new opcodes to enable the VF to request a variable
number of queues (VIRTCHNL_VF_LARGE_NUM_QPAIRS). It defines a capability
exchange using VIRTCHNL_OP_GET_MAX_RSS_QREGION and introduces V2 opcodes for
enabling/disabling queues and mapping queue vectors.

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 8059e855dc096..f8193b0cae4ab 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -1775,10 +1775,14 @@ int iavf_config_rss(struct iavf_adapter *adapter)
>   **/
>  static void iavf_fill_rss_lut(struct iavf_adapter *adapter)
>  {
> -	u16 i;
> +	struct virtchnl_max_rss_qregion *qregion = &adapter->max_rss_qregion;
> +	int max = adapter->num_active_queues;
> +
> +	if (LARGE_NUM_QPAIRS_SUPPORT(adapter) && qregion->qregion_width)
> +		max = min(max, (int)BIT(qregion->qregion_width));

[Severity: High]
Is it possible for a malicious or buggy PF to trigger a division by zero here?

If qregion_width is >= 32 (on 32-bit systems) or >= 64, the BIT() shift invokes
undefined behavior. On 64-bit systems, a width of 32 results in 4294967296,
which becomes 0 when cast to int. This makes max = 0 via min(), causing a
divide-by-zero panic on the subsequent modulo operation.

>  
> -	for (i = 0; i < adapter->rss_lut_size; i++)
> -		adapter->rss_lut[i] = i % adapter->num_active_queues;
> +	for (int i = 0; i < adapter->rss_lut_size; i++)
> +		adapter->rss_lut[i] = i % max;
>  }

[ ... ]

> @@ -2486,6 +2493,23 @@ int iavf_parse_vf_resource_msg(struct iavf_adapter *adapter)
>  
>  		return -EAGAIN;
>  	}
> +	/* in LARGE mode num_queue_pairs is the PF limit, not our allocation */
> +	if (!num_req_queues && LARGE_NUM_QPAIRS_SUPPORT(adapter) &&
> +	    adapter->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_REQ_QUEUES) {
> +		adapter->current_op = VIRTCHNL_OP_UNKNOWN;
> +		qnum = min(adapter->vsi_res->num_queue_pairs,
> +			   num_online_cpus());

[Severity: High]
Does this code trigger a strict type mismatch build failure?

The min() macro requires both arguments to be of the exact same type. Here,
adapter->vsi_res->num_queue_pairs is a u16, but num_online_cpus() returns an
unsigned int.

> +
> +		return iavf_request_queues(adapter, qnum);

[Severity: High]
Could returning 0 here (success) instead of -EAGAIN falsely signal the state
machine to proceed?

If it returns 0 instead of -EAGAIN, the initialization state machine falsely
believes parsing completed and advances before the PF can respond. This leads
to calling netdev_rss_key_fill() with a length of 0, hitting a BUG_ON.

[Severity: Critical]
Does this early return skip essential VSI initialization, causing a NULL pointer
dereference later?

By returning early, critical fields like adapter->vsi.back remain uninitialized
(NULL). Once the interface receives packets, a checksum error will crash the
kernel in iavf_rx_csum() when it executes vsi->back->hw_csum_rx_error++.

[Severity: Medium]
Will this unconditionally clear user configuration on every runtime reset?

Because adapter->num_req_queues is reset to 0 at the end of this function:

    adapter->num_req_queues = 0;

the initial auto-request condition (!num_req_queues) will evaluate to true on
the next normal reset. This causes an unexpected auto-request to the PF,
resulting in a double-reset and persistent loss of user queue configurations.

> +	}
> +	/* PF could lower the limit on reset, e.g. when RSS LUT got smaller */
> +	if (adapter->num_active_queues > adapter->vsi_res->num_queue_pairs) {
> +		adapter->flags |= IAVF_FLAG_REINIT_MSIX_NEEDED;
> +		adapter->num_req_queues = adapter->vsi_res->num_queue_pairs;
> +		iavf_schedule_reset(adapter, IAVF_FLAG_RESET_NEEDED);
> +
> +		return -EAGAIN;
> +	}
>  	adapter->num_req_queues = 0;
>  	adapter->vsi.id = adapter->vsi_res->vsi_id;

[ ... ]

> @@ -2494,13 +2518,40 @@ int iavf_parse_vf_resource_msg(struct iavf_adapter *adapter)
>  	vsi->netdev = adapter->netdev;
>  	vsi->qs_handle = adapter->vsi_res->qset_handle;
>  	if (adapter->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_RSS_PF) {
> -		adapter->rss_key_size = adapter->vf_res->rss_key_size;
> -		adapter->rss_lut_size = adapter->rss_lut_size;
> +		if ((adapter->rss_key &&
> +		     adapter->rss_key_size != adapter->vf_res->rss_key_size) ||
> +		    (adapter->rss_lut &&
> +		     adapter->rss_lut_size != adapter->vf_res->rss_lut_size)) {
> +			reconfig_rss = true;
> +		} else {
> +			adapter->rss_key_size = adapter->vf_res->rss_key_size;
> +			adapter->rss_lut_size = adapter->vf_res->rss_lut_size;
> +		}
>  	} else {
>  		adapter->rss_key_size = IAVF_HKEY_ARRAY_SIZE;
>  		adapter->rss_lut_size = IAVF_HLUT_ARRAY_SIZE;
>  	}
>  
> +	if (reconfig_rss) {
> +		u8 *rss_key, *rss_lut;
> +
> +		rss_key = kmalloc(adapter->vf_res->rss_key_size, GFP_KERNEL);
> +		rss_lut = kmalloc(adapter->vf_res->rss_lut_size, GFP_KERNEL);
> +		if (!rss_lut || !rss_key) {
> +			kfree(rss_key);
> +			kfree(rss_lut);
> +			return -ENOMEM;

[Severity: Medium]
Is it safe to ignore the -ENOMEM return value when this allocation fails?

In iavf_virtchnl_completion(), the caller ignores this return value and blindly
proceeds to iavf_process_config(). This leaves the driver's RSS state out of
sync with the PF, causing silent misconfigurations.

> +		}

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> index 3b9545d2c0217..89d95d3b564ce 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> @@ -602,6 +793,40 @@ void iavf_map_queues(struct iavf_adapter *adapter)
>  	kfree(vimi);
>  }
>  
> +/**
> + * iavf_request_queues - request queues via virtchnl
> + * @adapter: adapter structure
> + * @num: number of requested queues
> + *
> + * Return: 0 on success, negative on failure.
> + */
> +int iavf_request_queues(struct iavf_adapter *adapter, int num)
> +{
> +	struct virtchnl_vf_res_request vfres = { num };
> +	int err;
> +
> +	if (adapter->current_op != VIRTCHNL_OP_UNKNOWN) {
> +		/* bail because we already have a command pending */
> +		dev_err(&adapter->pdev->dev, "Cannot request queues, command %d pending\n",
> +			adapter->current_op);
> +		return -EBUSY;
> +	}
> +
> +	adapter->current_op = VIRTCHNL_OP_REQUEST_QUEUES;
> +	adapter->flags |= IAVF_FLAG_REINIT_ITR_NEEDED;

[Severity: High]
Does this non-atomic read-modify-write operation on adapter->flags race with
lockless contexts?

For example, iavf_tx_timeout() updates this variable from a softirq context:

    adapter->flags |= flags;

If iavf_request_queues() executes concurrently, the updates from the watchdog
timer could be lost, preventing the device from recovering from a TX hang.

> +	adapter->num_req_queues = num;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009121433.30347-1-przemyslaw.kitszel@intel.com?part=10