Gerrit,
Here are two patches to address Chris' comments. I've done minimal testing (the functional tests). The patches apply on top of the remaining ones I'd sent earlier (though there should be no dependency).
We will need one more patch for fs/socket_fs.c to deal with the possibility of rcfs_create_internal failing. Right now, it isn't doing any error handling. I'll send that patch asap after consulting with Vivek.
Thanks, Shailabh
Gerrit Huizenga wrote:
Shailabh, I know you addressed some of Chris's comments in the patch you sent me either intentionally or otherwise as those are removed. ;) However, there were some yet unaddressed that do look wrong to me. Can you comment on these or send me patches to clean these up?
I think these are the last remaining unaddressed comments I have.
(Chris, sorry in taking so long on this - we've been focused on too many things in this space lately, trying to get our ducks in order).
On Thu, 24 Feb 2005 10:18:12 PST, Chris Wright wrote:
* Gerrit Huizenga ([EMAIL PROTECTED]) wrote:
+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.
This looks to be a problem. Why is it returning mfdentry on error?
Changed to NULL return. All users of this function except fs/socket_fs.c: rcfs_sock_mkdir have been verified/modified to handle this return value.
+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?
Did all the suggested changes.
+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.
Done.
+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.
Done.
+ 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?
Yes, removed the printk.
Index: ckrm-2611rc5/fs/rcfs/magic.c
===================================================================
--- ckrm-2611rc5.orig/fs/rcfs/magic.c 2005-03-07 14:49:19.000000000 -0500
+++ ckrm-2611rc5/fs/rcfs/magic.c 2005-03-18 16:50:58.313766048 -0500
@@ -165,27 +165,24 @@ magic_write(struct file *file, const cha
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));
+ optbuf = kmalloc(count+1, GFP_KERNEL);
+ if (!optbuf)
return -ENOMEM;
- }
- __copy_from_user(optbuf, buf, count);
+
+ rc = copy_from_user(optbuf, buf, count);
+ if (rc)
+ goto out_optbuf;
mkvalidstr(optbuf);
+
+ down(&(ri->vfs_inode.i_sem));
done = magic_parse(ri->mfdentry->d_name.name,
- optbuf, &resname, &otherstr);
+ optbuf, &resname, &otherstr);
if (!done) {
printk(KERN_ERR "Error parsing data written to %s\n",
- ri->mfdentry->d_name.name);
+ ri->mfdentry->d_name.name);
goto out;
}
if (!strcmp(ri->mfdentry->d_name.name, RCFS_CONFIG_NAME)) {
@@ -197,14 +194,15 @@ magic_write(struct file *file, const cha
rc = func(core, resname, otherstr);
if (rc) {
printk(KERN_ERR "magic_write: %s: error\n",
- ri->mfdentry->d_name.name);
+ ri->mfdentry->d_name.name);
}
}
out:
up(&(ri->vfs_inode.i_sem));
- kfree(optbuf);
kfree(otherstr);
kfree(resname);
+out_optbuf:
+ kfree(optbuf);
return rc ? rc : count;
}
@@ -223,16 +221,22 @@ target_reclassify_write(struct file *fil
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);
+
+ optbuf = kmalloc(count+1, GFP_KERNEL);
+ if (!optbuf)
+ return -ENOMEM;
+
+ rc = copy_from_user(optbuf, buf, count);
+ if (rc)
+ goto out_optbuf;
mkvalidstr(optbuf);
+
+ down(&(ri->vfs_inode.i_sem));
clstype = ri->core->classtype;
if (clstype->forced_reclassify)
rc = (*clstype->forced_reclassify) (manual ? ri->core: NULL,
optbuf);
up(&(ri->vfs_inode.i_sem));
+out_optbuf:
kfree(optbuf);
return (!rc ? count : rc);
@@ -426,40 +430,40 @@ shares_write(struct file *file, const ch
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);
+ if (!optbuf)
return -ENOMEM;
- }
- __copy_from_user(optbuf, buf, count);
+
+ rc = copy_from_user(optbuf, buf, count);
+ if (rc)
+ goto out_optbuf;
mkvalidstr(optbuf);
+
+
+ down(&inode->i_sem);
+ core = ri->core;
done = shares_parse(optbuf, &resname, &newshares);
if (!done) {
printk(KERN_ERR "Error parsing shares\n");
rc = -EINVAL;
- goto write_out;
+ goto 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;
+ goto 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);
rc = count;
-write_out:
+out:
up(&inode->i_sem);
- kfree(optbuf);
kfree(resname);
+out_optbuf:
+ kfree(optbuf);
return rc;
}
Index: ckrm-2611rc5/fs/rcfs/inode.c
===================================================================
--- ckrm-2611rc5.orig/fs/rcfs/inode.c 2005-03-07 14:49:19.000000000 -0500
+++ ckrm-2611rc5/fs/rcfs/inode.c 2005-03-18 17:23:42.331190016 -0500
@@ -128,7 +128,7 @@ struct dentry *rcfs_create_internal(stru
up(&parent->d_inode->i_sem);
if (err) {
dput(mfdentry);
- return mfdentry;
+ return NULL;
}
}
return mfdentry;
Index: ckrm-2611rc5/fs/rcfs/magic.c
===================================================================
--- ckrm-2611rc5.orig/fs/rcfs/magic.c 2005-03-18 16:50:58.313766048 -0500
+++ ckrm-2611rc5/fs/rcfs/magic.c 2005-03-18 17:38:07.359685696 -0500
@@ -502,7 +502,7 @@ int rcfs_create_magic(struct dentry *par
for (i = 0; i < count; i++) {
mfdentry = rcfs_create_internal(parent, &magf[i], 0);
- if (IS_ERR(mfdentry)) {
+ if (!mfdentry) {
rcfs_clear_magic(parent);
return -ENOMEM;
}
