Devices accessibility control group (v2)

The main thing that I want from the kobj mapper is to add the mode_t on the struct kobj_map that reflects with permissions are associated with this particular map. This mode is to be returned via the kobj_lookup.

I use the FMODE_XXX flags to handle the permissions bits, as I will compare these ones to the file->f_mode later. By default all bits are set (for the initial container).

The additional things I need are kobj_remap() to change that permission and kobj_iterate() to walk the map.

The kobj_map_fini() is the roll-back for the kobj_map_init().

Signed-off-by Pavel Emelyanov <[email protected]>

...

drivers/base/map.c +11 -0

+		p = map->probes[i];
+		while (p->next != NULL) {
+			next = p->next;
+			kfree(p);
+			p = next;
+		}
+	}
+
+	kfree(p);
+	kfree(map);
+}

include/linux/kobj_map.h +6 -1

index bafe178..ecfe772 100644
@@ -7,8 +7,13 @@ struct kobj_map;
 
 int kobj_map(struct kobj_map *, dev_t, unsigned long, struct module *,
 	     kobj_probe_t *, int (*)(dev_t, void *), void *);
+int kobj_remap(struct kobj_map *, dev_t, mode_t, unsigned long, struct module *,
+	     kobj_probe_t *, int (*)(dev_t, void *), void *);
 void kobj_unmap(struct kobj_map *, dev_t, unsigned long);
-struct kobject *kobj_lookup(struct kobj_map *, dev_t, int *);
+struct kobject *kobj_lookup(struct kobj_map *, dev_t, mode_t *, int *);
+void kobj_map_iterate(struct kobj_map *, int (*fn)(dev_t, int, mode_t, void *),
+		void *);
 struct kobj_map *kobj_map_init(kobj_probe_t *, struct mutex *);
+void kobj_map_fini(struct kobj_map *);
 
 #endif

_______________________________________________
Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

drivers/base/map.c struct kobj_map *kobj_map_init(kobj_probe_t *base_probe, struct mutex *lock) · line 246

Quoted 7 lines

drivers/base/map.c +6 -0

