[PATCH v2 1/2] lib/ethdev: add reverse macro to quit testpmd

[PATCH v2 1/2] lib/ethdev: add reverse macro to quit testpmd

[wenxuanx.wu]

From: wenxuan wu <wenxuanx.wu@intel.com>

There is a heap-free-after-use bug when quit testpmd
with pf and vfs, stop and close ports in reverse order
is a more reasonable approach.

Cc: stable@dpdk.org

Signed-off-by: wenxuan wu <wenxuanx.wu@intel.com>
---
 lib/ethdev/rte_ethdev.h | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 2660e4f374..813f72e825 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -2185,7 +2185,8 @@ struct rte_eth_dev_owner {
  * @return
  *   Next valid port ID owned by owner_id, RTE_MAX_ETHPORTS if there is none.
  */
-uint64_t rte_eth_find_next_owned_by(uint16_t port_id,
+uint64_t
+rte_eth_find_next_owned_by(uint16_t port_id,
 		const uint64_t owner_id);
 
 /**
@@ -2212,6 +2213,14 @@ uint16_t rte_eth_find_next(uint16_t port_id);
 #define RTE_ETH_FOREACH_DEV(p) \
 	RTE_ETH_FOREACH_DEV_OWNED_BY(p, RTE_ETH_DEV_NO_OWNER)
 
+/**
+ * Macro to iterate over all enabled and ownerless ethdev ports in reverse order.
+ */
+#define RTE_ETH_FOREACH_DEV_REVERSE(p) \
+	for (p = rte_eth_dev_count_total() - 1; \
+	     p < rte_eth_dev_count_total(); \
+	     p--)
+
 /**
  * Iterates over ethdev ports of a specified device.
  *
-- 
2.25.1

[PATCH v2 2/2] app/testpmd:fix testpmd quit failure

[wenxuanx.wu]

From: wenxuan wu <wenxuanx.wu@intel.com>

When testpmd start ed with 1 pf and 2 vfs, testpmd quited while vfs 
were still alive would result in failure. Root cause is that pf had
been released already but vfs were still accessing by func 
rte_eth_dev_info_get, which would result in heap-free-after-use error.

By quitting our ports in reverse order to avoid this.And the order is
guaranteed that vf are created after pfs.

Fixes: d3a274ce9dee ("app/testpmd: handle SIGINT and SIGTERM")
Cc: stable@dpdk.org

Signed-off-by: wenxuan wu <wenxuanx.wu@intel.com>
---
 app/test-pmd/testpmd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index e1da961311..698b6d8cc4 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -3384,12 +3384,12 @@ pmd_test_exit(void)
 #endif
 	if (ports != NULL) {
 		no_link_check = 1;
-		RTE_ETH_FOREACH_DEV(pt_id) {
+		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
 			printf("\nStopping port %d...\n", pt_id);
 			fflush(stdout);
 			stop_port(pt_id);
 		}
-		RTE_ETH_FOREACH_DEV(pt_id) {
+		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
 			printf("\nShutting down port %d...\n", pt_id);
 			fflush(stdout);
 			close_port(pt_id);
-- 
2.25.1

Re: [PATCH v2 2/2] app/testpmd:fix testpmd quit failure

[Ferruh Yigit]

On 2/23/2022 11:32 AM, wenxuanx.wu@intel.com wrote:
> From: wenxuan wu <wenxuanx.wu@intel.com>
> 
> When testpmd start ed with 1 pf and 2 vfs, testpmd quited while vfs
> were still alive would result in failure. Root cause is that pf had
> been released already but vfs were still accessing by func
> rte_eth_dev_info_get, which would result in heap-free-after-use error.
> 
> By quitting our ports in reverse order to avoid this.And the order is
> guaranteed that vf are created after pfs.
> 
> Fixes: d3a274ce9dee ("app/testpmd: handle SIGINT and SIGTERM")
> Cc: stable@dpdk.org
> 
> Signed-off-by: wenxuan wu <wenxuanx.wu@intel.com>
> ---
>   app/test-pmd/testpmd.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index e1da961311..698b6d8cc4 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3384,12 +3384,12 @@ pmd_test_exit(void)
>   #endif
>   	if (ports != NULL) {
>   		no_link_check = 1;
> -		RTE_ETH_FOREACH_DEV(pt_id) {
> +		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {

The main problem with this patch was this logic,
can you please check comment on previous version?

>   			printf("\nStopping port %d...\n", pt_id);
>   			fflush(stdout);
>   			stop_port(pt_id);
>   		}
> -		RTE_ETH_FOREACH_DEV(pt_id) {
> +		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
>   			printf("\nShutting down port %d...\n", pt_id);
>   			fflush(stdout);
>   			close_port(pt_id);

[PATCH v3] app/testpmd: fix heap-free-before-use when quit

[wenxuanx.wu]

From: wenxuan wu <wenxuanx.wu@intel.com>

Change func logic is_bonding_slave and set_port_slave_flag to avoid this
error.Port->slave_flag with dev_flag check to ensure slave_flag is securely
set.

Remove eth_dev_info_get in func is_bonding_slave, which would lead to
a heap-free-before-use error the reason is that vf was still accessing a
freed pf resource.

Fixes: 0a0821bcf312 ("app/testpmd: remove most uses of internal ethdev array")
Cc: stable@dpdk.org

Signed-off-by: Wenxuan Wu <wenxuanx.wu@intel.com>
---
 app/test-pmd/testpmd.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index e1da961311..dcba11f7a6 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -3805,15 +3805,23 @@ init_port_config(void)
 	}
 }
 
-void set_port_slave_flag(portid_t slave_pid)
+void
+set_port_slave_flag(portid_t slave_pid)
 {
 	struct rte_port *port;
-
+	struct rte_eth_dev_info dev_info;
+	int ret;
 	port = &ports[slave_pid];
-	port->slave_flag = 1;
+	ret = eth_dev_info_get_print_err(slave_pid, &dev_info);
+	if (ret != 0)
+		return;
+
+	if (*dev_info.dev_flags & RTE_ETH_DEV_BONDED_SLAVE)
+		port->slave_flag = 1;
 }
 
-void clear_port_slave_flag(portid_t slave_pid)
+void
+clear_port_slave_flag(portid_t slave_pid)
 {
 	struct rte_port *port;
 
@@ -3821,26 +3829,17 @@ void clear_port_slave_flag(portid_t slave_pid)
 	port->slave_flag = 0;
 }
 
-uint8_t port_is_bonding_slave(portid_t slave_pid)
+uint8_t
+port_is_bonding_slave(portid_t slave_pid)
 {
 	struct rte_port *port;
-	struct rte_eth_dev_info dev_info;
-	int ret;
-
 	port = &ports[slave_pid];
-	ret = eth_dev_info_get_print_err(slave_pid, &dev_info);
-	if (ret != 0) {
-		TESTPMD_LOG(ERR,
-			"Failed to get device info for port id %d,"
-			"cannot determine if the port is a bonded slave",
-			slave_pid);
-		return 0;
-	}
-	if ((*dev_info.dev_flags & RTE_ETH_DEV_BONDED_SLAVE) || (port->slave_flag == 1))
+	if (port->slave_flag == 1)
 		return 1;
 	return 0;
 }
 
+
 const uint16_t vlan_tags[] = {
 		0,  1,  2,  3,  4,  5,  6,  7,
 		8,  9, 10, 11,  12, 13, 14, 15,
-- 
2.25.1

[PATCH v3] app/testpmd: fix heap-free-before-use when quit

[wenxuanx.wu]

From: wenxuan wu <wenxuanx.wu@intel.com>

Change func logic is_bonding_slave and set_port_slave_flag to avoid this
error.Port->slave_flag with dev_flag check to ensure slave_flag is securely
set.

Remove eth_dev_info_get in func is_bonding_slave, which would lead to
a heap-free-before-use error the reason is that vf was still accessing a
freed pf resource.

Fixes: 0a0821bcf312 ("app/testpmd: remove most uses of internal ethdev array")
Cc: stable@dpdk.org

Signed-off-by: Wenxuan Wu <wenxuanx.wu@intel.com>
---
 app/test-pmd/testpmd.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index e1da961311..dcba11f7a6 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -3805,15 +3805,23 @@ init_port_config(void)
 	}
 }
 
-void set_port_slave_flag(portid_t slave_pid)
+void
+set_port_slave_flag(portid_t slave_pid)
 {
 	struct rte_port *port;
-
+	struct rte_eth_dev_info dev_info;
+	int ret;
 	port = &ports[slave_pid];
-	port->slave_flag = 1;
+	ret = eth_dev_info_get_print_err(slave_pid, &dev_info);
+	if (ret != 0)
+		return;
+
+	if (*dev_info.dev_flags & RTE_ETH_DEV_BONDED_SLAVE)
+		port->slave_flag = 1;
 }
 
-void clear_port_slave_flag(portid_t slave_pid)
+void
+clear_port_slave_flag(portid_t slave_pid)
 {
 	struct rte_port *port;
 
@@ -3821,26 +3829,17 @@ void clear_port_slave_flag(portid_t slave_pid)
 	port->slave_flag = 0;
 }
 
-uint8_t port_is_bonding_slave(portid_t slave_pid)
+uint8_t
+port_is_bonding_slave(portid_t slave_pid)
 {
 	struct rte_port *port;
-	struct rte_eth_dev_info dev_info;
-	int ret;
-
 	port = &ports[slave_pid];
-	ret = eth_dev_info_get_print_err(slave_pid, &dev_info);
-	if (ret != 0) {
-		TESTPMD_LOG(ERR,
-			"Failed to get device info for port id %d,"
-			"cannot determine if the port is a bonded slave",
-			slave_pid);
-		return 0;
-	}
-	if ((*dev_info.dev_flags & RTE_ETH_DEV_BONDED_SLAVE) || (port->slave_flag == 1))
+	if (port->slave_flag == 1)
 		return 1;
 	return 0;
 }
 
+
 const uint16_t vlan_tags[] = {
 		0,  1,  2,  3,  4,  5,  6,  7,
 		8,  9, 10, 11,  12, 13, 14, 15,
-- 
2.25.1

[PATCH v3] app/testpmd: fix heap-free-before-use when quit

[wenxuanx.wu]

From: wenxuan wu <wenxuanx.wu@intel.com>

Change func logic is_bonding_slave and set_port_slave_flag to avoid this
error. Port->slave_flag with dev_flag check to ensure slave_flag is securely
set.

Remove eth_dev_info_get in func is_bonding_slave, which would lead to
a heap-free-before-use error the reason is that vf was still accessing a
freed pf resource.

Fixes: 0a0821bcf312 ("app/testpmd: remove most uses of internal ethdev array")
Cc: stable@dpdk.org

Signed-off-by: Wenxuan Wu <wenxuanx.wu@intel.com>
---
 ...d-fix-heap-free-before-use-when-quit.patch | 87 +++++++++++++++++++
 app/test-pmd/testpmd.c                        | 31 +++----
 2 files changed, 101 insertions(+), 17 deletions(-)
 create mode 100644 0001-app-testpmd-fix-heap-free-before-use-when-quit.patch

diff --git a/0001-app-testpmd-fix-heap-free-before-use-when-quit.patch b/0001-app-testpmd-fix-heap-free-before-use-when-quit.patch
new file mode 100644
index 0000000000..70d7fc8342
--- /dev/null
+++ b/0001-app-testpmd-fix-heap-free-before-use-when-quit.patch
@@ -0,0 +1,87 @@
+From 78fdeab26f9f01bdee1ca5648c1314acb5a4e6fe Mon Sep 17 00:00:00 2001
+From: wenxuan wu <wenxuanx.wu@intel.com>
+Date: Tue, 1 Mar 2022 14:07:37 +0800
+Subject: [DPDK] app/testpmd: fix heap-free-before-use when quit
+
+Change func logic is_bonding_slave and set_port_slave_flag to avoid this
+error.Port->slave_flag with dev_flag check to ensure slave_flag is securely
+set.
+
+Remove eth_dev_info_get in func is_bonding_slave, which would lead to
+a heap-free-before-use error the reason is that vf was still accessing a
+freed pf resource.
+
+Fixes: 0a0821bcf312 ("app/testpmd: remove most uses of internal ethdev array")
+Cc: stable@dpdk.org
+
+Signed-off-by: Wenxuan Wu <wenxuanx.wu@intel.com>
+---
+ app/test-pmd/testpmd.c | 33 ++++++++++++++++-----------------
+ 1 file changed, 16 insertions(+), 17 deletions(-)
+
+diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
+index e1da961311..dcba11f7a6 100644
+--- a/app/test-pmd/testpmd.c
++++ b/app/test-pmd/testpmd.c
+@@ -3805,15 +3805,23 @@ init_port_config(void)
+ 	}
+ }
+ 
+-void set_port_slave_flag(portid_t slave_pid)
++void
++set_port_slave_flag(portid_t slave_pid)
+ {
+ 	struct rte_port *port;
+-
++	struct rte_eth_dev_info dev_info;
++	int ret;
+ 	port = &ports[slave_pid];
+-	port->slave_flag = 1;
++	ret = eth_dev_info_get_print_err(slave_pid, &dev_info);
++	if (ret != 0)
++		return;
++
++	if (*dev_info.dev_flags & RTE_ETH_DEV_BONDED_SLAVE)
++		port->slave_flag = 1;
+ }
+ 
+-void clear_port_slave_flag(portid_t slave_pid)
++void
++clear_port_slave_flag(portid_t slave_pid)
+ {
+ 	struct rte_port *port;
+ 
+@@ -3821,26 +3829,17 @@ void clear_port_slave_flag(portid_t slave_pid)
+ 	port->slave_flag = 0;
+ }
+ 
+-uint8_t port_is_bonding_slave(portid_t slave_pid)
++uint8_t
++port_is_bonding_slave(portid_t slave_pid)
+ {
+ 	struct rte_port *port;
+-	struct rte_eth_dev_info dev_info;
+-	int ret;
+-
+ 	port = &ports[slave_pid];
+-	ret = eth_dev_info_get_print_err(slave_pid, &dev_info);
+-	if (ret != 0) {
+-		TESTPMD_LOG(ERR,
+-			"Failed to get device info for port id %d,"
+-			"cannot determine if the port is a bonded slave",
+-			slave_pid);
+-		return 0;
+-	}
+-	if ((*dev_info.dev_flags & RTE_ETH_DEV_BONDED_SLAVE) || (port->slave_flag == 1))
++	if (port->slave_flag == 1)
+ 		return 1;
+ 	return 0;
+ }
+ 
++
+ const uint16_t vlan_tags[] = {
+ 		0,  1,  2,  3,  4,  5,  6,  7,
+ 		8,  9, 10, 11,  12, 13, 14, 15,
+-- 
+2.25.1
+
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index e1da961311..6f7ad974b6 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -3805,15 +3805,22 @@ init_port_config(void)
 	}
 }
 
-void set_port_slave_flag(portid_t slave_pid)
+void
+set_port_slave_flag(portid_t slave_pid)
 {
 	struct rte_port *port;
-
+	struct rte_eth_dev_info dev_info;
+	int ret;
 	port = &ports[slave_pid];
-	port->slave_flag = 1;
+	ret = eth_dev_info_get_print_err(slave_pid, &dev_info);
+	if (ret != 0)
+		return;
+	if (*dev_info.dev_flags & RTE_ETH_DEV_BONDED_SLAVE)
+		port->slave_flag = 1;
 }
 
-void clear_port_slave_flag(portid_t slave_pid)
+void
+clear_port_slave_flag(portid_t slave_pid)
 {
 	struct rte_port *port;
 
@@ -3821,22 +3828,12 @@ void clear_port_slave_flag(portid_t slave_pid)
 	port->slave_flag = 0;
 }
 
-uint8_t port_is_bonding_slave(portid_t slave_pid)
+uint8_t
+port_is_bonding_slave(portid_t slave_pid)
 {
 	struct rte_port *port;
-	struct rte_eth_dev_info dev_info;
-	int ret;
-
 	port = &ports[slave_pid];
-	ret = eth_dev_info_get_print_err(slave_pid, &dev_info);
-	if (ret != 0) {
-		TESTPMD_LOG(ERR,
-			"Failed to get device info for port id %d,"
-			"cannot determine if the port is a bonded slave",
-			slave_pid);
-		return 0;
-	}
-	if ((*dev_info.dev_flags & RTE_ETH_DEV_BONDED_SLAVE) || (port->slave_flag == 1))
+	if (port->slave_flag == 1)
 		return 1;
 	return 0;
 }
-- 
2.25.1

[PATCH v3] app/testpmd: fix heap-free-before-use when quit

[wenxuanx.wu]

From: wenxuan wu <wenxuanx.wu@intel.com>

Change func logic is_bonding_slave and set_port_slave_flag to avoid this
error. Port->slave_flag with dev_flag check to ensure slave_flag is securely
set.

Remove eth_dev_info_get in func is_bonding_slave, which would lead to
a heap-free-before-use error the reason is that vf was still accessing a
freed pf resource.

Fixes: 0a0821bcf312 ("app/testpmd: remove most uses of internal ethdev array")
Cc: stable@dpdk.org

Signed-off-by: Wenxuan Wu <wenxuanx.wu@intel.com>
---
 ...d-fix-heap-free-before-use-when-quit.patch | 87 +++++++++++++++++++
 app/test-pmd/testpmd.c                        | 31 +++----
 2 files changed, 101 insertions(+), 17 deletions(-)
 create mode 100644 0001-app-testpmd-fix-heap-free-before-use-when-quit.patch

diff --git a/0001-app-testpmd-fix-heap-free-before-use-when-quit.patch b/0001-app-testpmd-fix-heap-free-before-use-when-quit.patch
new file mode 100644
index 0000000000..70d7fc8342
--- /dev/null
+++ b/0001-app-testpmd-fix-heap-free-before-use-when-quit.patch
@@ -0,0 +1,87 @@
+From 78fdeab26f9f01bdee1ca5648c1314acb5a4e6fe Mon Sep 17 00:00:00 2001
+From: wenxuan wu <wenxuanx.wu@intel.com>
+Date: Tue, 1 Mar 2022 14:07:37 +0800
+Subject: [DPDK] app/testpmd: fix heap-free-before-use when quit
+
+Change func logic is_bonding_slave and set_port_slave_flag to avoid this
+error.Port->slave_flag with dev_flag check to ensure slave_flag is securely
+set.
+
+Remove eth_dev_info_get in func is_bonding_slave, which would lead to
+a heap-free-before-use error the reason is that vf was still accessing a
+freed pf resource.
+
+Fixes: 0a0821bcf312 ("app/testpmd: remove most uses of internal ethdev array")
+Cc: stable@dpdk.org
+
+Signed-off-by: Wenxuan Wu <wenxuanx.wu@intel.com>
+---
+ app/test-pmd/testpmd.c | 33 ++++++++++++++++-----------------
+ 1 file changed, 16 insertions(+), 17 deletions(-)
+
+diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
+index e1da961311..dcba11f7a6 100644
+--- a/app/test-pmd/testpmd.c
++++ b/app/test-pmd/testpmd.c
+@@ -3805,15 +3805,23 @@ init_port_config(void)
+ 	}
+ }
+ 
+-void set_port_slave_flag(portid_t slave_pid)
++void
++set_port_slave_flag(portid_t slave_pid)
+ {
+ 	struct rte_port *port;
+-
++	struct rte_eth_dev_info dev_info;
++	int ret;
+ 	port = &ports[slave_pid];
+-	port->slave_flag = 1;
++	ret = eth_dev_info_get_print_err(slave_pid, &dev_info);
++	if (ret != 0)
++		return;
++
++	if (*dev_info.dev_flags & RTE_ETH_DEV_BONDED_SLAVE)
++		port->slave_flag = 1;
+ }
+ 
+-void clear_port_slave_flag(portid_t slave_pid)
++void
++clear_port_slave_flag(portid_t slave_pid)
+ {
+ 	struct rte_port *port;
+ 
+@@ -3821,26 +3829,17 @@ void clear_port_slave_flag(portid_t slave_pid)
+ 	port->slave_flag = 0;
+ }
+ 
+-uint8_t port_is_bonding_slave(portid_t slave_pid)
++uint8_t
++port_is_bonding_slave(portid_t slave_pid)
+ {
+ 	struct rte_port *port;
+-	struct rte_eth_dev_info dev_info;
+-	int ret;
+-
+ 	port = &ports[slave_pid];
+-	ret = eth_dev_info_get_print_err(slave_pid, &dev_info);
+-	if (ret != 0) {
+-		TESTPMD_LOG(ERR,
+-			"Failed to get device info for port id %d,"
+-			"cannot determine if the port is a bonded slave",
+-			slave_pid);
+-		return 0;
+-	}
+-	if ((*dev_info.dev_flags & RTE_ETH_DEV_BONDED_SLAVE) || (port->slave_flag == 1))
++	if (port->slave_flag == 1)
+ 		return 1;
+ 	return 0;
+ }
+ 
++
+ const uint16_t vlan_tags[] = {
+ 		0,  1,  2,  3,  4,  5,  6,  7,
+ 		8,  9, 10, 11,  12, 13, 14, 15,
+-- 
+2.25.1
+
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index e1da961311..6f7ad974b6 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -3805,15 +3805,22 @@ init_port_config(void)
 	}
 }
 
-void set_port_slave_flag(portid_t slave_pid)
+void
+set_port_slave_flag(portid_t slave_pid)
 {
 	struct rte_port *port;
-
+	struct rte_eth_dev_info dev_info;
+	int ret;
 	port = &ports[slave_pid];
-	port->slave_flag = 1;
+	ret = eth_dev_info_get_print_err(slave_pid, &dev_info);
+	if (ret != 0)
+		return;
+	if (*dev_info.dev_flags & RTE_ETH_DEV_BONDED_SLAVE)
+		port->slave_flag = 1;
 }
 
-void clear_port_slave_flag(portid_t slave_pid)
+void
+clear_port_slave_flag(portid_t slave_pid)
 {
 	struct rte_port *port;
 
@@ -3821,22 +3828,12 @@ void clear_port_slave_flag(portid_t slave_pid)
 	port->slave_flag = 0;
 }
 
-uint8_t port_is_bonding_slave(portid_t slave_pid)
+uint8_t
+port_is_bonding_slave(portid_t slave_pid)
 {
 	struct rte_port *port;
-	struct rte_eth_dev_info dev_info;
-	int ret;
-
 	port = &ports[slave_pid];
-	ret = eth_dev_info_get_print_err(slave_pid, &dev_info);
-	if (ret != 0) {
-		TESTPMD_LOG(ERR,
-			"Failed to get device info for port id %d,"
-			"cannot determine if the port is a bonded slave",
-			slave_pid);
-		return 0;
-	}
-	if ((*dev_info.dev_flags & RTE_ETH_DEV_BONDED_SLAVE) || (port->slave_flag == 1))
+	if (port->slave_flag == 1)
 		return 1;
 	return 0;
 }
-- 
2.25.1

RE: [PATCH v2 2/2] app/testpmd:fix testpmd quit failure

[Wu, WenxuanX]

I found this meaning in DPDK testplan. 
Note that currently hot-plugging of representor ports is not supported so all the required representors must be specified on the creation of the PF or the trusted VF.
When testpmd is started with pf and vf representors, the order of representor is determined on creation. So it is guaranteed that ,pf is beneath the vf representors, we implemented in a reverse way is acceptable just at present,  depends on when the hot-plugging of representor is supported.  

> -----Original Message-----
> From: Wu, WenxuanX <wenxuanx.wu@intel.com>
> Sent: 2022年2月23日 19:33
> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; dev@dpdk.org
> Cc: Wu, WenxuanX <wenxuanx.wu@intel.com>; stable@dpdk.org
> Subject: [PATCH v2 2/2] app/testpmd:fix testpmd quit failure
> 
> From: wenxuan wu <wenxuanx.wu@intel.com>
> 
> When testpmd start ed with 1 pf and 2 vfs, testpmd quited while vfs were
> still alive would result in failure. Root cause is that pf had been released
> already but vfs were still accessing by func rte_eth_dev_info_get, which
> would result in heap-free-after-use error.
> 
> By quitting our ports in reverse order to avoid this.And the order is
> guaranteed that vf are created after pfs.
> 
> Fixes: d3a274ce9dee ("app/testpmd: handle SIGINT and SIGTERM")
> Cc: stable@dpdk.org
> 
> Signed-off-by: wenxuan wu <wenxuanx.wu@intel.com>
> ---
>  app/test-pmd/testpmd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> e1da961311..698b6d8cc4 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3384,12 +3384,12 @@ pmd_test_exit(void)  #endif
>  	if (ports != NULL) {
>  		no_link_check = 1;
> -		RTE_ETH_FOREACH_DEV(pt_id) {
> +		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
>  			printf("\nStopping port %d...\n", pt_id);
>  			fflush(stdout);
>  			stop_port(pt_id);
>  		}
> -		RTE_ETH_FOREACH_DEV(pt_id) {
> +		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
>  			printf("\nShutting down port %d...\n", pt_id);
>  			fflush(stdout);
>  			close_port(pt_id);
> --
> 2.25.1

Re: [PATCH v2 2/2] app/testpmd:fix testpmd quit failure

[Ferruh Yigit]

On 3/3/2022 1:22 PM, Wu, WenxuanX wrote:

moved down, please don't top post

>> -----Original Message-----
>> From: Wu, WenxuanX <wenxuanx.wu@intel.com>
>> Sent: 2022年2月23日 19:33
>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
>> <ferruh.yigit@intel.com>; dev@dpdk.org
>> Cc: Wu, WenxuanX <wenxuanx.wu@intel.com>; stable@dpdk.org
>> Subject: [PATCH v2 2/2] app/testpmd:fix testpmd quit failure
>>
>> From: wenxuan wu <wenxuanx.wu@intel.com>
>>
>> When testpmd start ed with 1 pf and 2 vfs, testpmd quited while vfs were
>> still alive would result in failure. Root cause is that pf had been released
>> already but vfs were still accessing by func rte_eth_dev_info_get, which
>> would result in heap-free-after-use error.
>>
>> By quitting our ports in reverse order to avoid this.And the order is
>> guaranteed that vf are created after pfs.
>>
>> Fixes: d3a274ce9dee ("app/testpmd: handle SIGINT and SIGTERM")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: wenxuan wu <wenxuanx.wu@intel.com>
>> ---
>>   app/test-pmd/testpmd.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>> e1da961311..698b6d8cc4 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -3384,12 +3384,12 @@ pmd_test_exit(void)  #endif
>>   	if (ports != NULL) {
>>   		no_link_check = 1;
>> -		RTE_ETH_FOREACH_DEV(pt_id) {
>> +		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
>>   			printf("\nStopping port %d...\n", pt_id);
>>   			fflush(stdout);
>>   			stop_port(pt_id);
>>   		}
>> -		RTE_ETH_FOREACH_DEV(pt_id) {
>> +		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
>>   			printf("\nShutting down port %d...\n", pt_id);
>>   			fflush(stdout);
>>   			close_port(pt_id);
>> --
>> 2.25.1
> 
> 
> I found this meaning in DPDK testplan.
> Note that currently hot-plugging of representor ports is not supported so all the required representors must be specified on the creation of the PF or the trusted VF.
> When testpmd is started with pf and vf representors, the order of representor is determined on creation. So it is guaranteed that ,pf is beneath the vf representors, we implemented in a reverse way is acceptable just at present,  depends on when the hot-plugging of representor is supported.
> 

The patch mentions from PF and VFs, and now you are referring
to port representor.
Is the problem related to VF or port representor.

For both, VF and port reporesentor should be closed before
PF, that part is OK. My comment is if reversing port id
traverse will fix the issue or do we need more complex
solution.
Like have APIs to get VF and representor ports from a given
port id, and free them first.

RE: [PATCH v2 2/2] app/testpmd:fix testpmd quit failure

[Wu, WenxuanX]

> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: 2022年3月5日 0:16
> To: Wu, WenxuanX <wenxuanx.wu@intel.com>; Li, Xiaoyun
> <xiaoyun.li@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: Re: [PATCH v2 2/2] app/testpmd:fix testpmd quit failure
> 
> On 3/3/2022 1:22 PM, Wu, WenxuanX wrote:
> 
> moved down, please don't top post
> 
> >> -----Original Message-----
> >> From: Wu, WenxuanX <wenxuanx.wu@intel.com>
> >> Sent: 2022年2月23日 19:33
> >> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
> >> <ferruh.yigit@intel.com>; dev@dpdk.org
> >> Cc: Wu, WenxuanX <wenxuanx.wu@intel.com>; stable@dpdk.org
> >> Subject: [PATCH v2 2/2] app/testpmd:fix testpmd quit failure
> >>
> >> From: wenxuan wu <wenxuanx.wu@intel.com>
> >>
> >> When testpmd start ed with 1 pf and 2 vfs, testpmd quited while vfs
> >> were still alive would result in failure. Root cause is that pf had
> >> been released already but vfs were still accessing by func
> >> rte_eth_dev_info_get, which would result in heap-free-after-use error.
> >>
> >> By quitting our ports in reverse order to avoid this.And the order is
> >> guaranteed that vf are created after pfs.
> >>
> >> Fixes: d3a274ce9dee ("app/testpmd: handle SIGINT and SIGTERM")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: wenxuan wu <wenxuanx.wu@intel.com>
> >> ---
> >>   app/test-pmd/testpmd.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> >> e1da961311..698b6d8cc4 100644
> >> --- a/app/test-pmd/testpmd.c
> >> +++ b/app/test-pmd/testpmd.c
> >> @@ -3384,12 +3384,12 @@ pmd_test_exit(void)  #endif
> >>   	if (ports != NULL) {
> >>   		no_link_check = 1;
> >> -		RTE_ETH_FOREACH_DEV(pt_id) {
> >> +		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
> >>   			printf("\nStopping port %d...\n", pt_id);
> >>   			fflush(stdout);
> >>   			stop_port(pt_id);
> >>   		}
> >> -		RTE_ETH_FOREACH_DEV(pt_id) {
> >> +		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
> >>   			printf("\nShutting down port %d...\n", pt_id);
> >>   			fflush(stdout);
> >>   			close_port(pt_id);
> >> --
> >> 2.25.1
> >
> >
> > I found this meaning in DPDK testplan.
> > Note that currently hot-plugging of representor ports is not supported so all
> the required representors must be specified on the creation of the PF or the
> trusted VF.
> > When testpmd is started with pf and vf representors, the order of
> representor is determined on creation. So it is guaranteed that ,pf is beneath
> the vf representors, we implemented in a reverse way is acceptable just at
> present,  depends on when the hot-plugging of representor is supported.
> >
> 
> The patch mentions from PF and VFs, and now you are referring to port
> representor.
> Is the problem related to VF or port representor.
> 
> For both, VF and port reporesentor should be closed before PF, that part is
> OK. My comment is if reversing port id traverse will fix the issue or do we
> need more complex solution.
> Like have APIs to get VF and representor ports from a given port id, and free
> them first.
This patch did fix the issue ,and was verified.But it was rejected.

RE: [PATCH v2 2/2] app/testpmd:fix testpmd quit failure

[Wu, WenxuanX]

> -----Original Message-----
> From: Wu, WenxuanX
> Sent: 2022年3月9日 11:07
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>;
> dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [PATCH v2 2/2] app/testpmd:fix testpmd quit failure
> 
> 
> 
> > -----Original Message-----
> > From: Yigit, Ferruh <ferruh.yigit@intel.com>
> > Sent: 2022年3月5日 0:16
> > To: Wu, WenxuanX <wenxuanx.wu@intel.com>; Li, Xiaoyun
> > <xiaoyun.li@intel.com>; dev@dpdk.org
> > Cc: stable@dpdk.org
> > Subject: Re: [PATCH v2 2/2] app/testpmd:fix testpmd quit failure
> >
> > On 3/3/2022 1:22 PM, Wu, WenxuanX wrote:
> >
> > moved down, please don't top post
> >
> > >> -----Original Message-----
> > >> From: Wu, WenxuanX <wenxuanx.wu@intel.com>
> > >> Sent: 2022年2月23日 19:33
> > >> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
> > >> <ferruh.yigit@intel.com>; dev@dpdk.org
> > >> Cc: Wu, WenxuanX <wenxuanx.wu@intel.com>; stable@dpdk.org
> > >> Subject: [PATCH v2 2/2] app/testpmd:fix testpmd quit failure
> > >>
> > >> From: wenxuan wu <wenxuanx.wu@intel.com>
> > >>
> > >> When testpmd start ed with 1 pf and 2 vfs, testpmd quited while vfs
> > >> were still alive would result in failure. Root cause is that pf had
> > >> been released already but vfs were still accessing by func
> > >> rte_eth_dev_info_get, which would result in heap-free-after-use error.
> > >>
> > >> By quitting our ports in reverse order to avoid this.And the order
> > >> is guaranteed that vf are created after pfs.
> > >>
> > >> Fixes: d3a274ce9dee ("app/testpmd: handle SIGINT and SIGTERM")
> > >> Cc: stable@dpdk.org
> > >>
> > >> Signed-off-by: wenxuan wu <wenxuanx.wu@intel.com>
> > >> ---
> > >>   app/test-pmd/testpmd.c | 4 ++--
> > >>   1 file changed, 2 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > >> e1da961311..698b6d8cc4 100644
> > >> --- a/app/test-pmd/testpmd.c
> > >> +++ b/app/test-pmd/testpmd.c
> > >> @@ -3384,12 +3384,12 @@ pmd_test_exit(void)  #endif
> > >>   	if (ports != NULL) {
> > >>   		no_link_check = 1;
> > >> -		RTE_ETH_FOREACH_DEV(pt_id) {
> > >> +		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
> > >>   			printf("\nStopping port %d...\n", pt_id);
> > >>   			fflush(stdout);
> > >>   			stop_port(pt_id);
> > >>   		}
> > >> -		RTE_ETH_FOREACH_DEV(pt_id) {
> > >> +		RTE_ETH_FOREACH_DEV_REVERSE(pt_id) {
> > >>   			printf("\nShutting down port %d...\n", pt_id);
> > >>   			fflush(stdout);
> > >>   			close_port(pt_id);
> > >> --
> > >> 2.25.1
> > >
> > >
> > > I found this meaning in DPDK testplan.
> > > Note that currently hot-plugging of representor ports is not
> > > supported so all
> > the required representors must be specified on the creation of the PF
> > or the trusted VF.
> > > When testpmd is started with pf and vf representors, the order of
> > representor is determined on creation. So it is guaranteed that ,pf is
> > beneath the vf representors, we implemented in a reverse way is
> > acceptable just at present,  depends on when the hot-plugging of
> representor is supported.
> > >
> >
> > The patch mentions from PF and VFs, and now you are referring to port
> > representor.
> > Is the problem related to VF or port representor.
> >
> > For both, VF and port reporesentor should be closed before PF, that
> > part is OK. My comment is if reversing port id traverse will fix the
> > issue or do we need more complex solution.
> > Like have APIs to get VF and representor ports from a given port id,
> > and free them first.
The problem is that when I attempted to use a func like get_representor_info(pid,info); I didn't found this func implemented by driver ,
so I can not get the type of port(VF or PF ) directly by get_representor_info(pid,info),especially when the connected driver is i40e, 
representor_info_get occurred below.

./drivers/net/mlx5/mlx5.c:      .representor_info_get = mlx5_representor_info_get,
./drivers/net/mlx5/mlx5.c:      .representor_info_get = mlx5_representor_info_get,
./drivers/net/mlx5/mlx5.c:      .representor_info_get = mlx5_representor_info_get,
./drivers/net/mlx5/mlx5.h:int mlx5_representor_info_get(struct rte_eth_dev *dev,
./drivers/net/mlx5/mlx5_ethdev.c:mlx5_representor_info_get(struct rte_eth_dev *dev,
./drivers/net/sfc/sfc_ethdev.c:sfc_representor_info_get(struct rte_eth_dev *dev,
./drivers/net/sfc/sfc_ethdev.c: .representor_info_get           = sfc_representor_info_get,
./app/test-pmd/cmdline.c:       ret = rte_eth_representor_info_get(res->cmd_pid, NULL);
./app/test-pmd/cmdline.c:       ret = rte_eth_representor_info_get(res->cmd_pid, info);
./app/test-pmd/testpmd.c:       ret = rte_eth_representor_info_get(pi,&info);
./lib/ethdev/version.map:       rte_eth_representor_info_get;
./lib/ethdev/rte_ethdev.h:int rte_eth_representor_info_get(uint16_t port_id,
./lib/ethdev/ethdev_driver.h:typedef int (*eth_representor_info_get_t)(struct rte_eth_dev *dev,
./lib/ethdev/ethdev_driver.h:   eth_representor_info_get_t representor_info_get;
./lib/ethdev/ethdev_driver.h: * The mapping is retrieved from rte_eth_representor_info_get().
./lib/ethdev/rte_ethdev.c:      ret = rte_eth_representor_info_get(port_id, NULL);
./lib/ethdev/rte_ethdev.c:      ret = rte_eth_representor_info_get(port_id, info);
./lib/ethdev/rte_ethdev.c:rte_eth_representor_info_get(uint16_t port_id,
./lib/ethdev/rte_ethdev.c:      RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->representor_info_get, -ENOTSUP);
./lib/ethdev/rte_ethdev.c:      return eth_err(port_id, (*dev->dev_ops->representor_info_get)(dev, info));
We should focus on changing the logic of is_bonding_slave to avoid this bug ,right ? The procedure of port_close is like this :
FOR_EACH_DEV(pi):
	If Is_bonding_slave(pi)
		Continue;
	.... 
	Etc.
In is_bonding_slave(pi): 

This func get_dev_info(pid,dev_info) would be called to get dev.dev_flag &SLAVE ,this would 
be result in error ,when port sequence is like port 0  PF ,port 1 VF_REPR, port 2 VF_REPR, there is no 
obstacles in closing port 0 ,then pid iterate to port 1.

But port 1 is a VF_REPR which based on port 0 ,when in func is_bonding_slave(port 1), it would
 call get_dev_info(1,dev_info) ,error occurred. Because we use get_dev_info(especially when PF is released )
 not ports[1] which had been pre allocated . would result in this error.
 Two solutions :
                        1. Reverse order to close like I mentioned before (PF and VF_repr  order is determined at creation time with no representor hot_plug).
                        2.change func get_dev_info(pid,info) to  ports[pid)  to get dev_flag
Both can resolve this bug ,which one do you prefer?
             

[PATCH v3] app/testpmd: fix heap-free-before-use when quit

[wenxuanx.wu]

From: wenxuan wu <wenxuanx.wu@intel.com>

Change func logic is_bonding_slave and set_port_slave_flag to avoid this
error.Port->slave_flag with dev_flag check to ensure slave_flag is securely
set.

Remove eth_dev_info_get in func is_bonding_slave, which would lead to
a heap-free-before-use error the reason is that vf was still accessing a
freed pf resource.

Fixes: 0a0821bcf312 ("app/testpmd: remove most uses of internal ethdev array")
Cc: stable@dpdk.org

Signed-off-by: Wenxuan Wu <wenxuanx.wu@intel.com>
---
 app/test-pmd/testpmd.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index e1da961311..dcba11f7a6 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -3805,15 +3805,23 @@ init_port_config(void)
 	}
 }
 
-void set_port_slave_flag(portid_t slave_pid)
+void
+set_port_slave_flag(portid_t slave_pid)
 {
 	struct rte_port *port;
-
+	struct rte_eth_dev_info dev_info;
+	int ret;
 	port = &ports[slave_pid];
-	port->slave_flag = 1;
+	ret = eth_dev_info_get_print_err(slave_pid, &dev_info);
+	if (ret != 0)
+		return;
+
+	if (*dev_info.dev_flags & RTE_ETH_DEV_BONDED_SLAVE)
+		port->slave_flag = 1;
 }
 
-void clear_port_slave_flag(portid_t slave_pid)
+void
+clear_port_slave_flag(portid_t slave_pid)
 {
 	struct rte_port *port;
 
@@ -3821,26 +3829,17 @@ void clear_port_slave_flag(portid_t slave_pid)
 	port->slave_flag = 0;
 }
 
-uint8_t port_is_bonding_slave(portid_t slave_pid)
+uint8_t
+port_is_bonding_slave(portid_t slave_pid)
 {
 	struct rte_port *port;
-	struct rte_eth_dev_info dev_info;
-	int ret;
-
 	port = &ports[slave_pid];
-	ret = eth_dev_info_get_print_err(slave_pid, &dev_info);
-	if (ret != 0) {
-		TESTPMD_LOG(ERR,
-			"Failed to get device info for port id %d,"
-			"cannot determine if the port is a bonded slave",
-			slave_pid);
-		return 0;
-	}
-	if ((*dev_info.dev_flags & RTE_ETH_DEV_BONDED_SLAVE) || (port->slave_flag == 1))
+	if (port->slave_flag == 1)
 		return 1;
 	return 0;
 }
 
+
 const uint16_t vlan_tags[] = {
 		0,  1,  2,  3,  4,  5,  6,  7,
 		8,  9, 10, 11,  12, 13, 14, 15,
-- 
2.25.1