On Wed, Sep 28, 2016 at 11:45:02AM +0200, Greg Kroah-Hartman wrote:
> If it is freed, why is a read even able to happen? Ah, ick, no proper
> reference counting is happening here :(
>
> Oh, no, wait, it is happening properly, it's just that it's not the
> lifespan that the devm_kzalloc() is attached to, so yes, the correct fix
> here is to revert that patch as it is incorrect.
Well, reference counting is also suspicious as kref_get(&data->kref) in
probe function with comment "will reference data in int urb" gives no
clue why there's explicit reference. Also what if we add classic
error unwinding and leave usb_submit_urb to open time? But wait, this
driver allows multiple opens? Is it intentional? It could be done this
way (note patch is only for reference as there's nothing to prevent
multiple open and therefore multiple usb_submit_urb):
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 917a55c..0964e49 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -147,7 +147,6 @@ static int usbtmc_open(struct inode *inode, struct file *filp)
{
struct usb_interface *intf;
struct usbtmc_device_data *data;
- int retval = 0;
intf = usb_find_interface(&usbtmc_driver, iminor(inode));
if (!intf) {
@@ -156,12 +155,19 @@ static int usbtmc_open(struct inode *inode, struct file *filp)
}
data = usb_get_intfdata(intf);
- kref_get(&data->kref);
+ if (data->iin_ep_present) {
+ int retval = usb_submit_urb(data->iin_urb, GFP_KERNEL);
+ if (retval) {
+ dev_err(&intf->dev, "Failed to submit iin_urb\n");
+ return retval;
+ }
+ }
/* Store pointer in file structure's private data field */
filp->private_data = data;
+ kref_get(&data->kref);
- return retval;
+ return 0;
}
static int usbtmc_release(struct inode *inode, struct file *file)
@@ -1358,16 +1364,6 @@ exit:
dev_err(dev, "usb_submit_urb failed: %d\n", rv);
}
-static void usbtmc_free_int(struct usbtmc_device_data *data)
-{
- if (!data->iin_ep_present || !data->iin_urb)
- return;
- usb_kill_urb(data->iin_urb);
- kfree(data->iin_buffer);
- usb_free_urb(data->iin_urb);
- kref_put(&data->kref, usbtmc_delete);
-}
-
static int usbtmc_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
@@ -1469,18 +1465,16 @@ static int usbtmc_probe(struct usb_interface *intf,
data->iin_urb = usb_alloc_urb(0, GFP_KERNEL);
if (!data->iin_urb) {
dev_err(&intf->dev, "Failed to allocate int urb\n");
- goto error_register;
+ goto error_alloc_urb;
}
- /* will reference data in int urb */
- kref_get(&data->kref);
/* allocate buffer for interrupt in */
data->iin_buffer = kmalloc(data->iin_wMaxPacketSize,
GFP_KERNEL);
if (!data->iin_buffer) {
dev_err(&intf->dev, "Failed to allocate int buf\n");
- goto error_register;
+ goto error_alloc_buffer;
}
/* fill interrupt urb */
@@ -1490,11 +1484,6 @@ static int usbtmc_probe(struct usb_interface *intf,
usbtmc_interrupt,
data, data->iin_interval);
- retcode = usb_submit_urb(data->iin_urb, GFP_KERNEL);
- if (retcode) {
- dev_err(&intf->dev, "Failed to submit iin_urb\n");
- goto error_register;
- }
}
retcode = sysfs_create_group(&intf->dev.kobj, &data_attr_grp);
@@ -1511,10 +1500,16 @@ static int usbtmc_probe(struct usb_interface *intf,
return 0;
error_register:
- sysfs_remove_group(&intf->dev.kobj, &capability_attr_grp);
sysfs_remove_group(&intf->dev.kobj, &data_attr_grp);
- usbtmc_free_int(data);
- kref_put(&data->kref, usbtmc_delete);
+ if (data->iin_ep_present) {
+ kfree(data->iin_buffer);
+error_alloc_buffer:
+ usb_free_urb(data->iin_urb);
+ }
+error_alloc_urb:
+ sysfs_remove_group(&intf->dev.kobj, &capability_attr_grp);
+ usb_put_dev(data->usb_dev);
+
return retcode;
}
@@ -1532,7 +1527,11 @@ static void usbtmc_disconnect(struct usb_interface *intf)
data->zombie = 1;
wake_up_all(&data->waitq);
mutex_unlock(&data->io_mutex);
- usbtmc_free_int(data);
+ if (data->iin_ep_present) {
+ usb_kill_urb(data->iin_urb);
+ kfree(data->iin_buffer);
+ usb_free_urb(data->iin_urb);
+ }
kref_put(&data->kref, usbtmc_delete);
}
--
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
