Re: [PATCH] hid: usbhid: fix possible deadlock in __usbhid_submit_report

[Oliver Neukum <oneukum@suse.de>]

Am Mittwoch, 25. April 2012, 03:27:19 schrieb Ming Lei:
> On Wed, Apr 25, 2012 at 2:57 AM, Oliver Neukum <oliver@neukum.org> wrote:
> 
>  usb_submit_urb()
> >>
> >> This submit won't happen because HID_OUT_RUNNING is not cleared.
> >
> > I may be dense, but as far as I can tell a resubmit will happen, exactly if
> > HID_OUT_RUNNING is _not_ cleared.
> 
> In fact, it should be a double unlink bug, usb_unlink_urb can handle
> it correctly
> if the lock is held. We also can deal with it easily by checking urb->unlinked,
> so how about the below patch?
> 
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index aa1c503..b530463 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -411,10 +411,10 @@ static void hid_irq_out(struct urb *urb)
>  {
>  	struct hid_device *hid = urb->context;
>  	struct usbhid_device *usbhid = hid->driver_data;
> -	unsigned long flags;
> +	unsigned long status = urb->status;

Error codes are negative.


> @@ -546,8 +557,13 @@ static void __usbhid_submit_report(struct
> hid_device *hid, struct hid_report *re
>  			 * no race because this is called under
>  			 * spinlock
>  			 */
> -			if (time_after(jiffies, usbhid->last_out + HZ * 5))
> +
> +			if (time_after(jiffies, usbhid->last_out + HZ * 5) &&
> +					!usbhid->urbout->unlinked) {
> +				spin_unlock(&usbhid->lock);
>  				usb_unlink_urb(usbhid->urbout);
> +				spin_lock(&usbhid->lock);
> +			}
>  		}
>  		return;
>  	}

Same objection. You are just making the race unlikelier. The flag
needs to be set under a lock you hold while checking time_after().
We'd be back at the original proposal.

	Regards
		Oliver

-- 
- - - 
SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg) 
Maxfeldstraße 5                         
90409 Nürnberg 
Germany 
- - - 
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html