+void kobj_map_fini(struct kobj_map *map)
+{
+     int i;
+     struct probe *p, *next;
+
+     for (i = 0; i < 256; i++) {

This should be 255, shouldn't it?

These changes include the API for the control group to map/remap/unmap the devices with their permissions and one important thing.

The fact is that the struct cdev is cached in the inode for faster access, so once we looked one up we go through the fast path and omit the kobj_lookup() call. This is no longer good when we restrict the access to cdevs.

To address this issue, I store the last_perm and last(_map) fields on the struct cdev (and protect them with the cdev_lock) and force the re-lookup in the kobj mappings if needed.

I know, this might be slow, but I have two points for it:
1. The re-lookup happens on open() only which is not a fast-path. Besides, this is so for block layer and nobody complains;
2. On a well-isolated setup, when each container has its own filesystem this is no longer a problem - each cgroup will cache the cdev on its inode and work good.

Signed-off-by Pavel Emelyanov <[email protected]>

---

fs/char_dev.c +93 -3

index c3bfa76..2b821ef 100644
@@ -22,6 +22,8 @@
 #include <linux/mutex.h>
 #include <linux/backing-dev.h>
 
+#include <linux/devscontrol.h>
+
 #ifdef CONFIG_KMOD
 #include <linux/kmod.h>
 #endif

@@ -362,17 +364,25 @@ int chrdev_open(struct inode * inode, struct file * filp)
 	struct cdev *p;
 	struct cdev *new = NULL;
 	int ret = 0;
+	struct kobj_map *map;
+	mode_t mode;
+
+	map = task_cdev_map(current);
+	if (map == NULL)
+		map = cdev_map;
 
 	spin_lock(&cdev_lock);
 	p = inode->i_cdev;
-	if (!p) {
+	if (!p || p->last != map) {
 		struct kobject *kobj;
 		int idx;
+
 		spin_unlock(&cdev_lock);
-		kobj = kobj_lookup(cdev_map, inode->i_rdev, &idx);
+		kobj = kobj_lookup(map, inode->i_rdev, &mode, &idx);
 		if (!kobj)
 			return -ENXIO;
 		new = container_of(kobj, struct cdev, kobj);
+		BUG_ON(p != NULL && p != new);
 		spin_lock(&cdev_lock);
 		p = inode->i_cdev;
 		if (!p) {

@@ -382,12 +392,24 @@ int chrdev_open(struct inode * inode, struct file * filp)
 			new = NULL;
 		} else if (!cdev_get(p))
 			ret = -ENXIO;
+		else {
+			p->last = map;
+			p->last_mode = mode;
+		}
 	} else if (!cdev_get(p))
 		ret = -ENXIO;
+	else
+		mode = p->last_mode;
 	spin_unlock(&cdev_lock);
 	cdev_put(new);
 	if (ret)
 		return ret;
+
+	if ((filp->f_mode & mode) != filp->f_mode) {
+		cdev_put(p);
+		return -EACCES;
+	}
+
 	filp->f_op = fops_get(p->ops);
 	if (!filp->f_op) {
 		cdev_put(p);

@@ -461,6 +483,64 @@ int cdev_add(struct cdev *p, dev_t dev, unsigned count)
 	return kobj_map(cdev_map, dev, count, NULL, exact_match, exact_lock, p);
 }
 
+#ifdef CONFIG_CGROUP_DEVS
+static inline void cdev_map_reset(struct kobj_map *map, struct cdev *c)
+{
+	spin_lock(&cdev_lock);
+	if (c->last == map)
+		c->last = NULL;
+	spin_unlock(&cdev_lock);
+}
+
+int cdev_add_to_map(struct kobj_map *map, dev_t dev, int all, mode_t mode)
+{
+	int tmp;
+	struct kobject *k;
+	struct cdev *c;
+
+	k = kobj_lookup(cdev_map, dev, NULL, &tmp);
+	if (k == NULL)
+		return -ENODEV;
+
+	c = container_of(k, struct cdev, kobj);
+	tmp = kobj_remap(map, dev, mode, all ? MINORMASK : 1, NULL,
+			exact_match, exact_lock, c);
+	if (tmp < 0) {
+		cdev_put(c);
+		return tmp;
+	}
+
+	cdev_map_reset(map, c);
+	return 0;
+}
+
+int cdev_del_from_map(struct kobj_map *map, dev_t dev, int all)
+{
+	int tmp;
+	struct kobject *k;
+	struct cdev *c;
+
+	k = kobj_lookup(cdev_map, dev, NULL, &tmp);
+	if (k == NULL)
+		return -ENODEV;
+
+	c = container_of(k, struct cdev, kobj);
+	kobj_unmap(map, dev, all ? MINORMASK : 1);
+
+	cdev_map_reset(map, c);
+
+	cdev_put(c);
+	cdev_put(c);
+	return 0;
+}
+
+void cdev_iterate_map(struct kobj_map *map,
+		int (*fn)(dev_t, int, mode_t, void *), void *x)
+{
+	kobj_map_iterate(map, fn, x);
+}
+#endif
+
 static void cdev_unmap(dev_t dev, unsigned count)
 {
 	kobj_unmap(cdev_map, dev, count);

@@ -542,9 +622,19 @@ static struct kobject *base_probe(dev_t dev, int *part, void *data)
 	return NULL;
 }
 
+struct kobj_map *cdev_map_init(void)
+{
+	return kobj_map_init(base_probe, &chrdevs_lock);
+}
+
+void cdev_map_fini(struct kobj_map *map)
+{
+	kobj_map_fini(map);
+}
+
 void __init chrdev_init(void)
 {
-	cdev_map = kobj_map_init(base_probe, &chrdevs_lock);
+	cdev_map = cdev_map_init();
 	bdi_init(&directly_mappable_cdev_bdi);
 }
 

include/linux/cdev.h +9 -0

index 1e29b13..d72a2a1 100644
@@ -9,6 +9,7 @@
 struct file_operations;
 struct inode;
 struct module;
+struct kobj_map;
 
 struct cdev {
 	struct kobject kobj;

@@ -17,6 +18,8 @@ struct cdev {
 	struct list_head list;
 	dev_t dev;
 	unsigned int count;
+	struct kobj_map *last;
+	mode_t last_mode;
 };
 
 void cdev_init(struct cdev *, const struct file_operations *);

@@ -33,5 +36,11 @@ void cd_forget(struct inode *);
 
 extern struct backing_dev_info directly_mappable_cdev_bdi;
 
+int cdev_add_to_map(struct kobj_map *map, dev_t dev, int all, mode_t mode);
+int cdev_del_from_map(struct kobj_map *map, dev_t dev, int all);
+struct kobj_map *cdev_map_init(void);
+void cdev_map_fini(struct kobj_map *map);
+void cdev_iterate_map(struct kobj_map *,
+		int (*fn)(dev_t, int, mode_t, void *), void *);
 #endif
 #endif

Quoted 13 lines · Pavel Emelyanov

These changes include the API for the control group to map/remap/unmap the devices with their permissions and one important thing.

The fact is that the struct cdev is cached in the inode for faster access, so once we looked one up we go through the fast path and omit the kobj_lookup() call. This is no longer good when we restrict the access to cdevs.

To address this issue, I store the last_perm and last(_map) fields on the struct cdev (and protect them with the cdev_lock) and force the re-lookup in the kobj mappings if needed.

Quoted 7 lines

I know, this might be slow, but I have two points for it:
1. The re-lookup happens on open() only which is not a fast-path. Besides, this is so for block layer and nobody complains;
2. On a well-isolated setup, when each container has its own filesystem this is no longer a problem - each cgroup will cache the cdev on its inode and work good.

What about simply returning -EPERM when open()ing a cdev with ->map!=task_cdev_map(current)?

Shouldn't be a problem for ttys, since the container init already has the tty open, right?

Otherwise, the patchset looks good to me. Want to look through this one a little more (i think that'd be easier with the -EPERM approach) and scrutinize patch 4, but overall it makes sense.

If I understand right, we're taking 14k per cgroup for kobjmaps? Do we consider that a problem?

thanks,
-serge

Quoted 22 lines · Serge E. Hallyn

Quoted 20 lines · Pavel Emelyanov

These changes include the API for the control group to map/remap/unmap the devices with their permissions and one important thing.

The fact is that the struct cdev is cached in the inode for faster access, so once we looked one up we go through the fast path and omit the kobj_lookup() call. This is no longer good when we restrict the access to cdevs.

To address this issue, I store the last_perm and last(_map) fields on the struct cdev (and protect them with the cdev_lock) and force the re-lookup in the kobj mappings if needed.

I know, this might be slow, but I have two points for it:
1. The re-lookup happens on open() only which is not a fast-path. Besides, this is so for block layer and nobody complains;
2. On a well-isolated setup, when each container has its own filesystem this is no longer a problem - each cgroup will cache the cdev on its inode and work good.

Quoted 2 lines

What about simply returning -EPERM when open()ing a cdev with ->map!=task_cdev_map(current)?

In this case it will HAVE to setup isolated filesystem for each cgroup. I thought that this flexibility doesn't hurt.

Quoted 2 lines

Shouldn't be a problem for ttys, since the container init already has the tty open, right?

Yup, but this is not the case for /dev/null or /dev/zero.

Quoted 4 lines

Otherwise, the patchset looks good to me. Want to look through this one a little more (i think that'd be easier with the -EPERM approach) and scrutinize patch 4, but overall it makes sense.

OK, thanks.

Quoted 2 lines

If I understand right, we're taking 14k per cgroup for kobjmaps? Do we consider that a problem?

14k? I allocate the struct kobj_map which is only 256 pointers (i.e. - 2K) and the struct probe that is 32 bytes. I.e. 4k or a single page. I think this is OK.

Quoted 3 lines

thanks,
-serge

[snip]

Each new group will have its own maps for char and block layers. The devices access list is tuned via the devices.permissions file.

One may read from the file to get the configured
state. E.g.

# cat /cont/devices/0/devices.permissions
c 1:* rw
b 8:1 rw

The top container isn't initialized, so that the char and block layers will use the global maps to lookup their devices.

Signed-off-by Pavel Emelyanov <[email protected]>

---

fs/Makefile +2 -0

index 82b6ae1..a085706 100644
@@ -63,6 +63,8 @@ obj-y				+= devpts/
 
 obj-$(CONFIG_PROFILING)		+= dcookies.o
 obj-$(CONFIG_DLM)		+= dlm/
+
+obj-$(CONFIG_CGROUP_DEVS)	+= devscontrol.o
 
 # Do not add any filesystems before this line
 obj-$(CONFIG_REISERFS_FS)	+= reiserfs/

fs/devscontrol.c new file +82 -0

new file mode 100644
index 0000000..ea282f3
@@ -0,0 +1,291 @@
+/*
+ * devscontrol.c - Device Controller
+ *
+ * Copyright 2007 OpenVZ SWsoft Inc
+ * Author: Pavel Emelyanov <xemul at openvz.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/cgroup.h>
+#include <linux/cdev.h>
+#include <linux/err.h>
+#include <linux/devscontrol.h>
+#include <linux/uaccess.h>
+#include <linux/fs.h>
+#include <linux/genhd.h>
+
+struct devs_cgroup {
+	struct cgroup_subsys_state css;
+
+	struct kobj_map *cdev_map;
+	struct kobj_map *bdev_map;
+};
+
+static inline
+struct devs_cgroup *css_to_devs(struct cgroup_subsys_state *css)
+{
+	return container_of(css, struct devs_cgroup, css);
+}
+
+static inline
+struct devs_cgroup *cgroup_to_devs(struct cgroup *cont)
+{
+	return css_to_devs(cgroup_subsys_state(cont, devs_subsys_id));
+}
+
+struct kobj_map *task_cdev_map(struct task_struct *tsk)
+{
+	struct cgroup_subsys_state *css;
+
+	css = task_subsys_state(tsk, devs_subsys_id);
+	if (css->cgroup->parent == NULL)
+		return NULL;
+	else
+		return css_to_devs(css)->cdev_map;
+}
+
+struct kobj_map *task_bdev_map(struct task_struct *tsk)
+{
+	struct cgroup_subsys_state *css;
+
+	css = task_subsys_state(tsk, devs_subsys_id);
+	if (css->cgroup->parent == NULL)
+		return NULL;
+	else
+		return css_to_devs(css)->bdev_map;
+}
+
+static struct cgroup_subsys_state *
+devs_create(struct cgroup_subsys *ss, struct cgroup *cont)
+{
+	struct devs_cgroup *devs;
+
+	devs = kzalloc(sizeof(struct devs_cgroup), GFP_KERNEL);
+	if (devs == NULL)
+		goto out;
+
+	devs->cdev_map = cdev_map_init();
+	if (devs->cdev_map == NULL)
+		goto out_free;
+
+	devs->bdev_map = bdev_map_init();
+	if (devs->bdev_map == NULL)
+		goto out_free_cdev;

fs/devscontrol.c new file +82 -0

+
+static void devs_destroy(struct cgroup_subsys *ss, struct cgroup *cont)
+{
+	struct devs_cgroup *devs;
+
+	devs = cgroup_to_devs(cont);
+	bdev_map_fini(devs->bdev_map);
+	cdev_map_fini(devs->cdev_map);
+	kfree(devs);
+}
+
+static int decode_perms_str(char *buf, int *chrdev, dev_t *dev,
+		int *all, mode_t *mode)
+{
+	unsigned int major, minor;
+	char *end;
+	mode_t tmp;
+
+	/* [cb] <major>:<minor> [r-][w-] */
+
+	if (buf[0] == 'c')
+		*chrdev = 1;
+	else if (buf[0] == 'b')
+		*chrdev = 0;
+	else
+		return -EINVAL;
+
+	if (buf[1] != ' ')
+		return -EINVAL;
+
+	major = simple_strtoul(buf + 2, &end, 10);
+	if (*end != ':')
+		return -EINVAL;
+
+	if (end[1] == '*') {
+		if (end[2] != ' ')
+			return -EINVAL;
+
+		*all = 1;
+		minor = 0;
+		end += 2;
+	} else {
+		minor = simple_strtoul(end + 1, &end, 10);
+		if (*end != ' ')
+			return -EINVAL;
+
+		*all = 0;
+	}
+
+	tmp = 0;
+
+	if (end[1] == 'r')
+		tmp |= FMODE_READ;
+	else if (end[1] != '-')
+		return -EINVAL;
+	if (end[2] == 'w')
+		tmp |= FMODE_WRITE;
+	else if (end[2] != '-')
+		return -EINVAL;
+
+	*dev = MKDEV(major, minor);
+	*mode = tmp;
+	return 0;
+}
+
+static int encode_perms_str(char *buf, int len, int chrdev, dev_t dev,
+		int all, mode_t mode)
+{
+	int ret;
+
+	ret = snprintf(buf, len, "%c %d:", chrdev ? 'c' : 'b', MAJOR(dev));
+	if (all)
+		ret += snprintf(buf + ret, len - ret, "*");
+	else
+		ret += snprintf(buf + ret, len - ret, "%d", MINOR(dev));
+
+	ret += snprintf(buf + ret, len - ret, " %c%c\n",
+			(mode & FMODE_READ) ? 'r' : '-',
+			(mode & FMODE_WRITE) ? 'w' : '-');
+
+	return ret + 1;
+}

fs/devscontrol.c new file +107 -0

+
+	if (copy_from_user(buf, ubuf, sizeof(buf)))
+		return -EFAULT;
+
+	buf[sizeof(buf) - 1] = 0;
+	err = decode_perms_str(buf, &chrdev, &dev, &all, &mode);
+	if (err < 0)
+		return err;
+
+	devs = cgroup_to_devs(cont);
+
+	if (mode == 0) {
+		if (chrdev)
+			err = cdev_del_from_map(devs->cdev_map, dev, all);
+		else
+			err = bdev_del_from_map(devs->bdev_map, dev, all);
+
+		if (err < 0)
+			return err;
+
+		css_put(&devs->css);
+	} else {
+		if (chrdev)
+			err = cdev_add_to_map(devs->cdev_map, dev, all, mode);
+		else
+			err = bdev_add_to_map(devs->bdev_map, dev, all, mode);
+
+		if (err < 0)
+			return err;
+
+		css_get(&devs->css);
+	}
+
+	return nbytes;
+}
+
+struct devs_dump_arg {
+	char *buf;
+	int pos;
+	int chrdev;
+};
+
+static int devs_dump_one(dev_t dev, int range, mode_t mode, void *x)
+{
+	struct devs_dump_arg *arg = x;
+	char tmp[64];
+	int len;
+
+	len = encode_perms_str(tmp, sizeof(tmp), arg->chrdev, dev,
+			range != 1, mode);
+
+	if (arg->pos >= PAGE_SIZE - len)
+		return 1;
+
+	memcpy(arg->buf + arg->pos, tmp, len);
+	arg->pos += len;
+	return 0;
+}
+
+static ssize_t devs_read(struct cgroup *cont, struct cftype *cft,
+		struct file *f, char __user *ubuf, size_t nbytes, loff_t *pos)
+{
+	struct devs_dump_arg arg;
+	struct devs_cgroup *devs;
+	ssize_t ret;
+
+	arg.buf = (char *)__get_free_page(GFP_KERNEL);
+	if (arg.buf == NULL)
+		return -ENOMEM;
+
+	devs = cgroup_to_devs(cont);
+	arg.pos = 0;
+
+	arg.chrdev = 1;
+	cdev_iterate_map(devs->cdev_map, devs_dump_one, &arg);
+
+	arg.chrdev = 0;
+	bdev_iterate_map(devs->bdev_map, devs_dump_one, &arg);
+
+	ret = simple_read_from_buffer(ubuf, nbytes, pos,
+			arg.buf, arg.pos);
+
+	free_page((unsigned long)arg.buf);
+	return ret;
+}
+
+static struct cftype devs_files[] = {
+	{
+		.name = "permissions",
+		.write = devs_write,
+		.read = devs_read,
+	},
+};
+
+static int devs_populate(struct cgroup_subsys *ss, struct cgroup *cont)
+{
+	return cgroup_add_files(cont, ss,
+			devs_files, ARRAY_SIZE(devs_files));
+}
+
+struct cgroup_subsys devs_subsys = {
+	.name = "devices",
+	.subsys_id = devs_subsys_id,
+	.create = devs_create,
+	.destroy = devs_destroy,
+	.populate = devs_populate,
+};

include/linux/cgroup_subsys.h +6 -0

index 228235c..9c0cd2c 100644
@@ -42,3 +42,9 @@ SUBSYS(mem_cgroup)
 #endif
 
 /* */
+
+#ifdef CONFIG_CGROUP_DEVS
+SUBSYS(devs)
+#endif
+
+/* */

include/linux/devscontrol.h new file +20 -0

new file mode 100644
index 0000000..5f574e0
@@ -0,0 +1,20 @@
+#ifndef __DEVS_CONTROL_H__
+#define __DEVS_CONTROL_H__
+struct kobj_map;
+struct task_struct;
+
+#ifdef CONFIG_CGROUP_DEVS
+struct kobj_map *task_cdev_map(struct task_struct *);
+struct kobj_map *task_bdev_map(struct task_struct *);
+#else
+static inline kobj_map *task_cdev_map(struct task_struct *tsk)
+{
+	return NULL;
+}
+
+static inline kobj_map *task_bdev_map(struct task_struct *tsk)
+{
+	return NULL;
+}
+#endif
+#endif

init/Kconfig +6 -0

index b886ac9..4dd53d6 100644
@@ -283,6 +283,12 @@ config CGROUP_DEBUG
 
 	  Say N if unsure
 
+config CGROUP_DEVS
+	bool "Devices cgroup subsystem"
+	depends on CGROUPS
+	help
+	  Controlls the visibility of devices
+
 config CGROUP_NS
         bool "Namespace cgroup subsystem"
         depends on CGROUPS

Thanks for working on this, Pavel.

My only question with this patch is - so if I create a devs cgroup which only has access to, say /dev/loop0 and /dev/tty3, and someone in that cgroup manages to create a new cgroup, the new cgroup will have all the default permissions again, rather than inherit the permissions from this cgroup, right?

Quoted 4 lines

fs/devscontrol.c new file +4 -0

+static ssize_t devs_write(struct cgroup *cont, struct cftype *cft,
+		struct file *f, const char __user *ubuf,
+		size_t nbytes, loff_t *pos)
+{

fs/devscontrol.c line 184

Quoted 5 lines

fs/devscontrol.c new file +5 -0

+	int err, all, chrdev;
+	dev_t dev;
+	char buf[64];
+	struct devs_cgroup *devs;
+	mode_t mode;

(Of course this will require some privilege, i assume that's a detail you'll add next time around)

[snip]

Quoted 7 lines

Thanks for working on this, Pavel.

My only question with this patch is - so if I create a devs cgroup which only has access to, say /dev/loop0 and /dev/tty3, and someone in that cgroup manages to create a new cgroup, the new cgroup will have all the default permissions again, rather than inherit the permissions from this cgroup, right?

Right. When you create a new cgroup you have an empty perms set. Maybe it's worth inheriting the perms from the parent container, but I think that empty set is better as you will reconfigure it anyway.

[snip]

Quoted 4 lines

Quoted 4 lines

fs/devscontrol.c new file +4 -0

+static ssize_t devs_write(struct cgroup *cont, struct cftype *cft,
+		struct file *f, const char __user *ubuf,
+		size_t nbytes, loff_t *pos)
+{

Quoted 8 lines

fs/devscontrol.c line 184

Quoted 5 lines

fs/devscontrol.c new file +5 -0

+	int err, all, chrdev;
+	dev_t dev;
+	char buf[64];
+	struct devs_cgroup *devs;
+	mode_t mode;

(Of course this will require some privilege, i assume that's a detail you'll add next time around)

Hm... I though that privileges are governed at the cgroup level.... No?

I don't think so... Wouldn't really make sense for the cgroup infrastructure to presume to know what to enforce, and I don't see any checks around the _write functions in cgroup.c, and no capable() calls at all.

Quoted 5 lines · Serge E. Hallyn

I don't think so... Wouldn't really make sense for the cgroup infrastructure to presume to know what to enforce, and I don't see any checks around the _write functions in cgroup.c, and no capable() calls at all.

The cgroup filesystem can provide simple unix-level permissions on any given file. Am I right in thinking that having an entry in the mapper doesn't automatically give privileges for a device to the members of the cgroup, but they also have to have sufficient privilege in their own right? If so, that might be sufficient.

One other thought - should the parse/print routines themselves do a translation based on the device mappings for the writer/reader's cgroup? That way you could safely give a VE full permission to write to its children's device maps, but it would only be able to add/remap device targets that it could address itself.

Quoted 5 lines

One other thought - should the parse/print routines themselves do a translation based on the device mappings for the writer/reader's cgroup? That way you could safely give a VE full permission to write to its children's device maps, but it would only be able to add/remap device targets that it could address itself.

Oh, well if we do this then we can just as well use the translation functions to not allow a VE to add to its own set of devices, right?

Then maybe capable(CAP_NS_OVERRIDE|CAP_SYS_ADMIN) would only be required to add devices.

Quoted 8 lines · Serge E. Hallyn

Quoted 5 lines

One other thought - should the parse/print routines themselves do a translation based on the device mappings for the writer/reader's cgroup? That way you could safely give a VE full permission to write to its children's device maps, but it would only be able to add/remap device targets that it could address itself.

Oh, well if we do this then we can just as well use the translation functions to not allow a VE to add to its own set of devices, right?

Right.

Quoted 1 line

Quoted 2 lines

Then maybe capable(CAP_NS_OVERRIDE|CAP_SYS_ADMIN) would only be required to add devices.

Or simply require that they be added by someone who already has access to that device via their own control group? The root cgroup would have access to all devices.

Each new group will have its own maps for char and block layers. The devices access list is tuned via the devices.permissions file.

One may read from the file to get the configured state. E.g.

# cat /cont/devices/0/devices.permissions
c 1:* rw
b 8:1 rw

The top container isn't initialized, so that the char and block layers will use the global maps to lookup their devices.

Signed-off-by Pavel Emelyanov <[email protected]>

---

fs/Makefile +2 -0

index 82b6ae1..a085706 100644
@@ -63,6 +63,8 @@ obj-y                         += devpts/
 
 obj-$(CONFIG_PROFILING)                += dcookies.o
 obj-$(CONFIG_DLM)              += dlm/
+
+obj-$(CONFIG_CGROUP_DEVS)      += devscontrol.o
 
 # Do not add any filesystems before this line
 obj-$(CONFIG_REISERFS_FS)      += reiserfs/

fs/devscontrol.c new file +44 -0

new file mode 100644
index 0000000..ea282f3
@@ -0,0 +1,291 @@
+/*
+ * devscontrol.c - Device Controller
+ *
+ * Copyright 2007 OpenVZ SWsoft Inc
+ * Author: Pavel Emelyanov <xemul at openvz.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/cgroup.h>
+#include <linux/cdev.h>
+#include <linux/err.h>
+#include <linux/devscontrol.h>
+#include <linux/uaccess.h>
+#include <linux/fs.h>
+#include <linux/genhd.h>
+
+struct devs_cgroup {
+       struct cgroup_subsys_state css;
+
+       struct kobj_map *cdev_map;
+       struct kobj_map *bdev_map;
+};
+
+static inline
+struct devs_cgroup *css_to_devs(struct cgroup_subsys_state *css)
+{
+       return container_of(css, struct devs_cgroup, css);
+}
+
+static inline
+struct devs_cgroup *cgroup_to_devs(struct cgroup *cont)
+{
+       return css_to_devs(cgroup_subsys_state(cont, devs_subsys_id));
+}
+

fs/devscontrol.c new file +16 -0

+
+static ssize_t devs_write(struct cgroup *cont, struct cftype *cft,
+               struct file *f, const char __user *ubuf,
+               size_t nbytes, loff_t *pos)
+{
+       int err, all, chrdev;
+       dev_t dev;
+       char buf[64];
+       struct devs_cgroup *devs;
+       mode_t mode;
+
+       if (copy_from_user(buf, ubuf, sizeof(buf)))
+               return -EFAULT;
+
+       buf[sizeof(buf) - 1] = 0;
+       err = decode_perms_str(buf, &chrdev, &dev, &all, &mode);

Quoted 82 lines · Pavel Emelyanov

Each new group will have its own maps for char and block layers. The devices access list is tuned via the devices.permissions file.

One may read from the file to get the configured state. E.g.

# cat /cont/devices/0/devices.permissions
c 1:* rw
b 8:1 rw

The top container isn't initialized, so that the char and block layers will use the global maps to lookup their devices.

Signed-off-by Pavel Emelyanov <[email protected]>

---

fs/Makefile +2 -0

index 82b6ae1..a085706 100644
@@ -63,6 +63,8 @@ obj-y                         += devpts/
 
 obj-$(CONFIG_PROFILING)                += dcookies.o
 obj-$(CONFIG_DLM)              += dlm/
+
+obj-$(CONFIG_CGROUP_DEVS)      += devscontrol.o
 
 # Do not add any filesystems before this line
 obj-$(CONFIG_REISERFS_FS)      += reiserfs/

fs/devscontrol.c new file +44 -0

new file mode 100644
index 0000000..ea282f3
@@ -0,0 +1,291 @@
+/*
+ * devscontrol.c - Device Controller
+ *
+ * Copyright 2007 OpenVZ SWsoft Inc
+ * Author: Pavel Emelyanov <xemul at openvz.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/cgroup.h>
+#include <linux/cdev.h>
+#include <linux/err.h>
+#include <linux/devscontrol.h>
+#include <linux/uaccess.h>
+#include <linux/fs.h>
+#include <linux/genhd.h>
+
+struct devs_cgroup {
+       struct cgroup_subsys_state css;
+
+       struct kobj_map *cdev_map;
+       struct kobj_map *bdev_map;
+};
+
+static inline
+struct devs_cgroup *css_to_devs(struct cgroup_subsys_state *css)
+{
+       return container_of(css, struct devs_cgroup, css);
+}
+
+static inline
+struct devs_cgroup *cgroup_to_devs(struct cgroup *cont)
+{
+       return css_to_devs(cgroup_subsys_state(cont, devs_subsys_id));
+}
+

fs/devscontrol.c line 54

Quoted 10 lines

fs/devscontrol.c new file +10 -0

+struct kobj_map *task_cdev_map(struct task_struct *tsk)
+{
+       struct cgroup_subsys_state *css;
+
+       css = task_subsys_state(tsk, devs_subsys_id);
+       if (css->cgroup->parent == NULL)
+               return NULL;
+       else
+               return css_to_devs(css)->cdev_map;
+}

For this (and task_bdev_map) it would be cleaner to just leave the cdev_map and bdev_map in the root cgroup as NULL, so you could just make this

struct kobj_map *task_cdev_map(struct task_struct *tsk)
{
return css_to_devs(task_subsys_state(tsk, devs_subsys_id))->cdev_map;
}

Quoted 24 lines

fs/devscontrol.c new file +24 -0

+
+static ssize_t devs_write(struct cgroup *cont, struct cftype *cft,
+               struct file *f, const char __user *ubuf,
+               size_t nbytes, loff_t *pos)
+{
+       int err, all, chrdev;
+       dev_t dev;
+       char buf[64];
+       struct devs_cgroup *devs;
+       mode_t mode;
+
+       if (copy_from_user(buf, ubuf, sizeof(buf)))
+               return -EFAULT;
+
+       buf[sizeof(buf) - 1] = 0;
+       err = decode_perms_str(buf, &chrdev, &dev, &all, &mode);
+       if (err < 0)
+               return err;
+
+       devs = cgroup_to_devs(cont);
+
+       if (mode == 0) {
+               if (chrdev)
+                       err = cdev_del_from_map(devs->cdev_map, dev, all);

fs/devscontrol.c line 80

Quoted 2 lines

fs/devscontrol.c new file +2 -0

+               else
+                       err = bdev_del_from_map(devs->bdev_map, dev, all);

There's no locking involved on these calls, other than the cgroups code guaranteeing that the subsystem objects themselves won't go away. A quick look over the kobj_map calls suggests that these calls may be thread-safe - can you confirm that. (And maybe comment at the top of the function?)

Quoted 6 lines

fs/devscontrol.c new file +6 -0

+       arg.buf = (char *)__get_free_page(GFP_KERNEL);
+       if (arg.buf == NULL)
+               return -ENOMEM;
+
+       devs = cgroup_to_devs(cont);
+       arg.pos = 0;

fs/devscontrol.c line 262

Quoted 6 lines

fs/devscontrol.c new file +6 -0

+
+       arg.chrdev = 1;
+       cdev_iterate_map(devs->cdev_map, devs_dump_one, &arg);
+
+       arg.chrdev = 0;
+       bdev_iterate_map(devs->bdev_map, devs_dump_one, &arg);

Is there any chance of this overflowing the buffer page?

Paul

Pavel Emelianov [[email protected]] wrote:

| The first version was posted long ago 
| (http://openvz.org/pipermail/devel/2007-September/007647.html)
| and since then there are many (good I hope) changes:
| 
| * Added the block devices support :) It turned out to
|   be a bit simpler than the char one (or I missed
|   something significant);
| * Now we can enable/disable not just individual devices,
|   but the whole major with all its minors (see the TODO
|   list beyond as well);
| * Added the ability to restrict the read/write permissions
|   to devices, not just visible/invisible state.
| 
| That is - the main features I wished to implement right
| after the v1 was sent. Some minor changes are:
| 
| * I merged the devices.char and devices.block files into
|   one - devices.permissions;
| * As the result of the change above - the strings passed
|   to this file has changed. Now they are
|          [bc] <major>:{<minor>|*} [r-][w-]
|   E.g. b 5:2 r- will grant the read permissions to the
|   block 5:2 device and c 3:* -w will grant the write-only
|   access to all the character devices with the major 5.
| 
| However, there are some things to be done:
| 
| * Make the /proc/devices show relevant info depending on
|   who is reading it. This seems to be easy to do, since
|   I already have the support to dump similar info into the
|   devices.permissions file, but I haven't tried to use
|   this in /proc/devices yet;
| * Add the support for devices ranges. I.e. someone might
|   wish to tell smth like b 5:[0-10] r- to this subsystem.
|   Currently this is not supported and I'm afraid that if we
|   start support minor ranges we'll have smth similar to
|   VMA-s or FLOCK-s ranges management in one more place in the
|   kernel.
| * One more question is - are there any other permissions to
|   work with? E.g. in OpenVZ we have a separate bit for 
|   quota management, maybe we can invent some more...
| 
| Currently I didn't pay much attention to split this set well,
| so this will most likely won't work with git-bisect, but I 
| think this is OK for now. I will sure split it better when I
| send the v3 and further.
| 
| The set is prepared against the 2.6.24-rc5-mm1.
| 
| All this is minimally tested and seems to work. Hope to hear
| you comments, wishes and patches soon :)
| 
| To play with it - run a standard procedure:
| 
|  # mount -t container none /cont/devs -o devices

This should be '-t cgroup'

|  # mkdir /cont/devs/0
|  # echo -n $$ > /cont/devs/0/tasks
| 
| and tune device permissions.

I started playing with this and noticed that even if I try to enable read access to device [c, 1:3] it also grants access to device [c, 1:5].

i.e the access restrictions seem to apply to all devices with a given major number. Is that really the intent ?

Both devices accessible here:

	# hexdump /dev/null
	# hexdump /dev/zero
	0000000 0000 0000 0000 0000 0000 0000 0000 0000
	*
	^C

Neither device accessible:

	# echo $$ > /container/devs/0/tasks
	# hexdump /dev/zero
	hexdump: /dev/zero: No such device or address
	hexdump: /dev/zero: Bad file descriptor
	# hexdump /dev/null
	hexdump: /dev/null: No such device or address
	hexdump: /dev/null: Bad file descriptor

Grant read access to /dev/null, but /dev/zero is also readable

	# echo c 1:3 r- > /container/devs/0/devices.permissions
	# hexdump /dev/null
	# hexdump /dev/zero
	0000000 0000 0000 0000 0000 0000 0000 0000 0000
	*
	^C

Remove read access to /dev/null, but /dev/zero is also not readable.

	# echo c 1:3 -- > /container/devs/0/devices.permissions
	# hexdump /dev/zero
	hexdump: /dev/zero: No such device or address
	hexdump: /dev/zero: Bad file descriptor

BTW, a question about cgroups: If we 'echo $$ > /container/devs/0/tasks' is there a way to remove/undo it later (so that the process has access as before) ?

| > I started playing with this and noticed that even if I try to
| > enable read access to device [c, 1:3] it also grants access
| > to device [c, 1:5]. 
| 
| Hm... I can't reproduce this:
| 
| # /bin/echo 'c 1:3 r-' > /cnt/dev/0/devices.permissions
| # /bin/echo -n $$ > /cnt/dev/0/tasks
| # cat /cnt/dev/0/devices.permissions 
| c 1:3 r-
| # hexdump /dev/null 
| # hexdump /dev/zero 
| hexdump: /dev/zero: No such device or address
| hexdump: /dev/zero: Bad file descriptor
| 
| Maybe you have played with devs cgroups before getting this?
| Can you show what's the contents of the devices.permissions file
| in your case?

Here is the repro again. I even tried after a reboot. Basically, granting access to /dev/null is also granting access to /dev/zero.

	# cat devices.permissions
	# hexdump /dev/zero
	hexdump: /dev/zero: No such device or address
	hexdump: /dev/zero: Bad file descriptor
	# hexdump /dev/null
	hexdump: /dev/null: No such device or address
	hexdump: /dev/null: Bad file descriptor
	# echo 'c 1:3 r-' > devices.permissions
	# hexdump /dev/null
	# hexdump /dev/zero
	0000000 0000 0000 0000 0000 0000 0000 0000 0000
	*

	^C
	# cat tasks
	3279
	22266
	# ps
	  PID TTY          TIME CMD
	 3279 pts/0    00:00:00 bash
	22267 pts/0    00:00:00 ps

Pavel Emelianov [[email protected]] wrote:

| sukadev-r/[email protected] wrote:
| > | > I started playing with this and noticed that even if I try to
| > | > enable read access to device [c, 1:3] it also grants access
| > | > to device [c, 1:5]. 
| > | 
| > | Hm... I can't reproduce this:
| > | 
| > | # /bin/echo 'c 1:3 r-' > /cnt/dev/0/devices.permissions
| > | # /bin/echo -n $$ > /cnt/dev/0/tasks
| > | # cat /cnt/dev/0/devices.permissions 
| > | c 1:3 r-
| > | # hexdump /dev/null 
| > | # hexdump /dev/zero 
| > | hexdump: /dev/zero: No such device or address
| > | hexdump: /dev/zero: Bad file descriptor
| > | 
| > | Maybe you have played with devs cgroups before getting this?
| > | Can you show what's the contents of the devices.permissions file
| > | in your case?
| > 
| > Here is the repro again. I even tried after a reboot. Basically,
| > granting access to /dev/null is also granting access to /dev/zero.
| > 
| > 	# cat devices.permissions
| > 	# hexdump /dev/zero
| > 	hexdump: /dev/zero: No such device or address
| > 	hexdump: /dev/zero: Bad file descriptor
| > 	# hexdump /dev/null
| > 	hexdump: /dev/null: No such device or address
| > 	hexdump: /dev/null: Bad file descriptor
| > 	# echo 'c 1:3 r-' > devices.permissions
| > 	# hexdump /dev/null
| > 	# hexdump /dev/zero
| > 	0000000 0000 0000 0000 0000 0000 0000 0000 0000
| > 	*
| > 	^C
| > 	# cat tasks
| > 	3279
| > 	22266
| > 	# ps
| > 	  PID TTY          TIME CMD
| > 	 3279 pts/0    00:00:00 bash
| > 	22267 pts/0    00:00:00 ps
| > 
| 
| This all looks completely incomprehensible :( 
| 
| Here's my test:
| # mount -t cgroup none /cnt/dev/ -o devices
| # mkdir /cnt/dev/0
| # /bin/echo -n $$ > /cnt/dev/0/tasks 
| # cat /cnt/dev/0/devices.permissions 
| # hexdump /dev/zero 
| hexdump: /dev/zero: No such device or address
| hexdump: /dev/zero: Bad file descriptor

Can you try this sequence:

- grant access to /dev/zero,
- hexdump /dev/zero
- revoke access to /dev/zero
- hexdump /dev/null
- hexdump /dev/zero.

| # hexdump /dev/null 
| hexdump: /dev/null: No such device or address
| hexdump: /dev/null: Bad file descriptor
| # echo 'c 1:3 r-' > /cnt/dev/0/devices.permissions
| # cat /cnt/dev/0/devices.permissions 
| c 1:3 r-
| # hexdump /dev/null 
| # hexdump /dev/zero 
| hexdump: /dev/zero: No such device or address
| hexdump: /dev/zero: Bad file descriptor
| 
| 
| Sukadev, could you please try to track the problem as you
| seem to be the only person who's experiencing problems
| with that.

I suspect the 'caching' of the last_mode (that you introduce in PATCH 2/4) combined with the fact that /dev/zero, /dev/null, /dev/kmem etc share a _SINGLE_ 'struct cdev' leads to the problem I am running into with /dev/zero and /dev/null.

Here is a what I suspect is happening (sorry, for low-level details)

Following sequence seems to repro it consistently for me:

	$ mount -t cgroup none /container/devs/ -o devices
	$ mkdir /container/devs/0
	$ cd !$
	cd /container/devs/0
	$ echo $$ > tasks

	$ hexdump /dev/zero
	hexdump: /dev/zero: No such device or address
	hexdump: /dev/zero: Bad file descriptor

	$ hexdump /dev/null
	hexdump: /dev/null: No such device or address
	hexdump: /dev/null: Bad file descriptor

	$ echo 'c 1:3 r-' > devices.permissions

	$ hexdump /dev/null

	$ hexdump /dev/zero
	hexdump: /dev/zero: No such device or address
	hexdump: /dev/zero: Bad file descriptor

No surprise so far.

	$ echo 'c 1:5 r-' > devices.permissions
	$ hexdump /dev/zero
	0000000 0000 0000 0000 0000 0000 0000 0000 0000
	*
	^C

Now grant read access to /dev/zero and more importantly, create a properly initialized inode for it.

	$ echo 'c 1:5 --' > devices.permissions

Then remove access to /dev/zero. This removes the kobject for /dev/zero from map. Also cdev_map_reset() sets cdev->last to NULL.

	$ hdz
	hexdump: /dev/zero: No such device or address
	hexdump: /dev/zero: Bad file descriptor

Since cdev->last is NULL, chrdev_open() calls kobj_lookup() which returns a NULL kobj and the open fails.

	$ hexdump /dev/null	# XXX 

Again, since cdev->last is NULL, kobj_lookup() is called, this time for /dev/null. This succeeds and cdev->last is correctly initialized. Eventually this open of /dev/null succeeds.

	$ hexdump /dev/zero
	0000000 0000 0000 0000 0000 0000 0000 0000 0000

Now the open of /dev/zero also succeeds !

I suspect that the reason is that when we first successfully read /dev/zero, we created/initialized an inode for it. This inode has the inode->i_cdev set correctly.

By reading /dev/null (marked XXX above), cdev->last is also correctly set.

But since /dev/zero and /dev/null _SHARE_ a 'struct cdev', when we call chrdev_open() for /dev/zero, we check the permissions of this common cdev and grant /dev/zero the same permissions as /dev/null.

I suspect we will get this behavior with all devices implemented by the 'mem' driver in drivers/char/mem.c. I was able to repro with /dev/full [c, 1:7])

Sukadev

* Add the support for devices ranges. I.e. someone might wish to tell smth like b 5:[0-10] r- to this subsystem.

Quoted 2 lines · Pavel Emelyanov

* Add the support for devices ranges. I.e. someone might wish to tell smth like b 5:[0-10] r- to this subsystem.

I'd be inclined to leave support for that in userspace, rather than adding fancier parsing to the kernel API.

Paul