| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <200809120212.33716.j@jannau.net>
Date: Fri, 12 Sep 2008 02:12:33 +0200
From: Janne Grunau <j@...nau.net>
To: Jonathan Corbet <corbet@....net>
Cc: Jarod Wilson <jwilson@...hat.com>, linux-kernel@...r.kernel.org,
Jarod Wilson <jarod@...hat.com>,
Christoph Bartelmus <lirc@...telmus.de>,
Mario Limonciello <superm1@...ntu.com>
Subject: Re: [PATCH 01/18] lirc core device driver infrastructure
On Tuesday 09 September 2008 17:33:08 Jonathan Corbet wrote:
> I think it's most cool that this code is finally making its way
> toward the mainline. What I have is mostly nits...
ignoring bits already handled in Christoph Hellwig's review
> > + dprintk("lirc_dev [%d]: open result = -ENODEV\n",
> > + MINOR(inode->i_rdev));
> > + return -ENODEV;
> > + }
> > +
> > + ir = &irctls[MINOR(inode->i_rdev)];
> > +
> > + dprintk(LOGHEAD "open called\n", ir->p.name, ir->p.minor);
> > +
> > + /* if the plugin has an open function use it instead */
> > + if (ir->p.fops && ir->p.fops->open)
> > + return ir->p.fops->open(inode, file);
> > +
> > + if (mutex_lock_interruptible(&plugin_lock))
> > + return -ERESTARTSYS;
>
> So the plugin open() function is called outside of any lock. Note
> that open() no longer has BKL protection as of 2.6.27. This might
> all be OK, but I hope you're convinced of it.
plugins fops are called directly now
> > + if (ir->p.owner != NULL && try_module_get(ir->p.owner)) {
> > + ++ir->open;
> > + retval = ir->p.set_use_inc(ir->p.data);
>
> Why is there a set_use_inc() function separate from open()?
since not all plugins set p.fops? some of those functions in the drivers
are even empty. I'll look at it.
> > + if (retval != SUCCESS) {
> > + module_put(ir->p.owner);
> > + --ir->open;
> > + }
> > + } else {
> > + if (ir->p.owner == NULL)
> > + dprintk(LOGHEAD "no module owner!!!\n",
> > + ir->p.name, ir->p.minor);
> > +
> > + retval = -ENODEV;
> > + }
>
> If "no owner" is a fatal condition, it seems better to check it when
> the plugin is registered.
maybe because it's not needed if the plugin has its own fops.open?
> (Also, BTW, your variant of dprintk() is
> confusing to read - I was wondering where all the %'s were. I still
> wonder, actually. dev_printk() would be better.)
the LOGHEAD define is confusing, I'll look into make more clear
> > +static int irctl_close(struct inode *inode, struct file *file)
> > +{
> > + struct irctl *ir = &irctls[MINOR(inode->i_rdev)];
> > +
> > + dprintk(LOGHEAD "close called\n", ir->p.name, ir->p.minor);
> > +
> > + /* if the plugin has a close function use it instead */
> > + if (ir->p.fops && ir->p.fops->release)
> > + return ir->p.fops->release(inode, file);
> > +
> > + if (mutex_lock_interruptible(&plugin_lock))
> > + return -ERESTARTSYS;
>
> Should this be interruptible? You probably want the close call to
> get its job done. Maybe mutex_lock_killable() - and still do the
> cleanup on a signal?
That's better than the current state.
> > +static int irctl_ioctl(struct inode *inode, struct file *file,
> > + unsigned int cmd, unsigned long arg)
> > +{
> > + unsigned long mode;
> > + int result;
> > + struct irctl *ir = &irctls[MINOR(inode->i_rdev)];
> > +
> > + dprintk(LOGHEAD "ioctl called (0x%x)\n",
> > + ir->p.name, ir->p.minor, cmd);
> > +
> > + /* if the plugin has a ioctl function use it instead */
> > + if (ir->p.fops && ir->p.fops->ioctl)
> > + return ir->p.fops->ioctl(inode, file, cmd, arg);
> > +
> > + if (ir->p.minor == NOPLUG || !ir->attached) {
> > + dprintk(LOGHEAD "ioctl result = -ENODEV\n",
> > + ir->p.name, ir->p.minor);
> > + return -ENODEV;
> > + }
> > +
> > + /* Give the plugin a chance to handle the ioctl */
> > + if (ir->p.ioctl) {
> > + result = ir->p.ioctl(inode, file, cmd, arg);
> > + if (result != -ENOIOCTLCMD)
> > + return result;
> > + }
>
> Why two ioctl() handlers? It seems better to just have one way for
> plugins to handle this call.
after a cursory look this is used if the plugin doesn't reimplement or
want to overwrite the commands handled in irctl. I'll look if this can
be handled better.
> > + default:
> > + result = -ENOIOCTLCMD;
>
> Hmm, I note with interest that unlocked_ioctl() remaps -ENOIOCTLCMD
> to -EINVAL, while regular, locked ioctl() (which this is) does not.
> Not sure what to make of that.
changed this return value to -EINVAL
> > +static ssize_t irctl_read(struct file *file,
> > + char *buffer,
> > + size_t length,
> > + loff_t *ppos)
> > +{
>
> ...
>
> > + schedule();
> > + set_current_state(TASK_INTERRUPTIBLE);
> > + if (!ir->attached) {
> > + ret = -ENODEV;
> > + break;
> > + }
>
> How can ir->attached go to zero? You checked it earlier and have
> been holding the mutex ever since.
I don't think the lock has to be held while sleeping
> > +static ssize_t irctl_write(struct file *file, const char *buffer,
> > + size_t length, loff_t *ppos)
> > +{
> > + struct irctl *ir =
> > &irctls[MINOR(file->f_dentry->d_inode->i_rdev)];
> > + dprintk(LOGHEAD "write called\n", ir->p.name, ir->p.minor);
> > +
> > + /* if the plugin has a specific read function use it instead
> > */
> > + if (ir->p.fops && ir->p.fops->write)
> > + return ir->p.fops->write(file, buffer, length, ppos);
>
> Looks like you're using the "specific write function" instead :)
plugins fops will be used directly now, no need for this comment and
code anymore ;)
> > +static struct file_operations fops = {
> > + .read = irctl_read,
> > + .write = irctl_write,
> > + .poll = irctl_poll,
> > + .ioctl = irctl_ioctl,
> > + .open = irctl_open,
> > + .release = irctl_close
> > +};
>
> You should probably set .owner too.
done
> > +static int lirc_dev_init(void)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < MAX_IRCTL_DEVICES; ++i)
> > + init_irctl(&irctls[i]);
> > +
> > + if (register_chrdev(IRCTL_DEV_MAJOR, IRCTL_DEV_NAME, &fops))
> > {
> > + printk(KERN_ERR "lirc_dev: register_chrdev
> > failed\n");
> > + goto out;
> > + }
> > +
> > + lirc_class = class_create(THIS_MODULE, "lirc");
> > + if (IS_ERR(lirc_class)) {
> > + printk(KERN_ERR "lirc_dev: class_create failed\n");
> > + goto out_unregister;
> > + }
> > +
> > + printk(KERN_INFO "lirc_dev: IR Remote Control driver
> > registered, "
> > + "major %d \n", IRCTL_DEV_MAJOR);
> > +
> > + return SUCCESS;
> > +
> > +out_unregister:
> > + /* unregister_chrdev returns void now */
> > + unregister_chrdev(IRCTL_DEV_MAJOR, IRCTL_DEV_NAME);
> > +out:
> > + return -1;
> > +}
>
> Do you want to fail completely if class_create() fails? What if
> somebody already opened one of your devices?
order reversed and using alloc_chrdev_region so there is no device which
could be opened. Also returning correct error values now.
Thanks for the review.
Janne
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists