| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20080902145642.GA8524@us.ibm.com>
Date: Tue, 2 Sep 2008 09:56:43 -0500
From: "Serge E. Hallyn" <serue@...ibm.com>
To: Lai Jiangshan <laijs@...fujitsu.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
Paul Menage <menage@...gle.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Linux Containers <containers@...ts.linux-foundation.org>
Subject: Re: [PATCH -mm] devcgroup: remove spin_lock()
Quoting Lai Jiangshan (laijs@...fujitsu.com):
> Since we introduced rcu for read side, spin_lock is used only for
> update. But we always hold cgroup_lock() when update, so spin_lock()
> is not need.
>
> Additional cleanup:
> 1) include linux/rcupdate.h explicitly
> 2) remove unused variable cur_devcgroup in devcgroup_update_access()
>
> Signed-off-by: Lai Jiangshan <laijs@...fujitsu.com>
It might be worth adding a comment over parent_has_perm that it is
called under cgroup_lock(). Also, the comment above
may_access_whitelist() saying 'call with c->lock held' should be
updated.
But the patch looks correct.
Acked-by: Serge Hallyn <serue@...ibm.com>
thanks,
-serge
> ---
> diff --git a/security/device_cgroup.c b/security/device_cgroup.c
> index d5c15a7..5ba7870 100644
> --- a/security/device_cgroup.c
> +++ b/security/device_cgroup.c
> @@ -1,5 +1,5 @@
> /*
> - * dev_cgroup.c - device cgroup subsystem
> + * device_cgroup.c - device cgroup subsystem
> *
> * Copyright 2007 IBM Corp
> */
> @@ -10,6 +10,7 @@
> #include <linux/list.h>
> #include <linux/uaccess.h>
> #include <linux/seq_file.h>
> +#include <linux/rcupdate.h>
>
> #define ACC_MKNOD 1
> #define ACC_READ 2
> @@ -22,18 +23,8 @@
>
> /*
> * whitelist locking rules:
> - * cgroup_lock() cannot be taken under dev_cgroup->lock.
> - * dev_cgroup->lock can be taken with or without cgroup_lock().
> - *
> - * modifications always require cgroup_lock
> - * modifications to a list which is visible require the
> - * dev_cgroup->lock *and* cgroup_lock()
> - * walking the list requires dev_cgroup->lock or cgroup_lock().
> - *
> - * reasoning: dev_whitelist_copy() needs to kmalloc, so needs
> - * a mutex, which the cgroup_lock() is. Since modifying
> - * a visible list requires both locks, either lock can be
> - * taken for walking the list.
> + * hold cgroup_lock() for update/read.
> + * hold rcu_read_lock() for read.
> */
>
> struct dev_whitelist_item {
> @@ -47,7 +38,6 @@ struct dev_whitelist_item {
> struct dev_cgroup {
> struct cgroup_subsys_state css;
> struct list_head whitelist;
> - spinlock_t lock;
> };
>
> static inline struct dev_cgroup *css_to_devcgroup(struct cgroup_subsys_state *s)
> @@ -103,7 +93,6 @@ free_and_exit:
> /* Stupid prototype - don't bother combining existing entries */
> /*
> * called under cgroup_lock()
> - * since the list is visible to other tasks, we need the spinlock also
> */
> static int dev_whitelist_add(struct dev_cgroup *dev_cgroup,
> struct dev_whitelist_item *wh)
> @@ -114,7 +103,6 @@ static int dev_whitelist_add(struct dev_cgroup *dev_cgroup,
> if (!whcopy)
> return -ENOMEM;
>
> - spin_lock(&dev_cgroup->lock);
> list_for_each_entry(walk, &dev_cgroup->whitelist, list) {
> if (walk->type != wh->type)
> continue;
> @@ -130,7 +118,6 @@ static int dev_whitelist_add(struct dev_cgroup *dev_cgroup,
>
> if (whcopy != NULL)
> list_add_tail_rcu(&whcopy->list, &dev_cgroup->whitelist);
> - spin_unlock(&dev_cgroup->lock);
> return 0;
> }
>
> @@ -144,14 +131,12 @@ static void whitelist_item_free(struct rcu_head *rcu)
>
> /*
> * called under cgroup_lock()
> - * since the list is visible to other tasks, we need the spinlock also
> */
> static void dev_whitelist_rm(struct dev_cgroup *dev_cgroup,
> struct dev_whitelist_item *wh)
> {
> struct dev_whitelist_item *walk, *tmp;
>
> - spin_lock(&dev_cgroup->lock);
> list_for_each_entry_safe(walk, tmp, &dev_cgroup->whitelist, list) {
> if (walk->type == DEV_ALL)
> goto remove;
> @@ -169,7 +154,6 @@ remove:
> call_rcu(&walk->rcu, whitelist_item_free);
> }
> }
> - spin_unlock(&dev_cgroup->lock);
> }
>
> /*
> @@ -209,7 +193,6 @@ static struct cgroup_subsys_state *devcgroup_create(struct cgroup_subsys *ss,
> }
> }
>
> - spin_lock_init(&dev_cgroup->lock);
> return &dev_cgroup->css;
> }
>
> @@ -325,15 +308,11 @@ static int parent_has_perm(struct dev_cgroup *childcg,
> {
> struct cgroup *pcg = childcg->css.cgroup->parent;
> struct dev_cgroup *parent;
> - int ret;
>
> if (!pcg)
> return 1;
> parent = cgroup_to_devcgroup(pcg);
> - spin_lock(&parent->lock);
> - ret = may_access_whitelist(parent, wh);
> - spin_unlock(&parent->lock);
> - return ret;
> + return may_access_whitelist(parent, wh);
> }
>
> /*
> @@ -352,7 +331,6 @@ static int parent_has_perm(struct dev_cgroup *childcg,
> static int devcgroup_update_access(struct dev_cgroup *devcgroup,
> int filetype, const char *buffer)
> {
> - struct dev_cgroup *cur_devcgroup;
> const char *b;
> char *endp;
> int count;
> @@ -361,8 +339,6 @@ static int devcgroup_update_access(struct dev_cgroup *devcgroup,
> if (!capable(CAP_SYS_ADMIN))
> return -EPERM;
>
> - cur_devcgroup = task_devcgroup(current);
> -
> memset(&wh, 0, sizeof(wh));
> b = buffer;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists