Re: [PATCH] net/ena: fix coverity issues

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 11/9/2023 2:08 PM, shaibran@amazon.com wrote:
> From: Shai Brandes <shaibran@amazon.com>
> 
> Changed the rte_memcpy call to use the precomputed buf_size.
> Rearranged the ena adapter structure and removed redundant
> '&' operators as a precaution.
> 

What is the reason of the structure rearrange?


> Coverity issue: 405363
> Coverity issue: 405357
> Coverity issue: 405359
>

Can you please split the patch per each fix if they are not logically
related or caused from same code.

> Fixes: 92401abfbcb9 ("net/ena: support connection tracking stats")
> Signed-off-by: Shai Brandes <shaibran@amazon.com>
> ---
>  drivers/net/ena/ena_ethdev.c | 21 ++++++++++-----------
>  drivers/net/ena/ena_ethdev.h |  4 ++--
>  2 files changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
> index dc846d2e84..53e7251874 100644
> --- a/drivers/net/ena/ena_ethdev.c
> +++ b/drivers/net/ena/ena_ethdev.c
> @@ -531,8 +531,8 @@ ENA_PROXY_DESC(ena_com_get_eni_stats, ENA_MP_ENI_STATS_GET,
>  ({
>  	ENA_TOUCH(rsp);
>  	ENA_TOUCH(ena_dev);
> -	if (stats != (struct ena_admin_eni_stats *)&adapter->metrics_stats)
> -		rte_memcpy(stats, &adapter->metrics_stats, sizeof(*stats));
> +	if (stats != (struct ena_admin_eni_stats *)adapter->metrics_stats)
> +		rte_memcpy(stats, adapter->metrics_stats, sizeof(*stats));
>  }),
>  	struct ena_com_dev *ena_dev, struct ena_admin_eni_stats *stats);
>  
> @@ -590,9 +590,8 @@ ENA_PROXY_DESC(ena_com_get_customer_metrics, ENA_MP_CUSTOMER_METRICS_GET,
>  ({
>  	ENA_TOUCH(rsp);
>  	ENA_TOUCH(ena_dev);
> -	ENA_TOUCH(buf_size);
> -	if (buf != (char *)&adapter->metrics_stats)
> -		rte_memcpy(buf, &adapter->metrics_stats, adapter->metrics_num * sizeof(uint64_t));
> +	if (buf != (char *)adapter->metrics_stats)
> +		rte_memcpy(buf, adapter->metrics_stats, buf_size);
>  }),
>  	struct ena_com_dev *ena_dev, char *buf, size_t buf_size);
>  
> @@ -3240,7 +3239,7 @@ static uint16_t eth_ena_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
>  }
>  
>  static void ena_copy_customer_metrics(struct ena_adapter *adapter, uint64_t *buf,
> -					     size_t num_metrics)
> +						 size_t num_metrics)
>

Please drop unrelated changed from the set.

>  {
>  	struct ena_com_dev *ena_dev = &adapter->ena_dev;
>  	int rc;
> @@ -3252,10 +3251,10 @@ static void ena_copy_customer_metrics(struct ena_adapter *adapter, uint64_t *buf
>  		}
>  		rte_spinlock_lock(&adapter->admin_lock);
>  		rc = ENA_PROXY(adapter,
> -					ena_com_get_customer_metrics,
> -					&adapter->ena_dev,
> -					(char *)buf,
> -					num_metrics * sizeof(uint64_t));
> +			       ena_com_get_customer_metrics,
> +			       &adapter->ena_dev,
> +			       (char *)buf,
> +			       num_metrics * sizeof(uint64_t));
>

ditto

>  		rte_spinlock_unlock(&adapter->admin_lock);
>  		if (rc != 0) {
>  			PMD_DRV_LOG(WARNING, "Failed to get customer metrics, rc: %d\n", rc);
> @@ -4088,7 +4087,7 @@ ena_mp_primary_handle(const struct rte_mp_msg *mp_msg, const void *peer)
>  	case ENA_MP_CUSTOMER_METRICS_GET:
>  		res = ena_com_get_customer_metrics(ena_dev,
>  				(char *)adapter->metrics_stats,
> -				sizeof(uint64_t) * adapter->metrics_num);
> +				adapter->metrics_num * sizeof(uint64_t));
>

Does above change makes any difference? What is the motivation?


>  		break;
>  	case ENA_MP_SRD_STATS_GET:
>  		res = ena_com_get_ena_srd_info(ena_dev,
> diff --git a/drivers/net/ena/ena_ethdev.h b/drivers/net/ena/ena_ethdev.h
> index 4988fbffb5..17d292101c 100644
> --- a/drivers/net/ena/ena_ethdev.h
> +++ b/drivers/net/ena/ena_ethdev.h
> @@ -344,8 +344,8 @@ struct ena_adapter {
>  	 * Helper variables for holding the information about the supported
>  	 * metrics.
>  	 */
> -	uint64_t metrics_stats[ENA_MAX_CUSTOMER_METRICS] __rte_cache_aligned;
> -	uint16_t metrics_num;
> +	uint16_t metrics_num __rte_cache_aligned;
> +	uint64_t metrics_stats[ENA_MAX_CUSTOMER_METRICS];
>  	struct ena_stats_srd srd_stats __rte_cache_aligned;
>  };
>