On 28.07.2020 20:53, Valeriy Vdovin wrote:
> Each container will now have access to it's own cgroup release_agent
> file.
> Creation:
> Normally all cgroup files are created during a call to cgroup_create
> by cgroup_populate_dir function. It creates or not creates all cgroup
> files once and they immediately become visible to userspace as
> filesystem objects.
> Due to specifics of container creation process, it is not possible to
> use the same code for 'release_agent' file creation. For VE to start
> operating, first a list of ordinary cgroups is being created for
> each subsystem, then the set of newly created cgroups are converted to
> "virtual roots", so at the time when cgroup_create is executed, there
> is no knowledge of wheather or not "release_agent" file should be
> created. This information only comes at "convertion" step which is
> 'cgroup_mark_ve_roots' function. As the file is created dynamically
> in a live cgroup, a rather delicate locking sequence is present in
> the new code:
> - each new "virtual root" cgroup will have to add "release_agent" file,
> thus each cgroup's directory would need to be locked during
> the insertion time by cgroup->dentry->d_inode->i_mutex.
> - d_inode->i_mutex has an ordering dependency with cgroup_mutex
> (see cgroup_mount/cgroup_remount). They can not be locked in order
> {lock(cgroup_mutex), lock(inode->i_mutex)}.
> - to collect a list of cgroups, that need to become virtual we need
> cgroup_mutex lock to iterate active roots.
> - to overcome the above conflict we first need to collect a list of
> all virtual cgroups under cgroup_mutex lock, then release it and
> after that to insert "release_agent" to each root under
> inode->i_mutex lock.
> - to collect a list of cgroups on stack we utilize
> cgroup->cft_q_node, made specially for that purpose under it's own
> cgroup_cft_mutex.
>
> Destruction:
> Destruction is done in reverse from the above within
> cgroup_unmark_ve_root.
> After file destruction we must prevent further write operations to
> this file in case when someone has opened this file prior to VE
> and cgroup destruction. This is achieved by checking if cgroup
> in the argument to cgroup_file_write function has features of host
> or virtual root.
>
> https://jira.sw.ru/browse/PSBM-83887
>
> Signed-off-by: Valeriy Vdovin <valeriy.vdo...@virtuozzo.com>
> ---
> include/linux/cgroup.h | 2 +-
> include/linux/ve.h | 2 +-
> kernel/cgroup.c | 86
> ++++++++++++++++++++++++++++++++++++++++++++++----
> kernel/ve/ve.c | 6 +++-
> 4 files changed, 87 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
> index fc138c0..645c9fd 100644
> --- a/include/linux/cgroup.h
> +++ b/include/linux/cgroup.h
> @@ -671,7 +671,7 @@ int cgroup_task_count(const struct cgroup *cgrp);
> void cgroup_release_agent(struct work_struct *work);
>
> #ifdef CONFIG_VE
> -void cgroup_mark_ve_roots(struct ve_struct *ve);
> +int cgroup_mark_ve_roots(struct ve_struct *ve);
> void cgroup_unmark_ve_roots(struct ve_struct *ve);
> struct ve_struct *cgroup_get_ve_owner(struct cgroup *cgrp);
> #endif
> diff --git a/include/linux/ve.h b/include/linux/ve.h
> index b6662637..5bf275f 100644
> --- a/include/linux/ve.h
> +++ b/include/linux/ve.h
> @@ -215,7 +215,7 @@ void do_update_load_avg_ve(void);
> void ve_add_to_release_list(struct cgroup *cgrp);
> void ve_rm_from_release_list(struct cgroup *cgrp);
>
> -int ve_set_release_agent_path(struct ve_struct *ve, struct cgroup *cgroot,
> +int ve_set_release_agent_path(struct cgroup *cgroot,
> const char *release_agent);
>
> const char *ve_get_release_agent_path(struct cgroup *cgrp_root);
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 1d9c889..bb77804 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -2360,13 +2360,28 @@ static int cgroup_release_agent_write(struct cgroup
> *cgrp, struct cftype *cft,
> if (!cgroup_lock_live_group(cgrp))
> return -ENODEV;
>
> + /*
> + * Call to cgroup_get_local_root is a way to make sure
> + * that cgrp in the argument is valid "virtual root"
> + * cgroup. If it's not - this means that cgroup_unmark_ve_roots
> + * has already cleared CGRP_VE_ROOT flag from this cgroup while
> + * the file was still opened by other process and
> + * that we shouldn't allow further writings via that file.
> + */
> root_cgrp = cgroup_get_local_root(cgrp);
> BUG_ON(!root_cgrp);
> +
> + if (root_cgrp != cgrp) {
> + ret = -EPERM;
> + goto out;
> + }
> +
> if (root_cgrp->ve_owner)
> ret = ve_set_release_agent_path(root_cgrp, buffer);
> else
> return -ENODEV;
>
> +out:
> mutex_unlock(&cgroup_mutex);
> return ret;
> }
> @@ -4204,7 +4219,7 @@ static struct cftype files[] = {
> },
> {
> .name = "release_agent",
> - .flags = CFTYPE_ONLY_ON_ROOT,
> + .flags = CFTYPE_ONLY_ON_ROOT | CFTYPE_VE_WRITABLE,
> .read_seq_string = cgroup_release_agent_show,
> .write_string = cgroup_release_agent_write,
> .max_write_len = PATH_MAX,
> @@ -4422,39 +4437,98 @@ static int subgroups_count(struct cgroup *cgroup)
> return cgrps_count;
> }
>
> +static struct cftype *get_cftype_by_name(const char *name)
> +{
> + struct cftype *cft;
> + for (cft = files; cft->name[0] != '\0'; cft++) {
> + if (!strcmp(cft->name, name))
> + return cft;
> + }
> + return NULL;
> +}
> +
> #ifdef CONFIG_VE
> -void cgroup_mark_ve_roots(struct ve_struct *ve)
> +int cgroup_mark_ve_roots(struct ve_struct *ve)
> {
> - struct cgroup *cgrp;
> + struct cgroup *cgrp, *tmp;
> struct cgroupfs_root *root;
> + int err = 0;
> + struct cftype *cft;
> + LIST_HEAD(pending);
>
> + cft = get_cftype_by_name("release_agent");
> + BUG_ON(!cft);
> +
> + mutex_lock(&cgroup_cft_mutex);
> mutex_lock(&cgroup_mutex);
> for_each_active_root(root) {
> - cgrp = task_cgroup_from_root(ve->init_task, root);
> + cgrp = css_cgroup_from_root(ve->root_css_set, root);
> rcu_assign_pointer(cgrp->ve_owner, ve);
> set_bit(CGRP_VE_ROOT, &cgrp->flags);
>
> + dget(cgrp->dentry);
> + list_add_tail(&cgrp->cft_q_node, &pending);
> if (test_bit(cpu_cgroup_subsys_id, &root->subsys_mask))
> link_ve_root_cpu_cgroup(cgrp);
> }
> mutex_unlock(&cgroup_mutex);
> synchronize_rcu();
> + list_for_each_entry_safe(cgrp, tmp, &pending, cft_q_node) {
> + struct inode *inode = cgrp->dentry->d_inode;
> +
> + if (err) {
> + list_del_init(&cgrp->cft_q_node);
> + dput(cgrp->dentry);
> + continue;
> + }
How does this work?
If cgroup_add_file() sets error on previous iteration, then here we do double
dput()?
> +
> + mutex_lock(&inode->i_mutex);
> + mutex_lock(&cgroup_mutex);
> + if (!cgroup_is_removed(cgrp))
> + err = cgroup_add_file(cgrp, NULL, cft);
> + mutex_unlock(&cgroup_mutex);
> + mutex_unlock(&inode->i_mutex);
> +
> + list_del_init(&cgrp->cft_q_node);
> + dput(cgrp->dentry);
> + }
> + mutex_unlock(&cgroup_cft_mutex);
> +
> + if (err)
> + cgroup_unmark_ve_roots(ve);
> +
> + return err;
> }
>
> void cgroup_unmark_ve_roots(struct ve_struct *ve)
> {
> - struct cgroup *cgrp;
> + struct cgroup *cgrp, *tmp;
> struct cgroupfs_root *root;
> + struct cftype *cft;
> + LIST_HEAD(pending);
> +
> + cft = get_cftype_by_name("release_agent");
>
> + mutex_lock(&cgroup_cft_mutex);
> mutex_lock(&cgroup_mutex);
> for_each_active_root(root) {
> cgrp = css_cgroup_from_root(ve->root_css_set, root);
> + dget(cgrp->dentry);
> + list_add_tail(&cgrp->cft_q_node, &pending);
> + }
> + mutex_unlock(&cgroup_mutex);
> + list_for_each_entry_safe(cgrp, tmp, &pending, cft_q_node) {
> + struct inode *inode = cgrp->dentry->d_inode;
> + mutex_lock(&inode->i_mutex);
> + mutex_lock(&cgroup_mutex);
> + cgroup_rm_file(cgrp, cft);
> BUG_ON(!rcu_dereference_protected(cgrp->ve_owner,
> lockdep_is_held(&cgroup_mutex)));
> rcu_assign_pointer(cgrp->ve_owner, NULL);
> clear_bit(CGRP_VE_ROOT, &cgrp->flags);
> + mutex_unlock(&cgroup_mutex);
> + mutex_unlock(&inode->i_mutex);
> }
> - mutex_unlock(&cgroup_mutex);
> + mutex_unlock(&cgroup_cft_mutex);
> /* ve_owner == NULL will be visible */
> synchronize_rcu();
>
> diff --git a/kernel/ve/ve.c b/kernel/ve/ve.c
> index f03f665..8d78270 100644
> --- a/kernel/ve/ve.c
> +++ b/kernel/ve/ve.c
> @@ -718,7 +718,9 @@ static int ve_start_container(struct ve_struct *ve)
> if (err < 0)
> goto err_iterate;
>
> - cgroup_mark_ve_roots(ve);
> + err = cgroup_mark_ve_roots(ve);
> + if (err)
> + goto err_mark_ve;
>
> ve->is_running = 1;
>
> @@ -728,6 +730,8 @@ static int ve_start_container(struct ve_struct *ve)
>
> return 0;
>
> +err_mark_ve:
> + ve_hook_iterate_fini(VE_SS_CHAIN, ve);
> err_iterate:
> ve_workqueue_stop(ve);
> err_workqueue:
>
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
