* [dpdk-dev] [PATCH 1/2] eal: add stailq safe iterator macro @ 2016-07-22 16:01 Sergio Gonzalez Monroy 2016-07-22 16:01 ` [dpdk-dev] [PATCH 2/2] mempool: fix unsafe tailq element removal Sergio Gonzalez Monroy 2016-07-22 16:16 ` [dpdk-dev] [PATCH 1/2] eal: add stailq safe iterator macro Thomas Monjalon 0 siblings, 2 replies; 7+ messages in thread From: Sergio Gonzalez Monroy @ 2016-07-22 16:01 UTC (permalink / raw) To: dev; +Cc: thomas.monjalon Removing/freeing elements elements within a STAILQ_FOREACH loop is not safe. FreeBSD defines STAILQ_FOREACH_SAFE macro, which permits these operations safely. This patch defines this macro for Linux systems, where it is not defined. Signed-off-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com> --- NOTE: This patch is based on top of: http://dpdk.org/dev/patchwork/patch/14995/ lib/librte_eal/common/include/rte_tailq.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/librte_eal/common/include/rte_tailq.h b/lib/librte_eal/common/include/rte_tailq.h index cc3c0f1..bba2835 100644 --- a/lib/librte_eal/common/include/rte_tailq.h +++ b/lib/librte_eal/common/include/rte_tailq.h @@ -163,6 +163,13 @@ void __attribute__((constructor, used)) tailqinitfn_ ##t(void) \ (var) = (tvar)) #endif +#ifndef SLIST_FOREACH_SAFE +#define SLIST_FOREACH_SAFE(var, head, field, tvar) \ + for ((var) = SLIST_FIRST((head)); \ + (var) && ((tvar) = SLIST_NEXT((var), field), 1); \ + (var) = (tvar)) +#endif + #ifdef __cplusplus } #endif -- 2.4.11 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [dpdk-dev] [PATCH 2/2] mempool: fix unsafe tailq element removal 2016-07-22 16:01 [dpdk-dev] [PATCH 1/2] eal: add stailq safe iterator macro Sergio Gonzalez Monroy @ 2016-07-22 16:01 ` Sergio Gonzalez Monroy 2016-07-25 16:30 ` Olivier Matz 2016-07-22 16:16 ` [dpdk-dev] [PATCH 1/2] eal: add stailq safe iterator macro Thomas Monjalon 1 sibling, 1 reply; 7+ messages in thread From: Sergio Gonzalez Monroy @ 2016-07-22 16:01 UTC (permalink / raw) To: dev; +Cc: thomas.monjalon Potentially user provided function could remove/free tailq elements. Doing so within a TAILQ_FOREACH loop is not safe. Use _SAFE versions of _FOREACH macros. Signed-off-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com> --- lib/librte_mempool/rte_mempool.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c index 8806633..394154a 100644 --- a/lib/librte_mempool/rte_mempool.c +++ b/lib/librte_mempool/rte_mempool.c @@ -157,10 +157,10 @@ rte_mempool_obj_iter(struct rte_mempool *mp, rte_mempool_obj_cb_t *obj_cb, void *obj_cb_arg) { struct rte_mempool_objhdr *hdr; - void *obj; + void *obj, *temp; unsigned n = 0; - STAILQ_FOREACH(hdr, &mp->elt_list, next) { + STAILQ_FOREACH_SAFE(hdr, &mp->elt_list, next, temp) { obj = (char *)hdr + sizeof(*hdr); obj_cb(mp, obj_cb_arg, obj, n); n++; @@ -176,8 +176,9 @@ rte_mempool_mem_iter(struct rte_mempool *mp, { struct rte_mempool_memhdr *hdr; unsigned n = 0; + void *temp; - STAILQ_FOREACH(hdr, &mp->mem_list, next) { + STAILQ_FOREACH_SAFE(hdr, &mp->mem_list, next, temp) { mem_cb(mp, mem_cb_arg, hdr, n); n++; } @@ -1283,12 +1284,13 @@ void rte_mempool_walk(void (*func)(struct rte_mempool *, void *), { struct rte_tailq_entry *te = NULL; struct rte_mempool_list *mempool_list; + void *temp; mempool_list = RTE_TAILQ_CAST(rte_mempool_tailq.head, rte_mempool_list); rte_rwlock_read_lock(RTE_EAL_MEMPOOL_RWLOCK); - TAILQ_FOREACH(te, mempool_list, next) { + TAILQ_FOREACH_SAFE(te, mempool_list, next, temp) { (*func)((struct rte_mempool *) te->data, arg); } -- 2.4.11 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH 2/2] mempool: fix unsafe tailq element removal 2016-07-22 16:01 ` [dpdk-dev] [PATCH 2/2] mempool: fix unsafe tailq element removal Sergio Gonzalez Monroy @ 2016-07-25 16:30 ` Olivier Matz 2016-07-25 19:54 ` [dpdk-dev] [PATCH v2] mempool: fix unsafe removal from list by callback Thomas Monjalon 0 siblings, 1 reply; 7+ messages in thread From: Olivier Matz @ 2016-07-25 16:30 UTC (permalink / raw) To: Sergio Gonzalez Monroy, dev; +Cc: thomas.monjalon Hi Sergio, On 07/22/2016 06:01 PM, Sergio Gonzalez Monroy wrote: > Potentially user provided function could remove/free tailq elements. > Doing so within a TAILQ_FOREACH loop is not safe. > > Use _SAFE versions of _FOREACH macros. > > Signed-off-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com> > --- > lib/librte_mempool/rte_mempool.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c > index 8806633..394154a 100644 > --- a/lib/librte_mempool/rte_mempool.c > +++ b/lib/librte_mempool/rte_mempool.c > @@ -157,10 +157,10 @@ rte_mempool_obj_iter(struct rte_mempool *mp, > rte_mempool_obj_cb_t *obj_cb, void *obj_cb_arg) > { > struct rte_mempool_objhdr *hdr; > - void *obj; > + void *obj, *temp; > unsigned n = 0; > > - STAILQ_FOREACH(hdr, &mp->elt_list, next) { > + STAILQ_FOREACH_SAFE(hdr, &mp->elt_list, next, temp) { > obj = (char *)hdr + sizeof(*hdr); > obj_cb(mp, obj_cb_arg, obj, n); > n++; > @@ -176,8 +176,9 @@ rte_mempool_mem_iter(struct rte_mempool *mp, > { > struct rte_mempool_memhdr *hdr; > unsigned n = 0; > + void *temp; > > - STAILQ_FOREACH(hdr, &mp->mem_list, next) { > + STAILQ_FOREACH_SAFE(hdr, &mp->mem_list, next, temp) { > mem_cb(mp, mem_cb_arg, hdr, n); > n++; > } Not sure it is required to use the _SAFE() variant here. The object or mem_chunk should be considered as const, because these objects are not allocated/freed by the user but by the mempool functions. > @@ -1283,12 +1284,13 @@ void rte_mempool_walk(void (*func)(struct rte_mempool *, void *), > { > struct rte_tailq_entry *te = NULL; > struct rte_mempool_list *mempool_list; > + void *temp; > > mempool_list = RTE_TAILQ_CAST(rte_mempool_tailq.head, rte_mempool_list); > > rte_rwlock_read_lock(RTE_EAL_MEMPOOL_RWLOCK); > > - TAILQ_FOREACH(te, mempool_list, next) { > + TAILQ_FOREACH_SAFE(te, mempool_list, next, temp) { > (*func)((struct rte_mempool *) te->data, arg); > } > > I think this one is legitimate and we should have it for 16.07. So only this hunk would be required, and the patch 1/2 may be dropped if we remove the first 2 chunks. Regards, Olivier ^ permalink raw reply [flat|nested] 7+ messages in thread
* [dpdk-dev] [PATCH v2] mempool: fix unsafe removal from list by callback 2016-07-25 16:30 ` Olivier Matz @ 2016-07-25 19:54 ` Thomas Monjalon 2016-07-25 20:09 ` Olivier Matz 0 siblings, 1 reply; 7+ messages in thread From: Thomas Monjalon @ 2016-07-25 19:54 UTC (permalink / raw) To: olivier.matz; +Cc: dev If a mempool is removed from the list by a callback function during rte_mempool_walk(), the TAILQ_FOREACH loop will fail unexpectedly. It is fixed by using the safe version of the loop macro. Reported-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com> --- lib/librte_mempool/rte_mempool.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c index 8806633..2e28e2e 100644 --- a/lib/librte_mempool/rte_mempool.c +++ b/lib/librte_mempool/rte_mempool.c @@ -1283,12 +1283,13 @@ void rte_mempool_walk(void (*func)(struct rte_mempool *, void *), { struct rte_tailq_entry *te = NULL; struct rte_mempool_list *mempool_list; + void *tmp_te; mempool_list = RTE_TAILQ_CAST(rte_mempool_tailq.head, rte_mempool_list); rte_rwlock_read_lock(RTE_EAL_MEMPOOL_RWLOCK); - TAILQ_FOREACH(te, mempool_list, next) { + TAILQ_FOREACH_SAFE(te, mempool_list, next, tmp_te) { (*func)((struct rte_mempool *) te->data, arg); } -- 2.7.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH v2] mempool: fix unsafe removal from list by callback 2016-07-25 19:54 ` [dpdk-dev] [PATCH v2] mempool: fix unsafe removal from list by callback Thomas Monjalon @ 2016-07-25 20:09 ` Olivier Matz 2016-07-25 20:21 ` Thomas Monjalon 0 siblings, 1 reply; 7+ messages in thread From: Olivier Matz @ 2016-07-25 20:09 UTC (permalink / raw) To: Thomas Monjalon; +Cc: dev Hello Thomas, On 07/25/2016 09:54 PM, Thomas Monjalon wrote: > If a mempool is removed from the list by a callback function > during rte_mempool_walk(), the TAILQ_FOREACH loop will fail unexpectedly. > It is fixed by using the safe version of the loop macro. > > Reported-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com> > Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com> > --- > lib/librte_mempool/rte_mempool.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c > index 8806633..2e28e2e 100644 > --- a/lib/librte_mempool/rte_mempool.c > +++ b/lib/librte_mempool/rte_mempool.c > @@ -1283,12 +1283,13 @@ void rte_mempool_walk(void (*func)(struct rte_mempool *, void *), > { > struct rte_tailq_entry *te = NULL; > struct rte_mempool_list *mempool_list; > + void *tmp_te; > > mempool_list = RTE_TAILQ_CAST(rte_mempool_tailq.head, rte_mempool_list); > > rte_rwlock_read_lock(RTE_EAL_MEMPOOL_RWLOCK); > > - TAILQ_FOREACH(te, mempool_list, next) { > + TAILQ_FOREACH_SAFE(te, mempool_list, next, tmp_te) { > (*func)((struct rte_mempool *) te->data, arg); > } > > Acked-by: Olivier Matz <olivier.matz@6wind.com> Thanks ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH v2] mempool: fix unsafe removal from list by callback 2016-07-25 20:09 ` Olivier Matz @ 2016-07-25 20:21 ` Thomas Monjalon 0 siblings, 0 replies; 7+ messages in thread From: Thomas Monjalon @ 2016-07-25 20:21 UTC (permalink / raw) To: Olivier Matz; +Cc: dev > > If a mempool is removed from the list by a callback function > > during rte_mempool_walk(), the TAILQ_FOREACH loop will fail unexpectedly. > > It is fixed by using the safe version of the loop macro. > > > > Reported-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com> > > Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com> > > Acked-by: Olivier Matz <olivier.matz@6wind.com> Applied, thanks ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [dpdk-dev] [PATCH 1/2] eal: add stailq safe iterator macro 2016-07-22 16:01 [dpdk-dev] [PATCH 1/2] eal: add stailq safe iterator macro Sergio Gonzalez Monroy 2016-07-22 16:01 ` [dpdk-dev] [PATCH 2/2] mempool: fix unsafe tailq element removal Sergio Gonzalez Monroy @ 2016-07-22 16:16 ` Thomas Monjalon 1 sibling, 0 replies; 7+ messages in thread From: Thomas Monjalon @ 2016-07-22 16:16 UTC (permalink / raw) To: Sergio Gonzalez Monroy; +Cc: dev 2016-07-22 17:01, Sergio Gonzalez Monroy: > Removing/freeing elements elements within a STAILQ_FOREACH loop > is not safe. FreeBSD defines STAILQ_FOREACH_SAFE macro, which permits > these operations safely. > > This patch defines this macro for Linux systems, where it is not defined. [...] > +#ifndef SLIST_FOREACH_SAFE > +#define SLIST_FOREACH_SAFE(var, head, field, tvar) \ > + for ((var) = SLIST_FIRST((head)); \ > + (var) && ((tvar) = SLIST_NEXT((var), field), 1); \ > + (var) = (tvar)) > +#endif The patch 2 requires STAILQ_FOREACH_SAFE, not SLIST_FOREACH_SAFE. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-07-25 20:22 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-07-22 16:01 [dpdk-dev] [PATCH 1/2] eal: add stailq safe iterator macro Sergio Gonzalez Monroy 2016-07-22 16:01 ` [dpdk-dev] [PATCH 2/2] mempool: fix unsafe tailq element removal Sergio Gonzalez Monroy 2016-07-25 16:30 ` Olivier Matz 2016-07-25 19:54 ` [dpdk-dev] [PATCH v2] mempool: fix unsafe removal from list by callback Thomas Monjalon 2016-07-25 20:09 ` Olivier Matz 2016-07-25 20:21 ` Thomas Monjalon 2016-07-22 16:16 ` [dpdk-dev] [PATCH 1/2] eal: add stailq safe iterator macro Thomas Monjalon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).