OpenVZ Forum - RDF feed
type: channel
items: [PATCH 0/4] Devices accessibility control group (v2)
items: [PATCH 1/4] Some changes in the kobject mapper
items: [PATCH 2/4] The character devices layer changes
items: [PATCH 3/4] The block devices layer changes
items: [PATCH 4/4] The control group itself
items: Re: [PATCH 1/4] Some changes in the kobject mapper
items: Re: [PATCH 0/4] Devices accessibility control group (v2)
items: Re: [PATCH 2/4] The character devices layer changes
items: Re: [PATCH 4/4] The control group itself

[PATCH 0/4] Devices accessibility control group (v2)
type: item
description: 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.…
creator: Pavel Emelianov
date: 2008-01-08T09:02:50-00:00

[PATCH 1/4] Some changes in the kobject mapper
type: item
description: 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 <xemul@openvz.org> --- diff --git a/drivers/base/map.c b/drivers/base/map.c index e87017f..1aa2b58 100644 --- a/drivers/base/map.c +++ b/drivers/base/map.c @@ -15,11 +15,13 @@ #include <linux/kdev_t.h> #include <linux/kobject.h> #include <linux/kobj_map.h> +#include <linux/fs.h> struct kobj_map { struct probe { struct probe *next; dev_t dev; + mode_t mode; unsigned long range; struct module *owner; kobj_probe_t *get; @@ -29,9 +31,9 @@ struct kobj_map { struct mutex *lock; }; -int kobj_map(struct kobj_map *domain, dev_t dev, unsigned long range, - struct module *module, kobj_probe_t *probe, - int (*lock)(dev_t, void *), void *data) +static int __kobj_map(struct kobj_map *domain, dev_t dev, mode_t mode, + unsigned long range, struct module *module, + kobj_probe_t *probe, int (*lock)(dev_t, void *), void *data) { unsigned n = MAJOR(dev + range - 1) - MAJOR(dev) + 1; unsigned index = MAJOR(dev); @@ -53,8 +55,10 @@ int kobj_map(struct kobj_map *domain, dev_t dev, unsigned long range, p->dev = dev; p->range = range; p->data = data; + /* we allow these ones always by default */ + p->mode = mode | FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE; } - mutex_lock(domain->lock); + for (i = 0, p -= n; i < n; i++, p++, index++) { struct probe **s = &domain->probes[index % 255]; while (*s && (*s)->range < range) @@ -62,10 +66,57 @@ int kobj_map(struct kobj_map *domain, dev_t dev, unsigned long range, p->next = *s;…
creator: Pavel Emelianov
date: 2008-01-08T09:06:46-00:00

[PATCH 2/4] The character devices layer changes
type: item
description: 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 <xemul@openvz.org> --- diff --git a/fs/char_dev.c b/fs/char_dev.c index c3bfa76..2b821ef 100644 --- a/fs/char_dev.c +++ b/fs/char_dev.c @@ -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->las…
creator: Pavel Emelianov
date: 2008-01-08T09:12:51-00:00

[PATCH 3/4] The block devices layer changes
type: item
description: They are the same as for the character layer, but the good news is that there are no caching in this case. So this patch is smaller and easier to understand as compared to the previous one. Signed-off-by: Pavel Emelyanov <xemul@openvz.org> --- diff --git a/block/genhd.c b/block/genhd.c index 5e4ab4b..6f9ef48 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -8,6 +8,7 @@ #include <linux/kdev_t.h> #include <linux/kernel.h> #include <linux/blkdev.h> +#include <linux/devscontrol.h> #include <linux/init.h> #include <linux/spinlock.h> #include <linux/seq_file.h> @@ -195,6 +196,57 @@ void unlink_gendisk(struct gendisk *disk) disk->minors); } +#ifdef CONFIG_CGROUP_DEVS +int bdev_add_to_map(struct kobj_map *map, dev_t dev, int all, mode_t mode) +{ + int tmp; + struct kobject *kobj; + struct device *d; + struct gendisk *disk; + + kobj = kobj_lookup(bdev_map, dev, NULL, &tmp); + if (kobj == NULL) + return -ENODEV; + + d = kobj_to_dev(kobj); + disk = dev_to_disk(d); + tmp = kobj_remap(map, dev, mode, all ? MINORBITS : 1, NULL, + exact_match, exact_lock, disk); + if (tmp < 0) { + put_disk(disk); + return tmp; + } + + return 0; +} + +int bdev_del_from_map(struct kobj_map *map, dev_t dev, int all) +{ + int tmp; + struct kobject *kobj; + struct device *d; + struct gendisk *disk; + + kobj = kobj_lookup(bdev_map, dev, NULL, &tmp); + if (kobj == NULL) + return -ENODEV; + + d = kobj_to_dev(kobj); + disk = dev_to_disk(d); + kobj_unmap(map, dev, all ? MINORBITS : 1); + + put_disk(disk); + put_disk(disk); + return 0; +} + +void bdev_iterate_map(struct kobj_map *map, + int (*fn)(dev_t, int, mode_t, void *), void *x) +{ + kobj_map_iterate(map, fn, x); +} +#endif + /** * get_gendisk - get partitioning information for a given device * @dev: device to get partitioning information for @@ -202,10 +254,18 @@ void unlink_gendisk(struct gendisk *disk) * This function gets the structure containing partitioning * information for the given device @dev. */ -struct gendisk *get_gendisk(dev_t devt, int *…
creator: Pavel Emelianov
date: 2008-01-08T09:15:09-00:00

