On Tue, Jan 07, 2020 at 08:32:04AM -0800, Tejun Heo wrote:
> On Mon, Dec 23, 2019 at 07:15:03AM +0100, Christian Brauner wrote:
> > +static struct cgroup *cgroup_get_from_file(struct file *f)
> > +{
> > + struct cgroup_subsys_state *css;
> > + struct cgroup *cgrp;
> > +
> > + css = css_tryget_online_from_dir(f->f_path.dentry, NULL);
> > + if (IS_ERR(css))
> > + return ERR_CAST(css);
> > +
> > + cgrp = css->cgroup;
> > + if (!cgroup_on_dfl(cgrp)) {
> > + cgroup_put(cgrp);
> > + return ERR_PTR(-EBADF);
> > + }
> > +
> > + return cgrp;
> > +}
>
> It's minor but can you put this refactoring into a separate patch?
Yep, will do.
>
> ...
> > +static int cgroup_css_set_fork(struct task_struct *parent,
> > + struct kernel_clone_args *kargs)
> > + __acquires(&cgroup_mutex) __acquires(&cgroup_threadgroup_rwsem)
> > +{
> > + int ret;
> > + struct cgroup *dst_cgrp = NULL, *src_cgrp;
> > + struct css_set *cset;
> > + struct super_block *sb;
> > + struct file *f;
> > +
> > + if (kargs->flags & CLONE_INTO_CGROUP) {
> > + ret = mutex_lock_killable(&cgroup_mutex);
> > + if (ret)
> > + return ret;
> > + }
>
> I don't think this is necessary. cgroup_mutex should always only be
> held for a finite enough time; otherwise, processes would get stuck on
> random cgroupfs accesses or even /proc/self/cgroup.
Ok, so a simple mutex_lock() should suffice then.
>
> ...
> > + spin_lock_irq(&css_set_lock);
> > + src_cgrp = task_cgroup_from_root(parent, &cgrp_dfl_root);
> > + spin_unlock_irq(&css_set_lock);
>
> You can simply do cset->dfl_root here, which is consistent with other
> code paths which know that they want the dfl cgroup.
Ah, great!
>
> > + ret = cgroup_attach_permissions(src_cgrp, dst_cgrp, sb,
> > + !!(kargs->flags & CLONE_THREAD));
> > + if (ret)
> > + goto err;
>
> So, the existing perm check depends on the fact that for the write
> operation to have started, it already should have passed write perm
> check on the destination cgroup.procs file. We're missing that here,
> so we prolly need to check that explicitly.
I need to look into this before I can say yay or nay. :)
>
> > @@ -214,13 +215,21 @@ static void pids_cancel_attach(struct cgroup_taskset *tset)
> > +static int pids_can_fork(struct task_struct *parent, struct task_struct *child,
> > + struct kernel_clone_args *args)
> > {
> > + struct css_set *new_cset = NULL;
> > struct cgroup_subsys_state *css;
> > struct pids_cgroup *pids;
> > int err;
> >
> > - css = task_css_check(current, pids_cgrp_id, true);
> > + if (args)
> > + new_cset = args->cset;
> > +
> > + if (!new_cset)
> > + css = task_css_check(current, pids_cgrp_id, true);
> > + else
> > + css = new_cset->subsys[pids_cgrp_id];
>
> Heh, this kinda sucks. Would it be better to pass in the new css into
> the callbacks rather than clone args?
Hm, maybe. My reasoning was that the can_fork callbacks are really only
ever used when - well - fork()ing/clone{3}()ing. Additionally, I was
trying to make sure that struct css_set doesn't show up in too many
places outside of cgroup core. But I'm fine with changing this to just
take the css_set directly. Let's try that...
>
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index 2508a4f238a3..1604552f7cd3 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -2165,16 +2165,15 @@ static __latent_entropy struct task_struct *copy_process(
> > INIT_LIST_HEAD(&p->thread_group);
> > p->task_works = NULL;
> >
> > - cgroup_threadgroup_change_begin(current);
> > /*
> > * Ensure that the cgroup subsystem policies allow the new process to be
> > * forked. It should be noted the the new process's css_set can be changed
> > * between here and cgroup_post_fork() if an organisation operation is in
> > * progress.
> > */
> > - retval = cgroup_can_fork(p);
> > + retval = cgroup_can_fork(current, p, args);
> > if (retval)
> > - goto bad_fork_cgroup_threadgroup_change_end;
> > + goto bad_fork_put_pidfd;
> >
> > /*
> > * From this point on we must avoid any synchronous user-space
>
> Maybe we can move these changes into a prep patch together with the
> get_from_file change so that this patch only contains the actual
> feature implementation?
Should be doable!
>
> Other than that, looks good to me. Once the above review points are
> addressed and Oleg is okay with it, I'll be happy to route this
> through the cgroup tree.
>
> Thanks so much for working on this. This is really cool.
Thanks and I agree! :)
Christian
