linux-kernel - Re: [PATCH 5/9] usb: xhci: use list_for_each

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Date:	Fri, 18 Dec 2015 19:30:20 +0100 (CET)
From:	Julia Lawall <julia.lawall@...6.fr>
To:	Geliang Tang <geliangtang@....com>
cc:	Mathias Nyman <mathias.nyman@...el.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/9] usb: xhci: use list_for_each_entry

Geliang,

Please check whether it is acceptable that last_unlinked_td point to the
dummy entry at th beginning of the list, in the case where the
list_for_each_entry loop runs out normally.

It seems that you have sent a bunch of these patches.  Please recheck them
all to see if they really follow the semantics of list_for_each_entry
properly.  If particular, you should not normally use the index variable
after leaving the loop, unless it is guaranteed that the exit from the
loop was via a break.

julia

On Sat, 19 Dec 2015, kbuild test robot wrote:

> CC: kbuild-all@...org
> In-Reply-To: <0fbea26fdbcb76e24188fc0800d425f927f45b6f.1450455485.git.geliangtang@....com>
> TO: Geliang Tang <geliangtang@....com>
> CC: Mathias Nyman <mathias.nyman@...el.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> CC: Geliang Tang <geliangtang@....com>, linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
>
> Hi Geliang,
>
> [auto build test WARNING on usb/usb-testing]
> [also build test WARNING on v4.4-rc5 next-20151218]
>
> url:    https://github.com/0day-ci/linux/commits/Geliang-Tang/usb-host-fotg210-use-list_for_each_entry_safe/20151219-003955
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
> :::::: branch date: 2 hours ago
> :::::: commit date: 2 hours ago
>
> >> drivers/usb/host/xhci-ring.c:714:20-26: ERROR: invalid reference to the index variable of the iterator on line 672
>
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout 0af06cc5bab47032f7fc8e2e6a3df0fb29513b52
> vim +714 drivers/usb/host/xhci-ring.c
>
> ae6367471 Sarah Sharp      2009-04-29  666
> ae6367471 Sarah Sharp      2009-04-29  667  	/* Fix up the ep ring first, so HW stops executing cancelled TDs.
> ae6367471 Sarah Sharp      2009-04-29  668  	 * We have the xHCI lock, so nothing can modify this list until we drop
> ae6367471 Sarah Sharp      2009-04-29  669  	 * it.  We're also in the event handler, so we can't get re-interrupted
> ae6367471 Sarah Sharp      2009-04-29  670  	 * if another Stop Endpoint command completes
> ae6367471 Sarah Sharp      2009-04-29  671  	 */
> 0af06cc5b Geliang Tang     2015-12-19 @672  	list_for_each_entry(cur_td, &ep->cancelled_td_list, cancelled_td_list) {
> aa50b2906 Xenia Ragiadakou 2013-08-14  673  		xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
> aa50b2906 Xenia Ragiadakou 2013-08-14  674  				"Removing canceled TD starting at 0x%llx (dma).",
> 79688acfb Sarah Sharp      2011-12-19  675  				(unsigned long long)xhci_trb_virt_to_dma(
> 79688acfb Sarah Sharp      2011-12-19  676  					cur_td->start_seg, cur_td->first_trb));
> e9df17eb1 Sarah Sharp      2010-04-02  677  		ep_ring = xhci_urb_to_transfer_ring(xhci, cur_td->urb);
> e9df17eb1 Sarah Sharp      2010-04-02  678  		if (!ep_ring) {
> e9df17eb1 Sarah Sharp      2010-04-02  679  			/* This shouldn't happen unless a driver is mucking
> e9df17eb1 Sarah Sharp      2010-04-02  680  			 * with the stream ID after submission.  This will
> e9df17eb1 Sarah Sharp      2010-04-02  681  			 * leave the TD on the hardware ring, and the hardware
> e9df17eb1 Sarah Sharp      2010-04-02  682  			 * will try to execute it, and may access a buffer
> e9df17eb1 Sarah Sharp      2010-04-02  683  			 * that has already been freed.  In the best case, the
> e9df17eb1 Sarah Sharp      2010-04-02  684  			 * hardware will execute it, and the event handler will
> e9df17eb1 Sarah Sharp      2010-04-02  685  			 * ignore the completion event for that TD, since it was
> e9df17eb1 Sarah Sharp      2010-04-02  686  			 * removed from the td_list for that endpoint.  In
> e9df17eb1 Sarah Sharp      2010-04-02  687  			 * short, don't muck with the stream ID after
> e9df17eb1 Sarah Sharp      2010-04-02  688  			 * submission.
> e9df17eb1 Sarah Sharp      2010-04-02  689  			 */
> e9df17eb1 Sarah Sharp      2010-04-02  690  			xhci_warn(xhci, "WARN Cancelled URB %p "
> e9df17eb1 Sarah Sharp      2010-04-02  691  					"has invalid stream ID %u.\n",
> e9df17eb1 Sarah Sharp      2010-04-02  692  					cur_td->urb,
> e9df17eb1 Sarah Sharp      2010-04-02  693  					cur_td->urb->stream_id);
> e9df17eb1 Sarah Sharp      2010-04-02  694  			goto remove_finished_td;
> e9df17eb1 Sarah Sharp      2010-04-02  695  		}
> ae6367471 Sarah Sharp      2009-04-29  696  		/*
> ae6367471 Sarah Sharp      2009-04-29  697  		 * If we stopped on the TD we need to cancel, then we have to
> ae6367471 Sarah Sharp      2009-04-29  698  		 * move the xHC endpoint ring dequeue pointer past this TD.
> ae6367471 Sarah Sharp      2009-04-29  699  		 */
> 63a0d9abd Sarah Sharp      2009-09-04  700  		if (cur_td == ep->stopped_td)
> e9df17eb1 Sarah Sharp      2010-04-02  701  			xhci_find_new_dequeue_state(xhci, slot_id, ep_index,
> e9df17eb1 Sarah Sharp      2010-04-02  702  					cur_td->urb->stream_id,
> e9df17eb1 Sarah Sharp      2010-04-02  703  					cur_td, &deq_state);
> ae6367471 Sarah Sharp      2009-04-29  704  		else
> 522989a27 Sarah Sharp      2011-07-29  705  			td_to_noop(xhci, ep_ring, cur_td, false);
> e9df17eb1 Sarah Sharp      2010-04-02  706  remove_finished_td:
> ae6367471 Sarah Sharp      2009-04-29  707  		/*
> ae6367471 Sarah Sharp      2009-04-29  708  		 * The event handler won't see a completion for this TD anymore,
> ae6367471 Sarah Sharp      2009-04-29  709  		 * so remove it from the endpoint ring's TD list.  Keep it in
> ae6367471 Sarah Sharp      2009-04-29  710  		 * the cancelled TD list for URB completion later.
> ae6367471 Sarah Sharp      2009-04-29  711  		 */
> 585df1d90 Sarah Sharp      2011-08-02  712  		list_del_init(&cur_td->td_list);
> ae6367471 Sarah Sharp      2009-04-29  713  	}
> ae6367471 Sarah Sharp      2009-04-29 @714  	last_unlinked_td = cur_td;
> 6f5165cf9 Sarah Sharp      2009-10-27  715  	xhci_stop_watchdog_timer_in_irq(xhci, ep);
> ae6367471 Sarah Sharp      2009-04-29  716
> ae6367471 Sarah Sharp      2009-04-29  717  	/* If necessary, queue a Set Transfer Ring Dequeue Pointer command */
>
> :::::: The code at line 714 was first introduced by commit
> :::::: ae636747146ea97efa18e04576acd3416e2514f5 USB: xhci: URB cancellation support.
>
> :::::: TO: Sarah Sharp <sarah.a.sharp@...ux.intel.com>
> :::::: CC: Greg Kroah-Hartman <gregkh@...e.de>
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/