Re: [PATCH iwl-next v4 10/15] iavf: use new opcodes to request more than 16 queues

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] iavf: divide-by-zero in iavf_fill_rss_lut() on reinit after iavf_disable_vf()
- [Medium] iavf: unreachable request cancellation and validation bypass in iavf_set_channels()
- [Medium] iavf: dangling adapter->vsi_res pointer on iavf_parse_vf_resource_msg() error
- [Medium] iavf: unchecked iavf_parse_vf_resource_msg() error in completion path
--

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

iavf: use new opcodes to request more than 16 queues

This commit adds support for negotiating VIRTCHNL_VF_LARGE_NUM_QPAIRS and
using the V2 queue enable/disable, queue-to-vector mapping, and RSS queue
region virtchnl opcodes to support more than 16 queues. It also updates
ethtool channel configuration to request queues asynchronously in large queue
mode and adjusts VF resource parsing to handle queue count reductions and RSS
key/LUT size changes across resets.

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> index e7cf12eaa2689..3938c78597cb6 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
[ ... ]
> @@ -1730,15 +1742,29 @@ static int iavf_set_channels(struct net_device *netdev,
>  	/* All of these should have already been checked by ethtool before this
>  	 * even gets to us, but just to be sure.
>  	 */
> -	if (num_req == 0 || num_req > adapter->vsi_res->num_queue_pairs)
> +	if (num_req == 0 || num_req > iavf_max_channels(adapter))
>  		return -EINVAL;
>  
> -	if (num_req == adapter->num_active_queues)
> +	/* PF has not answered our previous request yet */
> +	if (adapter->current_op == VIRTCHNL_OP_REQUEST_QUEUES)
> +		return -EBUSY;
> +
> +	if (num_req == adapter->num_active_queues) {
> +		/* drop a request that is not sent yet */
> +		adapter->aq_required &= ~IAVF_FLAG_AQ_REQUEST_QUEUES;
>  		return 0;
> +	}
>  
>  	if (ch->rx_count || ch->tx_count || ch->other_count != NONQ_VECS)
>  		return -EINVAL;

[Severity: Medium]
Can this cancellation path in iavf_set_channels() be reached when a user tries
to revert a pending channel change back to adapter->num_active_queues?

In LARGE_NUM_QPAIRS_SUPPORT mode, queuing a channel request sets
adapter->num_req_queues_next and IAVF_FLAG_AQ_REQUEST_QUEUES while leaving
adapter->num_active_queues unchanged. Because iavf_get_channels() still
reports:

    ch->combined_count = adapter->num_active_queues;

both ethnl_set_channels() and ethtool_set_channels() compare the requested
channels against the current values from iavf_get_channels() and return 0
without calling iavf_set_channels() when combined_count equals
adapter->num_active_queues.

Conversely, if a user requests a change to other_count (such as setting other
to 0) while leaving combined_count at adapter->num_active_queues, the ethtool
core does invoke iavf_set_channels(), which hits the
num_req == adapter->num_active_queues check before validating:

    if (ch->rx_count || ch->tx_count || ch->other_count != NONQ_VECS)
        return -EINVAL;

Wouldn't that cause an invalid other_count request to drop
IAVF_FLAG_AQ_REQUEST_QUEUES and return 0 instead of -EINVAL?

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index db769eead68f3..7285f665b4fb4 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
[ ... ]
> @@ -2496,6 +2523,14 @@ int iavf_parse_vf_resource_msg(struct iavf_adapter *adapter)
>  
>  		return -EAGAIN;
>  	}
> +	/* PF could cut our queues 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;
> +	}

[Severity: Medium]
When iavf_parse_vf_resource_msg() schedules a reset and returns an error here
(or -ENOMEM on RSS reallocation failure below), iavf_virtchnl_completion()
ignores the return value during VIRTCHNL_OP_GET_VF_RESOURCES handling:

drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:iavf_virtchnl_completion() {
    ...
    case VIRTCHNL_OP_GET_VF_RESOURCES: {
        ...
        iavf_parse_vf_resource_msg(adapter);

        if (VLAN_V2_ALLOWED(adapter))
            break;
    }
    fallthrough;
    case VIRTCHNL_OP_GET_OFFLOAD_VLAN_V2_CAPS: {
        ...
        iavf_process_config(adapter);
        adapter->flags |= IAVF_FLAG_SETUP_NETDEV_FEATURES;
        iavf_schedule_finish_config(adapter);
        iavf_set_queue_vlan_tag_loc(adapter);
        ...
}

If the PF reduced vsi_res->num_queue_pairs across a reset (for instance, when
switching a VF from a larger LUT back to a 16-queue VSI LUT and clearing
VIRTCHNL_VF_LARGE_NUM_QPAIRS), adapter->num_active_queues still holds the old
count (up to 256) until reset_task runs.

If VLAN_V2_ALLOWED(adapter) is false, execution falls through and runs
iavf_process_config() and iavf_schedule_finish_config() before adapter->vsi or
RSS fields have been updated. In addition, if iavf_close() runs before the
scheduled reset_task executes, it sets IAVF_FLAG_AQ_DISABLE_QUEUES and runs
iavf_disable_queues() with LARGE_NUM_QPAIRS_SUPPORT(adapter) now false while
adapter->num_active_queues is still greater than 16:

    vqs.tx_queues = BIT(adapter->num_active_queues) - 1;

Should iavf_virtchnl_completion() check the return value of
iavf_parse_vf_resource_msg() and abort further post-resource configuration
when a reset has been scheduled?

>  	adapter->num_req_queues = 0;
>  	adapter->vsi.id = adapter->vsi_res->vsi_id;
>  
> @@ -2504,13 +2539,54 @@ 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->vf_res->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 = kzalloc(adapter->vf_res->rss_key_size, GFP_KERNEL);
> +		rss_lut = kzalloc(adapter->vf_res->rss_lut_size, GFP_KERNEL);
> +		if (!rss_lut || !rss_key) {
> +			kfree(rss_key);
> +			kfree(rss_lut);
> +			/* old buffers and sizes are kept, retry via reset */
> +			iavf_schedule_reset(adapter, IAVF_FLAG_RESET_NEEDED);
> +			return -ENOMEM;
> +		}
> +
> +		kfree(adapter->rss_key);
> +		kfree(adapter->rss_lut);
> +		adapter->rss_key = rss_key;
> +		adapter->rss_lut = rss_lut;
> +		adapter->rss_key_size = adapter->vf_res->rss_key_size;
> +		adapter->rss_lut_size = adapter->vf_res->rss_lut_size;
> +		iavf_init_rss(adapter);
> +	}

