Skip to contentSkip to conversation

net/ena: fix coverity issues

PATCH In review [email protected] 2 participants 5 messages Nov 9, 2023 Network drivers

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 net/ena: fix coverity issues

From: Shai Brandes <[email protected]>

Changed the rte_memcpy call to use the precomputed buf_size. Rearranged the ena adapter structure and removed redundant '&' operators as a precaution.

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

Fixes 92401abfbcb9 ("net/ena: support connection tracking stats")
Signed-off-by Shai Brandes <[email protected]>
---
 drivers/net/ena/ena_ethdev.c | 21 ++++++++++-----------
 drivers/net/ena/ena_ethdev.h |  4 ++--
 2 files changed, 12 insertions(+), 13 deletions(-)
drivers/net/ena/ena_ethdev.c +10 -11
index dc846d2e84..53e7251874 100644
@@ -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)
 {
 	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));
 		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));
 		break;
 	case ENA_MP_SRD_STATS_GET:
 		res = ena_com_get_ena_srd_info(ena_dev,
drivers/net/ena/ena_ethdev.h +2 -2
index 4988fbffb5..17d292101c 100644
@@ -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;
 };
Signature · 1 line
2.17.1
#1 lore raw
Quoted 6 lines · On 11/9/2023 2:08 PM, [email protected] wrote:

From: Shai Brandes <[email protected]>

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?

Quoted 4 lines

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.

Quoted 35 lines
Fixes 92401abfbcb9 ("net/ena: support connection tracking stats")
Signed-off-by Shai Brandes <[email protected]>
---
 drivers/net/ena/ena_ethdev.c | 21 ++++++++++-----------
 drivers/net/ena/ena_ethdev.h |  4 ++--
 2 files changed, 12 insertions(+), 13 deletions(-)
drivers/net/ena/ena_ethdev.c +4 -5
index dc846d2e84..53e7251874 100644
@@ -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);

drivers/net/ena/ena_ethdev.c static uint16_t eth_ena_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, · line 3243

Quoted 6 lines
drivers/net/ena/ena_ethdev.c +1 -1
@@ -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.

Quoted 6 lines
drivers/net/ena/ena_ethdev.c +0 -0
 {
 	struct ena_com_dev *ena_dev = &adapter->ena_dev;
drivers/net/ena/ena_ethdev.c +0 -0
 	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);

drivers/net/ena/ena_ethdev.c static void ena_copy_customer_metrics(struct ena_adapter *adapter, uint64_t *buf · line 3258

Quoted 10 lines
drivers/net/ena/ena_ethdev.c +4 -4
 		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

drivers/net/ena/ena_ethdev.c ena_mp_primary_handle(const struct rte_mp_msg *mp_msg, const void *peer) · line 4091

Quoted 10 lines
drivers/net/ena/ena_ethdev.c +0 -0
 		rte_spinlock_unlock(&adapter->admin_lock);
 		if (rc != 0) {
drivers/net/ena/ena_ethdev.c +1 -1
 			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?

Quoted 18 lines
drivers/net/ena/ena_ethdev.c +0 -0
 		break;
 	case ENA_MP_SRD_STATS_GET:
drivers/net/ena/ena_ethdev.c +0 -0
 		res = ena_com_get_ena_srd_info(ena_dev,
drivers/net/ena/ena_ethdev.h +2 -2
index 4988fbffb5..17d292101c 100644
@@ -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;
 };
author
#2 lore raw

See inside

Quoted 15 lines

-----Original Message-----
From: Ferruh Yigit <[email protected]>
Sent: Thursday, November 9, 2023 4:30 PM
To: Brandes, Shai <[email protected]>
Cc: [email protected]; Beider, Ron <[email protected]>; Atrash, Wajeeh <[email protected]>; Bernstein, Amit <[email protected]>
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, [email protected] wrote:

Quoted 8 lines
Quoted 6 lines

From: Shai Brandes <[email protected]>

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.

Quoted 9 lines
Quoted 6 lines

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.

Quoted 41 lines

drivers/net/ena/ena_ethdev.c

Quoted 25 lines
Fixes 92401abfbcb9 ("net/ena: support connection tracking stats")
Signed-off-by Shai Brandes <[email protected]>
---
 drivers/net/ena/ena_ethdev.c | 21 ++++++++++-----------
drivers/net/ena/ena_ethdev.h |  4 ++--
 2 files changed, 12 insertions(+), 13 deletions(-)
(quoted diff) +0 -0

b/drivers/net/ena/ena_ethdev.c index dc846d2e84..53e7251874 100644

drivers/net/ena/ena_ethdev.c +3 -2
@@ -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,

drivers/net/ena/ena_ethdev.c ENA_PROXY_DESC(ena_com_get_customer_metrics, ENA_MP_CUSTOMER_METRICS_GET,

Quoted 6 lines
drivers/net/ena/ena_ethdev.c +0 -3
 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));

Quoted 8 lines
drivers/net/ena/ena_ethdev.c +2 -0
+     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,  }
Quoted 7 lines

