Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754479Ab2FFNXH (ORCPT ); Wed, 6 Jun 2012 09:23:07 -0400 Received: from www84.your-server.de ([213.133.104.84]:48830 "EHLO www84.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752178Ab2FFNXE (ORCPT ); Wed, 6 Jun 2012 09:23:04 -0400 From: stefani@seibold.net To: linux-kernel@vger.kernel.org, gregkh@linuxfoundation.org, oneukum@suse.de Cc: alan@lxorguk.ukuu.org.uk, linux-usb@vger.kernel.org, Stefani Seibold Subject: [PATCH] fix usb skeleton driver Date: Wed, 6 Jun 2012 15:23:18 +0200 Message-Id: <1338988998-9061-1-git-send-email-stefani@seibold.net> X-Mailer: git-send-email 1.7.8.6 X-Authenticated-Sender: stefani@seibold.net Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 14764 Lines: 504 From: Stefani Seibold This is a fix for the USB skeleton driver to bring it in shape. - The usb_interface structure pointer will be no longer stored - Every access to the USB will be handled trought the usb_interface pointer - Add a new bool 'connected' for signaling a disconnect (== false) - Handle a non blocking read without blocking - Code cleanup - Synchronize disconnect() handler with open() and release(), to fix races - Introduced fsync - Single user mode - More code cleanup :-) - Save some bytes in the dev structure Some word about the open()/release() races with disconnect() of an USB device (which can happen any time): - The return interface pointer from usb_find_interface() could be already owned by an other driver and no more longer handle by the skeleton driver. - Or the dev pointer return by usb_get_intfdata() could point to an already release memory. This races can not handled by a per device mutex. Only a driver global mutex could do this job, since the kref_put() in the skel_disconnect() must be protected, otherwise skel_open() could access an already released memory. I know that this races are very small, but on heavy load or misdesigned or buggy hardware this could lead in a OOPS or unpredictable behavior. The patch is against linux 3.5.0 commit eea5b5510fc5545d15b69da8e485a7424ae388cf Grek ask me to do this in more pieces, but i will post it for a first RFC. Hope this helps Signed-off-by: Stefani Seibold --- drivers/usb/usb-skeleton.c | 218 ++++++++++++++++++++++---------------------- 1 files changed, 108 insertions(+), 110 deletions(-) diff --git a/drivers/usb/usb-skeleton.c b/drivers/usb/usb-skeleton.c index 0616f23..fce5a54 100644 --- a/drivers/usb/usb-skeleton.c +++ b/drivers/usb/usb-skeleton.c @@ -1,7 +1,8 @@ /* - * USB Skeleton driver - 2.2 + * USB Skeleton driver - 2.3 * * Copyright (C) 2001-2004 Greg Kroah-Hartman (greg@kroah.com) + * fixes by Stefani Seibold (stefani@seibold.net) * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License as @@ -48,8 +49,7 @@ MODULE_DEVICE_TABLE(usb, skel_table); /* Structure to hold all of our device specific stuff */ struct usb_skel { - struct usb_device *udev; /* the usb device for this device */ - struct usb_interface *interface; /* the interface for this device */ + struct usb_device *udev; /* the usb device */ struct semaphore limit_sem; /* limiting the number of writes in progress */ struct usb_anchor submitted; /* in case we need to retract our submissions */ struct urb *bulk_in_urb; /* the urb to read data with */ @@ -62,15 +62,19 @@ struct usb_skel { int errors; /* the last request tanked */ bool ongoing_read; /* a read is going on */ bool processed_urb; /* indicates we haven't processed the urb */ + bool connected; /* connected flag */ + bool in_use; /* in use flag */ spinlock_t err_lock; /* lock for errors */ struct kref kref; - struct mutex io_mutex; /* synchronize I/O with disconnect */ + struct mutex io_mutex; /* synchronize I/O */ struct completion bulk_in_completion; /* to wait for an ongoing read */ }; #define to_skel_dev(d) container_of(d, struct usb_skel, kref) static struct usb_driver skel_driver; -static void skel_draw_down(struct usb_skel *dev); + +/* synchronize open/release with disconnect */ +static DEFINE_MUTEX(sync_mutex); static void skel_delete(struct kref *kref) { @@ -86,15 +90,13 @@ static int skel_open(struct inode *inode, struct file *file) { struct usb_skel *dev; struct usb_interface *interface; - int subminor; - int retval = 0; + int retval; - subminor = iminor(inode); + /* lock against skel_disconnect() */ + mutex_lock(&sync_mutex); - interface = usb_find_interface(&skel_driver, subminor); + interface = usb_find_interface(&skel_driver, iminor(inode)); if (!interface) { - pr_err("%s - error, can't find device for minor %d\n", - __func__, subminor); retval = -ENODEV; goto exit; } @@ -105,52 +107,61 @@ static int skel_open(struct inode *inode, struct file *file) goto exit; } - /* increment our usage count for the device */ - kref_get(&dev->kref); - - /* lock the device to allow correctly handling errors - * in resumption */ - mutex_lock(&dev->io_mutex); + if (dev->in_use) { + retval = -EBUSY; + goto exit; + } retval = usb_autopm_get_interface(interface); if (retval) - goto out_err; + goto exit; + + /* increment our usage count for the device */ + kref_get(&dev->kref); + + dev->in_use = true; + mutex_unlock(&sync_mutex); /* save our object in the file's private structure */ file->private_data = dev; - mutex_unlock(&dev->io_mutex); - + return 0; exit: + mutex_unlock(&sync_mutex); return retval; } static int skel_release(struct inode *inode, struct file *file) { - struct usb_skel *dev; - - dev = file->private_data; - if (dev == NULL) - return -ENODEV; + struct usb_skel *dev = file->private_data; + /* lock against skel_disconnect() */ + mutex_lock(&sync_mutex); /* allow the device to be autosuspended */ - mutex_lock(&dev->io_mutex); - if (dev->interface) - usb_autopm_put_interface(dev->interface); - mutex_unlock(&dev->io_mutex); + if (dev->connected) + usb_autopm_put_interface( + usb_find_interface(&skel_driver, iminor(inode))); + dev->in_use = false; /* decrement the count on our device */ kref_put(&dev->kref, skel_delete); + mutex_unlock(&sync_mutex); return 0; } -static int skel_flush(struct file *file, fl_owner_t id) +static void skel_draw_down(struct usb_skel *dev) { - struct usb_skel *dev; - int res; + int time; + + time = usb_wait_anchor_empty_timeout(&dev->submitted, 1000); + if (!time) + usb_kill_anchored_urbs(&dev->submitted); + usb_kill_urb(dev->bulk_in_urb); +} - dev = file->private_data; - if (dev == NULL) - return -ENODEV; +static int skel_fsync(struct file *file, loff_t start, loff_t end, int datasync) +{ + struct usb_skel *dev = file->private_data; + int res; /* wait for io to stop */ mutex_lock(&dev->io_mutex); @@ -167,6 +178,11 @@ static int skel_flush(struct file *file, fl_owner_t id) return res; } +static int skel_flush(struct file *file, fl_owner_t id) +{ + return skel_fsync(file, 0, LLONG_MAX, 0); +} + static void skel_read_bulk_callback(struct urb *urb) { struct usb_skel *dev; @@ -179,7 +195,7 @@ static void skel_read_bulk_callback(struct urb *urb) if (!(urb->status == -ENOENT || urb->status == -ECONNRESET || urb->status == -ESHUTDOWN)) - dev_err(&dev->interface->dev, + dev_err(&urb->dev->dev, "%s - nonzero write bulk status received: %d\n", __func__, urb->status); @@ -187,7 +203,7 @@ static void skel_read_bulk_callback(struct urb *urb) } else { dev->bulk_in_filled = urb->actual_length; } - dev->ongoing_read = 0; + dev->ongoing_read = false; spin_unlock(&dev->err_lock); complete(&dev->bulk_in_completion); @@ -195,7 +211,7 @@ static void skel_read_bulk_callback(struct urb *urb) static int skel_do_read_io(struct usb_skel *dev, size_t count) { - int rv; + int retval; /* prepare a read */ usb_fill_bulk_urb(dev->bulk_in_urb, @@ -207,67 +223,61 @@ static int skel_do_read_io(struct usb_skel *dev, size_t count) skel_read_bulk_callback, dev); /* tell everybody to leave the URB alone */ - spin_lock_irq(&dev->err_lock); - dev->ongoing_read = 1; - spin_unlock_irq(&dev->err_lock); + dev->ongoing_read = true; /* do it */ - rv = usb_submit_urb(dev->bulk_in_urb, GFP_KERNEL); - if (rv < 0) { - dev_err(&dev->interface->dev, + retval = usb_submit_urb(dev->bulk_in_urb, GFP_KERNEL); + if (retval < 0) { + dev_err(&dev->udev->dev, "%s - failed submitting read urb, error %d\n", - __func__, rv); + __func__, retval); dev->bulk_in_filled = 0; - rv = (rv == -ENOMEM) ? rv : -EIO; - spin_lock_irq(&dev->err_lock); - dev->ongoing_read = 0; - spin_unlock_irq(&dev->err_lock); + retval = (retval == -ENOMEM) ? retval : -EIO; + dev->ongoing_read = false; } - return rv; + return retval; } static ssize_t skel_read(struct file *file, char *buffer, size_t count, loff_t *ppos) { - struct usb_skel *dev; - int rv; - bool ongoing_io; - - dev = file->private_data; + struct usb_skel *dev = file->private_data; + int retval; /* if we cannot read at all, return EOF */ if (!dev->bulk_in_urb || !count) return 0; /* no concurrent readers */ - rv = mutex_lock_interruptible(&dev->io_mutex); - if (rv < 0) - return rv; + if (file->f_flags & O_NONBLOCK) { + if (!mutex_trylock(&dev->io_mutex)) + return -EAGAIN; + } else { + retval = mutex_lock_interruptible(&dev->io_mutex); + if (retval < 0) + return retval; + } - if (!dev->interface) { /* disconnect() was called */ - rv = -ENODEV; + if (!dev->connected) { /* disconnect() was called */ + retval = -ENODEV; goto exit; } /* if IO is under way, we must not touch things */ retry: - spin_lock_irq(&dev->err_lock); - ongoing_io = dev->ongoing_read; - spin_unlock_irq(&dev->err_lock); - - if (ongoing_io) { + if (dev->ongoing_read) { /* nonblocking IO shall not wait */ if (file->f_flags & O_NONBLOCK) { - rv = -EAGAIN; + retval = -EAGAIN; goto exit; } /* * IO may take forever * hence wait in an interruptible state */ - rv = wait_for_completion_interruptible(&dev->bulk_in_completion); - if (rv < 0) + retval = wait_for_completion_interruptible(&dev->bulk_in_completion); + if (retval < 0) goto exit; /* * by waiting we also semiprocessed the urb @@ -288,12 +298,12 @@ retry: } /* errors must be reported */ - rv = dev->errors; - if (rv < 0) { + retval = dev->errors; + if (retval < 0) { /* any error is reported once */ dev->errors = 0; - /* to preserve notifications about reset */ - rv = (rv == -EPIPE) ? rv : -EIO; + /* to preseretvale notifications about reset */ + retval = (retval == -EPIPE) ? retval : -EIO; /* no data to deliver */ dev->bulk_in_filled = 0; /* report it */ @@ -315,8 +325,8 @@ retry: * all data has been used * actual IO needs to be done */ - rv = skel_do_read_io(dev, count); - if (rv < 0) + retval = skel_do_read_io(dev, count); + if (retval < 0) goto exit; else goto retry; @@ -329,9 +339,9 @@ retry: if (copy_to_user(buffer, dev->bulk_in_buffer + dev->bulk_in_copied, chunk)) - rv = -EFAULT; + retval = -EFAULT; else - rv = chunk; + retval = chunk; dev->bulk_in_copied += chunk; @@ -343,16 +353,16 @@ retry: skel_do_read_io(dev, count - chunk); } else { /* no data in the buffer */ - rv = skel_do_read_io(dev, count); - if (rv < 0) + retval = skel_do_read_io(dev, count); + if (retval < 0) goto exit; else if (!(file->f_flags & O_NONBLOCK)) goto retry; - rv = -EAGAIN; + retval = -EAGAIN; } exit: mutex_unlock(&dev->io_mutex); - return rv; + return retval; } static void skel_write_bulk_callback(struct urb *urb) @@ -366,7 +376,7 @@ static void skel_write_bulk_callback(struct urb *urb) if (!(urb->status == -ENOENT || urb->status == -ECONNRESET || urb->status == -ESHUTDOWN)) - dev_err(&dev->interface->dev, + dev_err(&urb->dev->dev, "%s - nonzero write bulk status received: %d\n", __func__, urb->status); @@ -384,14 +394,12 @@ static void skel_write_bulk_callback(struct urb *urb) static ssize_t skel_write(struct file *file, const char *user_buffer, size_t count, loff_t *ppos) { - struct usb_skel *dev; + struct usb_skel *dev = file->private_data; int retval = 0; struct urb *urb = NULL; char *buf = NULL; size_t writesize = min(count, (size_t)MAX_TRANSFER); - dev = file->private_data; - /* verify that we actually have some data to write */ if (count == 0) goto exit; @@ -444,9 +452,7 @@ static ssize_t skel_write(struct file *file, const char *user_buffer, } /* this lock makes sure we don't submit URBs to gone devices */ - mutex_lock(&dev->io_mutex); - if (!dev->interface) { /* disconnect() was called */ - mutex_unlock(&dev->io_mutex); + if (!dev->connected) { /* disconnect() was called */ retval = -ENODEV; goto error; } @@ -460,9 +466,8 @@ static ssize_t skel_write(struct file *file, const char *user_buffer, /* send the data out the bulk port */ retval = usb_submit_urb(urb, GFP_KERNEL); - mutex_unlock(&dev->io_mutex); if (retval) { - dev_err(&dev->interface->dev, + dev_err(&dev->udev->dev, "%s - failed submitting write urb, error %d\n", __func__, retval); goto error_unanchor; @@ -496,6 +501,7 @@ static const struct file_operations skel_fops = { .write = skel_write, .open = skel_open, .release = skel_release, + .fsync = skel_fsync, .flush = skel_flush, .llseek = noop_llseek, }; @@ -534,7 +540,8 @@ static int skel_probe(struct usb_interface *interface, init_completion(&dev->bulk_in_completion); dev->udev = usb_get_dev(interface_to_usbdev(interface)); - dev->interface = interface; + dev->connected = true; + dev->in_use = false; /* set up the endpoint information */ /* use only the first bulk-in and bulk-out endpoints */ @@ -603,35 +610,26 @@ error: static void skel_disconnect(struct usb_interface *interface) { struct usb_skel *dev; - int minor = interface->minor; - dev = usb_get_intfdata(interface); - usb_set_intfdata(interface, NULL); + dev_info(&interface->dev, "USB Skeleton disconnect #%d", + interface->minor); /* give back our minor */ usb_deregister_dev(interface, &skel_class); - /* prevent more I/O from starting */ - mutex_lock(&dev->io_mutex); - dev->interface = NULL; - mutex_unlock(&dev->io_mutex); - + dev = usb_get_intfdata(interface); usb_kill_anchored_urbs(&dev->submitted); - /* decrement our usage count */ - kref_put(&dev->kref, skel_delete); - - dev_info(&interface->dev, "USB Skeleton #%d now disconnected", minor); -} + /* lock against skel_open() and skel_release() */ + mutex_lock(&sync_mutex); + usb_set_intfdata(interface, NULL); -static void skel_draw_down(struct usb_skel *dev) -{ - int time; + /* prevent more I/O from starting */ + dev->connected = false; - time = usb_wait_anchor_empty_timeout(&dev->submitted, 1000); - if (!time) - usb_kill_anchored_urbs(&dev->submitted); - usb_kill_urb(dev->bulk_in_urb); + /* decrement our usage count */ + kref_put(&dev->kref, skel_delete); + mutex_unlock(&sync_mutex); } static int skel_suspend(struct usb_interface *intf, pm_message_t message) -- 1.7.8.6 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/