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

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

[Ke Zhang]

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.

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

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index f7e18aee25..ca6c77b14b 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -3378,8 +3378,11 @@ detach_devargs(char *identifier)
 void
 pmd_test_exit(void)
 {
+	unsigned int ports[RTE_MAX_ETHPORTS];
+	unsigned int count = 0;
 	portid_t pt_id;
 	unsigned int i;
+	int index;
 	int ret;
 
 	if (test_done == 0)
@@ -3396,15 +3399,30 @@ pmd_test_exit(void)
 #endif
 	if (ports != NULL) {
 		no_link_check = 1;
+		i = 0;
+
+		/* Fetch the valid port id from port list*/
 		RTE_ETH_FOREACH_DEV(pt_id) {
-			printf("\nStopping port %d...\n", pt_id);
+			ports[i] = pt_id;
+			i++;
+		}
+
+		count = i;
+		/*
+		 * 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", ports[index]);
 			fflush(stdout);
-			stop_port(pt_id);
+			stop_port(ports[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", ports[index]);
 			fflush(stdout);
-			close_port(pt_id);
+			close_port(ports[index]);
 		}
 	}
 
-- 
2.25.1

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

[Singh, Aman Deep]

On 3/22/2022 12:48 PM, 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.
>
> Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>
> ---
This fix looks similar to series: 21823. Please check Ferruh's comments 
on it.

There is a scenario, where this fix may not work.
"if there is an earlier port id which is released, after that new
vf is created taking that port id. In such case port id of vf < pf port id"

This patch covers most of the cases and that way its an improvement.
So I am ok with it.

Acked-by: Aman Singh <aman.deep.singh@intel.com>
>   app/test-pmd/testpmd.c | 28 +++++++++++++++++++++++-----
>   1 file changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index f7e18aee25..ca6c77b14b 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3378,8 +3378,11 @@ detach_devargs(char *identifier)
>   void
>   pmd_test_exit(void)
>   {
> +	unsigned int ports[RTE_MAX_ETHPORTS];
> +	unsigned int count = 0;
>   	portid_t pt_id;
>   	unsigned int i;
> +	int index;
>   	int ret;
>   
>   	if (test_done == 0)
> @@ -3396,15 +3399,30 @@ pmd_test_exit(void)
>   #endif
>   	if (ports != NULL) {
>   		no_link_check = 1;
> +		i = 0;
> +
> +		/* Fetch the valid port id from port list*/
>   		RTE_ETH_FOREACH_DEV(pt_id) {
> -			printf("\nStopping port %d...\n", pt_id);
> +			ports[i] = pt_id;
> +			i++;
> +		}
> +
> +		count = i;
> +		/*
> +		 * 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", ports[index]);
>   			fflush(stdout);
> -			stop_port(pt_id);
> +			stop_port(ports[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", ports[index]);
>   			fflush(stdout);
> -			close_port(pt_id);
> +			close_port(ports[index]);
>   		}
>   	}
>   

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

[Singh, Aman Deep]

On 3/28/2022 3:36 PM, Singh, Aman Deep wrote:
>
>
> On 3/22/2022 12:48 PM, 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.
>>
>> Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>
>> ---
> This fix looks similar to series: 21823. Please check Ferruh's 
> comments on it.
>
> There is a scenario, where this fix may not work.
> "if there is an earlier port id which is released, after that new
> vf is created taking that port id. In such case port id of vf < pf 
> port id"
>
> This patch covers most of the cases and that way its an improvement.
> So I am ok with it.
>
> Acked-by: Aman Singh <aman.deep.singh@intel.com>
>>   app/test-pmd/testpmd.c | 28 +++++++++++++++++++++++-----
>>   1 file changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>> index f7e18aee25..ca6c77b14b 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -3378,8 +3378,11 @@ detach_devargs(char *identifier)
>>   void
>>   pmd_test_exit(void)
>>   {
>> +    unsigned int ports[RTE_MAX_ETHPORTS];
>> +    unsigned int count = 0;
>>       portid_t pt_id;
>>       unsigned int i;
>> +    int index;
>>       int ret;
>>         if (test_done == 0)
>> @@ -3396,15 +3399,30 @@ pmd_test_exit(void)
>>   #endif
>>       if (ports != NULL) {
>>           no_link_check = 1;
>> +        i = 0;
>> +
>> +        /* Fetch the valid port id from port list*/
>>           RTE_ETH_FOREACH_DEV(pt_id) {
>> -            printf("\nStopping port %d...\n", pt_id);
>> +            ports[i] = pt_id;
>> +            i++;
>> +        }
>> +
>> +        count = i;
>> +        /*
>> +         * 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", ports[index]);
>>               fflush(stdout);
>> -            stop_port(pt_id);
>> +            stop_port(ports[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", ports[index]);
>>               fflush(stdout);
>> -            close_port(pt_id);
>> +            close_port(ports[index]);
>>           }
>>       }
>

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

[Ferruh Yigit]

On 3/28/2022 11:06 AM, Singh, Aman Deep wrote:
> 
> 
> On 3/22/2022 12:48 PM, 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.
>>
>> Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>
>> ---
> This fix looks similar to series: 21823. Please check Ferruh's comments 
> on it.
> 
> There is a scenario, where this fix may not work.
> "if there is an earlier port id which is released, after that new
> vf is created taking that port id. In such case port id of vf < pf port id"
> 
> This patch covers most of the cases and that way its an improvement.
> So I am ok with it.
> 
> Acked-by: Aman Singh <aman.deep.singh@intel.com>

Agree that patch covers most of the cases with simple/small change, so 
OK to continue with it.

@Ke, can you please check minor comments below.

>>   app/test-pmd/testpmd.c | 28 +++++++++++++++++++++++-----
>>   1 file changed, 23 insertions(+), 5 deletions(-)
>>
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
>> index f7e18aee25..ca6c77b14b 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -3378,8 +3378,11 @@ detach_devargs(char *identifier)
>>   void
>>   pmd_test_exit(void)
>>   {
>> +    unsigned int ports[RTE_MAX_ETHPORTS];

testpmd holds port IDs as 'portid_t', so can you please change type form 
'int' to 'portid_t'?

>> +    unsigned int count = 0;

For 'index' & 'count', no reason to make one 'unsigned int' and other 
'int', better to keep type same for both, and both can be 'int'.

>>       portid_t pt_id;
>>       unsigned int i;
>> +    int index;

These new variables only used in one if block, can you please move 
declaring them to that scope, to the beginning of that block.

>>       int ret;
>>       if (test_done == 0)
>> @@ -3396,15 +3399,30 @@ pmd_test_exit(void)
>>   #endif
>>       if (ports != NULL) {
>>           no_link_check = 1;
>> +        i = 0;
>> +
>> +        /* Fetch the valid port id from port list*/
>>           RTE_ETH_FOREACH_DEV(pt_id) {
>> -            printf("\nStopping port %d...\n", pt_id);
>> +            ports[i] = pt_id;
>> +            i++;

Can use 'count' directly, instead of 'i'.

>> +        }
>> +
>> +        count = i;
>> +        /*
>> +         * 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", ports[index]);
>>               fflush(stdout);
>> -            stop_port(pt_id);
>> +            stop_port(ports[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", ports[index]);
>>               fflush(stdout);
>> -            close_port(pt_id);
>> +            close_port(ports[index]);
>>           }
>>       }
> 

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

[Ke Zhang]

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 | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 04c39adc21..67b4941942 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -3491,17 +3491,34 @@ pmd_test_exit(void)
 		}
 	}
 #endif
+	portid_t ethports[RTE_MAX_ETHPORTS];
+	int count;
+	int index;
 	if (ports != NULL) {
 		no_link_check = 1;
+		count = 0;
+
+		/* 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]);
 		}
 	}
 
-- 
2.25.1

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

[Ferruh Yigit]

On 6/21/2022 10:03 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>
> ---
>   app/test-pmd/testpmd.c | 27 ++++++++++++++++++++++-----
>   1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 04c39adc21..67b4941942 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3491,17 +3491,34 @@ pmd_test_exit(void)
>   		}
>   	}
>   #endif
> +	portid_t ethports[RTE_MAX_ETHPORTS];
> +	int count;
> +	int index;

Hi Ke,

These variables only used in the 'if' block, to reduce the scope of them 
can you please move them at the very top of the block:

if (ports != NULL) {
	portid_t ethports[RTE_MAX_ETHPORTS];
	int count = 0;
	int index;

	no_link_check = 1;
...

>   	if (ports != NULL) {
>   		no_link_check = 1;
> +		count = 0;
> +
> +		/* 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]);
>   		}
>   	}
>   

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

[Ke Zhang]

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

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]);
 		}
 	}
 
-- 
2.25.1

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

[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.
> +		 */
> +		for (index = count - 1 ; index >= 0 ; index--) {

Space is not needed after ';'.


Since these are minor, I will fix while merging.

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

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

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

[Thomas Monjalon]

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.

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

[Ke Zhang]

A segmentation fault occurs when testpmd exit.

This is due to fetching the device name from PF
, PF is freed firstly and then VF representor
is called later.

This commit fixes the bug by fetching the device
name from vf representor not PF.

Fixes: e391a7b7f815 ("net/i40e: fix multi-process shared data")
Cc: stable@dpdk.org

Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>

---
v3: Change the design and fix code in driver
v2: Change the testpmd code to fix this issue
---
---
 drivers/net/i40e/i40e_vf_representor.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_vf_representor.c b/drivers/net/i40e/i40e_vf_representor.c
index 7f8e81858e..bcd445bcdd 100644
--- a/drivers/net/i40e/i40e_vf_representor.c
+++ b/drivers/net/i40e/i40e_vf_representor.c
@@ -29,8 +29,6 @@ i40e_vf_representor_dev_infos_get(struct rte_eth_dev *ethdev,
 	struct rte_eth_dev_info *dev_info)
 {
 	struct i40e_vf_representor *representor = ethdev->data->dev_private;
-	struct rte_eth_dev_data *pf_dev_data =
-		representor->adapter->pf.dev_data;
 
 	/* get dev info for the vdev */
 	dev_info->device = ethdev->device;
@@ -104,7 +102,7 @@ i40e_vf_representor_dev_infos_get(struct rte_eth_dev *ethdev,
 	};
 
 	dev_info->switch_info.name =
-		rte_eth_devices[pf_dev_data->port_id].device->name;
+		rte_eth_devices[ethdev->data->port_id].device->name;
 	dev_info->switch_info.domain_id = representor->switch_domain_id;
 	dev_info->switch_info.port_id = representor->vf_id;
 
-- 
2.25.1

Re: [PATCH v3] app/testpmd: fix quit testpmd with vfs and pf

[Thomas Monjalon]

13/07/2022 09:11, Ke Zhang:
> A segmentation fault occurs when testpmd exit.

The title talks about testpmd, but the change is in i40e.
Please update.

> This is due to fetching the device name from PF
> , PF is freed firstly and then VF representor
> is called later.

For info, a comma must follow the previous word without any space.
It is strange to see a comma at the beginning of a line.

> This commit fixes the bug by fetching the device
> name from vf representor not PF.

vf should be VF uppercase

> Fixes: e391a7b7f815 ("net/i40e: fix multi-process shared data")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>
> 
> ---
> v3: Change the design and fix code in driver
> v2: Change the testpmd code to fix this issue
> ---
> ---
>  drivers/net/i40e/i40e_vf_representor.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

[PATCH v4] net/i40e: fix the issue caused by PF and VF release order

[Ke Zhang]

A segmentation fault occurs when testpmd exit.

This is due to fetching the device name from PF, PF
is freed firstly and then VF representor is called
later.

This commit fixes the bug by fetching the device
name from VF representor not PF.

Fixes: e391a7b7f815 ("net/i40e: fix multi-process shared data")
Cc: stable@dpdk.org

Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>

---
v4: Update the commit log
v3: Change the design and fix code in driver
v2: Change the testpmd code to fix this issue
---
---
 drivers/net/i40e/i40e_vf_representor.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_vf_representor.c b/drivers/net/i40e/i40e_vf_representor.c
index 7f8e81858e..bcd445bcdd 100644
--- a/drivers/net/i40e/i40e_vf_representor.c
+++ b/drivers/net/i40e/i40e_vf_representor.c
@@ -29,8 +29,6 @@ i40e_vf_representor_dev_infos_get(struct rte_eth_dev *ethdev,
 	struct rte_eth_dev_info *dev_info)
 {
 	struct i40e_vf_representor *representor = ethdev->data->dev_private;
-	struct rte_eth_dev_data *pf_dev_data =
-		representor->adapter->pf.dev_data;
 
 	/* get dev info for the vdev */
 	dev_info->device = ethdev->device;
@@ -104,7 +102,7 @@ i40e_vf_representor_dev_infos_get(struct rte_eth_dev *ethdev,
 	};
 
 	dev_info->switch_info.name =
-		rte_eth_devices[pf_dev_data->port_id].device->name;
+		rte_eth_devices[ethdev->data->port_id].device->name;
 	dev_info->switch_info.domain_id = representor->switch_domain_id;
 	dev_info->switch_info.port_id = representor->vf_id;
 
-- 
2.25.1

RE: [PATCH v4] net/i40e: fix the issue caused by PF and VF release order

[Zhang, Yuying]

Hi Ke,

Please update commit log.
LGTM.

> -----Original Message-----
> From: Zhang, Ke1X <ke1x.zhang@intel.com>
> Sent: Friday, July 15, 2022 5:04 PM
> To: thomas@monjalon.net; Zhang, Yuying <yuying.zhang@intel.com>; Xing,
> Beilei <beilei.xing@intel.com>; ferruh.yigit@xilinx.com; Zhou, YidingX
> <yidingx.zhou@intel.com>; dev@dpdk.org
> Cc: Zhang, Ke1X <ke1x.zhang@intel.com>; stable@dpdk.org
> Subject: [PATCH v4] net/i40e: fix the issue caused by PF and VF release order
> 
> A segmentation fault occurs when testpmd exit.
> 
> This is due to fetching the device name from PF, PF is freed firstly and then VF
> representor is called later.
> 
> This commit fixes the bug by fetching the device name from VF representor
> not PF.

instead of PF

> 
> Fixes: e391a7b7f815 ("net/i40e: fix multi-process shared data")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>
Acked-by: Yuying Zhang <yuying.zhang@intel.com>

> 
> ---
> v4: Update the commit log
> v3: Change the design and fix code in driver
> v2: Change the testpmd code to fix this issue
> ---
> ---
>  drivers/net/i40e/i40e_vf_representor.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_vf_representor.c
> b/drivers/net/i40e/i40e_vf_representor.c
> index 7f8e81858e..bcd445bcdd 100644
> --- a/drivers/net/i40e/i40e_vf_representor.c
> +++ b/drivers/net/i40e/i40e_vf_representor.c
> @@ -29,8 +29,6 @@ i40e_vf_representor_dev_infos_get(struct rte_eth_dev
> *ethdev,
>  	struct rte_eth_dev_info *dev_info)
>  {
>  	struct i40e_vf_representor *representor = ethdev->data-
> >dev_private;
> -	struct rte_eth_dev_data *pf_dev_data =
> -		representor->adapter->pf.dev_data;
> 
>  	/* get dev info for the vdev */
>  	dev_info->device = ethdev->device;
> @@ -104,7 +102,7 @@ i40e_vf_representor_dev_infos_get(struct
> rte_eth_dev *ethdev,
>  	};
> 
>  	dev_info->switch_info.name =
> -		rte_eth_devices[pf_dev_data->port_id].device->name;
> +		rte_eth_devices[ethdev->data->port_id].device->name;
>  	dev_info->switch_info.domain_id = representor->switch_domain_id;
>  	dev_info->switch_info.port_id = representor->vf_id;
> 
> --
> 2.25.1

RE: [PATCH v4] net/i40e: fix the issue caused by PF and VF release order

[Zhang, Qi Z]

> -----Original Message-----
> From: Zhang, Yuying <yuying.zhang@intel.com>
> Sent: Tuesday, August 2, 2022 10:16 AM
> To: Zhang, Ke1X <ke1x.zhang@intel.com>; thomas@monjalon.net; Xing,
> Beilei <beilei.xing@intel.com>; ferruh.yigit@xilinx.com; Zhou, YidingX
> <yidingx.zhou@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [PATCH v4] net/i40e: fix the issue caused by PF and VF release
> order
> 
> Hi Ke,
> 
> Please update commit log.
> LGTM.
> 
> > -----Original Message-----
> > From: Zhang, Ke1X <ke1x.zhang@intel.com>
> > Sent: Friday, July 15, 2022 5:04 PM
> > To: thomas@monjalon.net; Zhang, Yuying <yuying.zhang@intel.com>; Xing,
> > Beilei <beilei.xing@intel.com>; ferruh.yigit@xilinx.com; Zhou, YidingX
> > <yidingx.zhou@intel.com>; dev@dpdk.org
> > Cc: Zhang, Ke1X <ke1x.zhang@intel.com>; stable@dpdk.org
> > Subject: [PATCH v4] net/i40e: fix the issue caused by PF and VF
> > release order
> >
> > A segmentation fault occurs when testpmd exit.
> >
> > This is due to fetching the device name from PF, PF is freed firstly
> > and then VF representor is called later.
> >
> > This commit fixes the bug by fetching the device name from VF
> > representor not PF.
> 
> instead of PF
> 
> >
> > Fixes: e391a7b7f815 ("net/i40e: fix multi-process shared data")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>
> Acked-by: Yuying Zhang <yuying.zhang@intel.com>

Applied to dpdk-next-net-intel.

Thanks
Qi

RE: [PATCH v4] net/i40e: fix the issue caused by PF and VF release order

[Zhang, Qi Z]

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Monday, August 8, 2022 8:06 AM
> To: Zhang, Yuying <yuying.zhang@intel.com>; Zhang, Ke1X
> <ke1x.zhang@intel.com>; thomas@monjalon.net; Xing, Beilei
> <beilei.xing@intel.com>; ferruh.yigit@xilinx.com; Zhou, YidingX
> <yidingx.zhou@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: RE: [PATCH v4] net/i40e: fix the issue caused by PF and VF release
> order
> 
> 
> 
> > -----Original Message-----
> > From: Zhang, Yuying <yuying.zhang@intel.com>
> > Sent: Tuesday, August 2, 2022 10:16 AM
> > To: Zhang, Ke1X <ke1x.zhang@intel.com>; thomas@monjalon.net; Xing,
> > Beilei <beilei.xing@intel.com>; ferruh.yigit@xilinx.com; Zhou, YidingX
> > <yidingx.zhou@intel.com>; dev@dpdk.org
> > Cc: stable@dpdk.org
> > Subject: RE: [PATCH v4] net/i40e: fix the issue caused by PF and VF
> > release order
> >
> > Hi Ke,
> >
> > Please update commit log.
> > LGTM.
> >
> > > -----Original Message-----
> > > From: Zhang, Ke1X <ke1x.zhang@intel.com>
> > > Sent: Friday, July 15, 2022 5:04 PM
> > > To: thomas@monjalon.net; Zhang, Yuying <yuying.zhang@intel.com>;
> > > Xing, Beilei <beilei.xing@intel.com>; ferruh.yigit@xilinx.com; Zhou,
> > > YidingX <yidingx.zhou@intel.com>; dev@dpdk.org
> > > Cc: Zhang, Ke1X <ke1x.zhang@intel.com>; stable@dpdk.org
> > > Subject: [PATCH v4] net/i40e: fix the issue caused by PF and VF
> > > release order
> > >
> > > A segmentation fault occurs when testpmd exit.
> > >
> > > This is due to fetching the device name from PF, PF is freed firstly
> > > and then VF representor is called later.
> > >
> > > This commit fixes the bug by fetching the device name from VF
> > > representor not PF.
> >
> > instead of PF
> >
> > >
> > > Fixes: e391a7b7f815 ("net/i40e: fix multi-process shared data")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>
> > Acked-by: Yuying Zhang <yuying.zhang@intel.com>
> 
> Applied to dpdk-next-net-intel.

Overwritten by v5 with refined commit log
> 
> Thanks
> Qi

[PATCH v5] net/i40e: fix the issue caused by PF and VF release order

[Ke Zhang]

A segmentation fault occurs when testpmd exit.

This is due to fetching the device name from PF, PF
is freed firstly and then VF representor is called
later.

This commit fixes the bug by fetching the device
name from VF representor instead of PF.

Fixes: e391a7b7f815 ("net/i40e: fix multi-process shared data")
Cc: stable@dpdk.org

Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>
Acked-by: Yuying Zhang <yuying.zhang@intel.com>
---
v5: Update the commit log
v4: Update the commit log
v3: Change the design and fix code in driver
v2: Change the testpmd code to fix this issue
---
---
 drivers/net/i40e/i40e_vf_representor.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_vf_representor.c b/drivers/net/i40e/i40e_vf_representor.c
index 7f8e81858e..bcd445bcdd 100644
--- a/drivers/net/i40e/i40e_vf_representor.c
+++ b/drivers/net/i40e/i40e_vf_representor.c
@@ -29,8 +29,6 @@ i40e_vf_representor_dev_infos_get(struct rte_eth_dev *ethdev,
 	struct rte_eth_dev_info *dev_info)
 {
 	struct i40e_vf_representor *representor = ethdev->data->dev_private;
-	struct rte_eth_dev_data *pf_dev_data =
-		representor->adapter->pf.dev_data;
 
 	/* get dev info for the vdev */
 	dev_info->device = ethdev->device;
@@ -104,7 +102,7 @@ i40e_vf_representor_dev_infos_get(struct rte_eth_dev *ethdev,
 	};
 
 	dev_info->switch_info.name =
-		rte_eth_devices[pf_dev_data->port_id].device->name;
+		rte_eth_devices[ethdev->data->port_id].device->name;
 	dev_info->switch_info.domain_id = representor->switch_domain_id;
 	dev_info->switch_info.port_id = representor->vf_id;
 
-- 
2.25.1

RE: [PATCH v5] net/i40e: fix the issue caused by PF and VF release order

[Zhang, Qi Z]

> -----Original Message-----
> From: Ke Zhang <ke1x.zhang@intel.com>
> Sent: Thursday, August 4, 2022 5:50 PM
> To: Xing, Beilei <beilei.xing@intel.com>; Zhang, Yuying
> <yuying.zhang@intel.com>; dev@dpdk.org
> Cc: Zhang, Ke1X <ke1x.zhang@intel.com>; stable@dpdk.org
> Subject: [PATCH v5] net/i40e: fix the issue caused by PF and VF release order
> 
> A segmentation fault occurs when testpmd exit.
> 
> This is due to fetching the device name from PF, PF is freed firstly and then VF
> representor is called later.
> 
> This commit fixes the bug by fetching the device name from VF representor
> instead of PF.
> 
> Fixes: e391a7b7f815 ("net/i40e: fix multi-process shared data")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ke Zhang <ke1x.zhang@intel.com>
> Acked-by: Yuying Zhang <yuying.zhang@intel.com>

Applied to dpdk-next-net-intel.

Thanks
Qi


> ---
> v5: Update the commit log
> v4: Update the commit log
> v3: Change the design and fix code in driver
> v2: Change the testpmd code to fix this issue
> ---
> ---
>  drivers/net/i40e/i40e_vf_representor.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_vf_representor.c
> b/drivers/net/i40e/i40e_vf_representor.c
> index 7f8e81858e..bcd445bcdd 100644
> --- a/drivers/net/i40e/i40e_vf_representor.c
> +++ b/drivers/net/i40e/i40e_vf_representor.c
> @@ -29,8 +29,6 @@ i40e_vf_representor_dev_infos_get(struct rte_eth_dev
> *ethdev,
>  	struct rte_eth_dev_info *dev_info)
>  {
>  	struct i40e_vf_representor *representor = ethdev->data-
> >dev_private;
> -	struct rte_eth_dev_data *pf_dev_data =
> -		representor->adapter->pf.dev_data;
> 
>  	/* get dev info for the vdev */
>  	dev_info->device = ethdev->device;
> @@ -104,7 +102,7 @@ i40e_vf_representor_dev_infos_get(struct
> rte_eth_dev *ethdev,
>  	};
> 
>  	dev_info->switch_info.name =
> -		rte_eth_devices[pf_dev_data->port_id].device->name;
> +		rte_eth_devices[ethdev->data->port_id].device->name;
>  	dev_info->switch_info.domain_id = representor->switch_domain_id;
>  	dev_info->switch_info.port_id = representor->vf_id;
> 
> --
> 2.25.1

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

[Ke Zhang]

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.

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

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index f7e18aee25..2299aef3dd 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -3378,8 +3378,11 @@ detach_devargs(char *identifier)
 void
 pmd_test_exit(void)
 {
+	unsigned int ethports[RTE_MAX_ETHPORTS];
+	unsigned int count = 0;
 	portid_t pt_id;
 	unsigned int i;
+	int index;
 	int ret;
 
 	if (test_done == 0)
@@ -3396,15 +3399,30 @@ pmd_test_exit(void)
 #endif
 	if (ports != NULL) {
 		no_link_check = 1;
+		i = 0;
+
+		/* Fetch the valid port id from port list*/
 		RTE_ETH_FOREACH_DEV(pt_id) {
-			printf("\nStopping port %d...\n", pt_id);
+			ethports[i] = pt_id;
+			i++;
+		}
+
+		count = i;
+		/*
+		 * 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]);
 		}
 	}
 
-- 
2.25.1