Re: [PATCH net-next v5 07/15] iavf: extend iavf_configure_queues() to support more queues

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] iavf_suspend() can let the multi-message CONFIG_VSI_QUEUES
  sequence carry on after DISABLE_QUEUES, then dereference a freed Rx…
- [Low] The kernel-doc of iavf_configure_queues()
  (drivers/net/ethernet/intel/iavf/iavf_virtchnl.c) still says "Request…

--

iavf: extend iavf_configure_queues() to support more queues

This patch changes iavf_configure_queues() to send, on each call, only
as many queue pairs as fit into one virtchnl message. It tracks how far
it got in adapter->cfg_qps_sent and keeps IAVF_FLAG_AQ_CONFIGURE_QUEUES
set until every pair has been sent. A PF NACK clears the flag so the
remaining pairs are not sent.

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> index e6b7e8f82c7c1..356565aa41c8c 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c

[ ... ]

> @@ -375,11 +380,12 @@ int iavf_get_vf_ptp_caps(struct iavf_adapter *adapter)
>  void iavf_configure_queues(struct iavf_adapter *adapter)
>  {
>  	struct virtchnl_vsi_queue_config_info *vqci;
> -	int pairs = adapter->num_active_queues;
>  	struct virtchnl_queue_pair_info *vqpi;
> +	int first = adapter->cfg_qps_sent;

[Severity: Low]
This isn't a bug, but the kernel-doc above this function still reads:

 * Request that the PF set up our (previously allocated) queues.

The function now has a different contract:

- Each call sends one chunk, starting at cfg_qps_sent.
- It depends on iavf_process_aq_command() calling it again until
  cfg_qps_sent == num_active_queues.
- The caller has to zero cfg_qps_sent before setting
  IAVF_FLAG_AQ_CONFIGURE_QUEUES. Today only iavf_configure() does that.

Could the kernel-doc be updated to describe this?

>  	u32 i, max_frame;
>  	u8 rx_flags = 0;
>  	size_t len;
> +	int pairs;
>  
>  	max_frame = LIBIE_MAX_RX_FRM_LEN(adapter->rx_rings->pp->p.offset);

[ ... ]

> @@ -425,7 +430,9 @@ void iavf_configure_queues(struct iavf_adapter *adapter)
>  		vqpi++;
>  	}
>  
> -	adapter->aq_required &= ~IAVF_FLAG_AQ_CONFIGURE_QUEUES;
> +	adapter->cfg_qps_sent += pairs;
> +	if (adapter->cfg_qps_sent == adapter->num_active_queues)
> +		adapter->aq_required &= ~IAVF_FLAG_AQ_CONFIGURE_QUEUES;

[Severity: Medium]
IAVF_FLAG_AQ_CONFIGURE_QUEUES now stays set across several watchdog runs.
What happens if iavf_suspend() runs while some chunks are still pending?

iavf_close() masks aq_required down to IAVF_FLAG_AQ_GET_CONFIG before it
calls iavf_down(). iavf_reset_step() masks it down to
IAVF_FLAG_AQ_REQUEST_QUEUES. Both of those drop the leftover flag.

iavf_suspend() calls iavf_down() directly:

iavf_suspend()
  iavf_down()
    adapter->current_op = VIRTCHNL_OP_UNKNOWN;
    ...
    iavf_schedule_aq_request(adapter, IAVF_FLAG_AQ_DISABLE_QUEUES);

On this path neither IAVF_FLAG_AQ_CONFIGURE_QUEUES nor cfg_qps_sent is
cleared, and the state stays __IAVF_RUNNING.

The watchdog sends DISABLE_QUEUES first. The late reply to the cancelled
CONFIG chunk goes through the default branch of
iavf_virtchnl_completion(). That clears current_op while DISABLE is
still outstanding, so the next CONFIG chunk can reach the PF after
DISABLE.

On the PF side, ice_vc_cfg_qs_msg() programs the Tx/Rx queue contexts
from each message independently. It uses ring DMA addresses that iavf
frees with dma_free_coherent() when the DISABLE reply arrives.

Once that reply has run iavf_free_all_rx_resources(), rx_ring->pp is
NULL. If the flag is still set, the next watchdog run calls
iavf_configure_queues(), which dereferences pp before any check:

	max_frame = LIBIE_MAX_RX_FRM_LEN(adapter->rx_rings->pp->p.offset);

Could this leave the PF holding queue contexts that point at freed DMA
memory, and then oops on the NULL rx_rings->pp?

This needs more than 62 queue pairs, which becomes possible later in the
series with IAVF_MAX_REQ_QUEUES = 256. Before this patch the flag was
cleared when the single message went out, so the window only lasted
until the first watchdog send.

Should iavf_down() or iavf_suspend() clear IAVF_FLAG_AQ_CONFIGURE_QUEUES
and cfg_qps_sent?

>  	iavf_send_pf_msg(adapter, VIRTCHNL_OP_CONFIG_VSI_QUEUES,
>  			 (u8 *)vqci, len);
>  	kfree(vqci);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110141534.95614-1-przemyslaw.kitszel%40intel.com