From patchwork Fri Jul 19 03:50:04 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ming Lei X-Patchwork-Id: 2830058 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id DEC29C0319 for ; Fri, 19 Jul 2013 03:50:54 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id F0EFE201EA for ; Fri, 19 Jul 2013 03:50:53 +0000 (UTC) Received: from casper.infradead.org (casper.infradead.org [85.118.1.10]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E394F201DD for ; Fri, 19 Jul 2013 03:50:52 +0000 (UTC) Received: from merlin.infradead.org ([2001:4978:20e::2]) by casper.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1V01iK-0002Vf-8y; Fri, 19 Jul 2013 03:50:40 +0000 Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1V01iD-0003hs-7r; Fri, 19 Jul 2013 03:50:33 +0000 Received: from mail-ie0-x234.google.com ([2607:f8b0:4001:c03::234]) by merlin.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1V01i9-0003hH-QO for linux-arm-kernel@lists.infradead.org; Fri, 19 Jul 2013 03:50:30 +0000 Received: by mail-ie0-f180.google.com with SMTP id f4so8516403iea.39 for ; Thu, 18 Jul 2013 20:50:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=S0rsM34tSf/4WRfhTDLIBVt7KXVzXgiXjE9Xd33lBQ4=; b=ULJ0ZeYihyT770bb2+FtO8Kd2grgw2l4CDcdunFabGr/fwKT1mSm1xneQYgUBBwl4l xIrqjdxs+Cy8SB6pi25Ue05uGqinAjOVo1HR2n6gxq8pohSnEE4U6pG16JfDRYrpvpuk iIvfMI43sCwKj7TLubZqbVigdzruYSr8lSM5eMi5nvdaxLEvHAXqwIpeDGFN9jjZ5LZF Dl7PVBGz4C+OfBLLvTIQ3BjURXQ0xH7+8iENEVzcKUv5uNkt/JOoJZzX5piQC63+yksR JAFi4EQLZBdo/vIOJPqp467+KIUqtU2twxBnnHLJ5mxZQNyKHpDMJFH1plumLnznSfWV bRtA== MIME-Version: 1.0 X-Received: by 10.42.123.139 with SMTP id s11mr8112609icr.82.1374205804438; Thu, 18 Jul 2013 20:50:04 -0700 (PDT) Received: by 10.43.73.5 with HTTP; Thu, 18 Jul 2013 20:50:04 -0700 (PDT) In-Reply-To: References: Date: Fri, 19 Jul 2013 11:50:04 +0800 Message-ID: Subject: Re: [PATCH] [RFC] EHCI: add to memory barrier to updating hw_next From: Ming Lei To: Alan Stern X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20130718_235029_918816_8876E9A6 X-CRM114-Status: GOOD ( 30.47 ) X-Spam-Score: -2.0 (--) Cc: namhyung.kim@lge.com, Jong-Sung Kim , Chanho Min , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, Gioh Kim , Mark Salter , Minchan Kim , linux-arm-kernel X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Jul 18, 2013 at 10:08 PM, Alan Stern wrote: > On Thu, 18 Jul 2013, Ming Lei wrote: > >> > I guess that HC could have a use-after-free problem like following situation. >> > >> > 1. A qtd which is not at the queue head should be removed in qh_completions(). >> > 2. The last->hw_next become be pointing at the next qtd but the hw_next value is delayed in write-buffer. >> > 3. The qtd is removed in the list. >> > 4. The qtd is freed into DMA pool and re-allocated for another urb. >> > 5. HC try to process last->hw_next and it is pointing re-allocated qtd. >> > >> > What do you think about it? Is it possible? >> >> I understand it might not be possible because: when 'stopped' is set, that >> said the HC might not advance the queue. But I don't understand why >> 'last->hw_next' is patched here under 'stopped' situation. > > It should not be possible. When "stopped" is set, the QH gets unlinked > and relinked before it can start up again. Relinking involves some > memory barriers, so the qTD will not be accessed again by the HC. > > last->hw_next gets patched because the qTD might belong to some URB in > the middle of the queue that is being unlinked. The URBs before it and > after it will still be active, so the queue link has to be updated. 'stopped' is set under below situations: - unlink over or shutdown - halt - short packet(URB_SHORT_NOT_OK) Looks EHCI won't advance the queue(qh) any more on above situations, so I think last->hw_next might not need patching. > >> Even the 'stopped' case may be seldom triggered, do you know under >> which condition the stopped is triggered in your problem?(stall, short read >> or others) > > I was going to ask the same question. This particular piece of code > gets executed _only_ when an URB is unlinked. Not during any other > kind of error. The code may run under 'halt' or short packet(URB_SHORT_NOT_OK) too. If Gioh's problem falls to these two situations, below patch might be helpful. Because the qh will be unlinked when there is fault in the endpoint(halt, short packet), maybe it is safer to complete these URBs after the qh becomes unlinked, like what the blew patch does: Thanks, diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c index b637a65..6a65e0a 100644 --- a/drivers/usb/host/ehci-q.c +++ b/drivers/usb/host/ehci-q.c @@ -454,6 +454,10 @@ qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh) } } + /* complete URBs after unlinking */ + if (stopped && state != QH_STATE_IDLE) + goto exit; + /* unless we already know the urb's status, collect qtd status * and update count of bytes transferred. in common short read * cases with only one data qtd (including control transfers), @@ -489,15 +493,6 @@ qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh) } } - /* if we're removing something not at the queue head, - * patch the hardware queue pointer. - */ - if (stopped && qtd->qtd_list.prev != &qh->qtd_list) { - last = list_entry (qtd->qtd_list.prev, - struct ehci_qtd, qtd_list); - last->hw_next = qtd->hw_next; - } - /* remove qtd; it's recycled after possible urb completion */ list_del (&qtd->qtd_list); last = qtd; @@ -520,7 +515,7 @@ qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh) /* Otherwise the caller must unlink the QH. */ } - + exit: /* restore original state; caller must unlink or relink */ qh->qh_state = state;