diff mbox series

[v2] app/testpmd: fix quit testpmd with vfs and pf

Message ID 20220621092453.185583-1-ke1x.zhang@intel.com (mailing list archive)
State Rejected, archived
Delegated to: Ferruh Yigit
Headers
Series [v2] app/testpmd: fix quit testpmd with vfs and pf |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/github-robot: build success github build: passed
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-abi-testing success Testing PASS

Commit Message

Zhang, Ke1X June 21, 2022, 9:24 a.m. UTC 
  When testpmd startups with pf and vfs,this error occurs when quitting,
results in pf is released before vfs ,so the vf would access an
freed heap memory.

The solution is two steps:
1. Fetch the valid port value from RTE_ETH_FOREACH_DEV.
2. free the port in reverse order.

Fixes: 08fd782b8454 ("app/testpmd: fix quit to stop all ports before close")
Cc: stable@dpdk.org

Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>
---
 app/test-pmd/testpmd.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

Comments

Ferruh Yigit June 21, 2022, 10:43 a.m. UTC | #1 
On 6/21/2022 10:24 AM, Ke Zhang wrote:
> When testpmd startups with pf and vfs,this error occurs when quitting,
> results in pf is released before vfs ,so the vf would access an
> freed heap memory.
> 
> The solution is two steps:
> 1. Fetch the valid port value from RTE_ETH_FOREACH_DEV.
> 2. free the port in reverse order.
> 
> Fixes: 08fd782b8454 ("app/testpmd: fix quit to stop all ports before close")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>

Acked-by: Aman Singh <aman.deep.singh@intel.com>
Acked-by: Ferruh Yigit <ferruh.yigit@xilinx.com>

Applied to dpdk-next-net/main, thanks.


