[dpdk-dev] [PATCH] mempool: fix local cache initialization

[dpdk-dev] [PATCH] mempool: fix local cache initialization

[Sergio Gonzalez Monroy]

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 <sergio.gonzalez.monroy@intel.com>
---
 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

Re: [dpdk-dev] [PATCH] mempool: fix local cache initialization

[Olivier Matz]

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 <sergio.gonzalez.monroy@intel.com>
> ---
>  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;
> 

Re: [dpdk-dev] [PATCH] mempool: fix local cache initialization

[Sergio Gonzalez Monroy]

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

Re: [dpdk-dev] [PATCH] mempool: fix local cache initialization

[Olivier Matz]

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

Re: [dpdk-dev] [PATCH] mempool: fix local cache initialization

[Sergio Gonzalez Monroy]

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 v2] mempool: fix local cache initialization

[Sergio Gonzalez Monroy]

The mempool local cache was not initialized properly leading to
undefined behavior in cases where the allocated memory was used
previously and left with data.

Fixes: 213af31e0960 ("mempool: reduce structure size if no cache needed")

Signed-off-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
---
 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

Re: [dpdk-dev] [PATCH v2] mempool: fix local cache initialization

[Olivier Matz]

On 06/09/2016 10:19 AM, Sergio Gonzalez Monroy wrote:
> The mempool local cache was not initialized properly leading to
> undefined behavior in cases where the allocated memory was used
> previously and left with data.
> 
> Fixes: 213af31e0960 ("mempool: reduce structure size if no cache needed")
> 
> Signed-off-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>

Acked-by: Olivier Matz <olivier.matz@6wind.com>

Thanks

Re: [dpdk-dev] [PATCH v2] mempool: fix local cache initialization

[Thomas Monjalon]

2016-06-09 10:26, Olivier Matz:
> On 06/09/2016 10:19 AM, Sergio Gonzalez Monroy wrote:
> > The mempool local cache was not initialized properly leading to
> > undefined behavior in cases where the allocated memory was used
> > previously and left with data.
> > 
> > Fixes: 213af31e0960 ("mempool: reduce structure size if no cache needed")
> > 
> > Signed-off-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
> 
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
> 
> Thanks

Applied, thanks