Duplicated code in hiddev_open()

Duplicated code in hiddev_open()

[Alan Stern]

Oliver and Jiri:

Why is there duplicated code in
drivers/hid/usbhid/hiddev.c:hiddev_open()?

Line 267:
	/*
	 * no need for locking because the USB major number
	 * is shared which usbcore guards against disconnect
	 */
	if (list->hiddev->exist) {
		if (!list->hiddev->open++) {
			res = hid_hw_open(hiddev->hid);
			if (res < 0)
				goto bail;
		}
	} else {
		res = -ENODEV;
		goto bail;
	}

Line 286:
	mutex_lock(&hiddev->existancelock);
	if (!list->hiddev->open++)
		if (list->hiddev->exist) {
			struct hid_device *hid = hiddev->hid;
			res = hid_hw_power(hid, PM_HINT_FULLON);
			if (res < 0)
				goto bail_unlock;
			res = hid_hw_open(hid);
			if (res < 0)
				goto bail_normal_power;
		}
	mutex_unlock(&hiddev->existancelock);

The second part can never execute, because the first part ensures that 
list->hiddev->open > 0 by the time the second part runs.

Even more disturbing, why is one of these code sections protected by a 
mutex and the other not?

Note: The second section was added in commit 0361a28d3f9a ("HID: 
autosuspend support for USB HID") over ten years ago!

Alan Stern

Re: Duplicated code in hiddev_open()

[Oliver Neukum]

Am Freitag, den 16.08.2019, 13:10 -0400 schrieb Alan Stern:
> Oliver and Jiri:
> 
> Why is there duplicated code in
> drivers/hid/usbhid/hiddev.c:hiddev_open()?
> 
> Line 267:
> 	/*
> 	 * no need for locking because the USB major number
> 	 * is shared which usbcore guards against disconnect
> 	 */
> 	if (list->hiddev->exist) {
> 		if (!list->hiddev->open++) {
> 			res = hid_hw_open(hiddev->hid);
> 			if (res < 0)
> 				goto bail;
> 		}
> 	} else {
> 		res = -ENODEV;
> 		goto bail;
> 	}
> 
> Line 286:
> 	mutex_lock(&hiddev->existancelock);
> 	if (!list->hiddev->open++)
> 		if (list->hiddev->exist) {
> 			struct hid_device *hid = hiddev->hid;
> 			res = hid_hw_power(hid, PM_HINT_FULLON);
> 			if (res < 0)
> 				goto bail_unlock;
> 			res = hid_hw_open(hid);
> 			if (res < 0)
> 				goto bail_normal_power;
> 		}
> 	mutex_unlock(&hiddev->existancelock);
> 
> The second part can never execute, because the first part ensures that 
> list->hiddev->open > 0 by the time the second part runs.
> 
> Even more disturbing, why is one of these code sections protected by a 
> mutex and the other not?

I suppose the comment I made back then:

079034073faf9 drivers/hid/usbhid/hiddev.c (Oliver Neukum               2008-12-16 10:55:15 +0100 268)    * no need for locking because the USB major number
079034073faf9 drivers/hid/usbhid/hiddev.c (Oliver Neukum               2008-12-16 10:55:15 +0100 269)    * is shared which usbcore guards against disconnect

has ceased to be true, but the section was not removed, as the check
for existance was duplicated.

> Note: The second section was added in commit 0361a28d3f9a ("HID: 
> autosuspend support for USB HID") over ten years ago!

Yes and I remember how frustrating keyboards were in testing, but
no further details.

	Regards
		Oliver

Re: Duplicated code in hiddev_open()

[Alan Stern]

On Mon, 19 Aug 2019, Oliver Neukum wrote:

> Am Freitag, den 16.08.2019, 13:10 -0400 schrieb Alan Stern:
> > Oliver and Jiri:
> > 
> > Why is there duplicated code in
> > drivers/hid/usbhid/hiddev.c:hiddev_open()?
> > 
> > Line 267:
> > 	/*
> > 	 * no need for locking because the USB major number
> > 	 * is shared which usbcore guards against disconnect
> > 	 */
> > 	if (list->hiddev->exist) {
> > 		if (!list->hiddev->open++) {
> > 			res = hid_hw_open(hiddev->hid);
> > 			if (res < 0)
> > 				goto bail;
> > 		}
> > 	} else {
> > 		res = -ENODEV;
> > 		goto bail;
> > 	}
> > 
> > Line 286:
> > 	mutex_lock(&hiddev->existancelock);
> > 	if (!list->hiddev->open++)
> > 		if (list->hiddev->exist) {
> > 			struct hid_device *hid = hiddev->hid;
> > 			res = hid_hw_power(hid, PM_HINT_FULLON);
> > 			if (res < 0)
> > 				goto bail_unlock;
> > 			res = hid_hw_open(hid);
> > 			if (res < 0)
> > 				goto bail_normal_power;
> > 		}
> > 	mutex_unlock(&hiddev->existancelock);
> > 
> > The second part can never execute, because the first part ensures that 
> > list->hiddev->open > 0 by the time the second part runs.
> > 
> > Even more disturbing, why is one of these code sections protected by a 
> > mutex and the other not?
> 
> I suppose the comment I made back then:
> 
> 079034073faf9 drivers/hid/usbhid/hiddev.c (Oliver Neukum               2008-12-16 10:55:15 +0100 268)    * no need for locking because the USB major number
> 079034073faf9 drivers/hid/usbhid/hiddev.c (Oliver Neukum               2008-12-16 10:55:15 +0100 269)    * is shared which usbcore guards against disconnect
> 
> has ceased to be true, but the section was not removed, as the check
> for existance was duplicated.
> 
> > Note: The second section was added in commit 0361a28d3f9a ("HID: 
> > autosuspend support for USB HID") over ten years ago!
> 
> Yes and I remember how frustrating keyboards were in testing, but
> no further details.

Indeed.  But more importantly for now, how should this be fixed?  This
may be the culprit in some of the syzbot bug reports (those involving 
hiddev).

Alan Stern

Re: Duplicated code in hiddev_open()

[Oliver Neukum]

Am Montag, den 19.08.2019, 10:17 -0400 schrieb Alan Stern:
> On Mon, 19 Aug 2019, Oliver Neukum wrote:
> 
> > Am Freitag, den 16.08.2019, 13:10 -0400 schrieb Alan Stern:
> > > Oliver and Jiri:
> > > 
> > > Why is there duplicated code in
> > > drivers/hid/usbhid/hiddev.c:hiddev_open()?
> > > 
> > > Line 267:
> > > 	/*
> > > 	 * no need for locking because the USB major number
> > > 	 * is shared which usbcore guards against disconnect
> > > 	 */
> > > 	if (list->hiddev->exist) {
> > > 		if (!list->hiddev->open++) {
> > > 			res = hid_hw_open(hiddev->hid);
> > > 			if (res < 0)
> > > 				goto bail;
> > > 		}
> > > 	} else {
> > > 		res = -ENODEV;
> > > 		goto bail;
> > > 	}
> > > 
> > > Line 286:
> > > 	mutex_lock(&hiddev->existancelock);
> > > 	if (!list->hiddev->open++)
> > > 		if (list->hiddev->exist) {
> > > 			struct hid_device *hid = hiddev->hid;
> > > 			res = hid_hw_power(hid, PM_HINT_FULLON);
> > > 			if (res < 0)
> > > 				goto bail_unlock;
> > > 			res = hid_hw_open(hid);
> > > 			if (res < 0)
> > > 				goto bail_normal_power;
> > > 		}
> > > 	mutex_unlock(&hiddev->existancelock);
> > > 
> > > The second part can never execute, because the first part ensures that 
> > > list->hiddev->open > 0 by the time the second part runs.
> > > 
> > > Even more disturbing, why is one of these code sections protected by a 
> > > mutex and the other not?
> > 
> > I suppose the comment I made back then:
> > 
> > 079034073faf9 drivers/hid/usbhid/hiddev.c (Oliver Neukum               2008-12-16 10:55:15 +0100 268)    * no need for locking because the USB major number
> > 079034073faf9 drivers/hid/usbhid/hiddev.c (Oliver Neukum               2008-12-16 10:55:15 +0100 269)    * is shared which usbcore guards against disconnect
> > 
> > has ceased to be true, but the section was not removed, as the check
> > for existance was duplicated.
> > 
> > > Note: The second section was added in commit 0361a28d3f9a ("HID: 
> > > autosuspend support for USB HID") over ten years ago!
> > 
> > Yes and I remember how frustrating keyboards were in testing, but
> > no further details.
> 
> Indeed.  But more importantly for now, how should this be fixed?  This
> may be the culprit in some of the syzbot bug reports (those involving 
> hiddev).


I doubt it. This looks like it would cause a resource leak, not the
other way round. But I'd say all operations need to be done under lock.

	Regards
		



oliver