On Wed, Jun 6, 2012 at 9:23 PM, <stefani@xxxxxxxxxxx> wrote: > From: Stefani Seibold <stefani@xxxxxxxxxxx> > > 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 So many purposes, you need to split your patches for review easily, :-) > > 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 <stefani@xxxxxxxxxxx> > --- > 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@xxxxxxxxx) > + * fixes by Stefani Seibold (stefani@xxxxxxxxxxx) > * > * 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); This one is not needed since we have minor_rwsem(drivers/usb/core/file.c) to avoid the race. > > - 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); Since the reference count is held now, so is there any race between release and disconnect? > /* 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@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ Thanks, -- Ming Lei -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html