See inside
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yi...@amd.com>
> Sent: Thursday, November 9, 2023 4:30 PM
> To: Brandes, Shai <shaib...@amazon.com>
> Cc: dev@dpdk.org; Beider, Ron <rbei...@amazon.com>; Atrash, Wajeeh
> <atrwa...@amazon.com>; Bernstein, Amit <amitb...@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, shaib...@amazon.com wrote:
> > From: Shai Brandes <shaib...@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.
>
>
> > 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 <shaib...@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; };
> >
