Re: [PATCH] CKRM [4/8] aFull directory support for rcfs

[Chris Wright]

* Gerrit Huizenga (gh@xxxxxxxxxx) wrote:
> +int rcfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
> +{
> +
> +	int retval = 0;
> +	struct ckrm_classtype *clstype;
> +
> +#if 0
> +	struct dentry *pd = list_entry(dir->i_dentry.next, struct dentry,
> +				       d_alias);
> +	if ((!strcmp(pd->d_name.name, "/") &&
> +	     !strcmp(dentry->d_name.name, "ce"))) {
> +		/* Call CE's mkdir if it has registered, else fail. */
> +		if (rcfs_eng_callbacks.mkdir) {
> +			return (*rcfs_eng_callbacks.mkdir) (dir, dentry, mode);
> +		} else {
> +			return -EINVAL;
> +		}
> +	}
> +#endif

This #if 0 should simply go, esp since it's broken w.r.t. namespaces
(i.e. mount --bind).

> +int rcfs_rmdir(struct inode *dir, struct dentry *dentry)
> +{
> +	struct rcfs_inode_info *ri = rcfs_get_inode_info(dentry->d_inode);
> +
> +#if 0
> +	struct dentry *pd = list_entry(dir->i_dentry.next,
> +				       struct dentry, d_alias);
> +	if ((!strcmp(pd->d_name.name, "/") &&
> +	     !strcmp(dentry->d_name.name, "ce"))) {
> +		/* Call CE's mkdir if it has registered, else fail. */
> +		if (rcfs_eng_callbacks.rmdir) {
> +			return (*rcfs_eng_callbacks.rmdir) (dir, dentry);
> +		} else {
> +			return simple_rmdir(dir, dentry);
> +		}
> +	} else if ((!strcmp(pd->d_name.name, "/") &&
> +		    !strcmp(dentry->d_name.name, "network"))) {
> +		return -EPERM;
> +	}
> +#endif

ditto

> +	if (!rcfs_empty(dentry)) {
> +		printk(KERN_ERR "rcfs_rmdir: directory not empty\n");
> +		return -ENOTEMPTY;
> +	}
> +	/* Core class removal  */
> +
> +	if (ri->core == NULL) {
> +		printk(KERN_ERR "rcfs_rmdir: core==NULL\n");
> +		/* likely a race condition */

If this is a concern, and isn't already sufficiently serialized by
vfs grabbing i_sem, then it should be fixed and handled internally,
not with printk.

> +int _rcfs_mknod(struct inode *dir, struct dentry *dentry, int mode, dev_t dev)
> +EXPORT_SYMBOL_GPL(_rcfs_mknod);
> +
> +int rcfs_mknod(struct inode *dir, struct dentry *dentry, int mode, dev_t dev)
> +
> +EXPORT_SYMBOL_GPL(rcfs_mknod);

OK, that's just not right ;-)

> +struct dentry *rcfs_create_internal(struct dentry *parent,
> +				    struct rcfs_magf *magf, int magic)
> +{
> +	struct qstr qstr;
> +	struct dentry *mfdentry;
> +
> +	/* Get new dentry for name */
> +	qstr.name = magf->name;
> +	qstr.len = strlen(magf->name);
> +	qstr.hash = full_name_hash(magf->name, qstr.len);
> +	mfdentry = lookup_hash(&qstr, parent);
> +
> +	if (!IS_ERR(mfdentry)) {
> +		int err;
> +
> +		down(&parent->d_inode->i_sem);
> +		if (magic && (magf->mode & S_IFDIR))
> +			err = parent->d_inode->i_op->mkdir(parent->d_inode,
> +							   mfdentry,
> +							   magf->mode);
> +		else {
> +			err = _rcfs_mknod(parent->d_inode, mfdentry,
> +					  magf->mode, 0);
> +			/*
> +			 * _rcfs_mknod doesn't increment parent's link count, 
> +			 * i_op->mkdir does.
> +			 */
> +			parent->d_inode->i_nlink++;
> +		}
> +		up(&parent->d_inode->i_sem);
> +		if (err) {
> +			dput(mfdentry);
> +			return mfdentry;

Error is lost, mfdentry dput yet returned...that looks broken.

> +static ssize_t
> +magic_write(struct file *file, const char __user *buf,
> +			   size_t count, loff_t *ppos)
> +{
> +	struct rcfs_inode_info *ri = 
> +		rcfs_get_inode_info(file->f_dentry->d_parent->d_inode);
> +	char *optbuf, *otherstr=NULL, *resname=NULL;
> +	int done, rc = 0;
> +	struct ckrm_core_class *core ;
> +	int (*func) (struct ckrm_core_class *, const char *,
> +			const char *) = NULL;
> +
> +	core = ri->core;
> +	if (!ckrm_is_core_valid(core))
> +		return -EINVAL;
> +
> +	if (count > MAX_INPUT_SIZE)
> +		return -EINVAL;
> +
> +	if (!access_ok(VERIFY_READ, buf, count))
> +		return -EFAULT;
> +
> +	down(&(ri->vfs_inode.i_sem));
> +
> +	optbuf = kmalloc(MAX_INPUT_SIZE+1, GFP_KERNEL);
> +	if (!optbuf) {
> +		up(&(ri->vfs_inode.i_sem));
> +		return -ENOMEM;
> +	}
> +	__copy_from_user(optbuf, buf, count);

Just use copy_from_user (w/out access_ok), and kmalloc outside of any
semaphore.  Why not use count to size the buffer (since it's size has
been validated?

> +	if (optbuf[count-1] == '\n')
> +		optbuf[count-1]='\0';
> +
> +	done = magic_parse(ri->mfdentry->d_name.name,
> +			optbuf, &resname, &otherstr);
> +
> +	if (!done) {
> +		printk(KERN_ERR "Error parsing data written to %s\n",
> +				ri->mfdentry->d_name.name);
> +		goto out;
> +	}
> +
> +	if (!strcmp(ri->mfdentry->d_name.name, RCFS_CONFIG_NAME)) {
> +		func = core->classtype->set_config;
> +	} else if (!strcmp(ri->mfdentry->d_name.name, RCFS_STATS_NAME)) {
> +		func = core->classtype->reset_stats;
> +	}
> +
> +	if (func) {
> +		rc = func(core, resname, otherstr);
> +		if (rc) {
> +			printk(KERN_ERR "magic_write: %s: error\n",
> +				ri->mfdentry->d_name.name);
> +		}
> +	}
> +
> +out:
> +	up(&(ri->vfs_inode.i_sem));
> +	kfree(optbuf);
> +	kfree(otherstr);
> +	kfree(resname);
> +	return rc ? rc : count;
> +}
> +
> +/*
> + * Shared function used by Target / Reclassify
> + */
> +
> +static ssize_t
> +target_reclassify_write(struct file *file, const char __user * buf,
> +			size_t count, loff_t * ppos, int manual)
> +{
> +	struct rcfs_inode_info *ri = rcfs_get_inode_info(file->f_dentry->d_inode);
> +	char *optbuf;
> +	int rc = -EINVAL;
> +	struct ckrm_classtype *clstype;
> +
> +	if (count > MAX_INPUT_SIZE)
> +		return -EINVAL;
> +	if (!access_ok(VERIFY_READ, buf, count))
> +		return -EFAULT;
> +	down(&(ri->vfs_inode.i_sem));
> +	optbuf = kmalloc(MAX_INPUT_SIZE, GFP_KERNEL);
> +	__copy_from_user(optbuf, buf, count);

Same here, and check for kmalloc failing.

> +	if (optbuf[count - 1] == '\n')
> +		optbuf[count - 1] = '\0';
> +	clstype = ri->core->classtype;
> +	if (clstype->forced_reclassify)
> +		rc = (*clstype->forced_reclassify) (manual ? ri->core: NULL, optbuf);
> +	up(&(ri->vfs_inode.i_sem));
> +	kfree(optbuf);
> +	return (!rc ? count : rc);
> +
> +}
[snip]
> +static ssize_t
> +shares_write(struct file *file, const char __user * buf,
> +	     size_t count, loff_t * ppos)
> +{
> +	struct inode *inode = file->f_dentry->d_inode;
> +	struct rcfs_inode_info *ri;
> +	char *optbuf;
> +	int rc = 0;
> +	struct ckrm_core_class *core;
> +	int done;
> +	char *resname = NULL;
> +
> +	struct ckrm_shares newshares = {
> +		CKRM_SHARE_UNCHANGED,
> +		CKRM_SHARE_UNCHANGED,
> +		CKRM_SHARE_UNCHANGED,
> +		CKRM_SHARE_UNCHANGED,
> +		CKRM_SHARE_UNCHANGED,
> +		CKRM_SHARE_UNCHANGED
> +	};
> +	if (count > SHARES_MAX_INPUT_SIZE)
> +		return -EINVAL;
> +	if (!access_ok(VERIFY_READ, buf, count))
> +		return -EFAULT;
> +	ri = rcfs_get_inode_info(file->f_dentry->d_parent->d_inode);
> +	if (!ri || !ckrm_is_core_valid((struct ckrm_core_class *) (ri->core))) {
> +		printk(KERN_ERR "shares_write: Error accessing core class\n");
> +		return -EFAULT;
> +	}
> +	down(&inode->i_sem);
> +	core = ri->core;
> +	optbuf = kmalloc(SHARES_MAX_INPUT_SIZE, GFP_KERNEL);
> +	if (!optbuf) {
> +		up(&inode->i_sem);
> +		return -ENOMEM;
> +	}
> +	__copy_from_user(optbuf, buf, count);

Same here.

> +	if (optbuf[count - 1] == '\n')
> +		optbuf[count - 1] = '\0';
> +	done = shares_parse(optbuf, &resname, &newshares);
> +	if (!done) {
> +		printk(KERN_ERR "Error parsing shares\n");
> +		rc = -EINVAL;
> +		goto write_out;
> +	}
> +	if (core->classtype->set_shares) {
> +		rc = (*core->classtype->set_shares) (core, resname, &newshares);
> +		if (rc) {
> +			printk(KERN_ERR
> +			       "shares_write: resctlr share set error\n");
> +			goto write_out;
> +		}
> +	}
> +	printk(KERN_ERR "Set %s shares to %d %d %d %d\n",
> +	       resname,
> +	       newshares.my_guarantee,
> +	       newshares.my_limit,
> +	       newshares.total_guarantee, newshares.max_limit);

This is pretty noisy, looks like it's really debugging, right?

> +	rc = count;
> +
> +write_out:
> +	up(&inode->i_sem);
> +	kfree(optbuf);
> +	kfree(resname);
> +	return rc;
> +}

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/