| 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:01:02 -0400
From: Christoph Hellwig <hch@...radead.org>
To: Jarod Wilson <jwilson@...hat.com>
Cc: linux-kernel@...r.kernel.org, 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
> +menuconfig INPUT_LIRC
> + bool "Linux Infrared Remote Control IR receiver/transmitter drivers"
> + default n
n is the default, no needed add a "default n" line for it.
Anyone configuring the kernel should know he's on Linux, so strike the
Linux away :)
> +if INPUT_LIRC
> +
> +config LIRC_DEV
> + tristate "LIRC device loadable module support"
> + default n
> + help
> + LIRC device loadable module support, required for most LIRC drivers
Obviously this can be built in, so it should be named better. Also I
don't think LIRC means anything to a user, so use Infrared Remote
Control support or something similar instead.
I don't quite understand why this needs a different option from
INPUT_LIRC.
> +#define __KERNEL_SYSCALLS__
No need for this in any semi-recent kernel.
> +#include <linux/unistd.h>
> +#include <linux/kthread.h>
> +
> +/* SysFS header */
> +#include <linux/device.h>
That comment is not quite correct, just remove it.
> +/* helper function
> + * initializes the irctl structure
> + */
> +static inline void init_irctl(struct irctl *ir)
> +{
> + memset(&ir->p, 0, sizeof(struct lirc_plugin));
> + mutex_init(&ir->buffer_lock);
> + ir->p.minor = NOPLUG;
> +
> + ir->task = NULL;
> + ir->jiffies_to_wait = 0;
> +
> + ir->open = 0;
> + ir->attached = 0;
> +}
Please don't mark funtion inline unless there's a very good reason for
it.
> +static void cleanup(struct irctl *ir)
> +{
> + dprintk(LOGHEAD "cleaning up\n", ir->p.name, ir->p.minor);
> +
> + device_destroy(lirc_class, MKDEV(IRCTL_DEV_MAJOR, ir->p.minor));
> +
> + if (ir->buf != ir->p.rbuf) {
> + lirc_buffer_free(ir->buf);
> + kfree(ir->buf);
> + }
> + ir->buf = NULL;
> +
> + init_irctl(ir);
> +}
What's the init doing in a cleanup routine? Oh, you initialize it again
becaus of the static array. I think the right approach is to
dynamically allocate struct irctl.
> +static inline int add_to_buf(struct irctl *ir)
The inline thing also applies here and in various other placez.
> +{
> + if (lirc_buffer_full(ir->buf)) {
> + dprintk(LOGHEAD "buffer overflow\n",
> + ir->p.name, ir->p.minor);
> + return -EOVERFLOW;
> + }
> +
> + if (ir->p.add_to_buf) {
> + int res = -ENODATA;
> + int got_data = 0;
> +
> + /* service the device as long as it is returning
> + * data and we have space
> + */
> + while (!lirc_buffer_full(ir->buf)) {
> + res = ir->p.add_to_buf(ir->p.data, ir->buf);
> + if (res == SUCCESS)
> + got_data++;
> + else
> + break;
> + }
> +
> + if (res == -ENODEV)
> + kthread_stop(ir->task);
> +
> + return got_data ? SUCCESS : res;
> + }
> +
> + return SUCCESS;
I guess success is a #define for 0? Just user 0 directly. Also the
kthread_stop here looks odd. The normal way to use kthreads is to
start them when bringing up an interface of some sorts, and call
kthread_stop when the interface is brought down. Doing it in a routine
like this screams "unclear lifetime rules".
> + do {
> + if (ir->open) {
> + if (ir->jiffies_to_wait) {
> + set_current_state(TASK_INTERRUPTIBLE);
> + schedule_timeout(ir->jiffies_to_wait);
> + } else {
> + interruptible_sleep_on(
> + ir->p.get_queue(ir->p.data));
> + }
No sleep on, please. But this one should be trivial to fix anyway, by
just changing this to a
set_current_state(TASK_INTERRUPTIBLE);
schedule();
and using wake_up_process in the wakeup path.
> + } else {
> + /* if device not opened so we can sleep half a second */
> + set_current_state(TASK_INTERRUPTIBLE);
> + schedule_timeout(HZ/2);
> + }
Yikes. This should use some form of more fine-grained wakeus.
> +int lirc_register_plugin(struct lirc_plugin *p)
> +{
Please add a kerneldoc comments for exported functions like this,
instead of the comments in the header.
> + struct irctl *ir;
> + int minor;
> + int bytes_in_key;
> + int err;
> + DECLARE_COMPLETION(tn);
> +
> + if (!p) {
> + printk(KERN_ERR "lirc_dev: lirc_register_plugin: "
> + "plugin pointer must be not NULL!\n");
> + err = -EBADRQC;
> + goto out;
> + }
No need for this, a null pointer derference should be a clear enough
warning for the writer of the broken pluging..
> +int lirc_unregister_plugin(int minor)
Why doesn't this one take a struct lirc_plugin pointer?
> +{
> + struct irctl *ir;
> + DECLARE_COMPLETION(tn);
> + DECLARE_COMPLETION(tn2);
both completion seems unused.
> + /* end up polling thread */
> + if (ir->task) {
> + wake_up_process(ir->task);
> + kthread_stop(ir->task);
> + }
kthread_stop already wakes the thread up.
> +/*
> + * Recent kernels should handle this autmatically by increasing/decreasing
> + * use count when a dependant module is loaded/unloaded.
> + */
> +
> + return SUCCESS;
The comment above looks superflous.
> +static int irctl_open(struct inode *inode, struct file *file)
> +{
> + struct irctl *ir;
> + int retval;
> +
> + if (MINOR(inode->i_rdev) >= MAX_IRCTL_DEVICES) {
iminor.
> + /* 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);
in which case this 'plugin' should just install it's own fops. Thanks
to cdev_add we can install fops at minor number granularity.
> +static unsigned int irctl_poll(struct file *file, poll_table *wait)
> +{
> + struct irctl *ir = &irctls[MINOR(file->f_dentry->d_inode->i_rdev)];
> + unsigned int ret;
> +
> + dprintk(LOGHEAD "poll called\n", ir->p.name, ir->p.minor);
> +
> + /* if the plugin has a poll function use it instead */
> + if (ir->p.fops && ir->p.fops->poll)
> + return ir->p.fops->poll(file, wait);
> +
> + mutex_lock(&ir->buffer_lock);
->poll ust not sleep.
> +/*
> + *
> + */
???
> +
> +void *lirc_get_pdata(struct file *file)
> +{
> + void *data = NULL;
> +
> + if (file && file->f_dentry && file->f_dentry->d_inode &&
> + file->f_dentry->d_inode->i_rdev) {
> + struct irctl *ir;
> + ir = &irctls[MINOR(file->f_dentry->d_inode->i_rdev)];
> + data = ir->p.data;
> + }
> +
> + return data;
> +}
> +EXPORT_SYMBOL(lirc_get_pdata);
Yikes. All these should never really be null. But once you have a
struct file you should just use file->private_data.
> +/* #define LIRC_BUFF_POWER_OF_2 */
> +#ifdef LIRC_BUFF_POWER_OF_2
> +#define mod(n, div) ((n) & ((div) - 1))
> +#else
> +#define mod(n, div) ((n) % (div))
> +#endif
??
> +static inline void _lirc_buffer_clear(struct lirc_buffer *buf)
> +{
> + buf->head = 0;
> + buf->tail = 0;
> + buf->fill = 0;
> +}
> +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;
> +}
> +static inline void lirc_buffer_free(struct lirc_buffer *buf)
> +{
> + kfree(buf->data);
> + buf->data = NULL;
> + buf->head = 0;
> + buf->tail = 0;
> + buf->fill = 0;
> + buf->chunk_size = 0;
> + buf->size = 0;
> +}
Please move these out of line. And please document all the functions.
Or switch to a kfifo or the existing tty buffering helpers.
> +static inline void lirc_buffer_lock(struct lirc_buffer *buf,
> + unsigned long *flags)
> +{
> + spin_lock_irqsave(&buf->lock, *flags);
> +}
> +static inline void lirc_buffer_unlock(struct lirc_buffer *buf,
> + unsigned long *flags)
> +{
> + spin_unlock_irqrestore(&buf->lock, *flags);
> +}
Please don't do you own spinlock wrappers.
--
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