Re: [PATCH v2 1/2] cgroups: allow a cgroup subsystem to reject a fork
On Thu, Mar 12, 2015 at 10:47:11AM +1100, Aleksa Sarai wrote: > Hi Tejun, > > > You can charge the parent's at can_attach(), remember which one you > > charged, and at post_fork() if the parent's has changed inbetween, fix > > it up. [...] > > Did you mean can_fork() instead of can_attach() here? Ah, yeah, can_fork(). Sorry about that. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] cgroups: allow a cgroup subsystem to reject a fork
Hi Tejun, > You can charge the parent's at can_attach(), remember which one you > charged, and at post_fork() if the parent's has changed inbetween, fix > it up. [...] Did you mean can_fork() instead of can_attach() here? -- Aleksa Sarai (cyphar) www.cyphar.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] cgroups: allow a cgroup subsystem to reject a fork
On Wed, Mar 11, 2015 at 04:16:30PM +1100, Aleksa Sarai wrote: > We know that the task will have its css_set set to task_css_set(current), and > we could just use that in cgroup_can_fork(). The only question is, can > task_css_set(current) change between cgroup_can_fork() and cgroup_post_fork()? Yes, that's what I've been writing in the previous messages. It can change. > If it can change between the two calls, then we're in trouble -- there'd be no > reliable way of checking that the future css_set allows for the fork without > going through the registration of the css_set *proper* in cgroup_post_fork() > unless we hold css_set_rwsem for the entirety of the can_fork() to post_fork() > segment (which I can't imagine is a good idea). Please re-read my previous messages. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] cgroups: allow a cgroup subsystem to reject a fork
Hi Tejun, You can charge the parent's at can_attach(), remember which one you charged, and at post_fork() if the parent's has changed inbetween, fix it up. [...] Did you mean can_fork() instead of can_attach() here? -- Aleksa Sarai (cyphar) www.cyphar.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] cgroups: allow a cgroup subsystem to reject a fork
On Wed, Mar 11, 2015 at 04:16:30PM +1100, Aleksa Sarai wrote: We know that the task will have its css_set set to task_css_set(current), and we could just use that in cgroup_can_fork(). The only question is, can task_css_set(current) change between cgroup_can_fork() and cgroup_post_fork()? Yes, that's what I've been writing in the previous messages. It can change. If it can change between the two calls, then we're in trouble -- there'd be no reliable way of checking that the future css_set allows for the fork without going through the registration of the css_set *proper* in cgroup_post_fork() unless we hold css_set_rwsem for the entirety of the can_fork() to post_fork() segment (which I can't imagine is a good idea). Please re-read my previous messages. Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] cgroups: allow a cgroup subsystem to reject a fork
On Thu, Mar 12, 2015 at 10:47:11AM +1100, Aleksa Sarai wrote: Hi Tejun, You can charge the parent's at can_attach(), remember which one you charged, and at post_fork() if the parent's has changed inbetween, fix it up. [...] Did you mean can_fork() instead of can_attach() here? Ah, yeah, can_fork(). Sorry about that. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] cgroups: allow a cgroup subsystem to reject a fork
Hello Tejun, On Wed, Mar 11, 2015 at 2:17 AM, Tejun Heo wrote: > On Wed, Mar 11, 2015 at 01:51:06AM +1100, Aleksa Sarai wrote: >> Actually, I'm fairly sure we can do it all inside cgroup_post_fork() because >> inside cgroup_post_fork() we have access to both the old css_set and the new >> one. Then it's just a matter of reverting and re-applying the charge to the >> hierarchies. > > But the problem isn't whether we know both the old and new ones. The > problem is that we can only abort before the fork commit point and the > "old" one may change between the abort point and post-commit point so > we need to trycharge the old one at the possible abort point, remember > to which css it got charged and then check whether the association has > changed inbetween at the post commit point and readjust if so. Actually, it appears I was wrong. Until we hit cgroup_post_fork()'s setting up of the task's css_set, cgroup_can_fork() ends up charging init_css_set *every time*. Which means a check to see if it changed will always show that it had changed. The issue is that we need to access the css_set which is going to be saved as the task's css_set in order to decide if the task should fork. We know that the task will have its css_set set to task_css_set(current), and we could just use that in cgroup_can_fork(). The only question is, can task_css_set(current) change between cgroup_can_fork() and cgroup_post_fork()? If it can change between the two calls, then we're in trouble -- there'd be no reliable way of checking that the future css_set allows for the fork without going through the registration of the css_set *proper* in cgroup_post_fork() unless we hold css_set_rwsem for the entirety of the can_fork() to post_fork() segment (which I can't imagine is a good idea). -- Aleksa Sarai (cyphar) www.cyphar.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] cgroups: allow a cgroup subsystem to reject a fork
Hello, On Wed, Mar 11, 2015 at 01:51:06AM +1100, Aleksa Sarai wrote: > Actually, I'm fairly sure we can do it all inside cgroup_post_fork() because > inside cgroup_post_fork() we have access to both the old css_set and the new > one. Then it's just a matter of reverting and re-applying the charge to the > hierarchies. But the problem isn't whether we know both the old and new ones. The problem is that we can only abort before the fork commit point and the "old" one may change between the abort point and post-commit point so we need to trycharge the old one at the possible abort point, remember to which css it got charged and then check whether the association has changed inbetween at the post commit point and readjust if so. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] cgroups: allow a cgroup subsystem to reject a fork
Hello Tejun, On Tue, Mar 10, 2015 at 11:47 PM, Tejun Heo wrote: >> of doing a charge that stops if you hit a certain `css` (unless we start >> passing `css_set`s to the fork/exit callbacks -- and then we can uncharge the >> old css_set and charge the new one). > > We'll have to pass the pointer from can_fork side to post_fork. I'm > not sure how that should be done either. Actually, I'm fairly sure we can do it all inside cgroup_post_fork() because inside cgroup_post_fork() we have access to both the old css_set and the new one. Then it's just a matter of reverting and re-applying the charge to the hierarchies. I'll send a patch in a few days, I need to make sure that this method *actually* works. :P -- Aleksa Sarai (cyphar) www.cyphar.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] cgroups: allow a cgroup subsystem to reject a fork
Hello, Aleksa. On Tue, Mar 10, 2015 at 07:19:06PM +1100, Aleksa Sarai wrote: > I'm not sure how to check for equality between two `css_set`s (or just two > `css`s). Is there a function to do so? Also, I'm not sure if there's a nice > way You can compare the css pointers for equality. > of doing a charge that stops if you hit a certain `css` (unless we start > passing `css_set`s to the fork/exit callbacks -- and then we can uncharge the > old css_set and charge the new one). We'll have to pass the pointer from can_fork side to post_fork. I'm not sure how that should be done either. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] cgroups: allow a cgroup subsystem to reject a fork
Hi Tejun, >> The reason is that when cgroup_can_fork() is called, the css_set doesn't >> contain the pids cgroup it's forking to. You can verify this by moving that >> segment of code back to it's original position and >> compiling/rebooting/testing >> the pids cgroup. You will get a WARN each time you try to attach to a new >> pids >> cgroup, because when your shell forks the counter for the original pids >> cgroup >> hierarchy gets incremented but when the command exits the counter for the >> *new* >> pids cgroup hierarchy gets decremented. Essentially it's because we need to >> reference the css_set of the newly forked process (as it exists when >> cgroup_fork() runs) when incrementing the hierarchy -- otherwise we will >> increment/decrement a different hierarchy between fork() and exit(). >> >> If there's a correct way of doing this, I'm all ears. I had a bad feeling >> that >> moving that section would break the whole visibility issue in the same >> fashion >> as I did in v1 of this patchset. The thing is, I'm not sure there's a way to >> access the new css_set of the task as though it is attached without making it >> visible prematurely (because we need the css_set to refer to the right >> hierarchy in order to conditionally decide the fork). > > You can charge the parent's at can_attach(), remember which one you > charged, and at post_fork() if the parent's has changed inbetween, fix > it up. Again, you may end up breaking the hard limit of the new pids > cgroup at this point but that's fine. The cgroup membership changing > inbetween pretty much implies that organization operation happened > inbetween. I'm not sure how to check for equality between two `css_set`s (or just two `css`s). Is there a function to do so? Also, I'm not sure if there's a nice way of doing a charge that stops if you hit a certain `css` (unless we start passing `css_set`s to the fork/exit callbacks -- and then we can uncharge the old css_set and charge the new one). -- Aleksa Sarai (cyphar) www.cyphar.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] cgroups: allow a cgroup subsystem to reject a fork
Hello, Aleksa. On Tue, Mar 10, 2015 at 07:19:06PM +1100, Aleksa Sarai wrote: I'm not sure how to check for equality between two `css_set`s (or just two `css`s). Is there a function to do so? Also, I'm not sure if there's a nice way You can compare the css pointers for equality. of doing a charge that stops if you hit a certain `css` (unless we start passing `css_set`s to the fork/exit callbacks -- and then we can uncharge the old css_set and charge the new one). We'll have to pass the pointer from can_fork side to post_fork. I'm not sure how that should be done either. Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] cgroups: allow a cgroup subsystem to reject a fork
Hello Tejun, On Wed, Mar 11, 2015 at 2:17 AM, Tejun Heo t...@kernel.org wrote: On Wed, Mar 11, 2015 at 01:51:06AM +1100, Aleksa Sarai wrote: Actually, I'm fairly sure we can do it all inside cgroup_post_fork() because inside cgroup_post_fork() we have access to both the old css_set and the new one. Then it's just a matter of reverting and re-applying the charge to the hierarchies. But the problem isn't whether we know both the old and new ones. The problem is that we can only abort before the fork commit point and the old one may change between the abort point and post-commit point so we need to trycharge the old one at the possible abort point, remember to which css it got charged and then check whether the association has changed inbetween at the post commit point and readjust if so. Actually, it appears I was wrong. Until we hit cgroup_post_fork()'s setting up of the task's css_set, cgroup_can_fork() ends up charging init_css_set *every time*. Which means a check to see if it changed will always show that it had changed. The issue is that we need to access the css_set which is going to be saved as the task's css_set in order to decide if the task should fork. We know that the task will have its css_set set to task_css_set(current), and we could just use that in cgroup_can_fork(). The only question is, can task_css_set(current) change between cgroup_can_fork() and cgroup_post_fork()? If it can change between the two calls, then we're in trouble -- there'd be no reliable way of checking that the future css_set allows for the fork without going through the registration of the css_set *proper* in cgroup_post_fork() unless we hold css_set_rwsem for the entirety of the can_fork() to post_fork() segment (which I can't imagine is a good idea). -- Aleksa Sarai (cyphar) www.cyphar.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] cgroups: allow a cgroup subsystem to reject a fork
Hello, On Wed, Mar 11, 2015 at 01:51:06AM +1100, Aleksa Sarai wrote: Actually, I'm fairly sure we can do it all inside cgroup_post_fork() because inside cgroup_post_fork() we have access to both the old css_set and the new one. Then it's just a matter of reverting and re-applying the charge to the hierarchies. But the problem isn't whether we know both the old and new ones. The problem is that we can only abort before the fork commit point and the old one may change between the abort point and post-commit point so we need to trycharge the old one at the possible abort point, remember to which css it got charged and then check whether the association has changed inbetween at the post commit point and readjust if so. Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] cgroups: allow a cgroup subsystem to reject a fork
Hello Tejun, On Tue, Mar 10, 2015 at 11:47 PM, Tejun Heo t...@kernel.org wrote: of doing a charge that stops if you hit a certain `css` (unless we start passing `css_set`s to the fork/exit callbacks -- and then we can uncharge the old css_set and charge the new one). We'll have to pass the pointer from can_fork side to post_fork. I'm not sure how that should be done either. Actually, I'm fairly sure we can do it all inside cgroup_post_fork() because inside cgroup_post_fork() we have access to both the old css_set and the new one. Then it's just a matter of reverting and re-applying the charge to the hierarchies. I'll send a patch in a few days, I need to make sure that this method *actually* works. :P -- Aleksa Sarai (cyphar) www.cyphar.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] cgroups: allow a cgroup subsystem to reject a fork
Hi Tejun, The reason is that when cgroup_can_fork() is called, the css_set doesn't contain the pids cgroup it's forking to. You can verify this by moving that segment of code back to it's original position and compiling/rebooting/testing the pids cgroup. You will get a WARN each time you try to attach to a new pids cgroup, because when your shell forks the counter for the original pids cgroup hierarchy gets incremented but when the command exits the counter for the *new* pids cgroup hierarchy gets decremented. Essentially it's because we need to reference the css_set of the newly forked process (as it exists when cgroup_fork() runs) when incrementing the hierarchy -- otherwise we will increment/decrement a different hierarchy between fork() and exit(). If there's a correct way of doing this, I'm all ears. I had a bad feeling that moving that section would break the whole visibility issue in the same fashion as I did in v1 of this patchset. The thing is, I'm not sure there's a way to access the new css_set of the task as though it is attached without making it visible prematurely (because we need the css_set to refer to the right hierarchy in order to conditionally decide the fork). You can charge the parent's at can_attach(), remember which one you charged, and at post_fork() if the parent's has changed inbetween, fix it up. Again, you may end up breaking the hard limit of the new pids cgroup at this point but that's fine. The cgroup membership changing inbetween pretty much implies that organization operation happened inbetween. I'm not sure how to check for equality between two `css_set`s (or just two `css`s). Is there a function to do so? Also, I'm not sure if there's a nice way of doing a charge that stops if you hit a certain `css` (unless we start passing `css_set`s to the fork/exit callbacks -- and then we can uncharge the old css_set and charge the new one). -- Aleksa Sarai (cyphar) www.cyphar.com -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] cgroups: allow a cgroup subsystem to reject a fork
On Fri, Feb 27, 2015 at 03:17:18PM +1100, Aleksa Sarai wrote:
...
> In order for can_fork to deal with a task that has an accurate css_set,
> move the css_set updating to cgroup_fork (where it belongs).
Hmmm? So, now the task is visible on cgroup side before the point of
no return? What happens if fork fails afterwards? Also, why is this
non-trivial change happening in tandem in this patch?
> @@ -946,6 +950,11 @@ struct cgroup_subsys_state
> *css_tryget_online_from_dir(struct dentry *dentry,
> static inline int cgroup_init_early(void) { return 0; }
> static inline int cgroup_init(void) { return 0; }
> static inline void cgroup_fork(struct task_struct *p) {}
> +static inline int cgroup_can_fork(struct task_struct *p)
> +{
> + return 0;
> +}
Please follow the surrounding style.
> @@ -4928,7 +4928,7 @@ static void __init cgroup_init_subsys(struct
> cgroup_subsys *ss, bool early)
>* init_css_set is in the subsystem's root cgroup. */
> init_css_set.subsys[ss->id] = css;
>
> - need_forkexit_callback |= ss->fork || ss->exit;
> + need_forkexit_callback |= ss->can_fork || ss->cancel_fork || ss->fork
> || ss->exit;
Your patch isn't the culprit but this is silly given that this flag is
set pretty much whenever cgroups are enabled. Per-callback subsys
mask would make far more sense.
Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] cgroups: allow a cgroup subsystem to reject a fork
On Fri, Feb 27, 2015 at 03:17:18PM +1100, Aleksa Sarai wrote:
...
In order for can_fork to deal with a task that has an accurate css_set,
move the css_set updating to cgroup_fork (where it belongs).
Hmmm? So, now the task is visible on cgroup side before the point of
no return? What happens if fork fails afterwards? Also, why is this
non-trivial change happening in tandem in this patch?
@@ -946,6 +950,11 @@ struct cgroup_subsys_state
*css_tryget_online_from_dir(struct dentry *dentry,
static inline int cgroup_init_early(void) { return 0; }
static inline int cgroup_init(void) { return 0; }
static inline void cgroup_fork(struct task_struct *p) {}
+static inline int cgroup_can_fork(struct task_struct *p)
+{
+ return 0;
+}
Please follow the surrounding style.
@@ -4928,7 +4928,7 @@ static void __init cgroup_init_subsys(struct
cgroup_subsys *ss, bool early)
* init_css_set is in the subsystem's root cgroup. */
init_css_set.subsys[ss-id] = css;
- need_forkexit_callback |= ss-fork || ss-exit;
+ need_forkexit_callback |= ss-can_fork || ss-cancel_fork || ss-fork
|| ss-exit;
Your patch isn't the culprit but this is silly given that this flag is
set pretty much whenever cgroups are enabled. Per-callback subsys
mask would make far more sense.
Thanks.
--
tejun
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
