[v3,2/5] Docs: usb: update comment and code near decrement our usage count for the device

Update comment: decrement our usage count ..
and code according to usb-skeleton.c

Signed-off-by: Philipp Hortmann <philipp.g.hortmann@gmail.com>
---
V1 -> V2: Corrected format of function name to skel_release()
V2 -> V3: Moved correction of the function name to an own patch in this
          patch series
---
 Documentation/driver-api/usb/writing_usb_driver.rst | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

On 06.12.21 21:57, Philipp Hortmann wrote:

> Update comment: decrement our usage count ..
> and code according to usb-skeleton.c

Hi,

and that is exactly the problem, I am afraid.
Your patch would be correct if the underlying code were correct.

>  
> -    /* decrement our usage count for the device */
> -    --skel->open_count;
> +    /* decrement the count on our device */
> +    kref_put(&dev->kref, skel_delete);
>  
>  
>  One of the more difficult problems that USB drivers must be able to

I am sorry but the code in usb-skel.c is wrong. You grab a reference
in skel_open():

        /* increment our usage count for the device */
        kref_get(&dev->kref);

which is good, but in skel_release() we do:

        /* decrement the count on our device */
        kref_put(&dev->kref, skel_delete);

unconditionally.

Think this through:

- Device is plugged in -> device node and internal data is created
- open() called -> kref_get(), we get a reference
- close() -> kref_put() -> refcount goes to zero -> skel_delete() is called, struct usb_skel is freed:

static void skel_delete(struct kref *kref)
{
        struct usb_skel *dev = to_skel_dev(kref);

        usb_free_urb(dev->bulk_in_urb);
        usb_put_intf(dev->interface);
        usb_put_dev(dev->udev);
        kfree(dev->bulk_in_buffer);
        kfree(dev);
}

with intfdata left intact.

- open() is called again -> We are following a dangling pointer into cloud cuckoo land.

Unfortunately this code is older than git, so I cannot just send a revert.
What to do?

	Regards
		Oliver

On 12/7/21 10:30 AM, Oliver Neukum wrote:
> On 06.12.21 21:57, Philipp Hortmann wrote:
> 
>> Update comment: decrement our usage count ..
>> and code according to usb-skeleton.c
> 
> Hi,
> 
> and that is exactly the problem, I am afraid.
> Your patch would be correct if the underlying code were correct.
> 
>>   
>> -    /* decrement our usage count for the device */
>> -    --skel->open_count;
>> +    /* decrement the count on our device */
>> +    kref_put(&dev->kref, skel_delete);
>>   
>>   
>>   One of the more difficult problems that USB drivers must be able to
> 
> I am sorry but the code in usb-skel.c is wrong. You grab a reference
> in skel_open():
> 
>          /* increment our usage count for the device */
>          kref_get(&dev->kref);
> 
> which is good, but in skel_release() we do:
> 
>          /* decrement the count on our device */
>          kref_put(&dev->kref, skel_delete);
> 
> unconditionally.
> 
> Think this through:
> 
> - Device is plugged in -> device node and internal data is created
> - open() called -> kref_get(), we get a reference
> - close() -> kref_put() -> refcount goes to zero -> skel_delete() is called, struct usb_skel is freed:
> 
> static void skel_delete(struct kref *kref)
> {
>          struct usb_skel *dev = to_skel_dev(kref);
> 
>          usb_free_urb(dev->bulk_in_urb);
>          usb_put_intf(dev->interface);
>          usb_put_dev(dev->udev);
>          kfree(dev->bulk_in_buffer);
>          kfree(dev);
> }
> 
> with intfdata left intact.
> 
> - open() is called again -> We are following a dangling pointer into cloud cuckoo land.
> 
> Unfortunately this code is older than git, so I cannot just send a revert.
> What to do?
> 
> 	Regards
> 		Oliver
> 
I cannot see the issue you described.

Think this through:
- probe() is called and kref_init() sets refcount to 1
- open() is called and refcount is increased to 2
- close() is called and refcount is decreased to 1 -> delete() is not called
- disconnect() is called and refcount is decreased to 0 -> delete() is 
called

Putting debug messages into the code and follow the log:
[12820.221534] skeleton 2-1.6:1.0: skel_probe called
[12820.221658] skeleton 2-1.6:1.0: USB Skeleton device now attached to 
USBSkel-1
[12820.221690] usbcore: registered new interface driver skeleton
[12824.046075] skeleton 2-1.6:1.0: skel_open called
[12825.047213] skeleton 2-1.6:1.0: skel_release called
[12826.047854] skeleton 2-1.6:1.0: skel_open called
[12827.049017] skeleton 2-1.6:1.0: skel_release called
[12831.035262] usb 2-1.6: USB disconnect, device number 4
[12831.035500] skeleton 2-1.6:1.0: skel_disconnect call
[12831.035504] skeleton 2-1.6:1.0: skel_delete called
[12831.035507] skeleton 2-1.6:1.0: USB Skeleton #1 now disconnected

delete() is only called on disconnect and not earlier.

This seems to be fine to me. Please find position of debug messages below.

Thanks for your reply.

Regards,

Philipp



  /* Define these values to match your devices */
-#define USB_SKEL_VENDOR_ID     0xfff0
-#define USB_SKEL_PRODUCT_ID    0xfff0
+#define USB_SKEL_VENDOR_ID     0x1a86
+#define USB_SKEL_PRODUCT_ID    0x7523

-/* table of devices that work with this driver */
  static const struct usb_device_id skel_table[] = {
         { USB_DEVICE(USB_SKEL_VENDOR_ID, USB_SKEL_PRODUCT_ID) },
         { }                                     /* Terminating entry */
@@ -73,6 +72,7 @@ static void skel_delete(struct kref *kref)
  {
         struct usb_skel *dev = to_skel_dev(kref);

+       dev_info(&dev->interface->dev, "skel_delete called\n");
         usb_free_urb(dev->bulk_in_urb);
         usb_put_intf(dev->interface);
         usb_put_dev(dev->udev);
@@ -110,6 +110,7 @@ static int skel_open(struct inode *inode, struct 
file *file)
         /* increment our usage count for the device */
         kref_get(&dev->kref);

+       dev_info(&interface->dev, "skel_open called\n");
         /* save our object in the file's private structure */
         file->private_data = dev;

@@ -125,6 +126,7 @@ static int skel_release(struct inode *inode, struct 
file *file)
         if (dev == NULL)
                 return -ENODEV;

+       dev_info(&dev->interface->dev, "skel_release called\n");
         /* allow the device to be autosuspended */
         usb_autopm_put_interface(dev->interface);

@@ -507,6 +509,7 @@ static int skel_probe(struct usb_interface *interface,
         dev->udev = usb_get_dev(interface_to_usbdev(interface));
         dev->interface = usb_get_intf(interface);

+       dev_info(&dev->interface->dev, "skel_probe called\n");
         /* set up the endpoint information */
         /* use only the first bulk-in and bulk-out endpoints */
         retval = usb_find_common_endpoints(interface->cur_altsetting,
@@ -577,6 +580,7 @@ static void skel_disconnect(struct usb_interface 
*interface)
         usb_kill_urb(dev->bulk_in_urb);
         usb_kill_anchored_urbs(&dev->submitted);

+       dev_info(&dev->interface->dev, "skel_disconnect call\n");
         /* decrement our usage count */
         kref_put(&dev->kref, skel_delete);