[dpdk-dev] 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 <sergio.gonzalez.monroy@intel.com>
---
 lib/librte_mempool/rte_mempool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

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;
>
  

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
  

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
  

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