Re: [PATCH net-next v2 06/14] ice: bump to 256qs for VF

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 10/10/26 2:16 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] ice: virtchnl error path disables wrong queues when loop index != queue_id

unlikely error, but easy to fix, will do as v3 is needed anyway

> --
> 
> commit a7e9d03ecfb2e4d593b7ee51e67a3a4e87ee7a85
> Author: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> 
> ice: bump to 256qs for VF
> 
> This commit adjusts limits, bitmaps, and arrays in the ice driver to support
> up to 256 VF queues. It updates the requested and default queue limits while
> retaining the legacy 16-queue limit for older virtchnl opcodes.
> 
>> diff --git a/drivers/net/ethernet/intel/ice/virt/queues.c b/drivers/net/ethernet/intel/ice/virt/queues.c
>> index 2667634e3c8c3..61304493af34b 100644
>> --- a/drivers/net/ethernet/intel/ice/virt/queues.c
>> +++ b/drivers/net/ethernet/intel/ice/virt/queues.c
> [ ... ]
>> @@ -841,7 +841,7 @@ int ice_vc_cfg_qs_msg(struct ice_vf *vf, u8 *msg)
>>   	if (!vsi)
>>   		goto error_param;
>>   
>> -	if (qci->num_queue_pairs > ICE_MAX_QS_PER_VF_VCV1 ||
>> +	if (qci->num_queue_pairs > ICE_MAX_QS_PER_VF ||
>>   	    qci->num_queue_pairs > min_t(u16, vsi->alloc_txq, vsi->alloc_rxq)) {
>>   		dev_err(ice_pf_to_dev(pf), "VF-%d requesting more than supported number of queues: %d\n",
>>   			vf->vf_id, min_t(u16, vsi->alloc_txq, vsi->alloc_rxq));
> 
> [Severity: High]
> With the queue limit raised to 256, does the error handling at the end of
> ice_vc_cfg_qs_msg() disable the correct queues when a failure occurs?
> 
> Since 256 queue pairs exceed the 4096-byte admin queue buffer limit, the VF
> driver must split queue configurations across multiple virtchnl messages. In
> the second and subsequent messages, the array index i will not match the
> actual queue ID being configured (qci->qpair[i].rxq.queue_id).
> 
> If an error occurs during these later messages, the cleanup path iterates
> using the array index i:
> 
> error_param:
>      /* disable whatever we can */
>      for (; i >= 0; i--) {
>          if (ice_vsi_ctrl_one_rx_ring(vsi, false, i, true))
>              ...
>          if (ice_vf_vsi_dis_single_txq(vf, vsi, i))
>              ...
>      }
> 
> Could this use of i instead of the actual queue ID inadvertently disable
> unrelated queues, causing a denial of service to other active queue pairs,
> while leaving the newly configured, partially-initialized queues active?
>