> ---
>   app/test-pmd/testpmd.c | 26 +++++++++++++++++++++-----
>   1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 04c39adc21..c01ecf7279 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3492,16 +3492,32 @@ pmd_test_exit(void)
>   	}
>   #endif
>   	if (ports != NULL) {
> +		portid_t ethports[RTE_MAX_ETHPORTS];
> +		int count = 0;
> +		int index = 0;
 >   		no_link_check = 1;

No need to initialize 'index'.

And better to have an empty line after deceleration block.

> +
> +		/* Fetch the valid port id from port list*/
>   		RTE_ETH_FOREACH_DEV(pt_id) {
> -			printf("\nStopping port %d...\n", pt_id);
> +			ethports[count] = pt_id;
> +			count++;
> +		}
> +
> +		/*
> +		 * Free the port from Reverse order, as general,
> +		 * PF port < VF port， VF should be free before PF
> +		 * be free.
> +		 */
> +		for (index = count - 1 ; index >= 0 ; index--) {

Space is not needed after ';'.


Since these are minor, I will fix while merging.
Thomas Monjalon June 22, 2022, 9:31 p.m. UTC | #2 
Sorry, this patch looks really wrong, I cannot pull it into the main branch.
See below for an explanation.


21/06/2022 12:43, Ferruh Yigit:
> On 6/21/2022 10:24 AM, Ke Zhang wrote:
> > When testpmd startups with pf and vfs,this error occurs when quitting,
> > results in pf is released before vfs ,so the vf would access an
> > freed heap memory.
> > 
> > The solution is two steps:
> > 1. Fetch the valid port value from RTE_ETH_FOREACH_DEV.
> > 2. free the port in reverse order.
> > 
> > Fixes: 08fd782b8454 ("app/testpmd: fix quit to stop all ports before close")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>
> 
> Acked-by: Aman Singh <aman.deep.singh@intel.com>
> Acked-by: Ferruh Yigit <ferruh.yigit@xilinx.com>
> 
> Applied to dpdk-next-net/main, thanks.
> 
> 
> > ---
> >   app/test-pmd/testpmd.c | 26 +++++++++++++++++++++-----
> >   1 file changed, 21 insertions(+), 5 deletions(-)
> > 
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> > index 04c39adc21..c01ecf7279 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -3492,16 +3492,32 @@ pmd_test_exit(void)
> >   	}
> >   #endif
> >   	if (ports != NULL) {
> > +		portid_t ethports[RTE_MAX_ETHPORTS];
> > +		int count = 0;
> > +		int index = 0;
>  >   		no_link_check = 1;
> 
> No need to initialize 'index'.
> 
> And better to have an empty line after deceleration block.
> 
> > +
> > +		/* Fetch the valid port id from port list*/
> >   		RTE_ETH_FOREACH_DEV(pt_id) {
> > -			printf("\nStopping port %d...\n", pt_id);
> > +			ethports[count] = pt_id;
> > +			count++;
> > +		}
> > +
> > +		/*
> > +		 * Free the port from Reverse order, as general,
> > +		 * PF port < VF port， VF should be free before PF
> > +		 * be free.

No sorry, this is wrong.
The port ID says nothing.
If a port is closed before creating a VF,
it can be VF < PF.

> > +		 */
> > +		for (index = count - 1 ; index >= 0 ; index--) {
> 
> Space is not needed after ';'.
> 
> 
> Since these are minor, I will fix while merging.
Thomas Monjalon June 23, 2022, 6:41 a.m. UTC | #3 
Adding more about this patch:

testpmd is a test application. It allows you to find some issues
in some scenarios. It seems you found an issue and you decided
to hide it in the test application. That's really wrong!

This kind of issue should be solved in the API or in the driver.
In this case, I believe it should be handled in the driver
which should release dependent ports.


22/06/2022 23:31, Thomas Monjalon:
> Sorry, this patch looks really wrong, I cannot pull it into the main branch.
> See below for an explanation.
> 
> 
> 21/06/2022 12:43, Ferruh Yigit:
> > On 6/21/2022 10:24 AM, Ke Zhang wrote:
> > > When testpmd startups with pf and vfs,this error occurs when quitting,
> > > results in pf is released before vfs ,so the vf would access an
> > > freed heap memory.
> > > 
> > > The solution is two steps:
> > > 1. Fetch the valid port value from RTE_ETH_FOREACH_DEV.
> > > 2. free the port in reverse order.
> > > 
> > > Fixes: 08fd782b8454 ("app/testpmd: fix quit to stop all ports before close")
> > > Cc: stable@dpdk.org
> > > 
> > > Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>
> > 
> > Acked-by: Aman Singh <aman.deep.singh@intel.com>
> > Acked-by: Ferruh Yigit <ferruh.yigit@xilinx.com>
> > 
> > Applied to dpdk-next-net/main, thanks.
> > 
> > 
> > > ---
> > >   app/test-pmd/testpmd.c | 26 +++++++++++++++++++++-----
> > >   1 file changed, 21 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> > > index 04c39adc21..c01ecf7279 100644
> > > --- a/app/test-pmd/testpmd.c
> > > +++ b/app/test-pmd/testpmd.c
> > > @@ -3492,16 +3492,32 @@ pmd_test_exit(void)
> > >   	}
> > >   #endif
> > >   	if (ports != NULL) {
> > > +		portid_t ethports[RTE_MAX_ETHPORTS];
> > > +		int count = 0;
> > > +		int index = 0;
> >  >   		no_link_check = 1;
> > 
> > No need to initialize 'index'.
> > 
> > And better to have an empty line after deceleration block.
> > 
> > > +
> > > +		/* Fetch the valid port id from port list*/
> > >   		RTE_ETH_FOREACH_DEV(pt_id) {
> > > -			printf("\nStopping port %d...\n", pt_id);
> > > +			ethports[count] = pt_id;
> > > +			count++;
> > > +		}
> > > +
> > > +		/*
> > > +		 * Free the port from Reverse order, as general,
> > > +		 * PF port < VF port， VF should be free before PF
> > > +		 * be free.
> 
> No sorry, this is wrong.
> The port ID says nothing.
> If a port is closed before creating a VF,
> it can be VF < PF.
> 
> > > +		 */
> > > +		for (index = count - 1 ; index >= 0 ; index--) {
> > 
> > Space is not needed after ';'.
> > 
> > 
> > Since these are minor, I will fix while merging.
diff mbox series

Patch

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 04c39adc21..c01ecf7279 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -3492,16 +3492,32 @@  pmd_test_exit(void)
 	}
 #endif
 	if (ports != NULL) {
+		portid_t ethports[RTE_MAX_ETHPORTS];
+		int count = 0;
+		int index = 0;
 		no_link_check = 1;
+
+		/* Fetch the valid port id from port list*/
 		RTE_ETH_FOREACH_DEV(pt_id) {
-			printf("\nStopping port %d...\n", pt_id);
+			ethports[count] = pt_id;
+			count++;
+		}
+
+		/*
+		 * Free the port from Reverse order, as general,
+		 * PF port < VF port， VF should be free before PF
+		 * be free.
+		 */
+		for (index = count - 1 ; index >= 0 ; index--) {
+			printf("\nStopping port %d...\n", ethports[index]);
 			fflush(stdout);
-			stop_port(pt_id);
+			stop_port(ethports[index]);
 		}
-		RTE_ETH_FOREACH_DEV(pt_id) {
-			printf("\nShutting down port %d...\n", pt_id);
+
+		for (index = count - 1 ; index >= 0 ; index--) {
+			printf("\nShutting down port %d...\n", ethports[index]);
 			fflush(stdout);
-			close_port(pt_id);
+			close_port(ethports[index]);
 		}
 	}