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?
> > +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?
And I see these haven't been addressed yet either.
> > + 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
gerrit
-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click
_______________________________________________
ckrm-tech mailing list
https://lists.sourceforge.net/lists/listinfo/ckrm-tech
