Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Felipe Balbi <balbi@kernel.org>
To:     Wesley Cheng <wcheng@codeaurora.org>, gregkh@linuxfoundation.org,
        peter.chen@kernel.org
Cc:     linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        jackp@codeaurora.org, Wesley Cheng <wcheng@codeaurora.org>
Subject: Re: [PATCH v2] usb: dwc3: gadget: Replace
 list_for_each_entry_safe() if using giveback
In-Reply-To: <1620716636-12422-1-git-send-email-wcheng@codeaurora.org>
References: <1620716636-12422-1-git-send-email-wcheng@codeaurora.org>
Date:   Tue, 11 May 2021 11:13:48 +0300
Message-ID: <87tun9g01v.fsf@kernel.org>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
        micalg=pgp-sha256; protocol="application/pgp-signature"
Precedence: bulk

--=-=-=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable


Hi,

Wesley Cheng <wcheng@codeaurora.org> writes:
> The list_for_each_entry_safe() macro saves the current item (n) and
> the item after (n+1), so that n can be safely removed without
> corrupting the list.  However, when traversing the list and removing
> items using gadget giveback, the DWC3 lock is briefly released,
> allowing other routines to execute.  There is a situation where, while
> items are being removed from the cancelled_list using
> dwc3_gadget_ep_cleanup_cancelled_requests(), the pullup disable
> routine is running in parallel (due to UDC unbind).  As the cleanup
> routine removes n, and the pullup disable removes n+1, once the
> cleanup retakes the DWC3 lock, it references a request who was already
> removed/handled.  With list debug enabled, this leads to a panic.
> Ensure all instances of the macro are replaced where gadget giveback
> is used.
>
> Example call stack:
>
> Thread#1:
> __dwc3_gadget_ep_set_halt() - CLEAR HALT
>   -> dwc3_gadget_ep_cleanup_cancelled_requests()
>     ->list_for_each_entry_safe()
>     ->dwc3_gadget_giveback(n)
>       ->dwc3_gadget_del_and_unmap_request()- n deleted[cancelled_list]
>       ->spin_unlock
>       ->Thread#2 executes
>       ...
>     ->dwc3_gadget_giveback(n+1)
>       ->Already removed!
>
> Thread#2:
> dwc3_gadget_pullup()
>   ->waiting for dwc3 spin_lock
>   ...
>   ->Thread#1 released lock
>   ->dwc3_stop_active_transfers()
>     ->dwc3_remove_requests()
>       ->fetches n+1 item from cancelled_list (n removed by Thread#1)
>       ->dwc3_gadget_giveback()
>         ->dwc3_gadget_del_and_unmap_request()- n+1 deleted[cancelled_list]
>         ->spin_unlock
>
> Fixes: d4f1afe5e896 ("usb: dwc3: gadget: move requests to cancelled_list")
> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
> Reviewed-by: Peter Chen <peter.chen@kernel.org>
> ---
> Changes in v2:
>  - Updated commit message with context call stack of an example scenario
>    seen on device.
>
>  drivers/usb/dwc3/gadget.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index dd80e5c..efa939b 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1737,10 +1737,10 @@ static void dwc3_gadget_ep_skip_trbs(struct dwc3_=
ep *dep, struct dwc3_request *r
>  static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *de=
p)
>  {
>  	struct dwc3_request		*req;
> -	struct dwc3_request		*tmp;
>  	struct dwc3			*dwc =3D dep->dwc;
>=20=20
> -	list_for_each_entry_safe(req, tmp, &dep->cancelled_list, list) {
> +	while (!list_empty(&dep->cancelled_list)) {
> +		req =3D next_request(&dep->cancelled_list);

couldn't this be solved list_replace_init() instead? Then we can keep
using the regular list_for_each_entry_safe() which has an added semantic
meaning due to its name.

=2D-=20
balbi

--=-=-=
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQFFBAEBCAAvFiEE9DumQ60WEZ09LIErzlfNM9wDzUgFAmCaPLwRHGJhbGJpQGtl
cm5lbC5vcmcACgkQzlfNM9wDzUg8+Af+Kaqjllefd0NFPCAIWF3wonEqqn79RqpM
KNiONGML4FhR6hKy6f5kkcIFvMrRGCVg4E7EQ5d3qrcSAgWCF2Ee6IZvUTTp/XWV
0HyOyID2MugKO0XEFHPunuT8i4XywCz8BLIaS7irJoz9oCYPXWcsY95wAPLPxa0C
Zm2PtZ1x5ig7ggz8LkaVRrk7+WsbD8GTb/qnxcvtJ6jZMlrYY03h0GVMuBhUttLW
gYedJOEr2Sfy+6guyIdkz77rqMX2FDuuLo7bHri3vQ0b7dmVYLnhgQpW3FuYkHBo
OtI7BD2USop4ZYcW4l4vw4q6Cv8KRQGwYvQ4cnQlKNm0KrA0tHK0+Q==
=Q9N8
-----END PGP SIGNATURE-----
--=-=-=--