[PATCH 4/4] The control group itself
type: item
description: 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 <xemul@openvz.org> --- diff --git a/fs/Makefile b/fs/Makefile index 82b6ae1..a085706 100644 --- a/fs/Makefile +++ b/fs/Makefile @@ -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/ diff --git a/fs/devscontrol.c b/fs/devscontrol.c new file mode 100644 index 0000000..ea282f3 --- /dev/null +++ b/fs/devscontrol.c @@ -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_cgro…
creator: Pavel Emelianov
date: 2008-01-08T09:18:03-00:00

Re: [PATCH 1/4] Some changes in the kobject mapper
type: item
description: Pavel Emelyanov wrote: > 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 <xemul@openvz.org> > > ... > @@ -153,3 +237,21 @@ struct kobj_map *kobj_map_init(kobj_probe_t *base_probe, struct mutex *lock) > p->lock = lock; > return p; > } > + > +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? > + p = map->probes[i]; > + while (p->next != NULL) { > + next = p->next; > + kfree(p); > + p = next; > + } > + } > + > + kfree(p); > + kfree(map); > +} > diff --git a/include/linux/kobj_map.h b/include/linux/kobj_map.h > index bafe178..ecfe772 100644 > --- a/include/linux/kobj_map.h > +++ b/include/linux/kobj_map.h > @@ -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 > > ___…
creator: Daniel Hokka Zakrisso
date: 2008-01-08T18:36:36-00:00

Re: [PATCH 1/4] Some changes in the kobject mapper
type: item
description: On Tue, 2008-01-08 at 19:36 +0100, Daniel Hokka Zakrisson wrote: > > > +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? Neither, it should be a named constant. ;) -- Dave _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Dave Hansen
date: 2008-01-08T19:17:28-00:00

Re: [PATCH 0/4] Devices accessibility control group (v2)
type: item
description: Pavel Emelianov [xemul@openvz.org] 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 | t…
creator: Sukadev Bhattiprolu
date: 2008-01-12T21:20:14-00:00

Re: [PATCH 0/4] Devices accessibility control group (v2)
type: item
description: sukadev@us.ibm.com wrote: > Pavel Emelianov [xemul@openvz.org] 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 …
creator: Pavel Emelianov
date: 2008-01-14T07:52:59-00:00

Re: [PATCH 2/4] The character devices layer changes
type: item
description: Quoting Pavel Emelyanov (xemul@openvz.org): > 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. 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 > Signed-off-by: Pavel Emelyanov <xemul@openvz.org> > > --- > > diff --git a/fs/char_dev.c b/fs/char_dev.c > index c3bfa76..2b821ef 100644 > --- a/fs/char_dev.c > +++ b/fs/char_dev.c > @@ -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; > + > + ma…
creator: serue
date: 2008-01-14T17:03:33-00:00

Re: [PATCH 4/4] The control group itself
type: item
description: Quoting Pavel Emelyanov (xemul@openvz.org): > 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 <xemul@openvz.org> > > --- > > diff --git a/fs/Makefile b/fs/Makefile > index 82b6ae1..a085706 100644 > --- a/fs/Makefile > +++ b/fs/Makefile > @@ -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/ > diff --git a/fs/devscontrol.c b/fs/devscontrol.c > new file mode 100644 > index 0000000..ea282f3 > --- /dev/null > +++ b/fs/devscontrol.c > @@ -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…
creator: serue
date: 2008-01-14T17:40:56-00:00

Re: [PATCH 0/4] Devices accessibility control group (v2)
type: item
description: | > 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 _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Sukadev Bhattiprolu
date: 2008-01-14T17:42:20-00:00

Re: [PATCH 0/4] Devices accessibility control group (v2)
type: item
description: On Jan 8, 2008 1:02 AM, Pavel Emelyanov <xemul@openvz.org> wrote: > * 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 _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Paul Menage
date: 2008-01-14T21:18:49-00:00

Re: [PATCH 4/4] The control group itself
type: item
description: On Jan 8, 2008 1:18 AM, Pavel Emelyanov <xemul@openvz.org> wrote: > 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 <xemul@openvz.org> > > --- > > diff --git a/fs/Makefile b/fs/Makefile > index 82b6ae1..a085706 100644 > --- a/fs/Makefile > +++ b/fs/Makefile > @@ -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/ > diff --git a/fs/devscontrol.c b/fs/devscontrol.c > new file mode 100644 > index 0000000..ea282f3 > --- /dev/null > +++ b/fs/devscontrol.c > @@ -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 cs…
creator: Paul Menage
date: 2008-01-14T21:54:33-00:00

