On Tue, 19 Oct 2021 13:59:34 +0300 Alexander Mikhalitsyn <alexander.mikhalit...@virtuozzo.com> wrote:
> On Mon, 18 Oct 2021 19:51:49 +0300 > Nikita Yushchenko <nikita.yushche...@virtuozzo.com> wrote: > > > The assumption that bm_fill_super() is not called for the second time > > for CT is wrong: umount operation clears sb->s_root, which causes > > vfs_get_super() to call fill_super again on the next mount. > > > > Make bm_fill_super() handle multiple-calls corrently: > > - initialize bm_data and set ve->binfmt_misc only if it is not done > > before, > > - delay desctruction of it up to CT destruction. > > > > https://jira.sw.ru/browse/PSBM-133968 > > Fixes: 8250ff41d190 ("ve/fs/binfmt: clean bm_data reference from ve on err > > path") > > Signed-off-by: Nikita Yushchenko <nikita.yushche...@virtuozzo.com> > > --- > > fs/binfmt_misc.c | 37 +++++++++++++------------------------ > > 1 file changed, 13 insertions(+), 24 deletions(-) > > > > diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c > > index 0946e7e6caa5..5f4e90c1ade2 100644 > > --- a/fs/binfmt_misc.c > > +++ b/fs/binfmt_misc.c > > @@ -853,38 +853,25 @@ static int bm_fill_super(struct super_block *sb, > > struct fs_context *fc) > > }; > > > > struct ve_struct *ve = get_exec_env(); > > - struct binfmt_misc *bm_data; > > + struct binfmt_misc *bm_data = ve->binfmt_misc; > > > > - /* > > - * bm_get_tree() > > - * get_tree_keyed(fc, bm_fill_super, get_ve(ve)) > > - * fc->s_fs_info = current VE > > - * vfs_get_super(fc, vfs_get_keyed_super, bm_fill_super) > > - * sb = sget_fc(fc, test, set_anon_super_fc) > > - * if (!sb->s_root) { > > - * err = bm_fill_super(sb, fc); > > - * > > - * => we should never get here with initialized ve->binfmt_misc. > > - */ > > - if (WARN_ON_ONCE(ve->binfmt_misc)) > > - return -EEXIST; > > + if (!bm_data) { > > + bm_data = kzalloc(sizeof(struct binfmt_misc), GFP_KERNEL); > > + if (!bm_data) > > + return -ENOMEM; > > > > - 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? > > > + /* 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. please ignore. misread ;) > > > + 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. this is actual > > > } > > > > static struct ve_hook ve_binfmt_hook = { > > -- > > 2.30.2 > > > > > _______________________________________________ > Devel mailing list > Devel@openvz.org > https://lists.openvz.org/mailman/listinfo/devel _______________________________________________ Devel mailing list Devel@openvz.org https://lists.openvz.org/mailman/listinfo/devel
