Re: [PATCH] usb:dwc3:fix access poisoned list_head in dwc3_gadget_giveback

[Felipe Balbi]

Hi,

Yu Chen <chenyu56@xxxxxxxxxx> writes:
> From: Yu Chen <chenyu56@xxxxxxxxxx>
>
> Unable to handle kernel paging request at virtual address dead000000000108
> pgd = fffffff7a3179000
> [dead000000000108] *pgd=00000000230e0003, *pud=00000000230e0003,
> *pmd=0000000000000000
> Internal error: Oops: 96000044 [#1] PREEMPT SMP
> Modules linked in:
> CPU: 2 PID: 1 Comm: init Tainted: G        W       4.4.23+ #1

try mainline

> TGID: 1 Comm: init
> Hardware name: kirin970 (DT)
> task: fffffff99f190000 ti: fffffff99f1740e0 task.ti: fffffff99f1740e0
> PC is at dwc3_gadget_giveback+0xa8/0x228
> LR is at dwc3_remove_requests+0x44/0x88
>
> The crash occurred when usb work as rndis device and
> __dwc3_gadget_kick_transfer return error in __dwc3_gadget_ep_queue.
> The request submited in __dwc3_gadget_ep_queue is moved to started_list
> but not kicked. It is stil on started_list although
> __dwc3_gadget_kick_transfer failed. When dwc3_gadget_ep_queue return

why did kick_transfer fail? Where are the tracepoints showing the
failure?

> error to u_ether driver, the request will be resubmit to dwc3 driver.
> At last, the same request is both on started_list and pending_list,
> it will be list_del twice in dwc3_remove_requests and cause crash.
>
> Signed-off-by: Yu Chen <chenyu56@xxxxxxxxxx>
> ---
>  drivers/usb/dwc3/gadget.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 639dd1b163a0..a913e64ca4e0 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1278,9 +1278,28 @@ static void dwc3_gadget_start_isoc(struct dwc3 *dwc,
>  	__dwc3_gadget_start_isoc(dwc, dep, cur_uf);
>  }
>  
> +static int dwc3_gadget_is_req_pengding_or_started(struct dwc3_ep *dep,
> +		struct dwc3_request *req)
> +{
> +	struct dwc3_request *iterate_req;
> +
> +	list_for_each_entry(iterate_req, &dep->pending_list, list) {
> +		if (iterate_req == req)
> +			return 1;
> +	}
> +
> +	list_for_each_entry(iterate_req, &dep->started_list, list) {
> +		if (iterate_req == req)
> +			return 1;
> +	}
> +
> +	return 0;
> +}
> +
>  static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
>  {
>  	struct dwc3		*dwc = dep->dwc;
> +	int			ret;
>  
>  	if (!dep->endpoint.desc) {
>  		dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
> @@ -1334,7 +1353,14 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
>  	}
>  
>  out:
> -	return __dwc3_gadget_kick_transfer(dep);
> +	ret = __dwc3_gadget_kick_transfer(dep);
> +	if (ret && dwc3_gadget_is_req_pengding_or_started(dep, req)) {

first we need to figure out why kick_transfer failed. It shouldn't fail,
so why did it? Then I need you to try with a more recent kernel v4.4 is
rather old and a lot has changed WRT transfer handling:

$ git rev-list --count --no-merges v4.4..linus/master -- drivers/usb/dwc3/gadget.c
184

-- 
balbi

Attachment: signature.asc
Description: PGP signature