From: Aleksa Sarai <cyphar@cyphar.com> To: Tejun Heo <tj@kernel.org> Cc: lizefan@huawei.com, mingo@redhat.com, peterz@infradead.org, richard@nod.at, "Frédéric Weisbecker" <fweisbec@gmail.com>, linux-kernel@vger.kernel.org, cgroups@vger.kernel.org Subject: Re: [PATCH v10 4/4] cgroups: implement the PIDs subsystem Date: Thu, 23 Apr 2015 10:43:12 +1000 [thread overview] Message-ID: <CAOviyagHJhxD8E+CeEdy399ARPaNyyiSJSJjByK=5ALN5jxbJw@mail.gmail.com> (raw) In-Reply-To: <20150422162954.GF10738@htj.duckdns.org> Hi Tejun, >> + rcu_read_lock(); >> + css = task_css(current, pids_cgrp_id); >> + if (!css_tryget_online(css)) { >> + retval = -EBUSY; >> + goto err_rcu_unlock; >> + } >> + rcu_read_unlock(); > > Hmmm... so, the above is guaranteed to succeed in finite amount of > time (the race window is actually very narrow) and it'd be silly to > fail fork because a task was being moved across cgroups. > > I think it'd be a good idea to implement task_get_css() which loops > and returns the current css for the requested subsystem with reference > count bumped and it can use css_tryget() too. Holding a ref doesn't > prevent css from dying anyway, so it doesn't make any difference. Hmmm, okay. I'll work on this later. >> + rcu_read_lock(); >> + css = task_css(task, pids_cgrp_id); >> + css_get(css); > > Why is this safe? What guarantees that css's ref isn't already zero > at this point? Because it's already been exposed by pids_fork, so the current css_set (which contains the current css)'s ref has been bumped. There isn't a guarantee that there is a ref to css, but there is a guarantee the css_set it is in has a ref. The problem with using tryget is that we can't fail here. >> +static ssize_t pids_max_write(struct kernfs_open_file *of, char *buf, >> + size_t nbytes, loff_t off) >> +{ >> + struct cgroup_subsys_state *css = of_css(of); >> + struct pids_cgroup *pids = css_pids(css); >> + int64_t limit; >> + int err; >> + >> + buf = strstrip(buf); >> + if (!strcmp(buf, PIDS_MAX_STR)) { >> + limit = PIDS_MAX; >> + goto set_limit; >> + } >> + >> + err = kstrtoll(buf, 0, &limit); >> + if (err) >> + return err; >> + >> + /* We use INT_MAX as the maximum value of pid_t. */ >> + if (limit < 0 || limit > INT_MAX) > > This is kinda weird if we're using PIDS_MAX for max as it may end up > showing "max" after some larger number is written to the file. The reason for this is because I believe you said "PIDS_MAX isn't meant to be exposed to userspace" (one of the previous patchsets used PIDS_MAX as the maximum valid value). -- Aleksa Sarai (cyphar) www.cyphar.com
WARNING: multiple messages have this Message-ID (diff)
From: Aleksa Sarai <cyphar-gVpy/LI/lHzQT0dZR+AlfA@public.gmane.org> To: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> Cc: lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org, mingo-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org, richard-/L3Ra7n9ekc@public.gmane.org, "Frédéric Weisbecker" <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Subject: Re: [PATCH v10 4/4] cgroups: implement the PIDs subsystem Date: Thu, 23 Apr 2015 10:43:12 +1000 [thread overview] Message-ID: <CAOviyagHJhxD8E+CeEdy399ARPaNyyiSJSJjByK=5ALN5jxbJw@mail.gmail.com> (raw) In-Reply-To: <20150422162954.GF10738-piEFEHQLUPpN0TnZuCh8vA@public.gmane.org> Hi Tejun, >> + rcu_read_lock(); >> + css = task_css(current, pids_cgrp_id); >> + if (!css_tryget_online(css)) { >> + retval = -EBUSY; >> + goto err_rcu_unlock; >> + } >> + rcu_read_unlock(); > > Hmmm... so, the above is guaranteed to succeed in finite amount of > time (the race window is actually very narrow) and it'd be silly to > fail fork because a task was being moved across cgroups. > > I think it'd be a good idea to implement task_get_css() which loops > and returns the current css for the requested subsystem with reference > count bumped and it can use css_tryget() too. Holding a ref doesn't > prevent css from dying anyway, so it doesn't make any difference. Hmmm, okay. I'll work on this later. >> + rcu_read_lock(); >> + css = task_css(task, pids_cgrp_id); >> + css_get(css); > > Why is this safe? What guarantees that css's ref isn't already zero > at this point? Because it's already been exposed by pids_fork, so the current css_set (which contains the current css)'s ref has been bumped. There isn't a guarantee that there is a ref to css, but there is a guarantee the css_set it is in has a ref. The problem with using tryget is that we can't fail here. >> +static ssize_t pids_max_write(struct kernfs_open_file *of, char *buf, >> + size_t nbytes, loff_t off) >> +{ >> + struct cgroup_subsys_state *css = of_css(of); >> + struct pids_cgroup *pids = css_pids(css); >> + int64_t limit; >> + int err; >> + >> + buf = strstrip(buf); >> + if (!strcmp(buf, PIDS_MAX_STR)) { >> + limit = PIDS_MAX; >> + goto set_limit; >> + } >> + >> + err = kstrtoll(buf, 0, &limit); >> + if (err) >> + return err; >> + >> + /* We use INT_MAX as the maximum value of pid_t. */ >> + if (limit < 0 || limit > INT_MAX) > > This is kinda weird if we're using PIDS_MAX for max as it may end up > showing "max" after some larger number is written to the file. The reason for this is because I believe you said "PIDS_MAX isn't meant to be exposed to userspace" (one of the previous patchsets used PIDS_MAX as the maximum valid value). -- Aleksa Sarai (cyphar) www.cyphar.com
next prev parent reply other threads:[~2015-04-23 0:43 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-04-19 12:22 [PATCH v10 0/4] cgroups: add pids subsystem Aleksa Sarai 2015-04-19 12:22 ` Aleksa Sarai 2015-04-19 12:22 ` [PATCH v10 1/4] cgroups: use bitmask to filter for_each_subsys Aleksa Sarai 2015-04-19 12:22 ` Aleksa Sarai 2015-04-22 15:25 ` Tejun Heo 2015-04-22 15:42 ` Peter Zijlstra 2015-04-22 16:02 ` Tejun Heo 2015-04-26 16:05 ` Aleksa Sarai 2015-04-26 16:09 ` Tejun Heo 2015-04-26 16:09 ` Tejun Heo 2015-05-13 5:44 ` Aleksa Sarai 2015-05-13 5:44 ` Aleksa Sarai 2015-05-13 13:50 ` Tejun Heo 2015-05-13 13:50 ` Tejun Heo 2015-04-22 15:30 ` Tejun Heo 2015-04-19 12:22 ` [PATCH v10 2/4] cgroups: replace explicit ss_mask checking with for_each_subsys_which Aleksa Sarai 2015-04-22 15:31 ` Tejun Heo 2015-04-22 15:31 ` Tejun Heo 2015-04-19 12:22 ` [PATCH v10 3/4] cgroups: allow a cgroup subsystem to reject a fork Aleksa Sarai 2015-04-19 12:22 ` Aleksa Sarai 2015-04-22 15:54 ` Tejun Heo 2015-04-22 15:54 ` Tejun Heo 2015-04-24 13:59 ` Aleksa Sarai 2015-04-24 15:48 ` Tejun Heo 2015-05-14 10:57 ` Aleksa Sarai 2015-05-14 15:08 ` Tejun Heo 2015-04-19 12:22 ` [PATCH v10 4/4] cgroups: implement the PIDs subsystem Aleksa Sarai 2015-04-19 12:22 ` Aleksa Sarai 2015-04-22 16:29 ` Tejun Heo 2015-04-22 16:29 ` Tejun Heo 2015-04-23 0:43 ` Aleksa Sarai [this message] 2015-04-23 0:43 ` Aleksa Sarai 2015-04-24 15:36 ` Tejun Heo 2015-04-24 15:36 ` Tejun Heo 2015-05-13 17:04 ` Aleksa Sarai 2015-05-13 17:04 ` Aleksa Sarai 2015-05-13 17:29 ` Tejun Heo 2015-05-13 17:29 ` Tejun Heo 2015-05-13 17:44 ` Aleksa Sarai 2015-05-13 17:47 ` Tejun Heo 2015-05-13 17:47 ` Tejun Heo 2015-05-16 3:59 ` Aleksa Sarai 2015-05-16 3:59 ` Aleksa Sarai 2015-05-18 1:24 ` Tejun Heo 2015-05-18 1:24 ` Tejun Heo 2015-04-24 14:24 ` Aleksa Sarai 2015-04-24 14:24 ` Aleksa Sarai 2015-04-24 14:07 Aleksa Sarai 2015-04-24 14:07 ` Aleksa Sarai 2015-04-24 15:26 ` Tejun Heo
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='CAOviyagHJhxD8E+CeEdy399ARPaNyyiSJSJjByK=5ALN5jxbJw@mail.gmail.com' \ --to=cyphar@cyphar.com \ --cc=cgroups@vger.kernel.org \ --cc=fweisbec@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=lizefan@huawei.com \ --cc=mingo@redhat.com \ --cc=peterz@infradead.org \ --cc=richard@nod.at \ --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.