| 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: <200809221747.57170.jwilson@redhat.com>
Date: Mon, 22 Sep 2008 17:47:56 -0400
From: Jarod Wilson <jwilson@...hat.com>
To: Jonathan Corbet <corbet@....net>
Cc: linux-kernel@...r.kernel.org, Janne Grunau <j@...nau.net>,
Christoph Bartelmus <lirc@...telmus.de>
Subject: Re: [PATCH 08/18] lirc driver for the Soundgraph IMON IR Receivers
On Wednesday 10 September 2008 17:02:29 Jonathan Corbet wrote:
> > +#define SUCCESS 0
> > +#define TRUE 1
> > +#define FALSE 0
>
> (See my grumble in previous reviews...:)
>
> > +#define LOCK_CONTEXT mutex_lock(&context->lock)
> > +#define UNLOCK_CONTEXT mutex_unlock(&context->lock)
>
> Here too.
All gone.
> > +/* to prevent races between open() and disconnect() */
> > +static DECLARE_MUTEX(disconnect_sem);
>
> This should be a real mutex, I think.
Done.
> > +/* lcd or vfd? */
> > +static int is_lcd;
>
> The driver can only do one or the other? You can't have both in the
> system? And somehow the user is supposed to configure it at load time to
> do the right thing?
Should be autodetected by default now, based on usb device ID, but can still
be overridden by a module para 'display_type'. Not sure exactly what would
happen if the user had more than one, but I don't know why they would in the
first place...
> > +static inline int send_packet(struct imon_context *context)
[...]
> > + /* fill request into kmalloc'ed space: */
> > + control_req = kmalloc(sizeof(struct usb_ctrlrequest), GFP_NOIO);
>
> Why GFP_NOIO?
Original author probably thought it was necessary. Certainly doesn't look like
it is to me, I'll make that just GFP_KERNEL.
> > + } else {
> > + /* Wait for tranmission to complete(or abort) */
> > + UNLOCK_CONTEXT;
> > + wait_for_completion(&context->tx.finished);
> > + LOCK_CONTEXT;
>
> Should that be an interruptible (or killable) wait?
Don't see why it couldn't be. Done.
> > +static ssize_t show_associate_remote(struct device *d,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct imon_context *context = dev_get_drvdata(d);
> > +
> > + if (!context)
> > + return -ENODEV;
> > +
> > + if (context->ir_isassociating) {
> > + strcpy(buf, "The device it associating press some button "
> > + "on the remote.\n");
> > + } else if (context->ir_isopen) {
> > + strcpy(buf, "Device is open and ready to associate.\n"
> > + "Echo something into this file to start "
> > + "the process.\n");
> > + } else {
> > + strcpy(buf, "Device is closed, you need to open it to "
> > + "associate the remote(you can use irw).\n");
> > + }
> > + return strlen(buf);
> > +}
>
> I *guess* that's one value per file, but it's still not quite the sysfs
> norm. How about one-word status codes which can be made more verbose by
> user space? (That's an API change, of course...)
Yuk. There really is no user space. But chances of someone stumbling upon
these directions in sysfs are pretty slim. Reducing to single-word, and
logging a url pointer to the lirc.org page on setting these up. Sound sane
enough?
> > +static ssize_t store_associate_remote(struct device *d,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct imon_context *context;
> > +
> > + context = dev_get_drvdata(d);
> > +
> > + if (!context)
> > + return -ENODEV;
> > +
> > + if (!context->ir_isopen)
> > + return -EINVAL;
> > +
> > + if (context->ir_isopen) {
> > + context->ir_isassociating = TRUE;
> > + send_associate_24g(context);
> > + }
> > +
> > + return count;
> > +}
>
> No need to take the mutex here?
Added mutex here and in show_associate_remote() for good measure.
> > +/**
> > + * Called by lirc_dev when the application opens /dev/lirc
> > + */
> > +static int ir_open(void *data)
> > +{
> > + int retval = SUCCESS;
> > + struct imon_context *context;
> > +
> > + /* prevent races with disconnect */
> > + down(&disconnect_sem);
> > +
> > + context = (struct imon_context *) data;
> > +
> > + LOCK_CONTEXT;
> > +
> > + if (context->ir_isopen) {
> > + err("%s: IR port is already open", __func__);
> > + retval = -EBUSY;
> > + goto exit;
> > + }
>
> I wonder if the single-open semantics are really doing what the author
> intended? It is unsufficient to prevent concurrent calls elsewhere.
I think we're good to go with disconnect_sem being converted to an actual
mutex, no?
> > +exit:
> > + UNLOCK_CONTEXT;
> > +
> > + up(&disconnect_sem);
> > + return SUCCESS;
> > +}
>
> This discards "retval", which could hold an error status - the function
> returns "SUCCESS" even if ->ir_isopen is not set.
Gah. That's fugly. Fixed.
> > +static void imon_disconnect(struct usb_interface *interface)
[...]
> > + /* Abort ongoing write */
> > + if (atomic_read(&context->tx.busy)) {
> > + usb_kill_urb(context->tx_urb);
> > + wait_for_completion(&context->tx.finished);
> > + }
>
> What if this races with another thread waiting for the completion? It
> seems like it should be completed with complete_all(), but that wasn't the
> case.
Agreed. At this point, we're disconnecting, no point in waiting on anyone
else.
I've attempted to remedy everything, compile-tested and briefly run-time
tested the changes, and have pushed 'em all into my git tree. My shiny new
iMon Knob receiver and remote are reasonably happy still (less some lockdep
spew that I have to figure out what to do with still), but I have no vfd or
lcd to test with, unfortunately.
--
Jarod Wilson
jarod@...hat.com
--
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