[Devel] [PATCH] binfmt_misc: fix mount after umount in CT

[Alexander Mikhalitsyn]

On Tue, 19 Oct 2021 16:44:20 +0300
Nikita Yushchenko <nikita.yushchenko at virtuozzo.com> wrote:

> >> -	bm_data = kzalloc(sizeof(struct binfmt_misc), GFP_KERNEL);
> >> -	if (!bm_data)
> >> -		return -ENOMEM;
> >> +		INIT_LIST_HEAD(&bm_data->entries);
> >> +		rwlock_init(&bm_data->entries_lock);
> >>   
> >> -	INIT_LIST_HEAD(&bm_data->entries);
> >> -	rwlock_init(&bm_data->entries_lock);
> >> +		ve->binfmt_misc = bm_data;
> > 
> > Isn't it better to move ve->binfmt_misc assignment to the
> > end of function where we know that all operations was successful?
> 
> Since ve->binfmt_misc is global, not per-mount, the logic is simpler if creation of bm_data is made 
> independent from mount success/failure.  I.e. regardless of success of mount operation, bm_data is 
> created at the first mount and saved in ve->binfmt_misc. Then, it will be cleaned at ve destruction time.

I agree.

> 
> > 
> >> +		/* this will be cleared by ve_binfmt_fini() */
> >> +	}
> >>   
> >>   	err = simple_fill_super(sb, BINFMTFS_MAGIC, bm_files);
> >> -	if (err) {
> >> -		kfree(bm_data);
> > 
> > If we have ve->binfmt_misc assignment in the upper part of code, then
> > we need to do ve->binfmt_misc = NULL here.
> 
> This will mishandle case when ve->binfmt_misc was initialized at previous mount.
> 
> > 
> >> +	if (err)
> >>   		return err;
> >> -	}
> >>   
> >>   	sb->s_op = &s_ops;
> >> -
> >> -	ve->binfmt_misc = bm_data;
> > see above
> > 
> >>   	bm_data->enabled = 1;
> >>   
> >>   	return 0;
> >> @@ -971,6 +958,8 @@ static void ve_binfmt_fini(void *data)
> >>   	while (!list_empty(&bm_data->entries))
> >>   		kill_node(bm_data, list_first_entry(
> >>   			&bm_data->entries, Node, list));
> >> +
> >> +	kfree(bm_data);
> > 
> > We have kfree in ve_destroy (kernel/ve/ve.c) already.
> 
> Indeed.
> 
> But, why splitting destruction into two parts?
> Why not doing both at the same location.

no problem, but then we have to remove kfree from kernel/ve/ve.c or
we get double kfree() here.

> 
> Nikita


Alex