* [PATCH 1/2] eal: add stailq safe iterator macro @ 2016-07-22 16:01 Sergio Gonzalez Monroy 2016-07-22 16:01 ` [PATCH 2/2] mempool: fix unsafe tailq element removal Sergio Gonzalez Monroy 2016-07-22 16:16 ` [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 related [flat|nested] 7+ messages in thread
* [PATCH 2/2] mempool: fix unsafe tailq element removal 2016-07-22 16:01 [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 ` [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 related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] mempool: fix unsafe tailq element removal 2016-07-22 16:01 ` [PATCH 2/2] mempool: fix unsafe tailq element removal Sergio Gonzalez Monroy @ 2016-07-25 16:30 ` Olivier Matz 2016-07-25 19:54 ` [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
* [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 related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] mempool: fix unsafe removal from list by callback 2016-07-25 19:54 ` [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: [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: [PATCH 1/2] eal: add stailq safe iterator macro 2016-07-22 16:01 [PATCH 1/2] eal: add stailq safe iterator macro Sergio Gonzalez Monroy 2016-07-22 16:01 ` [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 [PATCH 1/2] eal: add stailq safe iterator macro Sergio Gonzalez Monroy 2016-07-22 16:01 ` [PATCH 2/2] mempool: fix unsafe tailq element removal Sergio Gonzalez Monroy 2016-07-25 16:30 ` Olivier Matz 2016-07-25 19:54 ` [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 ` [PATCH 1/2] eal: add stailq safe iterator macro Thomas Monjalon
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.