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

["Brandes, Shai" <shaibran@amazon.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Thursday, November 9, 2023 5:54 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 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.
[Brandes, Shai] OK, I will update the Coverity issues on the webpage and remove this patch for now.
I see in Coverity additional issues where it warns on rte_memcpy accessing offset 160 which overruns the buffer (same offset 160 appears also in my Coverity issues).
Thanks.


> 
> >>
> >>
> >>> 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;  };
> >>>
> >