| 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
| ||
|
Date: Tue, 9 Sep 2008 09:33:08 -0600
From: Jonathan Corbet <corbet@....net>
To: Jarod Wilson <jwilson@...hat.com>
Cc: linux-kernel@...r.kernel.org, Jarod Wilson <jwilson@...hat.com>,
Jarod Wilson <jarod@...hat.com>, Janne Grunau <j@...nau.net>,
Christoph Bartelmus <lirc@...telmus.de>,
Mario Limonciello <superm1@...ntu.com>
Subject: Re: [PATCH 01/18] lirc core device driver infrastructure
I think it's most cool that this code is finally making its way toward
the mainline. What I have is mostly nits...
> +int lirc_unregister_plugin(int minor)
> +{
...
> + /* end up polling thread */
> + if (ir->task) {
> + wake_up_process(ir->task);
> + kthread_stop(ir->task);
> + }
kthread_stop() will wake up the process, so there is no need to do it
separately here. In fact, it almost looks like the separate
wake_up_process() call could create an (unlikely) race where the thread
would miss the fact that it's supposed to stop and sleep again. Fixing the
sleep_on() call there to use something like wait_event() would help in that
regard.
> +static int irctl_open(struct inode *inode, struct file *file)
> +{
> + struct irctl *ir;
> + int retval;
> +
> + if (MINOR(inode->i_rdev) >= MAX_IRCTL_DEVICES) {
All of these might be better as iminor(inode)
> + 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.
...
> + 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()?
> + 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. (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.)
> +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?
> +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.
...
> + 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.
> +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.
> +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 :)
> +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.
> +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?
> +static inline int lirc_buffer_init(struct lirc_buffer *buf,
> + unsigned int chunk_size,
> + unsigned int size)
> +{
> + /* Adjusting size to the next power of 2 would allow for
> + * inconditional LIRC_BUFF_POWER_OF_2 optimization */
> + init_waitqueue_head(&buf->wait_poll);
> + spin_lock_init(&buf->lock);
> + _lirc_buffer_clear(buf);
> + buf->chunk_size = chunk_size;
> + buf->size = size;
> + buf->data = kmalloc(size*chunk_size, GFP_KERNEL);
> + if (buf->data == NULL)
> + return -1;
> + memset(buf->data, 0, size*chunk_size);
> + return 0;
> +}
All of these inlines probably shouldn't be.
jon
--
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