drivers/net/ena/ena_ethdev.c static uint16_t eth_ena_xmit_pkts(void · line 3241

Quoted 1 line
drivers/net/ena/ena_ethdev.c +0 -0
 static void ena_copy_customer_metrics(struct ena_adapter *adapter,

uint64_t *buf,

drivers/net/ena/ena_ethdev.c static uint16_t eth_ena_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,

Quoted 3 lines
drivers/net/ena/ena_ethdev.c +1 -1
-                                          size_t num_metrics)
+                                              size_t num_metrics)

Please drop unrelated changed from the set.

[Brandes, Shai] ack

Quoted 19 lines

drivers/net/ena/ena_ethdev.c static void ena_copy_customer_metrics(struct

Quoted 5 lines
drivers/net/ena/ena_ethdev.c +0 -0
 {
      struct ena_com_dev *ena_dev = &adapter->ena_dev;

      int rc;
drivers/net/ena/ena_ethdev.c +0 -0
@@ -3252,10 +3251,10 @@ static void ena_copy_customer_metrics(struct

ena_adapter *adapter, uint64_t *buf

drivers/net/ena/ena_ethdev.c static void ena_copy_customer_metrics(struct ena_adapter *adapter, uint64_t *buf

Quoted 12 lines
drivers/net/ena/ena_ethdev.c +4 -4
              }
              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));
Quoted 1 line

ditto

[Brandes, Shai] ack

Quoted 15 lines
Quoted 5 lines
drivers/net/ena/ena_ethdev.c +0 -0
              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)

drivers/net/ena/ena_ethdev.c

Quoted 7 lines

case ENA_MP_CUSTOMER_METRICS_GET:
res = ena_com_get_customer_metrics(ena_dev,
(char *)adapter->metrics_stats,

drivers/net/ena/ena_ethdev.c +2 -1
-                             sizeof(uint64_t) * adapter->metrics_num);
+                             adapter->metrics_num *
+ sizeof(uint64_t));
Quoted 1 line

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

Quoted 20 lines
Quoted 10 lines

              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

drivers/net/ena/ena_ethdev.h struct ena_adapter { · line 347

Quoted 4 lines
drivers/net/ena/ena_ethdev.h +0 -1
@@ -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;

Quoted 5 lines
drivers/net/ena/ena_ethdev.h +2 -1
-     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;  };
#3 lore raw
Quoted 25 lines · Shai

See inside

Quoted 15 lines

-----Original Message-----
From: Ferruh Yigit <[email protected]>
Sent: Thursday, November 9, 2023 4:30 PM
To: Brandes, Shai <[email protected]>
Cc: [email protected]; Beider, Ron <[email protected]>; Atrash, Wajeeh <[email protected]>; Bernstein, Amit <[email protected]>
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, [email protected] wrote:

Quoted 8 lines
Quoted 6 lines

From: Shai Brandes <[email protected]>

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?

Quoted 10 lines

