Re: [dpdk-dev] [PATCH] timer: fix resource leak in finalize

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 01-May-19 8:00 PM, Erik Gabriel Carrillo wrote:
> The finalize function should free the memzone created in the init
> function, rather than freeing the allocation the memzone references,
> otherwise a memzone descriptor can be leaked.
> 
> Fixes: c0749f7096c7 ("timer: allow management in shared memory")
> 
> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> ---
>   lib/librte_timer/rte_timer.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
> index eb46009..fb7a87e 100644
> --- a/lib/librte_timer/rte_timer.c
> +++ b/lib/librte_timer/rte_timer.c
> @@ -60,6 +60,7 @@ struct rte_timer_data {
>   };
>   
>   #define RTE_MAX_DATA_ELS 64
> +static const struct rte_memzone *rte_timer_data_mz;
>   static struct rte_timer_data *rte_timer_data_arr;
>   static const uint32_t default_data_id;
>   static uint32_t rte_timer_subsystem_initialized;
> @@ -164,6 +165,7 @@ rte_timer_subsystem_init_v1905(void)
>   		if (mz == NULL)
>   			return -EEXIST;
>   
> +		rte_timer_data_mz = mz;
>   		rte_timer_data_arr = mz->addr;
>   
>   		rte_timer_data_arr[default_data_id].internal_flags |=
> @@ -180,6 +182,7 @@ rte_timer_subsystem_init_v1905(void)
>   	if (mz == NULL)
>   		return -ENOMEM;
>   
> +	rte_timer_data_mz = mz;
>   	rte_timer_data_arr = mz->addr;
>   
>   	for (i = 0; i < RTE_MAX_DATA_ELS; i++) {
> @@ -205,8 +208,13 @@ BIND_DEFAULT_SYMBOL(rte_timer_subsystem_init, _v1905, 19.05);
>   void __rte_experimental
>   rte_timer_subsystem_finalize(void)
>   {
> -	if (rte_timer_data_arr)
> -		rte_free(rte_timer_data_arr);
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return;
> +
> +	if (!rte_timer_subsystem_initialized)
> +		return;
> +
> +	rte_memzone_free(rte_timer_data_mz);

The patch is a correct fix, but the whole idea of this looks dangerous 
to me.

If we exit the primary while secondaries are still running, wouldn't it 
basically pull out timer data from under secondaries' feet?

>   
>   	rte_timer_subsystem_initialized = 0;
>   }
> 


-- 
Thanks,
Anatoly