[Devel] [PATCH vz10 v2] ve: set proper VE state on ve_create() error

[Pavel Tikhomirov]

On 9/6/25 01:03, Konstantin Khorenko wrote:
> If we fail to alloc ve_struct, we'll crash here on setting VE state.
> 
> Moving ve_set_state() earlier makes err_ve: label redundant,
> so get rid of it according to coding style rules.
> 
> Also let's set VE_STATE_DEAD state right before freeing ve_struct
> instead of VE_STATE_STOPPED, it will be more logical to see _DEAD state
> in the freed/not used memory and it corresponds to other cases when
> VE_STATE_DEAD is set:
> 
> ve_start_container()
> err handling:
>          ve_set_state(ve, VE_STATE_STOPPED);
>          ve_drop_context(ve); // not freeing memory
> 
> ve_exit_ns
>          ve_set_state(ve, VE_STATE_STOPPED);
> 	put_ve(ve); /* from ve_start_container() */
> 	// no memory free here, only later in ve_destroy()
> 
> ve_destroy
>          ve_set_state(ve, VE_STATE_DEAD);
>          kmem_cache_free(ve_cachep, ve); // freeing memory
> 
> ve_create
> err handling:
> 	ve_set_state(ve, VE_STATE_DEAD); // logical to use _DEAD here
>          kmem_cache_free(ve_cachep, ve);  // freeing memory as well

Agreed, thanks for explaining this one in detail!

> 
> Fixes: 666e40b308457 ("ve/cgroups: Drop lock when stopping workqueue to
> avoid dead lock")
> Signed-off-by: Konstantin Khorenko <khorenko at virtuozzo.com>
> 
> Feature: ve: ve generic structures
> ---
> v2:
>    * drop err_ve: label
> 
>   kernel/ve/ve.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/kernel/ve/ve.c b/kernel/ve/ve.c
> index 146f7922d4856..663c1c2255621 100644
> --- a/kernel/ve/ve.c
> +++ b/kernel/ve/ve.c
> @@ -951,10 +951,9 @@ static struct cgroup_subsys_state *ve_create(struct cgroup_subsys_state *parent_
>   	if (css_to_ve(parent_css) != &ve0)
>   		return ERR_PTR(-ENOTDIR);
>   
> -	err = -ENOMEM;

Due to removal of the above err variable initialization, we can end up 
using uninitialized err in "err_lat" error path.

>   	ve = kmem_cache_zalloc(ve_cachep, GFP_KERNEL);
>   	if (!ve)
> -		goto err_ve;
> +		return ERR_PTR(-ENOMEM);
>   
>   	ve->sched_lat_ve.cur = alloc_percpu(struct kstat_lat_pcpu_snap_struct);
>   	if (!ve->sched_lat_ve.cur)
> @@ -1019,7 +1018,6 @@ static struct cgroup_subsys_state *ve_create(struct cgroup_subsys_state *parent_
>   	ve->aio_nr = 0;
>   	ve->aio_max_nr = AIO_MAX_NR_DEFAULT;
>   #endif
> -
>   	return &ve->css;
>   
>   err_vdso:
> @@ -1028,9 +1026,9 @@ static struct cgroup_subsys_state *ve_create(struct cgroup_subsys_state *parent_
>   err_log:
>   	free_percpu(ve->sched_lat_ve.cur);
>   err_lat:
> +	ve_set_state(ve, VE_STATE_DEAD);
>   	kmem_cache_free(ve_cachep, ve);
> -err_ve:
> -	ve_set_state(ve, VE_STATE_STOPPED);
> +
>   	return ERR_PTR(err);
>   }
>   

-- 
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.