[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.

Quoted 119 lines
Quoted 9 lines
Quoted 6 lines

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.

Quoted 41 lines

drivers/net/ena/ena_ethdev.c

Quoted 25 lines
Fixes 92401abfbcb9 ("net/ena: support connection tracking stats")
Signed-off-by Shai Brandes <[email protected]>
---
 drivers/net/ena/ena_ethdev.c | 21 ++++++++++-----------
drivers/net/ena/ena_ethdev.h |  4 ++--
 2 files changed, 12 insertions(+), 13 deletions(-)
(quoted diff) +0 -0

b/drivers/net/ena/ena_ethdev.c index dc846d2e84..53e7251874 100644

drivers/net/ena/ena_ethdev.c +3 -2
@@ -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,

drivers/net/ena/ena_ethdev.c ENA_PROXY_DESC(ena_com_get_customer_metrics, ENA_MP_CUSTOMER_METRICS_GET,

Quoted 6 lines
drivers/net/ena/ena_ethdev.c +0 -3
 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));

Quoted 8 lines
drivers/net/ena/ena_ethdev.c +2 -0
+     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,  }
Quoted 7 lines

drivers/net/ena/ena_ethdev.c static uint16_t eth_ena_xmit_pkts(void · line 3241

Quoted 1 line
drivers/net/ena/ena_ethdev.c +0 -0
 static void ena_copy_customer_metrics(struct ena_adapter *adapter,

uint64_t *buf,

drivers/net/ena/ena_ethdev.c static uint16_t eth_ena_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,

Quoted 3 lines
drivers/net/ena/ena_ethdev.c +1 -1
-                                          size_t num_metrics)
+                                              size_t num_metrics)

Please drop unrelated changed from the set.

[Brandes, Shai] ack

Quoted 19 lines

drivers/net/ena/ena_ethdev.c static void ena_copy_customer_metrics(struct

Quoted 5 lines
drivers/net/ena/ena_ethdev.c +0 -0
 {
      struct ena_com_dev *ena_dev = &adapter->ena_dev;

      int rc;
drivers/net/ena/ena_ethdev.c +0 -0
@@ -3252,10 +3251,10 @@ static void ena_copy_customer_metrics(struct

ena_adapter *adapter, uint64_t *buf

drivers/net/ena/ena_ethdev.c static void ena_copy_customer_metrics(struct ena_adapter *adapter, uint64_t *buf

Quoted 12 lines
drivers/net/ena/ena_ethdev.c +4 -4
              }
              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));
Quoted 1 line

ditto

[Brandes, Shai] ack

Quoted 15 lines
Quoted 5 lines
drivers/net/ena/ena_ethdev.c +0 -0
              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)

drivers/net/ena/ena_ethdev.c

Quoted 7 lines

case ENA_MP_CUSTOMER_METRICS_GET:
res = ena_com_get_customer_metrics(ena_dev,
(char *)adapter->metrics_stats,

drivers/net/ena/ena_ethdev.c +2 -1
-                             sizeof(uint64_t) * adapter->metrics_num);
+                             adapter->metrics_num *
+ sizeof(uint64_t));
Quoted 1 line

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

Quoted 20 lines
Quoted 10 lines

              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

drivers/net/ena/ena_ethdev.h struct ena_adapter { · line 347

Quoted 4 lines
drivers/net/ena/ena_ethdev.h +0 -1
@@ -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;

Quoted 5 lines
drivers/net/ena/ena_ethdev.h +2 -1
-     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;  };
author
#4 lore raw
Quoted 75 lines

-----Original Message-----
From: Ferruh Yigit <[email protected]>
Sent: Thursday, November 9, 2023 5:54 PM
To: Brandes, Shai <[email protected]>
Cc: [email protected]; Beider, Ron <[email protected]>; Atrash, Wajeeh <[email protected]>; Bernstein, Amit <[email protected]>
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.

Quoted 19 lines · Shai

See inside

Quoted 17 lines

-----Original Message-----
From: Ferruh Yigit <[email protected]>
Sent: Thursday, November 9, 2023 4:30 PM
To: Brandes, Shai <[email protected]>
Cc: [email protected]; Beider, Ron <[email protected]>; Atrash, Wajeeh <[email protected]>; Bernstein, Amit <[email protected]>
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.