Re: [PATCH 4/4] The control group itself
type: item
description: [snip] > 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] >> +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; > > (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? [snip] _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Pavel Emelianov
date: 2008-01-15T07:53:13-00:00

Re: [PATCH 4/4] The control group itself
type: item
description: Paul Menage wrote: > On Jan 8, 2008 1:18 AM, Pavel Emelyanov <xemul@openvz.org> wrote: >> 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 <xemul@openvz.org> >> >> --- >> >> diff --git a/fs/Makefile b/fs/Makefile >> index 82b6ae1..a085706 100644 >> --- a/fs/Makefile >> +++ b/fs/Makefile >> @@ -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/ >> diff --git a/fs/devscontrol.c b/fs/devscontrol.c >> new file mode 100644 >> index 0000000..ea282f3 >> --- /dev/null >> +++ b/fs/devscontrol.c >> @@ -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> >> +#in…
creator: Pavel Emelianov
date: 2008-01-15T07:58:48-00:00

Re: [PATCH 2/4] The character devices layer changes
type: item
description: Serge E. Hallyn wrote: > Quoting Pavel Emelyanov (xemul@openvz.org): >> 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. > > 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. > 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. > 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. > 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. > thanks, > -serge > [snip] _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.li…
creator: Pavel Emelianov
date: 2008-01-15T08:05:22-00:00

Re: [PATCH 0/4] Devices accessibility control group (v2)
type: item
description: Paul Menage wrote: > On Jan 8, 2008 1:02 AM, Pavel Emelyanov <xemul@openvz.org> wrote: >> * 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. Yup. I agree with that. > Paul > Thanks, Pavel _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Pavel Emelianov
date: 2008-01-15T08:06:02-00:00

Re: [PATCH 0/4] Devices accessibility control group (v2)
type: item
description: sukadev@us.ibm.com 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 # 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…
creator: Pavel Emelianov
date: 2008-01-15T08:22:03-00:00

Re: [PATCH 4/4] The control group itself
type: item
description: Quoting Pavel Emelyanov (xemul@openvz.org): > [snip] > > > 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] > > >> +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; > > > > (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. -serge _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: serue
date: 2008-01-15T14:44:40-00:00

Re: [PATCH 2/4] The character devices layer changes
type: item
description: Quoting Pavel Emelyanov (xemul@openvz.org): > Serge E. Hallyn wrote: > > Quoting Pavel Emelyanov (xemul@openvz.org): > >> 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. > > > > 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. The cost and effort of setting up a private /dev seems so minimal to me it seems worth not dealing with the inode->map switching around. But maybe that's just me. > > 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. > > > 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. > > > If I understand right, we're taking 14k per cgroup for > > kobjmaps? Do we consider that a problem? > > 14k? I allocate the…
creator: serue
date: 2008-01-15T14:54:48-00:00

Re: [PATCH 4/4] The control group itself
type: item
description: On Jan 15, 2008 6:44 AM, Serge E. Hallyn <serue@us.ibm.com> wrote: > > 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. Paul _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Paul Menage
date: 2008-01-15T16:13:40-00:00

Re: [PATCH 4/4] The control group itself
type: item
description: Quoting Paul Menage (menage@google.com): > On Jan 15, 2008 6:44 AM, Serge E. Hallyn <serue@us.ibm.com> wrote: > > > > 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. Oh, well actually I think what we'd want is to require both CAP_NS_OVERRIDE and either CAP_MKNOD or CAP_SYS_ADMIN. So it's probably fine to leave this as is for now, and after I resend the patchset which pushes CAP_NS_OVERRIDE (which is in a 4-patch userns patchset I've been sitting on) the extra checks can be added. > 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. Though there *is* some bit of danger to removing devices from a privileged daemon, isn't there? Though I can't think of examples just now. (Sorry, piercing headache, can't think quite right, will think about this later) -serge _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: serue
date: 2008-01-15T17:49:41-00:00

Re: [PATCH 4/4] The control group itself
type: item
description: On Jan 15, 2008 9:49 AM, Serge E. Hallyn <serue@us.ibm.com> wrote: > > 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. > > 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. Paul _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: Paul Menage
date: 2008-01-15T17:54:27-00:00

Re: [PATCH 4/4] The control group itself
type: item
description: Quoting Paul Menage (menage@google.com): > On Jan 15, 2008 9:49 AM, Serge E. Hallyn <serue@us.ibm.com> wrote: > > > 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. > > > > > 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. Where by 'have access' you mean access to create the device? That sounds good. thanks, -serge _______________________________________________ Containers mailing list Containers@lists.linux-foundation.org https://lists.linux-foundation.org/mailman/listinfo/containers
creator: serue
date: 2008-01-15T18:17:17-00:00

Re: [PATCH 0/4] Devices accessibility control group (v2)
type: item
description: Pavel Emelianov [xemul@openvz.org] wrote: | sukadev@us.ibm.com 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…
creator: Sukadev Bhattiprolu
date: 2008-01-17T06:26:05-00:00

Re: [PATCH 0/4] Devices accessibility control group (v2)
type: item
description: sukadev@us.ibm.com wrote: > Pavel Emelianov [xemul@openvz.org] wrote: > | sukadev@us.ibm.com 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 /…
creator: Pavel Emelianov
date: 2008-01-21T08:31:45-00:00