[Severity: High]
Could calling iavf_init_rss() here divide by zero in iavf_fill_rss_lut() when
recovering after iavf_disable_vf()?

After initial probe allocates adapter->rss_key and adapter->rss_lut in
iavf_init_config_adapter(), if a reset fails or times out and invokes
iavf_disable_vf(), the call chain:

    iavf_disable_vf()
      -> iavf_free_interrupt_scheme()
        -> iavf_free_queues()

sets adapter->num_active_queues = 0 without freeing adapter->rss_key or
adapter->rss_lut.

If hardware later recovers in iavf_watchdog_step() from __IAVF_COMM_FAILED
back to __IAVF_STARTUP, iavf_init_get_resources() calls
iavf_parse_vf_resource_msg() while adapter->state is __IAVF_INIT_GET_RESOURCES
and before __IAVF_INIT_CONFIG_ADAPTER has allocated queues, so
adapter->num_active_queues is still 0. If the PF's rss_key_size or
rss_lut_size changed across the reset, reconfig_rss is true and
iavf_init_rss() calls iavf_fill_rss_lut():

    int max = adapter->num_active_queues;

    if (LARGE_NUM_QPAIRS_SUPPORT(adapter) && qregion->qregion_width)
        max = min(max, (int)BIT(qregion->qregion_width));

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

Since max is 0, wouldn't i % max trigger a divide-by-zero oops (and
iavf_init_config_adapter() later overwrite adapter->rss_key and
adapter->rss_lut without freeing them)?

> +
> +	if (init_req) {
> +		qnum = min_t(int, num_online_cpus(), IAVF_MAX_REQ_QUEUES);
> +		/* granted, or we have enough */
> +		if (num_req_queues || qnum <= adapter->vsi_res->num_queue_pairs)
> +			return 0;
> +
> +		adapter->current_op = VIRTCHNL_OP_UNKNOWN;
> +		adapter->extended_caps |= IAVF_EXTENDED_CAP_RECV_REQ_QUEUES;
> +
> +		return iavf_request_queues(adapter, qnum);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -2557,17 +2633,18 @@ static void iavf_init_get_resources(struct iavf_adapter *adapter)
>  		goto err_alloc;
>  	}
>  
> +	/* Some features require additional messages to negotiate extended
> +	 * capabilities. These are processed in sequence by the
> +	 * __IAVF_INIT_EXTENDED_CAPS driver state.
> +	 */
> +	adapter->extended_caps = IAVF_EXTENDED_CAPS;
> +
>  	err = iavf_parse_vf_resource_msg(adapter);
>  	if (err) {
>  		dev_err(&pdev->dev, "Failed to parse VF resource message from PF (%d)\n",
>  			err);
>  		goto err_alloc;
>  	}
> -	/* Some features require additional messages to negotiate extended
> -	 * capabilities. These are processed in sequence by the
> -	 * __IAVF_INIT_EXTENDED_CAPS driver state.
> -	 */
> -	adapter->extended_caps = IAVF_EXTENDED_CAPS;
>  
>  	iavf_change_state(adapter, __IAVF_INIT_EXTENDED_CAPS);
>  	return;

[Severity: Medium]
Does the err_alloc path in iavf_init_get_resources() leave adapter->vsi_res as
a dangling pointer when iavf_parse_vf_resource_msg() fails?

In iavf_parse_vf_resource_msg(), adapter->vsi_res is assigned to
&adapter->vf_res->vsi_res[i] without clearing adapter->vsi_res beforehand.
With this patch, iavf_parse_vf_resource_msg() can now return an error after
adapter->vsi_res is set (for example, if iavf_request_queues() fails in the
init_req path, or if num_req_queues > adapter->vsi_res->num_queue_pairs
returns -EAGAIN).

When iavf_init_get_resources() jumps to err_alloc:

err_alloc:
    kfree(adapter->vf_res);
    adapter->vf_res = NULL;

adapter->vsi_res is not reset to NULL. If a subsequent retry in
__IAVF_INIT_GET_RESOURCES receives a resource message with no
VIRTCHNL_VSI_SRIOV VSI, wouldn't adapter->vsi_res remain non-NULL and point to
the freed adapter->vf_res buffer?

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