On Mon, 23 Jun 2003, Oliver Neukum wrote:
>> >> If usb_disconnect() can happen while already executing in open(), and if
>> >> the former frees the data structures used by the driver, then the latter
>> >> call might continue working with already freed memory region which could
>> >> lead to catastrophe.
>Exactly. Very good. Therefore you must never free [in usb_disconnect() -tt]
>the lock and that piece of data (usually a pointer to a device descriptor)
Thank you very much. These were the magic words I wanted to hear. So the
solution is to use _static_ lock.
>Many examples can be found in the kernel sources.
Right. So let's look at drivers/usb/ov511.c (2.4.21).
--<clip>--
ov51x_v4l1_open(struct inode *inode, struct file *file)
{
struct video_device *vdev = video_devdata(file);
struct usb_ov511 *ov = vdev->priv;
int err, i;
PDEBUG(4, "opening");
down(&ov->lock);
--<clip>--
ov->lock is taken unconditionally, thus ov _must not freed in
usb_disconnect()_! And what do we see in usb_disconnect?
--<clip>--
ov51x_disconnect(struct usb_device *dev, void *ptr)
{
struct usb_ov511 *ov = (struct usb_ov511 *) ptr;
int n;
MOD_INC_USE_COUNT;
video_unregister_device(&ov->vdev);
/* Free the memory */
if (ov && !ov->user) {
down(&ov->cbuf_lock);
kfree(ov->cbuf);
ov->cbuf = NULL;
up(&ov->cbuf_lock);
ov51x_dealloc(ov, 1);
kfree(ov);
ov = NULL;
}
MOD_DEC_USE_COUNT;
}
--<clip>--
I deleted some (hopefully) irrelevant lines to make the example smaller
(so you may want to look at the original code). Looks exactly the same bug
that in the driver I'm maintaining (qc-usb) which isn't surpising
considering they are probably derived from same original code (CPiA?).
Another danger sign: video_unregister_device() is potentially called while
executing the open() function called from videodev.c. I don't know if this
is allowed (I should go asking from V4L list, I guess).
ov51 was actually the second driver I looked, the first one was the Philips
driver pwc. There I see:
--<clip>--
static void usb_pwc_disconnect(struct usb_device *udev, void *ptr)
{
struct pwc_device *pdev;
int hint;
DECLARE_WAITQUEUE(wait, current);
lock_kernel();
free_mem_leak();
--<clip>--
Does taking lock_kernel() prevent usb_disconnect() executing simultaneously
with the open() call (the code doesn't specifically take lock_kernel() in
open() call, but maybe kernel does that automatically)? If so, pwc should
be safe. But I don't know.
>> And wasn't it Linus not God... oh nevermind. I just remembered the
>> principle of equivalence ;)
>Blessed be he and all code that floweth from him.
Hallelujah!
-------------------------------------------------------
This SF.Net email is sponsored by: INetU
Attention Web Developers & Consultants: Become An INetU Hosting Partner.
Refer Dedicated Servers. We Manage Them. You Get 10% Monthly Commission!
INetU Dedicated Managed Hosting http://www.inetu.net/partner/index.php
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
