lkml.org 
[lkml]   [2008]   [May]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC/PATCH 1/8]: CGroup Files: Add locking mode to cgroups control files
    From
    Date

    On Tue, 2008-05-13 at 13:01 -0700, Andrew Morton wrote:
    > Fear, doubt and resistance!
    >
    > On Mon, 12 May 2008 23:37:08 -0700
    > menage@google.com wrote:
    >
    > > Different cgroup files have different stability requirements of the
    > > cgroups framework while the handler is running; currently most
    > > subsystems that don't have their own internal synchronization just
    > > call cgroup_lock()/cgroup_unlock(), which takes the global cgroup_mutex.
    > >
    > > This patch introduces a range of locking modes that can be requested
    > > by a control file; currently these are all implemented internally by
    > > taking cgroup_mutex, but expressing the intention will make it simpler
    > > to move to a finer-grained locking scheme in the future.
    > >
    >
    > This, umm, doesn't seem to do much to make the kernel a simpler place.
    >
    > Do we expect to gain much from this? Hope so... What?
    >
    > > Index: cgroup-2.6.25-mm1/include/linux/cgroup.h
    > > ===================================================================
    > > --- cgroup-2.6.25-mm1.orig/include/linux/cgroup.h
    > > +++ cgroup-2.6.25-mm1/include/linux/cgroup.h
    > > @@ -200,11 +200,87 @@ struct cgroup_map_cb {
    > > */
    > >
    > > #define MAX_CFTYPE_NAME 64
    > > +
    > > +/* locking modes for control files.
    > > + *
    > > + * These determine what level of guarantee the file handler wishes
    > > + * cgroups to provide about the stability of control group entities
    > > + * for the duration of the handler callback.
    > > + *
    > > + * The minimum guarantee is that the subsystem state for this
    > > + * subsystem will not be freed (via a call to the subsystem's
    > > + * destroy() callback) until after the control file handler
    > > + * returns. This guarantee is provided by the fact that the open
    > > + * dentry for the control file keeps its parent (cgroup) dentry alive,
    > > + * which in turn keeps the cgroup object from being actually freed
    > > + * (although it can be moved into the removed state in the
    > > + * meantime). This is suitable for subsystems that completely control
    > > + * their own synchronization.
    > > + *
    > > + * Other possible guarantees are given below.
    > > + *
    > > + * XXX_READ bits are used for a read operation on the control file,
    > > + * XXX_WRITE bits are used for a write operation on the control file
    > > + */
    >
    > Vague handwaving: lockdep doesn't know anything about any of this.
    > Whereas if we were more conventional in using separate locks and
    > suitable lock types for each application, we would retain full lockdep
    > coverage.
    >
    > > +/*
    > > + * CFT_LOCK_ATTACH_(READ|WRITE): This operation will not run
    > > + * concurrently with a task movement into or out of this cgroup.
    > > + */
    > > +#define CFT_LOCK_ATTACH_READ 1
    > > +#define CFT_LOCK_ATTACH_WRITE 2
    > > +#define CFT_LOCK_ATTACH (CFT_LOCK_ATTACH_READ | CFT_LOCK_ATTACH_WRITE)
    > > +
    > > +/*
    > > + * CFT_LOCK_RMDIR_(READ|WRITE): This operation will not run
    > > + * concurrently with the removal of the affected cgroup.
    > > + */
    > > +#define CFT_LOCK_RMDIR_READ 4
    > > +#define CFT_LOCK_RMDIR_WRITE 8
    > > +#define CFT_LOCK_RMDIR (CFT_LOCK_RMDIR_READ | CFT_LOCK_RMDIR_WRITE)
    > > +
    > > +/*
    > > + * CFT_LOCK_HIERARCHY_(READ|WRITE): This operation will not run
    > > + * concurrently with a cgroup creation or removal in this hierarchy,
    > > + * or a bind/move/unbind for this subsystem.
    > > + */
    > > +#define CFT_LOCK_HIERARCHY_READ 16
    > > +#define CFT_LOCK_HIERARCHY_WRITE 32
    > > +#define CFT_LOCK_HIERARCHY (CFT_LOCK_HIERARCHY_READ | CFT_LOCK_HIERARCHY_WRITE)
    > > +
    > > +/*
    > > + * CFT_LOCK_CGL_(READ|WRITE): This operation is called with
    > > + * cgroup_lock() held; it will not run concurrently with any of the
    > > + * above operations in any cgroup/hierarchy. This should be considered
    > > + * to be the BKL of cgroups - it should be avoided if you can use
    > > + * finer-grained locking
    > > + */
    > > +#define CFT_LOCK_CGL_READ 64
    > > +#define CFT_LOCK_CGL_WRITE 128
    > > +#define CFT_LOCK_CGL (CFT_LOCK_CGL_READ | CFT_LOCK_CGL_WRITE)
    > > +
    > > +#define CFT_LOCK_FOR_READ (CFT_LOCK_ATTACH_READ | \
    > > + CFT_LOCK_RMDIR_READ | \
    > > + CFT_LOCK_HIERARCHY_READ | \
    > > + CFT_LOCK_CGL_READ)
    > > +
    > > +#define CFT_LOCK_FOR_WRITE (CFT_LOCK_ATTACH_WRITE | \
    > > + CFT_LOCK_RMDIR_WRITE | \
    > > + CFT_LOCK_HIERARCHY_WRITE | \
    > > + CFT_LOCK_CGL_WRITE)
    > > +
    > > struct cftype {
    > > /* By convention, the name should begin with the name of the
    > > * subsystem, followed by a period */
    > > char name[MAX_CFTYPE_NAME];
    > > int private;
    > > +
    > > + /*
    > > + * Determine what locks (if any) are held across calls to
    > > + * read_X/write_X callback. See lockmode definitions above
    > > + */
    > > + int lockmode;
    > > +
    > > int (*open) (struct inode *inode, struct file *file);
    > > ssize_t (*read) (struct cgroup *cgrp, struct cftype *cft,
    > > struct file *file,
    > > Index: cgroup-2.6.25-mm1/kernel/cgroup.c
    > > ===================================================================
    > > --- cgroup-2.6.25-mm1.orig/kernel/cgroup.c
    > > +++ cgroup-2.6.25-mm1/kernel/cgroup.c
    > > @@ -1327,38 +1327,65 @@ enum cgroup_filetype {
    > > FILE_RELEASE_AGENT,
    > > };
    > >
    > > -static ssize_t cgroup_write_X64(struct cgroup *cgrp, struct cftype *cft,
    > > - struct file *file,
    > > - const char __user *userbuf,
    > > - size_t nbytes, loff_t *unused_ppos)
    > > +
    > > +
    > > +/**
    > > + * cgroup_file_lock(). Helper for cgroup read/write methods.
    > > + * @cgrp: the cgroup being acted on
    > > + * @cft: the control file being written to or read from
    > > + * *write: true if the access is a write access.
    > > + *
    > > + * Takes any necessary locks as requested by the control file's
    > > + * 'lockmode' field; checks (after locking if necessary) that the
    > > + * control group is not in the process of being destroyed.
    > > + *
    > > + * Currently all the locking options are implemented in the same way,
    > > + * by taking cgroup_mutex. Future patches will add finer-grained
    > > + * locking.
    > > + *
    > > + * Calls to cgroup_file_lock() should always be paired with calls to
    > > + * cgroup_file_unlock(), even if cgroup_file_lock() returns an error.
    > > + */
    > > +
    > > +static int cgroup_file_lock(struct cgroup *cgrp, struct cftype *cft, int write)
    > > {
    > > - char buffer[64];
    > > - int retval = 0;
    > > - char *end;
    > > + int mask = write ? CFT_LOCK_FOR_WRITE : CFT_LOCK_FOR_READ;
    > > + BUILD_BUG_ON(CFT_LOCK_FOR_READ != (CFT_LOCK_FOR_WRITE >> 1));
    > >
    > > - if (!nbytes)
    > > - return -EINVAL;
    > > - if (nbytes >= sizeof(buffer))
    > > - return -E2BIG;
    > > - if (copy_from_user(buffer, userbuf, nbytes))
    > > - return -EFAULT;
    > > + if (cft->lockmode & mask)
    > > + mutex_lock(&cgroup_mutex);
    > > + if (cgroup_is_removed(cgrp))
    > > + return -ENODEV;
    > > + return 0;
    > > +}
    > > +
    > > +/**
    > > + * cgroup_file_unlock(): undoes the effect of cgroup_file_lock()
    > > + */
    > > +
    > > +static void cgroup_file_unlock(struct cgroup *cgrp, struct cftype *cft,
    > > + int write)
    > > +{
    > > + int mask = write ? CFT_LOCK_FOR_WRITE : CFT_LOCK_FOR_READ;
    > > + if (cft->lockmode & mask)
    > > + mutex_unlock(&cgroup_mutex);
    > > +}
    > >
    > > - buffer[nbytes] = 0; /* nul-terminate */
    > > - strstrip(buffer);
    > > +static ssize_t cgroup_write_X64(struct cgroup *cgrp, struct cftype *cft,
    > > + const char *buffer)
    > > +{
    > > + char *end;
    > > if (cft->write_u64) {
    > > u64 val = simple_strtoull(buffer, &end, 0);
    > > if (*end)
    > > return -EINVAL;
    > > - retval = cft->write_u64(cgrp, cft, val);
    > > + return cft->write_u64(cgrp, cft, val);
    > > } else {
    > > s64 val = simple_strtoll(buffer, &end, 0);
    > > if (*end)
    > > return -EINVAL;
    > > - retval = cft->write_s64(cgrp, cft, val);
    > > + return cft->write_s64(cgrp, cft, val);
    > > }
    > > - if (!retval)
    > > - retval = nbytes;
    > > - return retval;
    > > }
    > >
    > > static ssize_t cgroup_common_file_write(struct cgroup *cgrp,
    > > @@ -1426,47 +1453,82 @@ out1:
    > > return retval;
    > > }
    > >
    > > -static ssize_t cgroup_file_write(struct file *file, const char __user *buf,
    > > +static ssize_t cgroup_file_write(struct file *file, const char __user *userbuf,
    > > size_t nbytes, loff_t *ppos)
    > > {
    > > struct cftype *cft = __d_cft(file->f_dentry);
    > > struct cgroup *cgrp = __d_cgrp(file->f_dentry->d_parent);
    > > -
    > > - if (!cft || cgroup_is_removed(cgrp))
    > > - return -ENODEV;
    > > - if (cft->write)
    > > - return cft->write(cgrp, cft, file, buf, nbytes, ppos);
    > > - if (cft->write_u64 || cft->write_s64)
    > > - return cgroup_write_X64(cgrp, cft, file, buf, nbytes, ppos);
    > > - if (cft->trigger) {
    > > - int ret = cft->trigger(cgrp, (unsigned int)cft->private);
    > > - return ret ? ret : nbytes;
    > > + ssize_t retval;
    > > + char static_buffer[64];
    > > + char *buffer = static_buffer;
    > > + ssize_t max_bytes = sizeof(static_buffer) - 1;
    > > + if (!cft->write && !cft->trigger) {
    > > + if (!nbytes)
    > > + return -EINVAL;
    > > + if (nbytes >= max_bytes)
    > > + return -E2BIG;
    > > + if (nbytes >= sizeof(static_buffer)) {
    >
    > afaict this can't happen - we would have already returned -E2BIG?
    >
    > > + /* +1 for nul-terminator */
    > > + buffer = kmalloc(nbytes + 1, GFP_KERNEL);
    > > + if (buffer == NULL)
    > > + return -ENOMEM;
    > > + }
    > > + if (copy_from_user(buffer, userbuf, nbytes)) {
    > > + retval = -EFAULT;
    > > + goto out_free;
    > > + }
    > > + buffer[nbytes] = 0; /* nul-terminate */
    > > + strstrip(buffer); /* strip -just- trailing whitespace */
    > > }
    > > - return -EINVAL;
    > > -}
    >
    > I'm trying to work out what protects static_buffer?

    One of us must be having a brain cramp because it looks to me like the
    buffer doesn't need protection -- it's on the stack. It's probably me
    but I'm just not seeing how this use is unsafe..

    > Why does it need to be static anyway? 64 bytes on-stack is OK.

    Uh, it is on stack. It doesn't use the C keyword "static". It's just
    poorly-named.

    <snip>

    Cheers,
    -Matt Helsley



    \
     
     \ /
      Last update: 2008-05-13 22:41    [W:7.026 / U:0.764 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site