> -----Original Message----- > From: Ferruh Yigit <ferruh.yi...@amd.com> > Sent: Thursday, November 9, 2023 5:54 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 3:25 PM, Brandes, Shai wrote: > > 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. > > > > > > 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 <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; }; > >>> > >