| Message ID | 20130122100938.GA26820@candlejack.bmw-carit.intra |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
[...] >> >> OK, I guess we should do something similar in the netprio, netcls >> cgroups and >> yes document it as you noted in your last comment. > > Here is my attempt to add such a check. I really don't know if this is the > correct way to do so. To test this I have written a test program, which > seems to test the right thing. Please have a look and let me know if > it is correct: http://www.monom.org/misc/scm_rights.c > > And here a dirty first version of the patch: > > > From 49a78d907eaf31c16673025e7e3b4844e419e416 Mon Sep 17 00:00:00 2001 > From: Daniel Wagner <daniel.wagner@bmw-carit.de> > Date: Tue, 22 Jan 2013 11:08:22 +0100 > Subject: [PATCH] net: net_prio: Block attach if a socket is shared > > --- > net/core/netprio_cgroup.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c > index 847c02b..de4e6c5 100644 > --- a/net/core/netprio_cgroup.c > +++ b/net/core/netprio_cgroup.c > @@ -274,9 +274,39 @@ static struct cftype ss_files[] = { > { } /* terminate */ > }; > > +static int check_cnt(const void *v, struct file *file, unsigned n) > +{ > + unsigned *flag = (unsigned *)v; > + int err; > + > + struct socket *sock = sock_from_file(file, &err); > + if (sock && file_count(file) > 1) > + *flag = 1; > + I think this check will catch a lot of cases that are not necessarily sharing a socket across tasks though. For example iscsid passes a file descriptor to the kernel which does a sockfd_lookup() incrementing f_count. Similarly look at dup/clone/etc. In many of these cases I believe it should be OK to move the task around when the sockets are not shared between multiple tasks. .John
On 23.01.2013 01:02, John Fastabend wrote: > [...] > >>> >>> OK, I guess we should do something similar in the netprio, netcls >>> cgroups and >>> yes document it as you noted in your last comment. >> >> Here is my attempt to add such a check. I really don't know if this is >> the >> correct way to do so. To test this I have written a test program, which >> seems to test the right thing. Please have a look and let me know if >> it is correct: http://www.monom.org/misc/scm_rights.c >> >> And here a dirty first version of the patch: >> >> >> From 49a78d907eaf31c16673025e7e3b4844e419e416 Mon Sep 17 00:00:00 2001 >> From: Daniel Wagner <daniel.wagner@bmw-carit.de> >> Date: Tue, 22 Jan 2013 11:08:22 +0100 >> Subject: [PATCH] net: net_prio: Block attach if a socket is shared >> >> --- >> net/core/netprio_cgroup.c | 30 ++++++++++++++++++++++++++++++ >> 1 file changed, 30 insertions(+) >> >> diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c >> index 847c02b..de4e6c5 100644 >> --- a/net/core/netprio_cgroup.c >> +++ b/net/core/netprio_cgroup.c >> @@ -274,9 +274,39 @@ static struct cftype ss_files[] = { >> { } /* terminate */ >> }; >> >> +static int check_cnt(const void *v, struct file *file, unsigned n) >> +{ >> + unsigned *flag = (unsigned *)v; >> + int err; >> + >> + struct socket *sock = sock_from_file(file, &err); >> + if (sock && file_count(file) > 1) >> + *flag = 1; >> + > > I think this check will catch a lot of cases that are not necessarily > sharing a socket across tasks though. For example iscsid passes a file > descriptor to the kernel which does a sockfd_lookup() incrementing > f_count. Similarly look at dup/clone/etc. Yep, I expected that this patch was too simple. > In many of these cases I believe it should be OK to move the task > around when the sockets are not shared between multiple tasks. Do you know of a different way to identify the shared sockets? thanks, daniel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>> BTW, isn't this a similar to what should happen with the block io cgroup? >>>> What is the behavior with a fd writing to a file in the scenario you >>>> describe above? >>>> >>> >>> It forbids task moving in this case: >>> >>> /* >>> * We cannot support shared io contexts, as we have no mean to support >>> * two tasks with the same ioc in two different groups without major rework >>> * of the main cic data structures. For now we allow a task to change >>> * its cgroup only if it's the only owner of its ioc. >>> */ >>> static int blkcg_can_attach(struct cgroup *cgrp, struct cgroup_taskset *tset) >>> >> >> OK, I guess we should do something similar in the netprio, netcls >> cgroups and >> yes document it as you noted in your last comment. > > Here is my attempt to add such a check. I really don't know if this is the > correct way to do so. To test this I have written a test program, which > seems to test the right thing. Please have a look and let me know if > it is correct: http://www.monom.org/misc/scm_rights.c > > And here a dirty first version of the patch: > Adding new can_attach() is discouraged. It's considered bad, because if cgroup is mounted with multi subsystems binded, and if one of them can forbid task moving for some reason, this will add burden on users to use cgroup properly. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c index 847c02b..de4e6c5 100644 --- a/net/core/netprio_cgroup.c +++ b/net/core/netprio_cgroup.c @@ -274,9 +274,39 @@ static struct cftype ss_files[] = { { } /* terminate */ }; +static int check_cnt(const void *v, struct file *file, unsigned n) +{ + unsigned *flag = (unsigned *)v; + int err; + + struct socket *sock = sock_from_file(file, &err); + if (sock && file_count(file) > 1) + *flag = 1; + + return 0; +} + +static int cgrp_can_attach(struct cgroup *cgrp, struct cgroup_taskset *tset) +{ + struct task_struct *p; + unsigned flag = 0; + + cgroup_taskset_for_each(p, cgrp, tset) { + task_lock(p); + iterate_fd(p->files, 0, check_cnt, &flag); + task_unlock(p); + } + + if (flag != 0) + return -EINVAL; + + return 0; +} + struct cgroup_subsys net_prio_subsys = { .name = "net_prio", .create = cgrp_create, + .can_attach = cgrp_can_attach, .destroy = cgrp_destroy, .attach = net_prio_attach, .subsys_id = net_prio_subsys_id,