diff mbox series

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

Message ID 20220601105455.166505-1-hamza.khan@intel.com (mailing list archive)
State Superseded, archived
Headers
Series [1/2] common: add safe version of foreach-list to Linux |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Khan, Hamza June 1, 2022, 10:54 a.m. UTC 
  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(+)

Comments

Khan, Hamza July 4, 2022, 1:45 p.m. UTC | #1 
> -----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/ )
Thomas Monjalon July 5, 2022, 4:16 p.m. UTC | #2 
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_*.
Khan, Hamza July 7, 2022, 3:59 p.m. UTC | #3 
> -----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
Thomas Monjalon July 7, 2022, 7:09 p.m. UTC | #4 
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.
Khan, Hamza July 8, 2022, 8:56 a.m. UTC | #5 
> -----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
Thomas Monjalon July 8, 2022, 9:25 a.m. UTC | #6 
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.
Khan, Hamza July 8, 2022, 9:28 a.m. UTC | #7 
> -----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
Hunt, David July 8, 2022, 9:28 a.m. UTC | #8 
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>
diff mbox series

Patch

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