From: Christian Brauner <christian.brauner@ubuntu.com> To: linux-api@vger.kernel.org, linux-kernel@vger.kernel.org, Tejun Heo <tj@kernel.org> Cc: Oleg Nesterov <oleg@redhat.com>, Christian Brauner <christian.brauner@ubuntu.com>, Li Zefan <lizefan@huawei.com>, Johannes Weiner <hannes@cmpxchg.org>, cgroups@vger.kernel.org Subject: [PATCH v4 1/6] cgroup: unify attach permission checking Date: Fri, 17 Jan 2020 19:12:14 +0100 [thread overview] Message-ID: <20200117181219.14542-2-christian.brauner@ubuntu.com> (raw) In-Reply-To: <20200117181219.14542-1-christian.brauner@ubuntu.com> The core codepaths to check whether a process can be attached to a cgroup are the same for threads and thread-group leaders. Only a small piece of code verifying that source and destination cgroup are in the same domain differentiates the thread permission checking from thread-group leader permission checking. Since cgroup_migrate_vet_dst() only matters cgroup2 - it is a noop on cgroup1 - we can move it out of cgroup_attach_task(). All checks can now be consolidated into a new helper cgroup_attach_permissions() callable from both cgroup_procs_write() and cgroup_threads_write(). Cc: Tejun Heo <tj@kernel.org> Cc: Li Zefan <lizefan@huawei.com> Cc: Johannes Weiner <hannes@cmpxchg.org> Cc: cgroups@vger.kernel.org Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> --- /* v1 */ Link: https://lore.kernel.org/r/20191218173516.7875-2-christian.brauner@ubuntu.com /* v2 */ Link: https://lore.kernel.org/r/20191223061504.28716-2-christian.brauner@ubuntu.com - Christian Brauner <christian.brauner@ubuntu.com>: - Fix return value of cgroup_attach_permissions. It used to return 0 when it should've returned -EOPNOTSUPP. - Fix call to cgroup_attach_permissions() in cgroup_procs_write(). It accidently specified that a thread was moved causing an additional check for domain-group equality to be executed that is not needed. /* v3 */ Link: https://lore.kernel.org/r/20200117002143.15559-2-christian.brauner@ubuntu.com unchanged /* v4 */ unchanged --- kernel/cgroup/cgroup.c | 46 +++++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 14 deletions(-) diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c index 735af8f15f95..ad1f9fea5c14 100644 --- a/kernel/cgroup/cgroup.c +++ b/kernel/cgroup/cgroup.c @@ -2719,11 +2719,7 @@ int cgroup_attach_task(struct cgroup *dst_cgrp, struct task_struct *leader, { DEFINE_CGROUP_MGCTX(mgctx); struct task_struct *task; - int ret; - - ret = cgroup_migrate_vet_dst(dst_cgrp); - if (ret) - return ret; + int ret = 0; /* look up all src csets */ spin_lock_irq(&css_set_lock); @@ -4690,6 +4686,33 @@ static int cgroup_procs_write_permission(struct cgroup *src_cgrp, return 0; } +static inline bool cgroup_same_domain(const struct cgroup *src_cgrp, + const struct cgroup *dst_cgrp) +{ + return src_cgrp->dom_cgrp == dst_cgrp->dom_cgrp; +} + +static int cgroup_attach_permissions(struct cgroup *src_cgrp, + struct cgroup *dst_cgrp, + struct super_block *sb, bool thread) +{ + int ret = 0; + + ret = cgroup_procs_write_permission(src_cgrp, dst_cgrp, sb); + if (ret) + return ret; + + ret = cgroup_migrate_vet_dst(dst_cgrp); + if (ret) + return ret; + + if (thread && + !cgroup_same_domain(src_cgrp->dom_cgrp, dst_cgrp->dom_cgrp)) + ret = -EOPNOTSUPP; + + return ret; +} + static ssize_t cgroup_procs_write(struct kernfs_open_file *of, char *buf, size_t nbytes, loff_t off) { @@ -4712,8 +4735,8 @@ static ssize_t cgroup_procs_write(struct kernfs_open_file *of, src_cgrp = task_cgroup_from_root(task, &cgrp_dfl_root); spin_unlock_irq(&css_set_lock); - ret = cgroup_procs_write_permission(src_cgrp, dst_cgrp, - of->file->f_path.dentry->d_sb); + ret = cgroup_attach_permissions(src_cgrp, dst_cgrp, + of->file->f_path.dentry->d_sb, false); if (ret) goto out_finish; @@ -4757,16 +4780,11 @@ static ssize_t cgroup_threads_write(struct kernfs_open_file *of, spin_unlock_irq(&css_set_lock); /* thread migrations follow the cgroup.procs delegation rule */ - ret = cgroup_procs_write_permission(src_cgrp, dst_cgrp, - of->file->f_path.dentry->d_sb); + ret = cgroup_attach_permissions(src_cgrp, dst_cgrp, + of->file->f_path.dentry->d_sb, true); if (ret) goto out_finish; - /* and must be contained in the same domain */ - ret = -EOPNOTSUPP; - if (src_cgrp->dom_cgrp != dst_cgrp->dom_cgrp) - goto out_finish; - ret = cgroup_attach_task(dst_cgrp, task, false); out_finish: -- 2.25.0
WARNING: multiple messages have this Message-ID (diff)
From: Christian Brauner <christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org> To: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> Cc: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, Christian Brauner <christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>, Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>, Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>, cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Subject: [PATCH v4 1/6] cgroup: unify attach permission checking Date: Fri, 17 Jan 2020 19:12:14 +0100 [thread overview] Message-ID: <20200117181219.14542-2-christian.brauner@ubuntu.com> (raw) In-Reply-To: <20200117181219.14542-1-christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org> The core codepaths to check whether a process can be attached to a cgroup are the same for threads and thread-group leaders. Only a small piece of code verifying that source and destination cgroup are in the same domain differentiates the thread permission checking from thread-group leader permission checking. Since cgroup_migrate_vet_dst() only matters cgroup2 - it is a noop on cgroup1 - we can move it out of cgroup_attach_task(). All checks can now be consolidated into a new helper cgroup_attach_permissions() callable from both cgroup_procs_write() and cgroup_threads_write(). Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org> Cc: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org> Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Signed-off-by: Christian Brauner <christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org> --- /* v1 */ Link: https://lore.kernel.org/r/20191218173516.7875-2-christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org /* v2 */ Link: https://lore.kernel.org/r/20191223061504.28716-2-christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org - Christian Brauner <christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>: - Fix return value of cgroup_attach_permissions. It used to return 0 when it should've returned -EOPNOTSUPP. - Fix call to cgroup_attach_permissions() in cgroup_procs_write(). It accidently specified that a thread was moved causing an additional check for domain-group equality to be executed that is not needed. /* v3 */ Link: https://lore.kernel.org/r/20200117002143.15559-2-christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org unchanged /* v4 */ unchanged --- kernel/cgroup/cgroup.c | 46 +++++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 14 deletions(-) diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c index 735af8f15f95..ad1f9fea5c14 100644 --- a/kernel/cgroup/cgroup.c +++ b/kernel/cgroup/cgroup.c @@ -2719,11 +2719,7 @@ int cgroup_attach_task(struct cgroup *dst_cgrp, struct task_struct *leader, { DEFINE_CGROUP_MGCTX(mgctx); struct task_struct *task; - int ret; - - ret = cgroup_migrate_vet_dst(dst_cgrp); - if (ret) - return ret; + int ret = 0; /* look up all src csets */ spin_lock_irq(&css_set_lock); @@ -4690,6 +4686,33 @@ static int cgroup_procs_write_permission(struct cgroup *src_cgrp, return 0; } +static inline bool cgroup_same_domain(const struct cgroup *src_cgrp, + const struct cgroup *dst_cgrp) +{ + return src_cgrp->dom_cgrp == dst_cgrp->dom_cgrp; +} + +static int cgroup_attach_permissions(struct cgroup *src_cgrp, + struct cgroup *dst_cgrp, + struct super_block *sb, bool thread) +{ + int ret = 0; + + ret = cgroup_procs_write_permission(src_cgrp, dst_cgrp, sb); + if (ret) + return ret; + + ret = cgroup_migrate_vet_dst(dst_cgrp); + if (ret) + return ret; + + if (thread && + !cgroup_same_domain(src_cgrp->dom_cgrp, dst_cgrp->dom_cgrp)) + ret = -EOPNOTSUPP; + + return ret; +} + static ssize_t cgroup_procs_write(struct kernfs_open_file *of, char *buf, size_t nbytes, loff_t off) { @@ -4712,8 +4735,8 @@ static ssize_t cgroup_procs_write(struct kernfs_open_file *of, src_cgrp = task_cgroup_from_root(task, &cgrp_dfl_root); spin_unlock_irq(&css_set_lock); - ret = cgroup_procs_write_permission(src_cgrp, dst_cgrp, - of->file->f_path.dentry->d_sb); + ret = cgroup_attach_permissions(src_cgrp, dst_cgrp, + of->file->f_path.dentry->d_sb, false); if (ret) goto out_finish; @@ -4757,16 +4780,11 @@ static ssize_t cgroup_threads_write(struct kernfs_open_file *of, spin_unlock_irq(&css_set_lock); /* thread migrations follow the cgroup.procs delegation rule */ - ret = cgroup_procs_write_permission(src_cgrp, dst_cgrp, - of->file->f_path.dentry->d_sb); + ret = cgroup_attach_permissions(src_cgrp, dst_cgrp, + of->file->f_path.dentry->d_sb, true); if (ret) goto out_finish; - /* and must be contained in the same domain */ - ret = -EOPNOTSUPP; - if (src_cgrp->dom_cgrp != dst_cgrp->dom_cgrp) - goto out_finish; - ret = cgroup_attach_task(dst_cgrp, task, false); out_finish: -- 2.25.0
next prev parent reply other threads:[~2020-01-17 18:12 UTC|newest] Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-01-17 18:12 [PATCH v4 0/6] clone3 & cgroups: allow spawning processes into cgroups Christian Brauner 2020-01-17 18:12 ` Christian Brauner 2020-01-17 18:12 ` Christian Brauner [this message] 2020-01-17 18:12 ` [PATCH v4 1/6] cgroup: unify attach permission checking Christian Brauner 2020-01-20 14:42 ` Oleg Nesterov 2020-01-20 14:42 ` Oleg Nesterov 2020-01-20 14:46 ` Christian Brauner 2020-01-20 14:46 ` Christian Brauner 2020-01-17 18:12 ` [PATCH v4 2/6] cgroup: add cgroup_get_from_file() helper Christian Brauner 2020-01-17 18:12 ` Christian Brauner 2020-01-17 18:12 ` [PATCH v4 3/6] cgroup: refactor fork helpers Christian Brauner 2020-01-17 18:12 ` Christian Brauner 2020-01-20 14:00 ` Oleg Nesterov 2020-01-20 14:04 ` Christian Brauner 2020-01-20 14:22 ` Oleg Nesterov 2020-01-20 14:33 ` Christian Brauner 2020-01-20 14:33 ` Christian Brauner 2020-01-17 18:12 ` [PATCH v4 4/6] cgroup: add cgroup_may_write() helper Christian Brauner 2020-01-17 18:12 ` Christian Brauner 2020-01-17 18:12 ` [PATCH v4 5/6] clone3: allow spawning processes into cgroups Christian Brauner 2020-01-17 18:12 ` Christian Brauner 2020-01-17 18:12 ` Christian Brauner 2020-01-20 15:39 ` Oleg Nesterov 2020-01-20 15:39 ` Oleg Nesterov 2020-01-20 21:38 ` Christian Brauner 2020-01-17 18:12 ` [PATCH v4 6/6] selftests/cgroup: add tests for cloning " Christian Brauner 2020-01-17 20:24 ` Roman Gushchin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200117181219.14542-2-christian.brauner@ubuntu.com \ --to=christian.brauner@ubuntu.com \ --cc=cgroups@vger.kernel.org \ --cc=hannes@cmpxchg.org \ --cc=linux-api@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=lizefan@huawei.com \ --cc=oleg@redhat.com \ --cc=tj@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.