Re: [dpdk-dev] [PATCH v3] net/i40e: fix core dumped when setting txq or rxq to 0 in VF

["Zhang, Qi Z" <qi.z.zhang@intel.com>]

Hi Andy:

> -----Original Message-----
> From: Pei, Andy
> Sent: Friday, June 21, 2019 5:23 PM
> To: dev@dpdk.org
> Cc: Pei, Andy <andy.pei@intel.com>; Zhang, Helin <helin.zhang@intel.com>;
> stable@dpdk.org; Zhang, Roy Fan <roy.fan.zhang@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>; Xu, Rosen
> <rosen.xu@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>
> Subject: [PATCH v3] net/i40e: fix core dumped when setting txq or rxq to 0 in
> VF
> 
> Testpmd would stuck and result in core dump when user specifies an invalid
> VF queue number. This patch fixes this issue.

Is that the case, dev_start will core dump due to violate memory access if rxq or txq number is 0?
It's better to have a more specific description of the issue..

> 
> Fixes: d6b19729093e ("i40evf: support configurable crc stripping")
> Cc: helin.zhang@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Andy Pei <andy.pei@intel.com>
> ---
> v3:
> * no need to use a new line for each parameter when call envoke a
>   function. A new line comes when the current line is more than
>   80 characters.
> 
> v2:
>  * modify commit meaasage so one line contains not more than 72
>    characters.
>  * delete unnecessary parentheses around 'queue_id < nb_txq'
>  * delete unnecessary parentheses around 'queue_id < nb_rxq'
> 
> Cc: roy.fan.zhang@intel.com
> Cc: qi.z.zhang@intel.com
> Cc: jingjing.wu@intel.com
> Cc: beilei.xing@intel.com
> Cc: ferruh.yigit@intel.com
> Cc: rosen.xu@intel.com
> Cc: xiaolong.ye@intel.com
> 
>  drivers/net/i40e/i40e_ethdev_vf.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev_vf.c
> b/drivers/net/i40e/i40e_ethdev_vf.c
> index 63dbe14..41097fd 100644
> --- a/drivers/net/i40e/i40e_ethdev_vf.c
> +++ b/drivers/net/i40e/i40e_ethdev_vf.c
> @@ -573,7 +573,7 @@ struct rte_i40evf_xstats_name_off {  {
>  	txq_info->vsi_id = vsi_id;
>  	txq_info->queue_id = queue_id;
> -	if (queue_id < nb_txq) {
> +	if (queue_id < nb_txq && txq) {
>  		txq_info->ring_len = txq->nb_tx_desc;
>  		txq_info->dma_ring_addr = txq->tx_ring_phys_addr;
>  	}
> @@ -590,7 +590,7 @@ struct rte_i40evf_xstats_name_off {
>  	rxq_info->vsi_id = vsi_id;
>  	rxq_info->queue_id = queue_id;
>  	rxq_info->max_pkt_size = max_pkt_size;
> -	if (queue_id < nb_rxq) {
> +	if (queue_id < nb_rxq && rxq) {
>  		rxq_info->ring_len = rxq->nb_rx_desc;
>  		rxq_info->dma_ring_addr = rxq->rx_ring_phys_addr;
>  		rxq_info->databuffer_size =
> @@ -622,11 +622,22 @@ struct rte_i40evf_xstats_name_off {
>  	vc_vqci->num_queue_pairs = nb_qp;
> 
>  	for (i = 0, vc_qpi = vc_vqci->qpair; i < nb_qp; i++, vc_qpi++) {
> -		i40evf_fill_virtchnl_vsi_txq_info(&vc_qpi->txq,
> -			vc_vqci->vsi_id, i, dev->data->nb_tx_queues, txq[i]);
> -		i40evf_fill_virtchnl_vsi_rxq_info(&vc_qpi->rxq,
> -			vc_vqci->vsi_id, i, dev->data->nb_rx_queues,
> -					vf->max_pkt_len, rxq[i]);
> +		if (!txq)
> +			i40evf_fill_virtchnl_vsi_txq_info(&vc_qpi->txq,
> +				vc_vqci->vsi_id, i, dev->data->nb_tx_queues,
> +				NULL);
> +		else
> +			i40evf_fill_virtchnl_vsi_txq_info(&vc_qpi->txq,
> +				vc_vqci->vsi_id, i, dev->data->nb_tx_queues,
> +				txq[i]);

Seems, we can simply to !txq?txq:txq[i] to avoid some duplicate, right?

> +		if (!rxq)
> +			i40evf_fill_virtchnl_vsi_rxq_info(&vc_qpi->rxq,
> +				vc_vqci->vsi_id, i, dev->data->nb_rx_queues,
> +				vf->max_pkt_len, NULL);
> +		else
> +			i40evf_fill_virtchnl_vsi_rxq_info(&vc_qpi->rxq,
> +				vc_vqci->vsi_id, i, dev->data->nb_rx_queues,
> +				vf->max_pkt_len, rxq[i]);
>  	}
>  	memset(&args, 0, sizeof(args));
>  	args.ops = VIRTCHNL_OP_CONFIG_VSI_QUEUES;
> --
> 1.8.3.1