Bug 281843 - "check for in-use endpoints" code in usb_config_parse() is missing an increment
Summary: "check for in-use endpoints" code in usb_config_parse() is missing an increment
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: usb (show other bugs)
Version: 13.3-RELEASE
Hardware: Any Any
: --- Affects Only Me
Assignee: Ed Maste
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-10-04 03:26 UTC by Matt Jacobson
Modified: 2024-11-16 14:32 UTC (History)
6 users (show)

See Also:
zlei: mfc-stable14+
zlei: mfc-stable13+


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Jacobson 2024-10-04 03:26:19 UTC
sys/dev/usb/usb_device.c:846 (in usb_config_parse()):

```
		/* check for in-use endpoints */

		if (cmd == USB_CFG_INIT) {
			ep = udev->endpoints;
			ep_max = udev->endpoints_max;
			while (ep_max--) {
				/* look for matching endpoints */
				if (iface_index == USB_IFACE_INDEX_ANY ||
				    iface_index == ep->iface_index) {
					if (ep->refcount_alloc != 0)
						return (USB_ERR_IN_USE);
				}
			}
		}
```

This code is missing an `ep++`.  See the similar loop below, which does have the increment.

I'm hitting a panic that seems to be caused by changing alternate interface index while transfers are outstanding, which this code is supposed to prevent.  I suspect (but don't know for sure) that this is at least part of the problem.
Comment 1 Matt Jacobson 2024-10-04 03:36:27 UTC
Looks like this was introduced by: <https://github.com/freebsd/freebsd-src/commit/e4611d26265fb9e3bd2a345cf4776863f49a2587>.
Comment 2 Matt Jacobson 2024-10-04 03:39:31 UTC
That commit was also picked back to 13.3, so that's where this bug first appears.
Comment 3 Matt Jacobson 2024-10-04 04:13:56 UTC
I (sorta) figured out what was causing me to be able to hit this case.  I accidentally had webcamd and my own libuvc-based program running at the same time, both trying to access the same ugen device.

Obviously that's never going to work well, so that was my error.  But it still shouldn't panic.
Comment 4 Mohammad Noureldin 2024-10-04 11:05:25 UTC
(In reply to Matt Jacobson from comment #3)

Hi Matt,

Out of curiosity, is it possible to share code of your libuvc-based program ?
Comment 5 Ed Maste freebsd_committer freebsd_triage 2024-10-04 15:53:19 UTC
Yes, I'd expect an ep++ in the loop, although I don't have enough context around the rest of the code yet. I've made the change in my local WIP tree for experimentation.

Have you been able to try your use case with the change?
Comment 6 Mark Johnston freebsd_committer freebsd_triage 2024-11-10 16:03:17 UTC
(In reply to Ed Maste from comment #5)
Should we just go ahead with fixing that loop?
Comment 7 Warner Losh freebsd_committer freebsd_triage 2024-11-10 16:23:36 UTC
My reading of the loop is that it needs one too...  Maybe hps@ forgot it when he did the code originally and his test case didn't need it so he didn't notice?
Comment 8 commit-hook freebsd_committer freebsd_triage 2024-11-11 02:54:10 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=114080d19973331426cd826f3a961c6ea9216a53

commit 114080d19973331426cd826f3a961c6ea9216a53
Author:     Ed Maste <emaste@FreeBSD.org>
AuthorDate: 2024-10-04 15:49:53 +0000
Commit:     Ed Maste <emaste@FreeBSD.org>
CommitDate: 2024-11-11 02:53:18 +0000

    usb: fix loop in usb_config_parse

    By inspection, index increment was missing.

    PR:             281843
    Reported by:    Matt Jacobson
    Reviewed by:    bz, markj
    Fixes: e4611d26265f ("usb(4): Call optional endpoint_uninit() when changing configuration or alternate setting.")
    Sponsored by:   The FreeBSD Foundation

 sys/dev/usb/usb_device.c | 1 +
 1 file changed, 1 insertion(+)
Comment 9 commit-hook freebsd_committer freebsd_triage 2024-11-13 14:28:01 UTC
A commit in branch stable/14 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=ddc9bb706808468b2808ab9a32efc5f8dfb346c9

commit ddc9bb706808468b2808ab9a32efc5f8dfb346c9
Author:     Ed Maste <emaste@FreeBSD.org>
AuthorDate: 2024-10-04 15:49:53 +0000
Commit:     Ed Maste <emaste@FreeBSD.org>
CommitDate: 2024-11-13 14:26:44 +0000

    usb: fix loop in usb_config_parse

    By inspection, index increment was missing.

    PR:             281843
    Reported by:    Matt Jacobson
    Reviewed by:    bz, markj
    Fixes: e4611d26265f ("usb(4): Call optional endpoint_uninit() when changing configuration or alternate setting.")
    Sponsored by:   The FreeBSD Foundation

    (cherry picked from commit 114080d19973331426cd826f3a961c6ea9216a53)

 sys/dev/usb/usb_device.c | 1 +
 1 file changed, 1 insertion(+)
Comment 10 commit-hook freebsd_committer freebsd_triage 2024-11-13 14:29:02 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=9e7356944da4693e9ceb40bf65ed525048faa106

commit 9e7356944da4693e9ceb40bf65ed525048faa106
Author:     Ed Maste <emaste@FreeBSD.org>
AuthorDate: 2024-10-04 15:49:53 +0000
Commit:     Ed Maste <emaste@FreeBSD.org>
CommitDate: 2024-11-13 14:27:54 +0000

    usb: fix loop in usb_config_parse

    By inspection, index increment was missing.

    PR:             281843
    Reported by:    Matt Jacobson
    Reviewed by:    bz, markj
    Fixes: e4611d26265f ("usb(4): Call optional endpoint_uninit() when changing configuration or alternate setting.")
    Sponsored by:   The FreeBSD Foundation

    (cherry picked from commit 114080d19973331426cd826f3a961c6ea9216a53)
    (cherry picked from commit ddc9bb706808468b2808ab9a32efc5f8dfb346c9)

 sys/dev/usb/usb_device.c | 1 +
 1 file changed, 1 insertion(+)