In preparation for better hierarchy support, it's needed to retain the local
settings in order to try to reapply them after a propagated change if they're
still valid.
Cc: Tejun Heo <[email protected]>
Cc: Serge Hallyn <[email protected]>
Signed-off-by: Aristeu Rozanski <[email protected]>
---
security/device_cgroup.c | 108 ++++++++++++++++++++++++++++++++++-------------
1 file changed, 80 insertions(+), 28 deletions(-)
Index: github/security/device_cgroup.c
===================================================================
--- github.orig/security/device_cgroup.c 2012-11-26 17:27:52.710834676 -0500
+++ github/security/device_cgroup.c 2012-11-27 11:58:09.333349468 -0500
@@ -39,13 +39,27 @@
struct rcu_head rcu;
};
+enum devcg_behavior {
+ DEVCG_DEFAULT_NONE,
+ DEVCG_DEFAULT_ALLOW,
+ DEVCG_DEFAULT_DENY,
+};
+
struct dev_cgroup {
struct cgroup_subsys_state css;
+
+ /* result of merging the parent's rules with local ones */
struct list_head exceptions;
- enum {
- DEVCG_DEFAULT_ALLOW,
- DEVCG_DEFAULT_DENY,
- } behavior;
+ enum devcg_behavior behavior;
+
+ /*
+ * local set rules, saved so when a parent propagates new rules, the
+ * local preferences can be preserved
+ */
+ struct {
+ struct list_head exceptions;
+ enum devcg_behavior behavior;
+ } local;
};
static inline struct dev_cgroup *css_to_devcgroup(struct cgroup_subsys_state *s)
@@ -104,8 +118,41 @@
/*
* called under devcgroup_mutex
*/
-static int dev_exception_add(struct list_head *exceptions,
- struct dev_exception_item *ex)
+static void __dev_exception_rm(struct list_head *exceptions,
+ struct dev_exception_item *ex)
+{
+ struct dev_exception_item *walk, *tmp;
+
+ lockdep_assert_held(&devcgroup_mutex);
+
+ list_for_each_entry_safe(walk, tmp, exceptions, list) {
+ if (walk->type != ex->type)
+ continue;
+ if (walk->major != ex->major)
+ continue;
+ if (walk->minor != ex->minor)
+ continue;
+
+ walk->access &= ~ex->access;
+ if (!walk->access) {
+ list_del_rcu(&walk->list);
+ kfree_rcu(walk, rcu);
+ }
+ }
+}
+
+static void dev_exception_rm(struct dev_cgroup *devcgroup,
+ struct dev_exception_item *ex)
+{
+ __dev_exception_rm(&devcgroup->local.exceptions, ex);
+ __dev_exception_rm(&devcgroup->exceptions, ex);
+}
+
+/*
+ * called under devcgroup_mutex
+ */
+static int __dev_exception_add(struct list_head *exceptions,
+ struct dev_exception_item *ex)
{
struct dev_exception_item *excopy, *walk;
@@ -133,30 +180,26 @@
return 0;
}
-/*
- * called under devcgroup_mutex
- */
-static void dev_exception_rm(struct list_head *exceptions,
+static int dev_exception_add(struct dev_cgroup *devcgroup,
struct dev_exception_item *ex)
{
- struct dev_exception_item *walk, *tmp;
+ int rc;
lockdep_assert_held(&devcgroup_mutex);
- list_for_each_entry_safe(walk, tmp, exceptions, list) {
- if (walk->type != ex->type)
- continue;
- if (walk->major != ex->major)
- continue;
- if (walk->minor != ex->minor)
- continue;
+ /*
+ * we add to the local list so we can preserve local preferences if
+ * the parent propagates down new rules
+ */
+ rc = __dev_exception_add(&devcgroup->local.exceptions, ex);
+ if (rc)
+ return rc;
+
+ rc = __dev_exception_add(&devcgroup->exceptions, ex);
+ if (rc)
+ __dev_exception_rm(&devcgroup->local.exceptions, ex);
- walk->access &= ~ex->access;
- if (!walk->access) {
- list_del_rcu(&walk->list);
- kfree_rcu(walk, rcu);
- }
- }
+ return rc;
}
/**
@@ -175,6 +218,11 @@
list_del_rcu(&ex->list);
kfree_rcu(ex, rcu);
}
+ list_for_each_entry_safe(ex, tmp, &dev_cgroup->local.exceptions,
+ list) {
+ list_del_rcu(&ex->list);
+ kfree_rcu(ex, rcu);
+ }
}
/*
@@ -190,6 +238,8 @@
if (!dev_cgroup)
return ERR_PTR(-ENOMEM);
INIT_LIST_HEAD(&dev_cgroup->exceptions);
+ INIT_LIST_HEAD(&dev_cgroup->local.exceptions);
+ dev_cgroup->local.behavior = DEVCG_DEFAULT_NONE;
parent_cgroup = cgroup->parent;
if (parent_cgroup == NULL)
@@ -413,6 +463,7 @@
return -EPERM;
dev_exception_clean(devcgroup);
devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
+ devcgroup->local.behavior = DEVCG_DEFAULT_ALLOW;
if (!parent)
break;
@@ -424,6 +475,7 @@
case DEVCG_DENY:
dev_exception_clean(devcgroup);
devcgroup->behavior = DEVCG_DEFAULT_DENY;
+ devcgroup->local.behavior = DEVCG_DEFAULT_DENY;
break;
default:
return -EINVAL;
@@ -513,10 +565,10 @@
* don't want to break compatibility
*/
if (devcgroup->behavior == DEVCG_DEFAULT_ALLOW) {
- dev_exception_rm(&devcgroup->exceptions, &ex);
+ dev_exception_rm(devcgroup, &ex);
return 0;
}
- return dev_exception_add(&devcgroup->exceptions, &ex);
+ return dev_exception_add(devcgroup, &ex);
case DEVCG_DENY:
/*
* If the default policy is to deny by default, try to remove
@@ -524,10 +576,10 @@
* don't want to break compatibility
*/
if (devcgroup->behavior == DEVCG_DEFAULT_DENY) {
- dev_exception_rm(&devcgroup->exceptions, &ex);
+ dev_exception_rm(devcgroup, &ex);
return 0;
}
- return dev_exception_add(&devcgroup->exceptions, &ex);
+ return dev_exception_add(devcgroup, &ex);
default:
return -EINVAL;
}
Quoting Aristeu Rozanski ([email protected]):
> In preparation for better hierarchy support, it's needed to retain the local
> settings in order to try to reapply them after a propagated change if they're
> still valid.
>
> Cc: Tejun Heo <[email protected]>
> Cc: Serge Hallyn <[email protected]>
> Signed-off-by: Aristeu Rozanski <[email protected]>
>
> ---
> security/device_cgroup.c | 108 ++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 80 insertions(+), 28 deletions(-)
Hi,
thanks for doing this. I've got one concern though. I don't see
any place where devcgroup_create() was updated to create the
local exceptions list. I think we need a guarantee that at
any time the local exceptions list will contain all the entries
contained in the cgroup->exceptions list. Otherwise you cannot
use the exception_add() the way you do (or you can't use RCU),
since there could be a window between the successful addition
of a rule to cgroup->local.exceptions and the failed addition to
cgroup->exceptions, during which a task (since it won't need
the mutex for devcg_allow()) could exceed allowed permissions.
It's possible I'm misunderstanding. If you think that's the case
just kick me and I'll take a fresh look.
(Btw is there a git tree or gitweb view I could look at alongside
the patchset?)
thanks,
-serge
On Thu, Nov 29, 2012 at 07:29:45PM +0000, Serge E. Hallyn wrote:
> Quoting Aristeu Rozanski ([email protected]):
> > In preparation for better hierarchy support, it's needed to retain the local
> > settings in order to try to reapply them after a propagated change if they're
> > still valid.
> >
> > Cc: Tejun Heo <[email protected]>
> > Cc: Serge Hallyn <[email protected]>
> > Signed-off-by: Aristeu Rozanski <[email protected]>
> >
> > ---
> > security/device_cgroup.c | 108 ++++++++++++++++++++++++++++++++++-------------
> > 1 file changed, 80 insertions(+), 28 deletions(-)
>
> Hi,
>
> thanks for doing this. I've got one concern though. I don't see
> any place where devcgroup_create() was updated to create the
> local exceptions list. I think we need a guarantee that at
the local exceptions list is part of the dev_cgroup structure and it's
initialized on the patch 'device_cgroup: keep track of local group
setting':
@@ -190,6 +238,8 @@
if (!dev_cgroup)
return ERR_PTR(-ENOMEM);
INIT_LIST_HEAD(&dev_cgroup->exceptions);
+ INIT_LIST_HEAD(&dev_cgroup->local.exceptions);
+ dev_cgroup->local.behavior = DEVCG_DEFAULT_NONE;
parent_cgroup = cgroup->parent;
if (parent_cgroup == NULL)
> any time the local exceptions list will contain all the entries
> contained in the cgroup->exceptions list. Otherwise you cannot
hm, no. the local exceptions are meant to be used for stuff written
locally. the devcgroup->exceptions list is the list in effect.
> use the exception_add() the way you do (or you can't use RCU),
> since there could be a window between the successful addition
> of a rule to cgroup->local.exceptions and the failed addition to
> cgroup->exceptions, during which a task (since it won't need
> the mutex for devcg_allow()) could exceed allowed permissions.
>
> It's possible I'm misunderstanding. If you think that's the case
> just kick me and I'll take a fresh look.
I see your point. it's indeed a problem. in dev_exception_add(), it
needs to check for permissions before actually adding to
devcgroup->exceptions.
> (Btw is there a git tree or gitweb view I could look at alongside
> the patchset?)
I'll rebase the patchset along with a fix for this and resubmit with a
link to the git repo.
--
Aristeu
On Thu, Nov 29, 2012 at 07:29:45PM +0000, Serge E. Hallyn wrote:
> thanks for doing this. I've got one concern though. I don't see
> any place where devcgroup_create() was updated to create the
> local exceptions list. I think we need a guarantee that at
> any time the local exceptions list will contain all the entries
> contained in the cgroup->exceptions list. Otherwise you cannot
> use the exception_add() the way you do (or you can't use RCU),
> since there could be a window between the successful addition
> of a rule to cgroup->local.exceptions and the failed addition to
> cgroup->exceptions, during which a task (since it won't need
> the mutex for devcg_allow()) could exceed allowed permissions.
>
> It's possible I'm misunderstanding. If you think that's the case
> just kick me and I'll take a fresh look.
>
> (Btw is there a git tree or gitweb view I could look at alongside
> the patchset?)
ah, yes, the missing thing you're not seeing is
4b1c7840b7d01b14a1a00fa0e61b761d4391ba67 from Tejun's tree
('device_cgroup: add lockdep asserts')
--
Aristeu
Quoting Aristeu Rozanski ([email protected]):
> On Thu, Nov 29, 2012 at 07:29:45PM +0000, Serge E. Hallyn wrote:
> > Quoting Aristeu Rozanski ([email protected]):
> > > In preparation for better hierarchy support, it's needed to retain the local
> > > settings in order to try to reapply them after a propagated change if they're
> > > still valid.
> > >
> > > Cc: Tejun Heo <[email protected]>
> > > Cc: Serge Hallyn <[email protected]>
> > > Signed-off-by: Aristeu Rozanski <[email protected]>
> > >
> > > ---
> > > security/device_cgroup.c | 108 ++++++++++++++++++++++++++++++++++-------------
> > > 1 file changed, 80 insertions(+), 28 deletions(-)
> >
> > Hi,
> >
> > thanks for doing this. I've got one concern though. I don't see
> > any place where devcgroup_create() was updated to create the
> > local exceptions list. I think we need a guarantee that at
>
> the local exceptions list is part of the dev_cgroup structure and it's
> initialized on the patch 'device_cgroup: keep track of local group
> setting':
>
> @@ -190,6 +238,8 @@
> if (!dev_cgroup)
> return ERR_PTR(-ENOMEM);
> INIT_LIST_HEAD(&dev_cgroup->exceptions);
> + INIT_LIST_HEAD(&dev_cgroup->local.exceptions);
> + dev_cgroup->local.behavior = DEVCG_DEFAULT_NONE;
> parent_cgroup = cgroup->parent;
>
> if (parent_cgroup == NULL)
>
> > any time the local exceptions list will contain all the entries
> > contained in the cgroup->exceptions list. Otherwise you cannot
>
> hm, no. the local exceptions are meant to be used for stuff written
> locally. the devcgroup->exceptions list is the list in effect.
>
> > use the exception_add() the way you do (or you can't use RCU),
> > since there could be a window between the successful addition
> > of a rule to cgroup->local.exceptions and the failed addition to
> > cgroup->exceptions, during which a task (since it won't need
> > the mutex for devcg_allow()) could exceed allowed permissions.
> >
> > It's possible I'm misunderstanding. If you think that's the case
> > just kick me and I'll take a fresh look.
>
> I see your point. it's indeed a problem. in dev_exception_add(), it
> needs to check for permissions before actually adding to
> devcgroup->exceptions.
>
> > (Btw is there a git tree or gitweb view I could look at alongside
> > the patchset?)
>
> I'll rebase the patchset along with a fix for this and resubmit with a
> link to the git repo.
Thanks, Aristeu!
-serge
On Thu, Nov 29, 2012 at 08:26:08PM +0000, Serge E. Hallyn wrote:
> Quoting Aristeu Rozanski ([email protected]):
> > I see your point. it's indeed a problem. in dev_exception_add(), it
> > needs to check for permissions before actually adding to
> > devcgroup->exceptions.
actually, checked again, it's done correctly. when adding an exception
that will allow extra device access (DEVCG_ALLOW), it does check it
before. It means that you can't add local exceptions unless at a certain
point of the time it was allowed to do so.
> >
> > > (Btw is there a git tree or gitweb view I could look at alongside
> > > the patchset?)
> >
> > I'll rebase the patchset along with a fix for this and resubmit with a
> > link to the git repo.
just pushed the same version to:
git://github.com/aristeu/linux-2.6.git
please use branch devcg_hiearchy_review
--
Aristeu