[dpdk-dev] [PATCH] mempool: fix local cache initialization
Hi Sergio, On 06/09/2016 09:57 AM, Sergio Gonzalez Monroy wrote: > Hi Olivier, > > On 08/06/2016 20:14, Olivier Matz wrote: >> Hi Sergio, >> >> Good catch, thanks. The patch looks ok, just few comments >> on the commit log: >> >> On 06/08/2016 05:10 PM, Sergio Gonzalez Monroy wrote: >>> The mempool local cache is not being initialize properly leading to >> 'initialize' -> 'initialized' ? >> and maybe 'is not being' -> 'was not' ? >> >>> undefined behavior in cases where the allocated memory was used and left >>> with data. >>> >>> Fixes: af75078fece3 ("first public release") >> I think it fixes this one instead: >> >> 213af31e0960 ("mempool: reduce structure size if no cache needed") > > Fair enough, I thought the issue was there as we never > initialized/zeroed the local cache > on mempool creation. Usually we would have allocated all mempools on > init (or close) > and that would be it (initially all memory would be zeroed), but I think > you could still > manage to reproduce the problem if somehow you where to do something like: > rte_malloc(), rte_free(), rte_mempool_create() and the memory was the > one we got > with malloc and never gets zeroed again. Before Keith's commit (213af31e0960), the local cache was initialized when doing the memset() because it was included in the mempool structure. So I think the problem did not exist before this patch. Or did I miss something in your explanation? Regards, Olivier
[dpdk-dev] [PATCH] mempool: fix local cache initialization
On 09/06/2016 09:03, Olivier Matz wrote: > Hi Sergio, > > On 06/09/2016 09:57 AM, Sergio Gonzalez Monroy wrote: >> Hi Olivier, >> >> On 08/06/2016 20:14, Olivier Matz wrote: >>> Hi Sergio, >>> >>> Good catch, thanks. The patch looks ok, just few comments >>> on the commit log: >>> >>> On 06/08/2016 05:10 PM, Sergio Gonzalez Monroy wrote: The mempool local cache is not being initialize properly leading to >>> 'initialize' -> 'initialized' ? >>> and maybe 'is not being' -> 'was not' ? >>> undefined behavior in cases where the allocated memory was used and left with data. Fixes: af75078fece3 ("first public release") >>> I think it fixes this one instead: >>> >>> 213af31e0960 ("mempool: reduce structure size if no cache needed") >> Fair enough, I thought the issue was there as we never >> initialized/zeroed the local cache >> on mempool creation. Usually we would have allocated all mempools on >> init (or close) >> and that would be it (initially all memory would be zeroed), but I think >> you could still >> manage to reproduce the problem if somehow you where to do something like: >> rte_malloc(), rte_free(), rte_mempool_create() and the memory was the >> one we got >> with malloc and never gets zeroed again. > Before Keith's commit (213af31e0960), the local cache was initialized > when doing the memset() because it was included in the mempool > structure. So I think the problem did not exist before this patch. > Or did I miss something in your explanation? > > Regards, > Olivier You are spot on! I did look at a wrong commit when checking for the old mempool struct. Cheers, Sergio
[dpdk-dev] [PATCH] mempool: fix local cache initialization
Hi Olivier, On 08/06/2016 20:14, Olivier Matz wrote: > Hi Sergio, > > Good catch, thanks. The patch looks ok, just few comments > on the commit log: > > On 06/08/2016 05:10 PM, Sergio Gonzalez Monroy wrote: >> The mempool local cache is not being initialize properly leading to > 'initialize' -> 'initialized' ? > and maybe 'is not being' -> 'was not' ? > >> undefined behavior in cases where the allocated memory was used and left >> with data. >> >> Fixes: af75078fece3 ("first public release") > I think it fixes this one instead: > > 213af31e0960 ("mempool: reduce structure size if no cache needed") Fair enough, I thought the issue was there as we never initialized/zeroed the local cache on mempool creation. Usually we would have allocated all mempools on init (or close) and that would be it (initially all memory would be zeroed), but I think you could still manage to reproduce the problem if somehow you where to do something like: rte_malloc(), rte_free(), rte_mempool_create() and the memory was the one we got with malloc and never gets zeroed again. Sergio
[dpdk-dev] [PATCH] mempool: fix local cache initialization
Hi Sergio, Good catch, thanks. The patch looks ok, just few comments on the commit log: On 06/08/2016 05:10 PM, Sergio Gonzalez Monroy wrote: > The mempool local cache is not being initialize properly leading to 'initialize' -> 'initialized' ? and maybe 'is not being' -> 'was not' ? > undefined behavior in cases where the allocated memory was used and left > with data. > > Fixes: af75078fece3 ("first public release") I think it fixes this one instead: 213af31e0960 ("mempool: reduce structure size if no cache needed") > > Signed-off-by: Sergio Gonzalez Monroy > --- > lib/librte_mempool/rte_mempool.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/librte_mempool/rte_mempool.c > b/lib/librte_mempool/rte_mempool.c > index b54de43..216514c 100644 > --- a/lib/librte_mempool/rte_mempool.c > +++ b/lib/librte_mempool/rte_mempool.c > @@ -787,7 +787,7 @@ rte_mempool_create_empty(const char *name, unsigned n, > unsigned elt_size, > > /* init the mempool structure */ > mp = mz->addr; > - memset(mp, 0, sizeof(*mp)); > + memset(mp, 0, MEMPOOL_HEADER_SIZE(mp, cache_size)); > ret = snprintf(mp->name, sizeof(mp->name), "%s", name); > if (ret < 0 || ret >= (int)sizeof(mp->name)) { > rte_errno = ENAMETOOLONG; >
[dpdk-dev] [PATCH] mempool: fix local cache initialization
The mempool local cache is not being initialize properly leading to undefined behavior in cases where the allocated memory was used and left with data. Fixes: af75078fece3 ("first public release") Signed-off-by: Sergio Gonzalez Monroy --- lib/librte_mempool/rte_mempool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c index b54de43..216514c 100644 --- a/lib/librte_mempool/rte_mempool.c +++ b/lib/librte_mempool/rte_mempool.c @@ -787,7 +787,7 @@ rte_mempool_create_empty(const char *name, unsigned n, unsigned elt_size, /* init the mempool structure */ mp = mz->addr; - memset(mp, 0, sizeof(*mp)); + memset(mp, 0, MEMPOOL_HEADER_SIZE(mp, cache_size)); ret = snprintf(mp->name, sizeof(mp->name), "%s", name); if (ret < 0 || ret >= (int)sizeof(mp->name)) { rte_errno = ENAMETOOLONG; -- 2.4.11