From: Oliver Neukum <oneukum@suse.com>
To: Philipp Hortmann <philipp.g.hortmann@gmail.com>,
corbet@lwn.net, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org
Cc: linux-usb@vger.kernel.org, gregkh@linuxfoundation.org
Subject: Re: [PATCH v3 2/5] Docs: usb: update comment and code near decrement our usage count for the device
Date: Tue, 7 Dec 2021 10:30:26 +0100 [thread overview]
Message-ID: <bc30a2f4-a913-1f5e-c1fa-e10f8f357128@suse.com> (raw)
In-Reply-To: <ca8fd26ccff6521c7477a2035e703e099da56214.1638771720.git.philipp.g.hortmann@gmail.com>
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
next prev parent reply other threads:[~2021-12-07 9:30 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-06 20:57 [PATCH v3 0/5] Docs: usb: Code and text updates from usb-skeleton Philipp Hortmann
2021-12-06 20:57 ` [PATCH v3 1/5] Docs: usb: update usb_bulk_msg receiving example Philipp Hortmann
2021-12-07 8:55 ` Oliver Neukum
2021-12-13 5:06 ` Philipp Hortmann
2021-12-06 20:57 ` [PATCH v3 2/5] Docs: usb: update comment and code near decrement our usage count for the device Philipp Hortmann
2021-12-07 9:30 ` Oliver Neukum [this message]
2021-12-12 1:25 ` Philipp Hortmann
2021-12-06 20:58 ` [PATCH v3 3/5] Docs: usb: update comment and code of function skel_delete Philipp Hortmann
2021-12-06 20:58 ` [PATCH v3 4/5] Docs: usb: update explanation for device_present to disconnected Philipp Hortmann
2021-12-06 20:58 ` [PATCH v3 5/5] Docs: usb: correct format of function names in the explanations Philipp Hortmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bc30a2f4-a913-1f5e-c1fa-e10f8f357128@suse.com \
--to=oneukum@suse.com \
--cc=corbet@lwn.net \
--cc=gregkh@linuxfoundation.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=philipp.g.hortmann@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).