Re: [USB] f16443a034: EIP:arch_local_irq_restore

[Felipe Balbi <felipe.balbi@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3186 bytes --]


Hi,

Alan Stern <stern@rowland.harvard.edu> writes:

> Felipe:
>
> On Thu, 29 Jun 2017, kernel test robot wrote:
>
>> FYI, we noticed the following commit:
>> 
>> commit: f16443a034c7aa359ddf6f0f9bc40d01ca31faea ("USB: gadgetfs, dummy-hcd, net2280: fix locking for callbacks")
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master
>> 
>> in testcase: trinity
>> with following parameters:
>> 
>> 	runtime: 300s
>> 
>> test-description: Trinity is a linux system call fuzz tester.
>> test-url: http://codemonkey.org.uk/projects/trinity/
>> 
>> 
>> on test machine: qemu-system-x86_64 -enable-kvm -m 420M
>> 
>> caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
> ...
>
> I won't include the entire report.  The gist is that we have a problem 
> with lock ordering.  The report is about dummy-hcd, but this could 
> affect any UDC driver.
>
>      1. When a SETUP request arrives, composite_setup() acquires
> 	cdev->lock before calling the function driver's callback.
> 	When that callback submits a reply, it causes the UDC driver
> 	to acquire its private lock.

this only seems to happen before calling set_config():

	case USB_REQ_SET_CONFIGURATION:
		if (ctrl->bRequestType != 0)
			goto unknown;
		if (gadget_is_otg(gadget)) {
			if (gadget->a_hnp_support)
				DBG(cdev, "HNP available\n");
			else if (gadget->a_alt_hnp_support)
				DBG(cdev, "HNP on another port\n");
			else
				VDBG(cdev, "HNP inactive\n");
		}
		spin_lock(&cdev->lock);
		value = set_config(cdev, ctrl, w_value);
		spin_unlock(&cdev->lock);
		break;

The "reply" here is a usb_ep_queue(gadget->ep0, req) call which should
hold UDC driver's private lock and disable interrupts for a short period
of time. Here's how it looks on dwc3:

int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,
		gfp_t gfp_flags)
{
	struct dwc3_request		*req = to_dwc3_request(request);
	struct dwc3_ep			*dep = to_dwc3_ep(ep);
	struct dwc3			*dwc = dep->dwc;

	unsigned long			flags;

	int				ret;

	spin_lock_irqsave(&dwc->lock, flags);
	if (!dep->endpoint.desc) {
		dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
				dep->name);
		ret = -ESHUTDOWN;
		goto out;
	}

	/* we share one TRB for ep0/1 */
	if (!list_empty(&dep->pending_list)) {
		ret = -EBUSY;
		goto out;
	}

	ret = __dwc3_gadget_ep0_queue(dep, req);

out:
	spin_unlock_irqrestore(&dwc->lock, flags);

	return ret;
}

>      2. When a bus reset occurs, the UDC's interrupt handler acquires
> 	its private lock before calling usb_gadget_udc_reset(), which
> 	calls composite_disconnect(), which acquires cdev->lock.

The reset handler will only run after B has been released though, no?
Also, UDC should *release* its private lock before calling UDC reset:

static void dwc3_reset_gadget(struct dwc3 *dwc)
{
	if (!dwc->gadget_driver)
		return;

	if (dwc->gadget.speed != USB_SPEED_UNKNOWN) {
		spin_unlock(&dwc->lock);
		usb_gadget_udc_reset(&dwc->gadget, dwc->gadget_driver);
		spin_lock(&dwc->lock);
	}
}

Are you sure this isn't a bug in dummy only?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]