[PATCH 1/2] common: add safe version of foreach-list to Linux

[PATCH 1/2] common: add safe version of foreach-list to Linux

[Hamza Khan]

Linux EAL does not have the LIST_FOREACH_SAFE version of the
iterator macros. Add it.

Signed-off-by: Hamza Khan <hamza.khan@intel.com>
---
 lib/eal/linux/include/rte_os.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lib/eal/linux/include/rte_os.h b/lib/eal/linux/include/rte_os.h
index c72bf5b7e6..00d7714181 100644
--- a/lib/eal/linux/include/rte_os.h
+++ b/lib/eal/linux/include/rte_os.h
@@ -26,6 +26,13 @@ extern "C" {
 #define RTE_STAILQ_HEAD(name, type) STAILQ_HEAD(name, type)
 #define RTE_STAILQ_ENTRY(type) STAILQ_ENTRY(type)
 
+#ifndef LIST_FOREACH_SAFE
+#define	LIST_FOREACH_SAFE(var, head, field, tvar)			\
+	for ((var) = LIST_FIRST((head));				\
+	    (var) && ((tvar) = LIST_NEXT((var), field), 1);		\
+	    (var) = (tvar))
+#endif
+
 #ifdef CPU_SETSIZE /* may require _GNU_SOURCE */
 typedef cpu_set_t rte_cpuset_t;
 #define RTE_HAS_CPUSET
-- 
2.25.1

[PATCH 2/2] examples/vm_power_manager: use safe version of list iterator

[Hamza Khan]

Currently, when vm_power_manager exits, we are using a LIST_FOREACH
macro to iterate over VM info structures while freeing them. This
leads to use-after-free error. To address this, use the newly added
LIST_FOREACH_SAFE macro.

Fixes: e8ae9b662506 ("examples/vm_power: channel manager and monitor in host")
Cc: alan.carew@intel.com
Cc: stable@dpdk.org

Signed-off-by: Hamza Khan <hamza.khan@intel.com>
---
 examples/vm_power_manager/channel_manager.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/examples/vm_power_manager/channel_manager.c b/examples/vm_power_manager/channel_manager.c
index 838465ab4b..bc95cec8d6 100644
--- a/examples/vm_power_manager/channel_manager.c
+++ b/examples/vm_power_manager/channel_manager.c
@@ -1005,9 +1005,9 @@ channel_manager_exit(void)
 {
 	unsigned i;
 	char mask[RTE_MAX_LCORE];
-	struct virtual_machine_info *vm_info;
+	struct virtual_machine_info *vm_info, *tmp;
 
-	LIST_FOREACH(vm_info, &vm_list_head, vms_info) {
+	LIST_FOREACH_SAFE(vm_info, &vm_list_head, vms_info, tmp) {
 
 		rte_spinlock_lock(&(vm_info->config_spinlock));
 
-- 
2.25.1

Re: [PATCH 2/2] examples/vm_power_manager: use safe version of list iterator

[Hunt, David]

On 01/06/2022 11:54, Hamza Khan wrote:
> Currently, when vm_power_manager exits, we are using a LIST_FOREACH
> macro to iterate over VM info structures while freeing them. This
> leads to use-after-free error. To address this, use the newly added
> LIST_FOREACH_SAFE macro.
>
> Fixes: e8ae9b662506 ("examples/vm_power: channel manager and monitor in host")
> Cc: alan.carew@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Hamza Khan <hamza.khan@intel.com>
> ---
>   examples/vm_power_manager/channel_manager.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/examples/vm_power_manager/channel_manager.c b/examples/vm_power_manager/channel_manager.c
> index 838465ab4b..bc95cec8d6 100644
> --- a/examples/vm_power_manager/channel_manager.c
> +++ b/examples/vm_power_manager/channel_manager.c
> @@ -1005,9 +1005,9 @@ channel_manager_exit(void)
>   {
>   	unsigned i;
>   	char mask[RTE_MAX_LCORE];
> -	struct virtual_machine_info *vm_info;
> +	struct virtual_machine_info *vm_info, *tmp;
>   
> -	LIST_FOREACH(vm_info, &vm_list_head, vms_info) {
> +	LIST_FOREACH_SAFE(vm_info, &vm_list_head, vms_info, tmp) {
>   
>   		rte_spinlock_lock(&(vm_info->config_spinlock));
>   


Acked-by: David Hunt <david.hunt@intel.com>

RE: [PATCH 1/2] common: add safe version of foreach-list to Linux

[Khan, Hamza]

> -----Original Message-----
> From: Khan, Hamza <hamza.khan@intel.com>
> Sent: Wednesday 1 June 2022 11:55
> Cc: dev@dpdk.org; Khan, Hamza <hamza.khan@intel.com>
> Subject: [PATCH 1/2] common: add safe version of foreach-list to Linux
> 
> Linux EAL does not have the LIST_FOREACH_SAFE version of the iterator
> macros. Add it.
> 
> Signed-off-by: Hamza Khan <hamza.khan@intel.com>
-- Snipped --

Request for review and Ack on this patch
Also Please merge the patch [2/2] examples/vm_power_manager: use safe version of list iterator ( https://patches.dpdk.org/project/dpdk/patch/20220601105455.166505-2-hamza.khan@intel.com/ ) 

RE: [PATCH 2/2] examples/vm_power_manager: use safe version of list iterator

[Li, WeiyuanX]

> -----Original Message-----
> From: Hunt, David <david.hunt@intel.com>
> Sent: Monday, July 4, 2022 8:32 PM
> To: Khan, Hamza <hamza.khan@intel.com>; Carew, Alan
> <alan.carew@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>
> Cc: dev@dpdk.org; stable@dpdk.org
> Subject: Re: [PATCH 2/2] examples/vm_power_manager: use safe version of
> list iterator
> 
> 
> On 01/06/2022 11:54, Hamza Khan wrote:
> > Currently, when vm_power_manager exits, we are using a LIST_FOREACH
> > macro to iterate over VM info structures while freeing them. This
> > leads to use-after-free error. To address this, use the newly added
> > LIST_FOREACH_SAFE macro.
> >
> > Fixes: e8ae9b662506 ("examples/vm_power: channel manager and
> monitor
> > in host")
> > Cc: alan.carew@intel.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Hamza Khan <hamza.khan@intel.com>
> > ---
> >   examples/vm_power_manager/channel_manager.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/examples/vm_power_manager/channel_manager.c
> > b/examples/vm_power_manager/channel_manager.c
> > index 838465ab4b..bc95cec8d6 100644
> > --- a/examples/vm_power_manager/channel_manager.c
> > +++ b/examples/vm_power_manager/channel_manager.c
> > @@ -1005,9 +1005,9 @@ channel_manager_exit(void)
> >   {
> >   	unsigned i;
> >   	char mask[RTE_MAX_LCORE];
> > -	struct virtual_machine_info *vm_info;
> > +	struct virtual_machine_info *vm_info, *tmp;
> >
> > -	LIST_FOREACH(vm_info, &vm_list_head, vms_info) {
> > +	LIST_FOREACH_SAFE(vm_info, &vm_list_head, vms_info, tmp) {
> >
> >   		rte_spinlock_lock(&(vm_info->config_spinlock));
> >
> 
> 
> Acked-by: David Hunt <david.hunt@intel.com>
> 

Tested-by: Weiyuan Li <weiyuanx.li@intel.com>

Re: [PATCH 1/2] common: add safe version of foreach-list to Linux

[Thomas Monjalon]

01/06/2022 12:54, Hamza Khan:
> Linux EAL does not have the LIST_FOREACH_SAFE version of the
> iterator macros. Add it.
> 
> Signed-off-by: Hamza Khan <hamza.khan@intel.com>
> ---
>  lib/eal/linux/include/rte_os.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/lib/eal/linux/include/rte_os.h b/lib/eal/linux/include/rte_os.h
> index c72bf5b7e6..00d7714181 100644
> --- a/lib/eal/linux/include/rte_os.h
> +++ b/lib/eal/linux/include/rte_os.h
> @@ -26,6 +26,13 @@ extern "C" {
>  #define RTE_STAILQ_HEAD(name, type) STAILQ_HEAD(name, type)
>  #define RTE_STAILQ_ENTRY(type) STAILQ_ENTRY(type)
>  
> +#ifndef LIST_FOREACH_SAFE
> +#define	LIST_FOREACH_SAFE(var, head, field, tvar)			\
> +	for ((var) = LIST_FIRST((head));				\
> +	    (var) && ((tvar) = LIST_NEXT((var), field), 1);		\
> +	    (var) = (tvar))
> +#endif

I'm not sure we want to add such thing without a RTE_ prefix.
And we should not need LIST_*, we have RTE_TAILQ_*.

[PATCH v2] examples/vm_power_manager: use safe version of list iterator

[Hamza Khan]

Currently, when vm_power_manager exits, we are using a LIST_FOREACH
macro to iterate over VM info structures while freeing them. This
leads to use-after-free error. To address this, replace all usages of
LIST_* with TAILQ_* macros, and use the RTE_TAILQ_FOREACH_SAFE macro
to iterate and delete VM info structures.

Fixes: e8ae9b662506 ("examples/vm_power: channel manager and monitor in host")
Cc: alan.carew@intel.com
Cc: stable@dpdk.org

Signed-off-by: Hamza Khan <hamza.khan@intel.com>
---
 examples/vm_power_manager/channel_manager.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/examples/vm_power_manager/channel_manager.c b/examples/vm_power_manager/channel_manager.c
index 838465ab4b..e82c26ddca 100644
--- a/examples/vm_power_manager/channel_manager.c
+++ b/examples/vm_power_manager/channel_manager.c
@@ -29,6 +29,8 @@
 #include "channel_monitor.h"
 #include "power_manager.h"
 
+#include "rte_tailq.h"
+
 
 #define RTE_LOGTYPE_CHANNEL_MANAGER RTE_LOGTYPE_USER1
 
@@ -58,16 +60,16 @@ struct virtual_machine_info {
 	virDomainInfo info;
 	rte_spinlock_t config_spinlock;
 	int allow_query;
-	LIST_ENTRY(virtual_machine_info) vms_info;
+	RTE_TAILQ_ENTRY(virtual_machine_info) vms_info;
 };
 
-LIST_HEAD(, virtual_machine_info) vm_list_head;
+RTE_TAILQ_HEAD(, virtual_machine_info) vm_list_head;
 
 static struct virtual_machine_info *
 find_domain_by_name(const char *name)
 {
 	struct virtual_machine_info *info;
-	LIST_FOREACH(info, &vm_list_head, vms_info) {
+	RTE_TAILQ_FOREACH(info, &vm_list_head, vms_info) {
 		if (!strncmp(info->name, name, CHANNEL_MGR_MAX_NAME_LEN-1))
 			return info;
 	}
@@ -878,7 +880,7 @@ add_vm(const char *vm_name)
 
 	new_domain->allow_query = 0;
 	rte_spinlock_init(&(new_domain->config_spinlock));
-	LIST_INSERT_HEAD(&vm_list_head, new_domain, vms_info);
+	TAILQ_INSERT_HEAD(&vm_list_head, new_domain, vms_info);
 	return 0;
 }
 
@@ -900,7 +902,7 @@ remove_vm(const char *vm_name)
 		rte_spinlock_unlock(&vm_info->config_spinlock);
 		return -1;
 	}
-	LIST_REMOVE(vm_info, vms_info);
+	TAILQ_REMOVE(&vm_list_head, vm_info, vms_info);
 	rte_spinlock_unlock(&vm_info->config_spinlock);
 	rte_free(vm_info);
 	return 0;
@@ -953,7 +955,7 @@ channel_manager_init(const char *path __rte_unused)
 {
 	virNodeInfo info;
 
-	LIST_INIT(&vm_list_head);
+	TAILQ_INIT(&vm_list_head);
 	if (connect_hypervisor(path) < 0) {
 		global_n_host_cpus = 64;
 		global_hypervisor_available = 0;
@@ -1005,9 +1007,9 @@ channel_manager_exit(void)
 {
 	unsigned i;
 	char mask[RTE_MAX_LCORE];
-	struct virtual_machine_info *vm_info;
+	struct virtual_machine_info *vm_info, *tmp;
 
-	LIST_FOREACH(vm_info, &vm_list_head, vms_info) {
+	RTE_TAILQ_FOREACH_SAFE(vm_info, &vm_list_head, vms_info, tmp) {
 
 		rte_spinlock_lock(&(vm_info->config_spinlock));
 
@@ -1022,7 +1024,7 @@ channel_manager_exit(void)
 		}
 		rte_spinlock_unlock(&(vm_info->config_spinlock));
 
-		LIST_REMOVE(vm_info, vms_info);
+		TAILQ_REMOVE(&vm_list_head, vm_info, vms_info);
 		rte_free(vm_info);
 	}
 
-- 
2.25.1

RE: [PATCH 1/2] common: add safe version of foreach-list to Linux

[Khan, Hamza]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday 5 July 2022 17:16
> To: Khan, Hamza <hamza.khan@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH 1/2] common: add safe version of foreach-list to Linux
> 
> 01/06/2022 12:54, Hamza Khan:
> > Linux EAL does not have the LIST_FOREACH_SAFE version of the iterator
> > macros. Add it.
> >
> > Signed-off-by: Hamza Khan <hamza.khan@intel.com>
> > ---
> >  lib/eal/linux/include/rte_os.h | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/lib/eal/linux/include/rte_os.h
> > b/lib/eal/linux/include/rte_os.h index c72bf5b7e6..00d7714181 100644
> > --- a/lib/eal/linux/include/rte_os.h
> > +++ b/lib/eal/linux/include/rte_os.h
> > @@ -26,6 +26,13 @@ extern "C" {
> >  #define RTE_STAILQ_HEAD(name, type) STAILQ_HEAD(name, type)
> #define
> > RTE_STAILQ_ENTRY(type) STAILQ_ENTRY(type)
> >
> > +#ifndef LIST_FOREACH_SAFE
> > +#define	LIST_FOREACH_SAFE(var, head, field, tvar)
> 	\
> > +	for ((var) = LIST_FIRST((head));				\
> > +	    (var) && ((tvar) = LIST_NEXT((var), field), 1);		\
> > +	    (var) = (tvar))
> > +#endif
> 
> I'm not sure we want to add such thing without a RTE_ prefix.
> And we should not need LIST_*, we have RTE_TAILQ_*.
> 
> 
I have sent v2 patch with the aforementioned fix. 
However Is being held until the list moderator can review it for approval

Re: [PATCH 1/2] common: add safe version of foreach-list to Linux

[Thomas Monjalon]

07/07/2022 17:59, Khan, Hamza:
> 
> > -----Original Message-----
> > From: Thomas Monjalon <thomas@monjalon.net>
> > Sent: Tuesday 5 July 2022 17:16
> > To: Khan, Hamza <hamza.khan@intel.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [PATCH 1/2] common: add safe version of foreach-list to Linux
> > 
> > 01/06/2022 12:54, Hamza Khan:
> > > Linux EAL does not have the LIST_FOREACH_SAFE version of the iterator
> > > macros. Add it.
> > >
> > > Signed-off-by: Hamza Khan <hamza.khan@intel.com>
> > > ---
> > >  lib/eal/linux/include/rte_os.h | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/lib/eal/linux/include/rte_os.h
> > > b/lib/eal/linux/include/rte_os.h index c72bf5b7e6..00d7714181 100644
> > > --- a/lib/eal/linux/include/rte_os.h
> > > +++ b/lib/eal/linux/include/rte_os.h
> > > @@ -26,6 +26,13 @@ extern "C" {
> > >  #define RTE_STAILQ_HEAD(name, type) STAILQ_HEAD(name, type)
> > #define
> > > RTE_STAILQ_ENTRY(type) STAILQ_ENTRY(type)
> > >
> > > +#ifndef LIST_FOREACH_SAFE
> > > +#define	LIST_FOREACH_SAFE(var, head, field, tvar)
> > 	\
> > > +	for ((var) = LIST_FIRST((head));				\
> > > +	    (var) && ((tvar) = LIST_NEXT((var), field), 1);		\
> > > +	    (var) = (tvar))
> > > +#endif
> > 
> > I'm not sure we want to add such thing without a RTE_ prefix.
> > And we should not need LIST_*, we have RTE_TAILQ_*.
> > 
> > 
> I have sent v2 patch with the aforementioned fix. 
> However Is being held until the list moderator can review it for approval

I've unblocked it.
This is blocked because you are not registered in the mailing list,
so it is considered as spam.

[PATCH v3] examples/vm_power_manager: use safe version of list iterator

[Hamza Khan]

Currently, when vm_power_manager exits, we are using a LIST_FOREACH
macro to iterate over VM info structures while freeing them. This
leads to use-after-free error. To address this, replace all usages of
LIST_* with TAILQ_* macros, and use the RTE_TAILQ_FOREACH_SAFE macro
to iterate and delete VM info structures.

* The change is small and doesn’t affect other code
* Testing was performed on the patch

Fixes: e8ae9b662506 ("examples/vm_power: channel manager and monitor in host")
Cc: alan.carew@intel.com
Cc: stable@dpdk.org

Signed-off-by: Hamza Khan <hamza.khan@intel.com>

---
V3: Update commit message
V2: Use RTE_TAILQ_* marcos
---
 examples/vm_power_manager/channel_manager.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/examples/vm_power_manager/channel_manager.c b/examples/vm_power_manager/channel_manager.c
index 838465ab4b..e82c26ddca 100644
--- a/examples/vm_power_manager/channel_manager.c
+++ b/examples/vm_power_manager/channel_manager.c
@@ -29,6 +29,8 @@
 #include "channel_monitor.h"
 #include "power_manager.h"
 
+#include "rte_tailq.h"
+
 
 #define RTE_LOGTYPE_CHANNEL_MANAGER RTE_LOGTYPE_USER1
 
@@ -58,16 +60,16 @@ struct virtual_machine_info {
 	virDomainInfo info;
 	rte_spinlock_t config_spinlock;
 	int allow_query;
-	LIST_ENTRY(virtual_machine_info) vms_info;
+	RTE_TAILQ_ENTRY(virtual_machine_info) vms_info;
 };
 
-LIST_HEAD(, virtual_machine_info) vm_list_head;
+RTE_TAILQ_HEAD(, virtual_machine_info) vm_list_head;
 
 static struct virtual_machine_info *
 find_domain_by_name(const char *name)
 {
 	struct virtual_machine_info *info;
-	LIST_FOREACH(info, &vm_list_head, vms_info) {
+	RTE_TAILQ_FOREACH(info, &vm_list_head, vms_info) {
 		if (!strncmp(info->name, name, CHANNEL_MGR_MAX_NAME_LEN-1))
 			return info;
 	}
@@ -878,7 +880,7 @@ add_vm(const char *vm_name)
 
 	new_domain->allow_query = 0;
 	rte_spinlock_init(&(new_domain->config_spinlock));
-	LIST_INSERT_HEAD(&vm_list_head, new_domain, vms_info);
+	TAILQ_INSERT_HEAD(&vm_list_head, new_domain, vms_info);
 	return 0;
 }
 
@@ -900,7 +902,7 @@ remove_vm(const char *vm_name)
 		rte_spinlock_unlock(&vm_info->config_spinlock);
 		return -1;
 	}
-	LIST_REMOVE(vm_info, vms_info);
+	TAILQ_REMOVE(&vm_list_head, vm_info, vms_info);
 	rte_spinlock_unlock(&vm_info->config_spinlock);
 	rte_free(vm_info);
 	return 0;
@@ -953,7 +955,7 @@ channel_manager_init(const char *path __rte_unused)
 {
 	virNodeInfo info;
 
-	LIST_INIT(&vm_list_head);
+	TAILQ_INIT(&vm_list_head);
 	if (connect_hypervisor(path) < 0) {
 		global_n_host_cpus = 64;
 		global_hypervisor_available = 0;
@@ -1005,9 +1007,9 @@ channel_manager_exit(void)
 {
 	unsigned i;
 	char mask[RTE_MAX_LCORE];
-	struct virtual_machine_info *vm_info;
+	struct virtual_machine_info *vm_info, *tmp;
 
-	LIST_FOREACH(vm_info, &vm_list_head, vms_info) {
+	RTE_TAILQ_FOREACH_SAFE(vm_info, &vm_list_head, vms_info, tmp) {
 
 		rte_spinlock_lock(&(vm_info->config_spinlock));
 
@@ -1022,7 +1024,7 @@ channel_manager_exit(void)
 		}
 		rte_spinlock_unlock(&(vm_info->config_spinlock));
 
-		LIST_REMOVE(vm_info, vms_info);
+		TAILQ_REMOVE(&vm_list_head, vm_info, vms_info);
 		rte_free(vm_info);
 	}
 
-- 
2.25.1

RE: [PATCH 1/2] common: add safe version of foreach-list to Linux

[Khan, Hamza]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday 7 July 2022 20:10
> To: Khan, Hamza <hamza.khan@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH 1/2] common: add safe version of foreach-list to Linux
> 
> 07/07/2022 17:59, Khan, Hamza:
> >
> > > -----Original Message-----
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > Sent: Tuesday 5 July 2022 17:16
> > > To: Khan, Hamza <hamza.khan@intel.com>
> > > Cc: dev@dpdk.org
> > > Subject: Re: [PATCH 1/2] common: add safe version of foreach-list to
> > > Linux
> > >
> > > 01/06/2022 12:54, Hamza Khan:
> > > > Linux EAL does not have the LIST_FOREACH_SAFE version of the
> > > > iterator macros. Add it.
> > > >
> > > > Signed-off-by: Hamza Khan <hamza.khan@intel.com>
> > > > ---
> > > >  lib/eal/linux/include/rte_os.h | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/lib/eal/linux/include/rte_os.h
> > > > b/lib/eal/linux/include/rte_os.h index c72bf5b7e6..00d7714181
> > > > 100644
> > > > --- a/lib/eal/linux/include/rte_os.h
> > > > +++ b/lib/eal/linux/include/rte_os.h
> > > > @@ -26,6 +26,13 @@ extern "C" {
> > > >  #define RTE_STAILQ_HEAD(name, type) STAILQ_HEAD(name, type)
> > > #define
> > > > RTE_STAILQ_ENTRY(type) STAILQ_ENTRY(type)
> > > >
> > > > +#ifndef LIST_FOREACH_SAFE
> > > > +#define	LIST_FOREACH_SAFE(var, head, field, tvar)
> > > 	\
> > > > +	for ((var) = LIST_FIRST((head));				\
> > > > +	    (var) && ((tvar) = LIST_NEXT((var), field), 1);		\
> > > > +	    (var) = (tvar))
> > > > +#endif
> > >
> > > I'm not sure we want to add such thing without a RTE_ prefix.
> > > And we should not need LIST_*, we have RTE_TAILQ_*.
> > >
> > >
> > I have sent v2 patch with the aforementioned fix.
> > However Is being held until the list moderator can review it for
> > approval
> 
> I've unblocked it.
> This is blocked because you are not registered in the mailing list, so it is
> considered as spam.
> 
The maintainers will only accept fixes into RC4 if they can be confident in the patch.
So I have edited the Commit Message and created a V3
I registered with the mailing list yesterday but it seems that it is being held again.
Do you have any suggestions on how to avoid this for future patches ?
Thanks
Hamza

Re: [PATCH 1/2] common: add safe version of foreach-list to Linux

[Thomas Monjalon]

08/07/2022 10:56, Khan, Hamza:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 07/07/2022 17:59, Khan, Hamza:
> > > I have sent v2 patch with the aforementioned fix.
> > > However Is being held until the list moderator can review it for
> > > approval
> > 
> > I've unblocked it.
> > This is blocked because you are not registered in the mailing list, so it is
> > considered as spam.
> > 
> The maintainers will only accept fixes into RC4 if they can be confident in the patch.
> So I have edited the Commit Message and created a V3
> I registered with the mailing list yesterday but it seems that it is being held again.
> Do you have any suggestions on how to avoid this for future patches ?

Once you register, you receive an email with a confirmation link.
Look in your spam folder.

RE: [PATCH 1/2] common: add safe version of foreach-list to Linux

[Khan, Hamza]

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Friday 8 July 2022 10:25
> To: Khan, Hamza <hamza.khan@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH 1/2] common: add safe version of foreach-list to Linux
> 
> 08/07/2022 10:56, Khan, Hamza:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 07/07/2022 17:59, Khan, Hamza:
> > > > I have sent v2 patch with the aforementioned fix.
> > > > However Is being held until the list moderator can review it for
> > > > approval
> > >
> > > I've unblocked it.
> > > This is blocked because you are not registered in the mailing list,
> > > so it is considered as spam.
> > >
> > The maintainers will only accept fixes into RC4 if they can be confident in
> the patch.
> > So I have edited the Commit Message and created a V3 I registered with
> > the mailing list yesterday but it seems that it is being held again.
> > Do you have any suggestions on how to avoid this for future patches ?
> 
> Once you register, you receive an email with a confirmation link.
> Look in your spam folder.
> 
 Yes it was found in the junk folder
Thank You 

Re: [PATCH v3] examples/vm_power_manager: use safe version of list iterator

[Hunt, David]

On 08/07/2022 09:51, Hamza Khan wrote:
> Currently, when vm_power_manager exits, we are using a LIST_FOREACH
> macro to iterate over VM info structures while freeing them. This
> leads to use-after-free error. To address this, replace all usages of
> LIST_* with TAILQ_* macros, and use the RTE_TAILQ_FOREACH_SAFE macro
> to iterate and delete VM info structures.
>
> * The change is small and doesn’t affect other code
> * Testing was performed on the patch
>
> Fixes: e8ae9b662506 ("examples/vm_power: channel manager and monitor in host")
> Cc: alan.carew@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Hamza Khan <hamza.khan@intel.com>
>
> ---
> V3: Update commit message
> V2: Use RTE_TAILQ_* marcos
> ---
>   examples/vm_power_manager/channel_manager.c | 20 +++++++++++---------
>   1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/examples/vm_power_manager/channel_manager.c b/examples/vm_power_manager/channel_manager.c
> index 838465ab4b..e82c26ddca 100644
> --- a/examples/vm_power_manager/channel_manager.c
> +++ b/examples/vm_power_manager/channel_manager.c
> @@ -29,6 +29,8 @@
>   #include "channel_monitor.h"
>   #include "power_manager.h"
>   
> +#include "rte_tailq.h"
> +
>   
>   #define RTE_LOGTYPE_CHANNEL_MANAGER RTE_LOGTYPE_USER1
>   
> @@ -58,16 +60,16 @@ struct virtual_machine_info {
>   	virDomainInfo info;
>   	rte_spinlock_t config_spinlock;
>   	int allow_query;
> -	LIST_ENTRY(virtual_machine_info) vms_info;
> +	RTE_TAILQ_ENTRY(virtual_machine_info) vms_info;
>   };
>   
> -LIST_HEAD(, virtual_machine_info) vm_list_head;
> +RTE_TAILQ_HEAD(, virtual_machine_info) vm_list_head;
>   
>   static struct virtual_machine_info *
>   find_domain_by_name(const char *name)
>   {
>   	struct virtual_machine_info *info;
> -	LIST_FOREACH(info, &vm_list_head, vms_info) {
> +	RTE_TAILQ_FOREACH(info, &vm_list_head, vms_info) {
>   		if (!strncmp(info->name, name, CHANNEL_MGR_MAX_NAME_LEN-1))
>   			return info;
>   	}
> @@ -878,7 +880,7 @@ add_vm(const char *vm_name)
>   
>   	new_domain->allow_query = 0;
>   	rte_spinlock_init(&(new_domain->config_spinlock));
> -	LIST_INSERT_HEAD(&vm_list_head, new_domain, vms_info);
> +	TAILQ_INSERT_HEAD(&vm_list_head, new_domain, vms_info);
>   	return 0;
>   }
>   
> @@ -900,7 +902,7 @@ remove_vm(const char *vm_name)
>   		rte_spinlock_unlock(&vm_info->config_spinlock);
>   		return -1;
>   	}
> -	LIST_REMOVE(vm_info, vms_info);
> +	TAILQ_REMOVE(&vm_list_head, vm_info, vms_info);
>   	rte_spinlock_unlock(&vm_info->config_spinlock);
>   	rte_free(vm_info);
>   	return 0;
> @@ -953,7 +955,7 @@ channel_manager_init(const char *path __rte_unused)
>   {
>   	virNodeInfo info;
>   
> -	LIST_INIT(&vm_list_head);
> +	TAILQ_INIT(&vm_list_head);
>   	if (connect_hypervisor(path) < 0) {
>   		global_n_host_cpus = 64;
>   		global_hypervisor_available = 0;
> @@ -1005,9 +1007,9 @@ channel_manager_exit(void)
>   {
>   	unsigned i;
>   	char mask[RTE_MAX_LCORE];
> -	struct virtual_machine_info *vm_info;
> +	struct virtual_machine_info *vm_info, *tmp;
>   
> -	LIST_FOREACH(vm_info, &vm_list_head, vms_info) {
> +	RTE_TAILQ_FOREACH_SAFE(vm_info, &vm_list_head, vms_info, tmp) {
>   
>   		rte_spinlock_lock(&(vm_info->config_spinlock));
>   
> @@ -1022,7 +1024,7 @@ channel_manager_exit(void)
>   		}
>   		rte_spinlock_unlock(&(vm_info->config_spinlock));
>   
> -		LIST_REMOVE(vm_info, vms_info);
> +		TAILQ_REMOVE(&vm_list_head, vm_info, vms_info);
>   		rte_free(vm_info);
>   	}
>   


Acked-by: David Hunt <david.hunt@intel.com>

RE: [PATCH v3] examples/vm_power_manager: use safe version of list iterator

[Pattan, Reshma]

> -----Original Message-----
> From: Hamza Khan <hamza.khan@intel.com>


<snip>

> +#include "rte_tailq.h"

Should include this as #include <rte_tailq.h>

Otherwise looks good. If you are sending next version, include my Ack in the patch.
Reviewed-by: Reshma Pattan <reshma.pattan@intel.com>
Acked-by: Reshma Pattan <reshma.pattan@intel.cm>

[PATCH v4] examples/vm_power_manager: use safe version of list iterator

[Reshma Pattan]

From: Hamza Khan <hamza.khan@intel.com>

Currently, when vm_power_manager exits, we are using a LIST_FOREACH
macro to iterate over VM info structures while freeing them. This
leads to use-after-free error. To address this, replace all usages of
LIST_* with TAILQ_* macros, and use the RTE_TAILQ_FOREACH_SAFE macro
to iterate and delete VM info structures.

* The change is small and doesn’t affect other code
* Testing was performed on the patch

Fixes: e8ae9b662506 ("examples/vm_power: channel manager and monitor in host")
Cc: alan.carew@intel.com
Cc: stable@dpdk.org

Signed-off-by: Hamza Khan <hamza.khan@intel.com>
Reviewed-by: Reshma Pattan <reshma.pattan@intel.com>
Acked-by: Reshma Pattan <reshma.pattan@intel.cm>
Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
---
v4: fix header file inclusion
---
 examples/vm_power_manager/channel_manager.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/examples/vm_power_manager/channel_manager.c b/examples/vm_power_manager/channel_manager.c
index 838465ab4b..cb872ad2d5 100644
--- a/examples/vm_power_manager/channel_manager.c
+++ b/examples/vm_power_manager/channel_manager.c
@@ -22,6 +22,7 @@
 #include <rte_mempool.h>
 #include <rte_log.h>
 #include <rte_spinlock.h>
+#include <rte_tailq.h>
 
 #include <libvirt/libvirt.h>
 
@@ -30,6 +31,7 @@
 #include "power_manager.h"
 
 
+
 #define RTE_LOGTYPE_CHANNEL_MANAGER RTE_LOGTYPE_USER1
 
 struct libvirt_vm_info lvm_info[MAX_CLIENTS];
@@ -58,16 +60,16 @@ struct virtual_machine_info {
 	virDomainInfo info;
 	rte_spinlock_t config_spinlock;
 	int allow_query;
-	LIST_ENTRY(virtual_machine_info) vms_info;
+	RTE_TAILQ_ENTRY(virtual_machine_info) vms_info;
 };
 
-LIST_HEAD(, virtual_machine_info) vm_list_head;
+RTE_TAILQ_HEAD(, virtual_machine_info) vm_list_head;
 
 static struct virtual_machine_info *
 find_domain_by_name(const char *name)
 {
 	struct virtual_machine_info *info;
-	LIST_FOREACH(info, &vm_list_head, vms_info) {
+	RTE_TAILQ_FOREACH(info, &vm_list_head, vms_info) {
 		if (!strncmp(info->name, name, CHANNEL_MGR_MAX_NAME_LEN-1))
 			return info;
 	}
@@ -878,7 +880,7 @@ add_vm(const char *vm_name)
 
 	new_domain->allow_query = 0;
 	rte_spinlock_init(&(new_domain->config_spinlock));
-	LIST_INSERT_HEAD(&vm_list_head, new_domain, vms_info);
+	TAILQ_INSERT_HEAD(&vm_list_head, new_domain, vms_info);
 	return 0;
 }
 
@@ -900,7 +902,7 @@ remove_vm(const char *vm_name)
 		rte_spinlock_unlock(&vm_info->config_spinlock);
 		return -1;
 	}
-	LIST_REMOVE(vm_info, vms_info);
+	TAILQ_REMOVE(&vm_list_head, vm_info, vms_info);
 	rte_spinlock_unlock(&vm_info->config_spinlock);
 	rte_free(vm_info);
 	return 0;
@@ -953,7 +955,7 @@ channel_manager_init(const char *path __rte_unused)
 {
 	virNodeInfo info;
 
-	LIST_INIT(&vm_list_head);
+	TAILQ_INIT(&vm_list_head);
 	if (connect_hypervisor(path) < 0) {
 		global_n_host_cpus = 64;
 		global_hypervisor_available = 0;
@@ -1005,9 +1007,9 @@ channel_manager_exit(void)
 {
 	unsigned i;
 	char mask[RTE_MAX_LCORE];
-	struct virtual_machine_info *vm_info;
+	struct virtual_machine_info *vm_info, *tmp;
 
-	LIST_FOREACH(vm_info, &vm_list_head, vms_info) {
+	RTE_TAILQ_FOREACH_SAFE(vm_info, &vm_list_head, vms_info, tmp) {
 
 		rte_spinlock_lock(&(vm_info->config_spinlock));
 
@@ -1022,7 +1024,7 @@ channel_manager_exit(void)
 		}
 		rte_spinlock_unlock(&(vm_info->config_spinlock));
 
-		LIST_REMOVE(vm_info, vms_info);
+		TAILQ_REMOVE(&vm_list_head, vm_info, vms_info);
 		rte_free(vm_info);
 	}
 
-- 
2.25.1

Re: [PATCH v4] examples/vm_power_manager: use safe version of list iterator

[Hunt, David]

On 22/08/2022 11:58, Reshma Pattan wrote:
> From: Hamza Khan <hamza.khan@intel.com>
>
> Currently, when vm_power_manager exits, we are using a LIST_FOREACH
> macro to iterate over VM info structures while freeing them. This
> leads to use-after-free error. To address this, replace all usages of
> LIST_* with TAILQ_* macros, and use the RTE_TAILQ_FOREACH_SAFE macro
> to iterate and delete VM info structures.
>
> * The change is small and doesn’t affect other code
> * Testing was performed on the patch
>
> Fixes: e8ae9b662506 ("examples/vm_power: channel manager and monitor in host")
> Cc: alan.carew@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Hamza Khan <hamza.khan@intel.com>
> Reviewed-by: Reshma Pattan <reshma.pattan@intel.com>
> Acked-by: Reshma Pattan <reshma.pattan@intel.cm>
> Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
> ---
> v4: fix header file inclusion
> ---

--snip--

Acked-by: David Hunt <david.hunt@intel.com>

[PATCH v5] examples/vm_power_manager: use safe version of list iterator

[Reshma Pattan]

From: Hamza Khan <hamza.khan@intel.com>

Currently, when vm_power_manager exits, we are using a LIST_FOREACH
macro to iterate over VM info structures while freeing them. This
leads to use-after-free error. To address this, replace all usages of
LIST_* with TAILQ_* macros, and use the RTE_TAILQ_FOREACH_SAFE macro
to iterate and delete VM info structures.

Fixes: e8ae9b662506 ("examples/vm_power: channel manager and monitor in host")
Cc: alan.carew@intel.com
Cc: stable@dpdk.org

Signed-off-by: Hamza Khan <hamza.khan@intel.com>
Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
Acked-by: David Hunt <david.hunt@intel.com>
---
 examples/vm_power_manager/channel_manager.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/examples/vm_power_manager/channel_manager.c b/examples/vm_power_manager/channel_manager.c
index 838465ab4b..cb872ad2d5 100644
--- a/examples/vm_power_manager/channel_manager.c
+++ b/examples/vm_power_manager/channel_manager.c
@@ -22,6 +22,7 @@
 #include <rte_mempool.h>
 #include <rte_log.h>
 #include <rte_spinlock.h>
+#include <rte_tailq.h>
 
 #include <libvirt/libvirt.h>
 
@@ -30,6 +31,7 @@
 #include "power_manager.h"
 
 
+
 #define RTE_LOGTYPE_CHANNEL_MANAGER RTE_LOGTYPE_USER1
 
 struct libvirt_vm_info lvm_info[MAX_CLIENTS];
@@ -58,16 +60,16 @@ struct virtual_machine_info {
 	virDomainInfo info;
 	rte_spinlock_t config_spinlock;
 	int allow_query;
-	LIST_ENTRY(virtual_machine_info) vms_info;
+	RTE_TAILQ_ENTRY(virtual_machine_info) vms_info;
 };
 
-LIST_HEAD(, virtual_machine_info) vm_list_head;
+RTE_TAILQ_HEAD(, virtual_machine_info) vm_list_head;
 
 static struct virtual_machine_info *
 find_domain_by_name(const char *name)
 {
 	struct virtual_machine_info *info;
-	LIST_FOREACH(info, &vm_list_head, vms_info) {
+	RTE_TAILQ_FOREACH(info, &vm_list_head, vms_info) {
 		if (!strncmp(info->name, name, CHANNEL_MGR_MAX_NAME_LEN-1))
 			return info;
 	}
@@ -878,7 +880,7 @@ add_vm(const char *vm_name)
 
 	new_domain->allow_query = 0;
 	rte_spinlock_init(&(new_domain->config_spinlock));
-	LIST_INSERT_HEAD(&vm_list_head, new_domain, vms_info);
+	TAILQ_INSERT_HEAD(&vm_list_head, new_domain, vms_info);
 	return 0;
 }
 
@@ -900,7 +902,7 @@ remove_vm(const char *vm_name)
 		rte_spinlock_unlock(&vm_info->config_spinlock);
 		return -1;
 	}
-	LIST_REMOVE(vm_info, vms_info);
+	TAILQ_REMOVE(&vm_list_head, vm_info, vms_info);
 	rte_spinlock_unlock(&vm_info->config_spinlock);
 	rte_free(vm_info);
 	return 0;
@@ -953,7 +955,7 @@ channel_manager_init(const char *path __rte_unused)
 {
 	virNodeInfo info;
 
-	LIST_INIT(&vm_list_head);
+	TAILQ_INIT(&vm_list_head);
 	if (connect_hypervisor(path) < 0) {
 		global_n_host_cpus = 64;
 		global_hypervisor_available = 0;
@@ -1005,9 +1007,9 @@ channel_manager_exit(void)
 {
 	unsigned i;
 	char mask[RTE_MAX_LCORE];
-	struct virtual_machine_info *vm_info;
+	struct virtual_machine_info *vm_info, *tmp;
 
-	LIST_FOREACH(vm_info, &vm_list_head, vms_info) {
+	RTE_TAILQ_FOREACH_SAFE(vm_info, &vm_list_head, vms_info, tmp) {
 
 		rte_spinlock_lock(&(vm_info->config_spinlock));
 
@@ -1022,7 +1024,7 @@ channel_manager_exit(void)
 		}
 		rte_spinlock_unlock(&(vm_info->config_spinlock));
 
-		LIST_REMOVE(vm_info, vms_info);
+		TAILQ_REMOVE(&vm_list_head, vm_info, vms_info);
 		rte_free(vm_info);
 	}
 
-- 
2.31.1

Re: [PATCH v5] examples/vm_power_manager: use safe version of list iterator

[Thomas Monjalon]

05/10/2022 00:09, Reshma Pattan:
> From: Hamza Khan <hamza.khan@intel.com>
> 
> Currently, when vm_power_manager exits, we are using a LIST_FOREACH
> macro to iterate over VM info structures while freeing them. This
> leads to use-after-free error. To address this, replace all usages of
> LIST_* with TAILQ_* macros, and use the RTE_TAILQ_FOREACH_SAFE macro
> to iterate and delete VM info structures.
> 
> Fixes: e8ae9b662506 ("examples/vm_power: channel manager and monitor in host")
> Cc: alan.carew@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Hamza Khan <hamza.khan@intel.com>
> Signed-off-by: Reshma Pattan <reshma.pattan@intel.com>
> Acked-by: David Hunt <david.hunt@intel.com>
> ---
>  examples/vm_power_manager/channel_manager.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/examples/vm_power_manager/channel_manager.c b/examples/vm_power_manager/channel_manager.c
> index 838465ab4b..cb872ad2d5 100644
> --- a/examples/vm_power_manager/channel_manager.c
> +++ b/examples/vm_power_manager/channel_manager.c
> @@ -22,6 +22,7 @@
>  #include <rte_mempool.h>
>  #include <rte_log.h>
>  #include <rte_spinlock.h>
> +#include <rte_tailq.h>
>  
>  #include <libvirt/libvirt.h>
>  
> @@ -30,6 +31,7 @@
>  #include "power_manager.h"
>  
>  
> +
>  #define RTE_LOGTYPE_CHANNEL_MANAGER RTE_LOGTYPE_USER1

I suppose we don't need a third blank line here.
Removing.

[...]
> -	LIST_FOREACH(vm_info, &vm_list_head, vms_info) {
> +	RTE_TAILQ_FOREACH_SAFE(vm_info, &vm_list_head, vms_info, tmp) {

Applied, thanks.