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

[Oliver Neukum]

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