Quoted 2 lines · On 11/9/2023 2:08 PM, [email protected] wrote:

From: Shai Brandes <[email protected]>

Quoted 7 lines
Quoted 6 lines
Quoted 4 lines

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.

Quoted 1 line

We debugged the Coverity issues and did not find any real issue, all

addresses, lenghts and accesses were as expected (false-positive).

Quoted 1 line

However, as precaution we decided to change few locations in the code

that might have confused the Coverity but can easily worked around.

Quoted 2 lines

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...).

Quoted 1 line

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).

Quoted 1 line

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.

Quoted 1 line

Please acknowledge if these changes makes sense, and I will upload an

updated patch.

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 127 lines
Quoted 11 lines
Quoted 10 lines
Quoted 7 lines

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.

Quoted 50 lines
Quoted 26 lines
Quoted 26 lines
Fixes 92401abfbcb9 ("net/ena: support connection tracking stats")
Signed-off-by Shai Brandes <[email protected]>
---
 drivers/net/ena/ena_ethdev.c | 21 ++++++++++-----------
drivers/net/ena/ena_ethdev.h |  4 ++--
 2 files changed, 12 insertions(+), 13 deletions(-)
(quoted diff) +0 -0

b/drivers/net/ena/ena_ethdev.c index dc846d2e84..53e7251874 100644

drivers/net/ena/ena_ethdev.c +3 -2
@@ -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);

Quoted 8 lines

drivers/net/ena/ena_ethdev.c

Quoted 1 line
drivers/net/ena/ena_ethdev.c +0 -0
@@ -590,9 +590,8 @@

ENA_PROXY_DESC(ena_com_get_customer_metrics,

Quoted 6 lines
drivers/net/ena/ena_ethdev.c +0 -3
 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

Quoted 15 lines

* sizeof(uint64_t));

drivers/net/ena/ena_ethdev.c static uint16_t eth_ena_xmit_pkts(void · line 3241

Quoted 9 lines
drivers/net/ena/ena_ethdev.c +2 -0
+     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,

Quoted 3 lines
drivers/net/ena/ena_ethdev.c +1 -1
-                                          size_t num_metrics)
+                                              size_t num_metrics)
Quoted 7 lines
Quoted 1 line

Please drop unrelated changed from the set.

[Brandes, Shai] ack

Quoted 5 lines

drivers/net/ena/ena_ethdev.c static void

Quoted 5 lines
drivers/net/ena/ena_ethdev.c +0 -0
 {
      struct ena_com_dev *ena_dev = &adapter->ena_dev;

      int rc;
drivers/net/ena/ena_ethdev.c +0 -0
@@ -3252,10 +3251,10 @@ static void

ena_copy_customer_metrics(struct

Quoted 31 lines
Quoted 14 lines

ena_adapter *adapter, uint64_t *buf

drivers/net/ena/ena_ethdev.c static void ena_copy_customer_metrics(struct ena_adapter *adapter, uint64_t *buf

Quoted 12 lines
drivers/net/ena/ena_ethdev.c +4 -4
              }
              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));
Quoted 1 line

ditto

[Brandes, Shai] ack

Quoted 15 lines
Quoted 5 lines
drivers/net/ena/ena_ethdev.c +0 -0
              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)

Quoted 7 lines

case ENA_MP_CUSTOMER_METRICS_GET:
res = ena_com_get_customer_metrics(ena_dev,
(char *)adapter->metrics_stats,

drivers/net/ena/ena_ethdev.c +2 -1
-                             sizeof(uint64_t) * adapter->metrics_num);
+                             adapter->metrics_num *
+ sizeof(uint64_t));
Quoted 9 lines
Quoted 1 line

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

Quoted 6 lines
Quoted 6 lines

              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

Quoted 15 lines
Quoted 14 lines

drivers/net/ena/ena_ethdev.h struct ena_adapter { · line 347

Quoted 8 lines

4988fbffb5..17d292101c 100644

drivers/net/ena/ena_ethdev.h +0 -1
@@ -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;

Quoted 5 lines
drivers/net/ena/ena_ethdev.h +2 -1
-     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;  };