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

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

On 11/9/2023 3:25 PM, Brandes, Shai wrote:
> See inside
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>> Sent: Thursday, November 9, 2023 4:30 PM
>> To: Brandes, Shai <shaibran@amazon.com>
>> Cc: dev@dpdk.org; Beider, Ron <rbeider@amazon.com>; Atrash, Wajeeh
>> <atrwajee@amazon.com>; Bernstein, Amit <amitbern@amazon.com>
>> Subject: RE: [EXTERNAL] [PATCH] net/ena: fix coverity issues
>>
>> CAUTION: This email originated from outside of the organization. Do not click
>> links or open attachments unless you can confirm the sender and know the
>> content is safe.
>>
>>
>>
>> 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?
> [Brandes, Shai] Sorry, I should have included a cover letter to better explain the problem.
> We debugged the Coverity issues and did not find any real issue, all addresses, lenghts and accesses were as expected (false-positive).
> However, as precaution we decided to change few locations in the code that might have confused the Coverity but can easily worked around.
> 1. In the case of the structure, we wanted to make sure that "metrics_stats" array and the "metrics_num" fields reside in the same cache line,
> We originally set "uint64_t metrics_stats[ENA_MAX_CUSTOMER_METRICS]  __rte_cache_aligned; " but setting this alignment on array (as opposed to structure for example) might have confused Coverity (I know it shouldn't be the case, but still...).
> Switching the fields and setting the alignment on the "metrics_num" seems straight forward change and we verified with Pahole that both still reside in the same cache line (56 bytes total) and that the overall padding for both cases is identical (14B padding, just with different partition).
> 2. The other problematic change was to remove some redundant "&" when providing memcpy with the address of the destination array ("&array" instead of just "array"). The "&" is not needed but should be harmless C-wise (double checked it by printing this array address both ways, with and without the "&"). Again, we suspect it might have confuses Coverity, so decided to go on the safe side.
> Please acknowledge if these changes makes sense, and I will upload an updated patch.
>  
> 

Got it, thanks for the clarification.

There are a set of 'rte_memcpy' related coverity warnings, I assume it
is because Coverity is confused from the 'rte_memcpy' implementation.
You may be observing same warnings, didn't check.

As far as I understand this patch has some refactoring but it is not
clear if these changes will help coverity issue or not.

In that case commit log needs to be updated and coverity tag needs to be
removed, but changes are not functional or doesn't help development,
perhaps changes can wait until related code updated for some functional
reason.

>>
>>
>>> 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.
> [Brandes, Shai] all cases are related to the same exact rte_memcpy line that allegedly access the source buffer at 160B offset although its length is 48B.
> 
>>
>>> 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.
> [Brandes, Shai] ack
>>
>>>  {
>>>       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
> [Brandes, Shai] ack
>>
>>>               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?
> [Brandes, Shai] No change, but I just wanted it to be in the same order as the other multiplications 
>>
>>
>>>               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;  };
>>>
>