Zitat von Greg KH <gre...@linuxfoundation.org>:

@@ -122,7 +141,7 @@ static int usbtmc_open(struct inode *inode, struct file *filp)
 {
        struct usb_interface *intf;
        struct usbtmc_device_data *data;
-       int retval = 0;
+       struct usbtmc_file_data *file_data;

        intf = usb_find_interface(&usbtmc_driver, iminor(inode));
        if (!intf) {
@@ -130,21 +149,54 @@ static int usbtmc_open(struct inode *inode, struct file *filp)
                return -ENODEV;
        }

+       file_data = kzalloc(sizeof(*file_data), GFP_KERNEL);
+       if (!file_data)
+               return -ENOMEM;
+
+       pr_debug("%s - called\n", __func__);

Please do not add "tracing" functions like this.  The kernel has a
wonderful built-in function tracing functionality, don't try to write
your own.  These lines should just be removed.


+
        data = usb_get_intfdata(intf);
        /* Protect reference to data from file structure until release */
        kref_get(&data->kref);

+       mutex_lock(&data->io_mutex);
+       file_data->data = data;
+
+       /* copy default values from device settings */
+       file_data->TermChar = data->TermChar;
+       file_data->TermCharEnabled = data->TermCharEnabled;
+       file_data->auto_abort = data->auto_abort;
+
+       INIT_LIST_HEAD(&file_data->file_elem);
+       spin_lock_irq(&data->dev_lock);
+       list_add_tail(&file_data->file_elem, &data->file_list);
+       spin_unlock_irq(&data->dev_lock);
+       mutex_unlock(&data->io_mutex);
+
        /* Store pointer in file structure's private data field */
-       filp->private_data = data;
+       filp->private_data = file_data;

-       return retval;
+       return 0;
 }

 static int usbtmc_release(struct inode *inode, struct file *file)
 {
-       struct usbtmc_device_data *data = file->private_data;
+       struct usbtmc_file_data *file_data = file->private_data;

-       kref_put(&data->kref, usbtmc_delete);
+       pr_debug("%s - called\n", __func__);

Again, these all need to be dropped.

+
+       /* prevent IO _AND_ usbtmc_interrupt */
+       mutex_lock(&file_data->data->io_mutex);
+       spin_lock_irq(&file_data->data->dev_lock);
+
+       list_del(&file_data->file_elem);
+
+       spin_unlock_irq(&file_data->data->dev_lock);
+       mutex_unlock(&file_data->data->io_mutex);

You protect the list, but what about removing the data itself?

+
+       kref_put(&file_data->data->kref, usbtmc_delete);

What protects this from being called at the same time a kref_get is
being called?

Yeah, it previously probably already had this race, sorry I never
noticed that.


I looked for a race here, but I do not find a race between open and release,
since a refcount of "file_data->data->kref" is always hold by
usbtmc_probe/disconnect.

However I see a race between usbtmc_open and usbtmc_disconnect. Are these
callback functions called mutual exclusive?
I'm not sure, but if not, then I think we have the same problem in usb-skeleton.c



+       file_data->data = NULL;
+       kfree(file_data);
        return 0;
 }


thanks,

greg k-h



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to