* [PATCH] usb: gadget: composite: req->complete not set, using wrong callback for complete
@ 2021-09-19 19:41 Florian Faber
2021-09-20 4:58 ` Greg KH
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Florian Faber @ 2021-09-19 19:41 UTC (permalink / raw)
To: gregkh; +Cc: linux-usb
In usb_composite_setup_continue, req->complete is not set, leaving the
previous value untouched. After completion of the ep0 transaction, the
UDC would then call whatever complete callback is set with the composite
cdev as context, leading to all sorts of havoc.
Signed-off-by: Florian Faber <faber@faberman.de>
---
drivers/usb/gadget/composite.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 504c1cbc255d..8d497be4be32 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -2518,6 +2518,7 @@ void usb_composite_setup_continue(struct
usb_composite_dev *cdev)
DBG(cdev, "%s: Completing delayed status\n", __func__);
req->length = 0;
req->context = cdev;
+ req->complete = composite_setup_complete;
value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
if (value < 0) {
DBG(cdev, "ep_queue --> %d\n", value);
--
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] usb: gadget: composite: req->complete not set, using wrong callback for complete
2021-09-19 19:41 [PATCH] usb: gadget: composite: req->complete not set, using wrong callback for complete Florian Faber
@ 2021-09-20 4:58 ` Greg KH
2021-09-20 5:46 ` Florian Faber
2021-10-13 8:41 ` [PATCH v2] " Florian Faber
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Greg KH @ 2021-09-20 4:58 UTC (permalink / raw)
To: Florian Faber; +Cc: linux-usb
On Sun, Sep 19, 2021 at 09:41:27PM +0200, Florian Faber wrote:
> In usb_composite_setup_continue, req->complete is not set, leaving the
> previous value untouched. After completion of the ep0 transaction, the UDC
> would then call whatever complete callback is set with the composite cdev as
> context, leading to all sorts of havoc.
>
> Signed-off-by: Florian Faber <faber@faberman.de>
>
> ---
> drivers/usb/gadget/composite.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 504c1cbc255d..8d497be4be32 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -2518,6 +2518,7 @@ void usb_composite_setup_continue(struct
> usb_composite_dev *cdev)
> DBG(cdev, "%s: Completing delayed status\n", __func__);
> req->length = 0;
> req->context = cdev;
> + req->complete = composite_setup_complete;
> value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
> if (value < 0) {
> DBG(cdev, "ep_queue --> %d\n", value);
> --
Is this a bugfix? If so, what commit does this fix?
How can you trigger this issue?
And your patch is line-wrapped and can not be applied :(
thanks,
greg k-h
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] usb: gadget: composite: req->complete not set, using wrong callback for complete
2021-09-20 4:58 ` Greg KH
@ 2021-09-20 5:46 ` Florian Faber
2021-09-20 6:24 ` Greg KH
0 siblings, 1 reply; 15+ messages in thread
From: Florian Faber @ 2021-09-20 5:46 UTC (permalink / raw)
To: Greg KH; +Cc: linux-usb
Greg,
On 9/20/21 6:58 AM, Greg KH wrote:
> On Sun, Sep 19, 2021 at 09:41:27PM +0200, Florian Faber wrote:
>> In usb_composite_setup_continue, req->complete is not set, leaving the
>> previous value untouched. After completion of the ep0 transaction, the UDC
>> would then call whatever complete callback is set with the composite cdev as
>> context, leading to all sorts of havoc.
>>
>> Signed-off-by: Florian Faber <faber@faberman.de>
>>
>> ---
>> drivers/usb/gadget/composite.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>> index 504c1cbc255d..8d497be4be32 100644
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -2518,6 +2518,7 @@ void usb_composite_setup_continue(struct
>> usb_composite_dev *cdev)
>> DBG(cdev, "%s: Completing delayed status\n", __func__);
>> req->length = 0;
>> req->context = cdev;
>> + req->complete = composite_setup_complete;
>> value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
>> if (value < 0) {
>> DBG(cdev, "ep_queue --> %d\n", value);
>> --
>
> Is this a bugfix?
Yes.
> If so, what commit does this fix?
Sorry, this mixed git/mail workflow confuses me a bit. Was I supposed to check that?
> How can you trigger this issue?
I can reliably reproduce it on a device here with three different gadgets (ACM, RNDIS and mass storage). A setup packet for mass storage ends up in RNDIS' complete function instead of composite's when plugging/unplugging. It uses the address set for a previous packet to call back the complete function, which points to rndis_response_complete, but with the correct req->context for composite_setup_complete (cdev). rndis_response_complete is then called from the udc driver with a usb_request on ep0 and assumes req->context is of struct rndis, which of course is fatal.
---------------------------snip---------------------------------
[ 183.795661] [<bf10b31c>] (rndis_response_complete [usb_f_rndis]) from [<bf0ec024>] (xgs_iproc_ep_enable+0x92c/0xd2c [xgs_iproc_udc])
[ 183.795666] r5:df5d73ac r4:df767c80
[ 183.795682] [<bf0ebf20>] (xgs_iproc_ep_enable [xgs_iproc_udc]) from [<bf0eca8c>] (xgs_iproc_ep_queue+0x384/0x5bc [xgs_iproc_udc])
[ 183.795687] r7:df767cb8 r6:df5d7380 r5:df767c80 r4:df5d73ac
[ 183.795706] [<bf0ec708>] (xgs_iproc_ep_queue [xgs_iproc_udc]) from [<c0384fec>] (usb_ep_queue+0x1f0/0x238)
[ 183.795713] r10:43425355 r9:df767c80 r8:df767c80 r7:a00f0013 r6:df5d73ac r5:df767c80
[ 183.795716] r4:df65dea8
[ 183.795743] [<c0384dfc>] (usb_ep_queue) from [<bf0f6910>] (usb_composite_overwrite_options+0x128/0x184 [libcomposite])
[ 183.795750] r9:00055302 r8:df767c80 r7:a00f0013 r6:df65df04 r5:df767c80 r4:df65dea8
[ 183.795777] [<bf0f68e0>] (usb_composite_overwrite_options [libcomposite]) from [<bf0f69f4>] (usb_composite_setup_continue+0x88/0x138 [libcomposite])
[ 183.795782] r7:a00f0013 r6:df65df04 r5:00000000 r4:df65dea8
[ 183.795812] [<bf0f696c>] (usb_composite_setup_continue [libcomposite]) from [<bf120cf8>] (fsg_alloc_inst+0xa5c/0xac8 [usb_f_mass_storage])
[ 183.795819] r9:00055302 r8:00000003 r7:deca5800 r6:00000001 r5:df595a80 r4:deca5948
[ 183.795840] [<bf120a68>] (fsg_alloc_inst [usb_f_mass_storage]) from [<bf120e00>] (fsg_main_thread+0x9c/0x15dc [usb_f_mass_storage])
[ 183.795846] r8:df770000 r7:df595a80 r6:deca1cc0 r5:df724000 r4:deca5800
[ 183.795864] [<bf120d64>] (fsg_main_thread [usb_f_mass_storage]) from [<c0046cd0>] (kthread+0x14c/0x154)
[ 183.795870] r10:df785d14 r9:00000000 r8:deca5800 r7:df6c31b8 r6:df70f580 r5:df724000
[ 183.795873] r4:df6c3180
[ 183.795881] [<c0046b84>] (kthread) from [<c000a67c>] (ret_from_fork+0x14/0x38)
[ 183.795887] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0046b84
[ 183.795889] r4:df70f580
--------------------------snip-------------------------------------
> And your patch is line-wrapped and can not be applied :(
Weird, I had line wrapping disabled and it looks fine in thunderbird.
---
drivers/usb/gadget/composite.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 504c1cbc255d..8d497be4be32 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -2518,6 +2518,7 @@ void usb_composite_setup_continue(struct usb_composite_dev *cdev)
DBG(cdev, "%s: Completing delayed status\n", __func__);
req->length = 0;
req->context = cdev;
+ req->complete = composite_setup_complete;
value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
if (value < 0) {
DBG(cdev, "ep_queue --> %d\n", value);
--
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] usb: gadget: composite: req->complete not set, using wrong callback for complete
2021-09-20 5:46 ` Florian Faber
@ 2021-09-20 6:24 ` Greg KH
0 siblings, 0 replies; 15+ messages in thread
From: Greg KH @ 2021-09-20 6:24 UTC (permalink / raw)
To: Florian Faber; +Cc: linux-usb
On Mon, Sep 20, 2021 at 07:46:34AM +0200, Florian Faber wrote:
> Greg,
>
> On 9/20/21 6:58 AM, Greg KH wrote:
> > On Sun, Sep 19, 2021 at 09:41:27PM +0200, Florian Faber wrote:
> > > In usb_composite_setup_continue, req->complete is not set, leaving the
> > > previous value untouched. After completion of the ep0 transaction, the UDC
> > > would then call whatever complete callback is set with the composite cdev as
> > > context, leading to all sorts of havoc.
> > >
> > > Signed-off-by: Florian Faber <faber@faberman.de>
> > >
> > > ---
> > > drivers/usb/gadget/composite.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> > > index 504c1cbc255d..8d497be4be32 100644
> > > --- a/drivers/usb/gadget/composite.c
> > > +++ b/drivers/usb/gadget/composite.c
> > > @@ -2518,6 +2518,7 @@ void usb_composite_setup_continue(struct
> > > usb_composite_dev *cdev)
> > > DBG(cdev, "%s: Completing delayed status\n", __func__);
> > > req->length = 0;
> > > req->context = cdev;
> > > + req->complete = composite_setup_complete;
> > > value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
> > > if (value < 0) {
> > > DBG(cdev, "ep_queue --> %d\n", value);
> > > --
> >
> > Is this a bugfix?
>
> Yes.
>
> > If so, what commit does this fix?
>
> Sorry, this mixed git/mail workflow confuses me a bit. Was I supposed to check that?
Yes please, otherwise we do not know how far back to backport this fix
to, right?
> > How can you trigger this issue?
>
> I can reliably reproduce it on a device here with three different gadgets (ACM, RNDIS and mass storage). A setup packet for mass storage ends up in RNDIS' complete function instead of composite's when plugging/unplugging. It uses the address set for a previous packet to call back the complete function, which points to rndis_response_complete, but with the correct req->context for composite_setup_complete (cdev). rndis_response_complete is then called from the udc driver with a usb_request on ep0 and assumes req->context is of struct rndis, which of course is fatal.
Please line-wrap your email text :(
>
> ---------------------------snip---------------------------------
> [ 183.795661] [<bf10b31c>] (rndis_response_complete [usb_f_rndis]) from [<bf0ec024>] (xgs_iproc_ep_enable+0x92c/0xd2c [xgs_iproc_udc])
> [ 183.795666] r5:df5d73ac r4:df767c80
> [ 183.795682] [<bf0ebf20>] (xgs_iproc_ep_enable [xgs_iproc_udc]) from [<bf0eca8c>] (xgs_iproc_ep_queue+0x384/0x5bc [xgs_iproc_udc])
> [ 183.795687] r7:df767cb8 r6:df5d7380 r5:df767c80 r4:df5d73ac
> [ 183.795706] [<bf0ec708>] (xgs_iproc_ep_queue [xgs_iproc_udc]) from [<c0384fec>] (usb_ep_queue+0x1f0/0x238)
> [ 183.795713] r10:43425355 r9:df767c80 r8:df767c80 r7:a00f0013 r6:df5d73ac r5:df767c80
> [ 183.795716] r4:df65dea8
> [ 183.795743] [<c0384dfc>] (usb_ep_queue) from [<bf0f6910>] (usb_composite_overwrite_options+0x128/0x184 [libcomposite])
> [ 183.795750] r9:00055302 r8:df767c80 r7:a00f0013 r6:df65df04 r5:df767c80 r4:df65dea8
> [ 183.795777] [<bf0f68e0>] (usb_composite_overwrite_options [libcomposite]) from [<bf0f69f4>] (usb_composite_setup_continue+0x88/0x138 [libcomposite])
> [ 183.795782] r7:a00f0013 r6:df65df04 r5:00000000 r4:df65dea8
> [ 183.795812] [<bf0f696c>] (usb_composite_setup_continue [libcomposite]) from [<bf120cf8>] (fsg_alloc_inst+0xa5c/0xac8 [usb_f_mass_storage])
> [ 183.795819] r9:00055302 r8:00000003 r7:deca5800 r6:00000001 r5:df595a80 r4:deca5948
> [ 183.795840] [<bf120a68>] (fsg_alloc_inst [usb_f_mass_storage]) from [<bf120e00>] (fsg_main_thread+0x9c/0x15dc [usb_f_mass_storage])
> [ 183.795846] r8:df770000 r7:df595a80 r6:deca1cc0 r5:df724000 r4:deca5800
> [ 183.795864] [<bf120d64>] (fsg_main_thread [usb_f_mass_storage]) from [<c0046cd0>] (kthread+0x14c/0x154)
> [ 183.795870] r10:df785d14 r9:00000000 r8:deca5800 r7:df6c31b8 r6:df70f580 r5:df724000
> [ 183.795873] r4:df6c3180
> [ 183.795881] [<c0046b84>] (kthread) from [<c000a67c>] (ret_from_fork+0x14/0x38)
> [ 183.795887] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0046b84
> [ 183.795889] r4:df70f580
> --------------------------snip-------------------------------------
>
> > And your patch is line-wrapped and can not be applied :(
>
> Weird, I had line wrapping disabled and it looks fine in thunderbird.
Odd, it looks ok now as well, must have been my editor this morning,
sorry.
Please resend a v2 with the Fixes: tag and more changelog information so
that we can properly apply this.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2] usb: gadget: composite: req->complete not set, using wrong callback for complete
2021-09-19 19:41 [PATCH] usb: gadget: composite: req->complete not set, using wrong callback for complete Florian Faber
2021-09-20 4:58 ` Greg KH
@ 2021-10-13 8:41 ` Florian Faber
2021-10-13 8:48 ` Greg KH
2021-10-13 8:54 ` [PATCH v3] " Florian Faber
2021-10-13 14:15 ` [PATCH v4] " Florian Faber
3 siblings, 1 reply; 15+ messages in thread
From: Florian Faber @ 2021-10-13 8:41 UTC (permalink / raw)
To: linux-usb; +Cc: gregkh
In usb_composite_setup_continue, req->complete is not set, leaving the
previous value untouched. After completion of the ep0 transaction, the
UDC would then call whatever complete callback was set previously with
the composite cdev as context, leading to all sorts of havoc.
A typical call trace looks like this: A setup packet for mass storage,
ending up in RNDIS's complete function:
---------------------------snip---------------------------------
[ 183.795661] [<bf10b31c>] (rndis_response_complete [usb_f_rndis]) from [<bf0ec024>] (xgs_iproc_ep_enable+0x92c/0xd2c [xgs_iproc_udc])
[ 183.795666] r5:df5d73ac r4:df767c80
[ 183.795682] [<bf0ebf20>] (xgs_iproc_ep_enable [xgs_iproc_udc]) from [<bf0eca8c>] (xgs_iproc_ep_queue+0x384/0x5bc [xgs_iproc_udc])
[ 183.795687] r7:df767cb8 r6:df5d7380 r5:df767c80 r4:df5d73ac
[ 183.795706] [<bf0ec708>] (xgs_iproc_ep_queue [xgs_iproc_udc]) from [<c0384fec>] (usb_ep_queue+0x1f0/0x238)
[ 183.795713] r10:43425355 r9:df767c80 r8:df767c80 r7:a00f0013 r6:df5d73ac r5:df767c80
[ 183.795716] r4:df65dea8
[ 183.795743] [<c0384dfc>] (usb_ep_queue) from [<bf0f6910>] (usb_composite_overwrite_options+0x128/0x184 [libcomposite])
[ 183.795750] r9:00055302 r8:df767c80 r7:a00f0013 r6:df65df04 r5:df767c80 r4:df65dea8
[ 183.795777] [<bf0f68e0>] (usb_composite_overwrite_options [libcomposite]) from [<bf0f69f4>] (usb_composite_setup_continue+0x88/0x138 [libcomposite])
[ 183.795782] r7:a00f0013 r6:df65df04 r5:00000000 r4:df65dea8
[ 183.795812] [<bf0f696c>] (usb_composite_setup_continue [libcomposite]) from [<bf120cf8>] (fsg_alloc_inst+0xa5c/0xac8 [usb_f_mass_storage])
[ 183.795819] r9:00055302 r8:00000003 r7:deca5800 r6:00000001 r5:df595a80 r4:deca5948
[ 183.795840] [<bf120a68>] (fsg_alloc_inst [usb_f_mass_storage]) from [<bf120e00>] (fsg_main_thread+0x9c/0x15dc [usb_f_mass_storage])
[ 183.795846] r8:df770000 r7:df595a80 r6:deca1cc0 r5:df724000 r4:deca5800
[ 183.795864] [<bf120d64>] (fsg_main_thread [usb_f_mass_storage]) from [<c0046cd0>] (kthread+0x14c/0x154)
[ 183.795870] r10:df785d14 r9:00000000 r8:deca5800 r7:df6c31b8 r6:df70f580 r5:df724000
[ 183.795873] r4:df6c3180
[ 183.795881] [<c0046b84>] (kthread) from [<c000a67c>] (ret_from_fork+0x14/0x38)
[ 183.795887] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0046b84
[ 183.795889] r4:df70f580
--------------------------snip-------------------------------------
Fixes: 57943716ff1b0733ab0d9879e572bad04166660a ("usb: gadget: composite: set our req->context to cdev")
Signed-off-by: Florian Faber <faber@faberman.de>
---
drivers/usb/gadget/composite.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 504c1cbc255d..8d497be4be32 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -2518,6 +2518,7 @@ void usb_composite_setup_continue(struct
usb_composite_dev *cdev)
DBG(cdev, "%s: Completing delayed status\n", __func__);
req->length = 0;
req->context = cdev;
+ req->complete = composite_setup_complete;
value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
if (value < 0) {
DBG(cdev, "ep_queue --> %d\n", value);
--
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2] usb: gadget: composite: req->complete not set, using wrong callback for complete
2021-10-13 8:41 ` [PATCH v2] " Florian Faber
@ 2021-10-13 8:48 ` Greg KH
0 siblings, 0 replies; 15+ messages in thread
From: Greg KH @ 2021-10-13 8:48 UTC (permalink / raw)
To: Florian Faber; +Cc: linux-usb
On Wed, Oct 13, 2021 at 10:41:21AM +0200, Florian Faber wrote:
> In usb_composite_setup_continue, req->complete is not set, leaving the
> previous value untouched. After completion of the ep0 transaction, the
> UDC would then call whatever complete callback was set previously with
> the composite cdev as context, leading to all sorts of havoc.
>
> A typical call trace looks like this: A setup packet for mass storage,
> ending up in RNDIS's complete function:
>
> ---------------------------snip---------------------------------
> [ 183.795661] [<bf10b31c>] (rndis_response_complete [usb_f_rndis]) from [<bf0ec024>] (xgs_iproc_ep_enable+0x92c/0xd2c [xgs_iproc_udc])
> [ 183.795666] r5:df5d73ac r4:df767c80
> [ 183.795682] [<bf0ebf20>] (xgs_iproc_ep_enable [xgs_iproc_udc]) from [<bf0eca8c>] (xgs_iproc_ep_queue+0x384/0x5bc [xgs_iproc_udc])
> [ 183.795687] r7:df767cb8 r6:df5d7380 r5:df767c80 r4:df5d73ac
> [ 183.795706] [<bf0ec708>] (xgs_iproc_ep_queue [xgs_iproc_udc]) from [<c0384fec>] (usb_ep_queue+0x1f0/0x238)
> [ 183.795713] r10:43425355 r9:df767c80 r8:df767c80 r7:a00f0013 r6:df5d73ac r5:df767c80
> [ 183.795716] r4:df65dea8
> [ 183.795743] [<c0384dfc>] (usb_ep_queue) from [<bf0f6910>] (usb_composite_overwrite_options+0x128/0x184 [libcomposite])
> [ 183.795750] r9:00055302 r8:df767c80 r7:a00f0013 r6:df65df04 r5:df767c80 r4:df65dea8
> [ 183.795777] [<bf0f68e0>] (usb_composite_overwrite_options [libcomposite]) from [<bf0f69f4>] (usb_composite_setup_continue+0x88/0x138 [libcomposite])
> [ 183.795782] r7:a00f0013 r6:df65df04 r5:00000000 r4:df65dea8
> [ 183.795812] [<bf0f696c>] (usb_composite_setup_continue [libcomposite]) from [<bf120cf8>] (fsg_alloc_inst+0xa5c/0xac8 [usb_f_mass_storage])
> [ 183.795819] r9:00055302 r8:00000003 r7:deca5800 r6:00000001 r5:df595a80 r4:deca5948
> [ 183.795840] [<bf120a68>] (fsg_alloc_inst [usb_f_mass_storage]) from [<bf120e00>] (fsg_main_thread+0x9c/0x15dc [usb_f_mass_storage])
> [ 183.795846] r8:df770000 r7:df595a80 r6:deca1cc0 r5:df724000 r4:deca5800
> [ 183.795864] [<bf120d64>] (fsg_main_thread [usb_f_mass_storage]) from [<c0046cd0>] (kthread+0x14c/0x154)
> [ 183.795870] r10:df785d14 r9:00000000 r8:deca5800 r7:df6c31b8 r6:df70f580 r5:df724000
> [ 183.795873] r4:df6c3180
> [ 183.795881] [<c0046b84>] (kthread) from [<c000a67c>] (ret_from_fork+0x14/0x38)
> [ 183.795887] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0046b84
> [ 183.795889] r4:df70f580
> --------------------------snip-------------------------------------
>
> Fixes: 57943716ff1b0733ab0d9879e572bad04166660a ("usb: gadget: composite: set our req->context to cdev")
> Signed-off-by: Florian Faber <faber@faberman.de>
>
> ---
> drivers/usb/gadget/composite.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 504c1cbc255d..8d497be4be32 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -2518,6 +2518,7 @@ void usb_composite_setup_continue(struct
> usb_composite_dev *cdev)
> DBG(cdev, "%s: Completing delayed status\n", __func__);
> req->length = 0;
> req->context = cdev;
> + req->complete = composite_setup_complete;
> value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
> if (value < 0) {
> DBG(cdev, "ep_queue --> %d\n", value);
> --
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- This looks like a new version of a previously submitted patch, but you
did not list below the --- line any changes from the previous version.
Please read the section entitled "The canonical patch format" in the
kernel file, Documentation/SubmittingPatches for what needs to be done
here to properly describe this.
If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.
thanks,
greg k-h's patch email bot
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3] usb: gadget: composite: req->complete not set, using wrong callback for complete
2021-09-19 19:41 [PATCH] usb: gadget: composite: req->complete not set, using wrong callback for complete Florian Faber
2021-09-20 4:58 ` Greg KH
2021-10-13 8:41 ` [PATCH v2] " Florian Faber
@ 2021-10-13 8:54 ` Florian Faber
2021-10-13 12:05 ` Greg KH
2021-10-13 14:15 ` [PATCH v4] " Florian Faber
3 siblings, 1 reply; 15+ messages in thread
From: Florian Faber @ 2021-10-13 8:54 UTC (permalink / raw)
To: linux-usb; +Cc: gregkh
In usb_composite_setup_continue, req->complete is not set, leaving the
previous value untouched. After completion of the ep0 transaction, the
UDC would then call whatever complete callback was set previously with
the composite cdev as context, leading to all sorts of havoc.
A typical call trace looks like this: A setup packet for mass storage,
ending up in RNDIS's complete function:
---------------------------snip---------------------------------
[ 183.795661] [<bf10b31c>] (rndis_response_complete [usb_f_rndis]) from [<bf0ec024>] (xgs_iproc_ep_enable+0x92c/0xd2c [xgs_iproc_udc])
[ 183.795666] r5:df5d73ac r4:df767c80
[ 183.795682] [<bf0ebf20>] (xgs_iproc_ep_enable [xgs_iproc_udc]) from [<bf0eca8c>] (xgs_iproc_ep_queue+0x384/0x5bc [xgs_iproc_udc])
[ 183.795687] r7:df767cb8 r6:df5d7380 r5:df767c80 r4:df5d73ac
[ 183.795706] [<bf0ec708>] (xgs_iproc_ep_queue [xgs_iproc_udc]) from [<c0384fec>] (usb_ep_queue+0x1f0/0x238)
[ 183.795713] r10:43425355 r9:df767c80 r8:df767c80 r7:a00f0013 r6:df5d73ac r5:df767c80
[ 183.795716] r4:df65dea8
[ 183.795743] [<c0384dfc>] (usb_ep_queue) from [<bf0f6910>] (usb_composite_overwrite_options+0x128/0x184 [libcomposite])
[ 183.795750] r9:00055302 r8:df767c80 r7:a00f0013 r6:df65df04 r5:df767c80 r4:df65dea8
[ 183.795777] [<bf0f68e0>] (usb_composite_overwrite_options [libcomposite]) from [<bf0f69f4>] (usb_composite_setup_continue+0x88/0x138 [libcomposite])
[ 183.795782] r7:a00f0013 r6:df65df04 r5:00000000 r4:df65dea8
[ 183.795812] [<bf0f696c>] (usb_composite_setup_continue [libcomposite]) from [<bf120cf8>] (fsg_alloc_inst+0xa5c/0xac8 [usb_f_mass_storage])
[ 183.795819] r9:00055302 r8:00000003 r7:deca5800 r6:00000001 r5:df595a80 r4:deca5948
[ 183.795840] [<bf120a68>] (fsg_alloc_inst [usb_f_mass_storage]) from [<bf120e00>] (fsg_main_thread+0x9c/0x15dc [usb_f_mass_storage])
[ 183.795846] r8:df770000 r7:df595a80 r6:deca1cc0 r5:df724000 r4:deca5800
[ 183.795864] [<bf120d64>] (fsg_main_thread [usb_f_mass_storage]) from [<c0046cd0>] (kthread+0x14c/0x154)
[ 183.795870] r10:df785d14 r9:00000000 r8:deca5800 r7:df6c31b8 r6:df70f580 r5:df724000
[ 183.795873] r4:df6c3180
[ 183.795881] [<c0046b84>] (kthread) from [<c000a67c>] (ret_from_fork+0x14/0x38)
[ 183.795887] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0046b84
[ 183.795889] r4:df70f580
--------------------------snip-------------------------------------
Fixes: 57943716ff1b0733ab0d9879e572bad04166660a ("usb: gadget: composite: set our req->context to cdev")
Signed-off-by: Florian Faber <faber@faberman.de>
---
Change in v3:
- Addes changes
Change in v2:
- More verbose explanation
- Added commit hash that introduced the bug
drivers/usb/gadget/composite.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 504c1cbc255d..8d497be4be32 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -2518,6 +2518,7 @@ void usb_composite_setup_continue(struct
usb_composite_dev *cdev)
DBG(cdev, "%s: Completing delayed status\n", __func__);
req->length = 0;
req->context = cdev;
+ req->complete = composite_setup_complete;
value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
if (value < 0) {
DBG(cdev, "ep_queue --> %d\n", value);
--
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3] usb: gadget: composite: req->complete not set, using wrong callback for complete
2021-10-13 8:54 ` [PATCH v3] " Florian Faber
@ 2021-10-13 12:05 ` Greg KH
0 siblings, 0 replies; 15+ messages in thread
From: Greg KH @ 2021-10-13 12:05 UTC (permalink / raw)
To: Florian Faber; +Cc: linux-usb
On Wed, Oct 13, 2021 at 10:54:47AM +0200, Florian Faber wrote:
> In usb_composite_setup_continue, req->complete is not set, leaving the
> previous value untouched. After completion of the ep0 transaction, the
> UDC would then call whatever complete callback was set previously with
> the composite cdev as context, leading to all sorts of havoc.
>
> A typical call trace looks like this: A setup packet for mass storage,
> ending up in RNDIS's complete function:
>
> ---------------------------snip---------------------------------
> [ 183.795661] [<bf10b31c>] (rndis_response_complete [usb_f_rndis]) from [<bf0ec024>] (xgs_iproc_ep_enable+0x92c/0xd2c [xgs_iproc_udc])
> [ 183.795666] r5:df5d73ac r4:df767c80
> [ 183.795682] [<bf0ebf20>] (xgs_iproc_ep_enable [xgs_iproc_udc]) from [<bf0eca8c>] (xgs_iproc_ep_queue+0x384/0x5bc [xgs_iproc_udc])
> [ 183.795687] r7:df767cb8 r6:df5d7380 r5:df767c80 r4:df5d73ac
> [ 183.795706] [<bf0ec708>] (xgs_iproc_ep_queue [xgs_iproc_udc]) from [<c0384fec>] (usb_ep_queue+0x1f0/0x238)
> [ 183.795713] r10:43425355 r9:df767c80 r8:df767c80 r7:a00f0013 r6:df5d73ac r5:df767c80
> [ 183.795716] r4:df65dea8
> [ 183.795743] [<c0384dfc>] (usb_ep_queue) from [<bf0f6910>] (usb_composite_overwrite_options+0x128/0x184 [libcomposite])
> [ 183.795750] r9:00055302 r8:df767c80 r7:a00f0013 r6:df65df04 r5:df767c80 r4:df65dea8
> [ 183.795777] [<bf0f68e0>] (usb_composite_overwrite_options [libcomposite]) from [<bf0f69f4>] (usb_composite_setup_continue+0x88/0x138 [libcomposite])
> [ 183.795782] r7:a00f0013 r6:df65df04 r5:00000000 r4:df65dea8
> [ 183.795812] [<bf0f696c>] (usb_composite_setup_continue [libcomposite]) from [<bf120cf8>] (fsg_alloc_inst+0xa5c/0xac8 [usb_f_mass_storage])
> [ 183.795819] r9:00055302 r8:00000003 r7:deca5800 r6:00000001 r5:df595a80 r4:deca5948
> [ 183.795840] [<bf120a68>] (fsg_alloc_inst [usb_f_mass_storage]) from [<bf120e00>] (fsg_main_thread+0x9c/0x15dc [usb_f_mass_storage])
> [ 183.795846] r8:df770000 r7:df595a80 r6:deca1cc0 r5:df724000 r4:deca5800
> [ 183.795864] [<bf120d64>] (fsg_main_thread [usb_f_mass_storage]) from [<c0046cd0>] (kthread+0x14c/0x154)
> [ 183.795870] r10:df785d14 r9:00000000 r8:deca5800 r7:df6c31b8 r6:df70f580 r5:df724000
> [ 183.795873] r4:df6c3180
> [ 183.795881] [<c0046b84>] (kthread) from [<c000a67c>] (ret_from_fork+0x14/0x38)
> [ 183.795887] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0046b84
> [ 183.795889] r4:df70f580
> --------------------------snip-------------------------------------
>
> Fixes: 57943716ff1b0733ab0d9879e572bad04166660a ("usb: gadget: composite: set our req->context to cdev")
No need for the full sha1, look at the documentation for the needed line
here:
Fixes: 57943716ff1b ("usb: gadget: composite: set our req->context to cdev")
> Signed-off-by: Florian Faber <faber@faberman.de>
>
> ---
> Change in v3:
> - Addes changes
>
> Change in v2:
> - More verbose explanation
> - Added commit hash that introduced the bug
>
> drivers/usb/gadget/composite.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 504c1cbc255d..8d497be4be32 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -2518,6 +2518,7 @@ void usb_composite_setup_continue(struct
> usb_composite_dev *cdev)
> DBG(cdev, "%s: Completing delayed status\n", __func__);
> req->length = 0;
> req->context = cdev;
> + req->complete = composite_setup_complete;
> value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
> if (value < 0) {
> DBG(cdev, "ep_queue --> %d\n", value);
> --
Patch is linewrapped :(
thanks,
greg k-h
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4] usb: gadget: composite: req->complete not set, using wrong callback for complete
2021-09-19 19:41 [PATCH] usb: gadget: composite: req->complete not set, using wrong callback for complete Florian Faber
` (2 preceding siblings ...)
2021-10-13 8:54 ` [PATCH v3] " Florian Faber
@ 2021-10-13 14:15 ` Florian Faber
2021-11-17 14:01 ` Greg KH
2021-11-27 5:20 ` Peter Chen
3 siblings, 2 replies; 15+ messages in thread
From: Florian Faber @ 2021-10-13 14:15 UTC (permalink / raw)
To: linux-usb; +Cc: gregkh
In usb_composite_setup_continue, req->complete is not set, leaving the
previous value untouched. After completion of the ep0 transaction, the
UDC would then call whatever complete callback was set previously with
the composite cdev as context, leading to all sorts of havoc.
A typical call trace looks like this: A setup packet for mass storage,
ending up in RNDIS's complete function:
---------------------------snip---------------------------------
[ 183.795661] [<bf10b31c>] (rndis_response_complete [usb_f_rndis]) from [<bf0ec024>] (xgs_iproc_ep_enable+0x92c/0xd2c [xgs_iproc_udc])
[ 183.795666] r5:df5d73ac r4:df767c80
[ 183.795682] [<bf0ebf20>] (xgs_iproc_ep_enable [xgs_iproc_udc]) from [<bf0eca8c>] (xgs_iproc_ep_queue+0x384/0x5bc [xgs_iproc_udc])
[ 183.795687] r7:df767cb8 r6:df5d7380 r5:df767c80 r4:df5d73ac
[ 183.795706] [<bf0ec708>] (xgs_iproc_ep_queue [xgs_iproc_udc]) from [<c0384fec>] (usb_ep_queue+0x1f0/0x238)
[ 183.795713] r10:43425355 r9:df767c80 r8:df767c80 r7:a00f0013 r6:df5d73ac r5:df767c80
[ 183.795716] r4:df65dea8
[ 183.795743] [<c0384dfc>] (usb_ep_queue) from [<bf0f6910>] (usb_composite_overwrite_options+0x128/0x184 [libcomposite])
[ 183.795750] r9:00055302 r8:df767c80 r7:a00f0013 r6:df65df04 r5:df767c80 r4:df65dea8
[ 183.795777] [<bf0f68e0>] (usb_composite_overwrite_options [libcomposite]) from [<bf0f69f4>] (usb_composite_setup_continue+0x88/0x138 [libcomposite])
[ 183.795782] r7:a00f0013 r6:df65df04 r5:00000000 r4:df65dea8
[ 183.795812] [<bf0f696c>] (usb_composite_setup_continue [libcomposite]) from [<bf120cf8>] (fsg_alloc_inst+0xa5c/0xac8 [usb_f_mass_storage])
[ 183.795819] r9:00055302 r8:00000003 r7:deca5800 r6:00000001 r5:df595a80 r4:deca5948
[ 183.795840] [<bf120a68>] (fsg_alloc_inst [usb_f_mass_storage]) from [<bf120e00>] (fsg_main_thread+0x9c/0x15dc [usb_f_mass_storage])
[ 183.795846] r8:df770000 r7:df595a80 r6:deca1cc0 r5:df724000 r4:deca5800
[ 183.795864] [<bf120d64>] (fsg_main_thread [usb_f_mass_storage]) from [<c0046cd0>] (kthread+0x14c/0x154)
[ 183.795870] r10:df785d14 r9:00000000 r8:deca5800 r7:df6c31b8 r6:df70f580 r5:df724000
[ 183.795873] r4:df6c3180
[ 183.795881] [<c0046b84>] (kthread) from [<c000a67c>] (ret_from_fork+0x14/0x38)
[ 183.795887] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0046b84
[ 183.795889] r4:df70f580
--------------------------snip-------------------------------------
Fixes: 57943716ff1b ("usb: gadget: composite: set our req->context to cdev")
Signed-off-by: Florian Faber <faber@faberman.de>
---
Change in v4:
- Short commit hash
- Fix line wrap
Change in v3:
- Addes changes
Change in v2:
- More verbose explanation
- Added commit hash that introduced the bug
drivers/usb/gadget/composite.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 504c1cbc255d..8d497be4be32 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -2518,6 +2518,7 @@ void usb_composite_setup_continue(struct usb_composite_dev *cdev)
DBG(cdev, "%s: Completing delayed status\n", __func__);
req->length = 0;
req->context = cdev;
+ req->complete = composite_setup_complete;
value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
if (value < 0) {
DBG(cdev, "ep_queue --> %d\n", value);
--
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v4] usb: gadget: composite: req->complete not set, using wrong callback for complete
2021-10-13 14:15 ` [PATCH v4] " Florian Faber
@ 2021-11-17 14:01 ` Greg KH
2021-11-17 14:06 ` Florian Faber
2021-11-27 5:20 ` Peter Chen
1 sibling, 1 reply; 15+ messages in thread
From: Greg KH @ 2021-11-17 14:01 UTC (permalink / raw)
To: Florian Faber; +Cc: linux-usb
On Wed, Oct 13, 2021 at 04:15:13PM +0200, Florian Faber wrote:
> In usb_composite_setup_continue, req->complete is not set, leaving the
> previous value untouched. After completion of the ep0 transaction, the
> UDC would then call whatever complete callback was set previously with
> the composite cdev as context, leading to all sorts of havoc.
>
> A typical call trace looks like this: A setup packet for mass storage,
> ending up in RNDIS's complete function:
>
> ---------------------------snip---------------------------------
> [ 183.795661] [<bf10b31c>] (rndis_response_complete [usb_f_rndis]) from [<bf0ec024>] (xgs_iproc_ep_enable+0x92c/0xd2c [xgs_iproc_udc])
> [ 183.795666] r5:df5d73ac r4:df767c80
> [ 183.795682] [<bf0ebf20>] (xgs_iproc_ep_enable [xgs_iproc_udc]) from [<bf0eca8c>] (xgs_iproc_ep_queue+0x384/0x5bc [xgs_iproc_udc])
> [ 183.795687] r7:df767cb8 r6:df5d7380 r5:df767c80 r4:df5d73ac
> [ 183.795706] [<bf0ec708>] (xgs_iproc_ep_queue [xgs_iproc_udc]) from [<c0384fec>] (usb_ep_queue+0x1f0/0x238)
> [ 183.795713] r10:43425355 r9:df767c80 r8:df767c80 r7:a00f0013 r6:df5d73ac r5:df767c80
> [ 183.795716] r4:df65dea8
> [ 183.795743] [<c0384dfc>] (usb_ep_queue) from [<bf0f6910>] (usb_composite_overwrite_options+0x128/0x184 [libcomposite])
> [ 183.795750] r9:00055302 r8:df767c80 r7:a00f0013 r6:df65df04 r5:df767c80 r4:df65dea8
> [ 183.795777] [<bf0f68e0>] (usb_composite_overwrite_options [libcomposite]) from [<bf0f69f4>] (usb_composite_setup_continue+0x88/0x138 [libcomposite])
> [ 183.795782] r7:a00f0013 r6:df65df04 r5:00000000 r4:df65dea8
> [ 183.795812] [<bf0f696c>] (usb_composite_setup_continue [libcomposite]) from [<bf120cf8>] (fsg_alloc_inst+0xa5c/0xac8 [usb_f_mass_storage])
> [ 183.795819] r9:00055302 r8:00000003 r7:deca5800 r6:00000001 r5:df595a80 r4:deca5948
> [ 183.795840] [<bf120a68>] (fsg_alloc_inst [usb_f_mass_storage]) from [<bf120e00>] (fsg_main_thread+0x9c/0x15dc [usb_f_mass_storage])
> [ 183.795846] r8:df770000 r7:df595a80 r6:deca1cc0 r5:df724000 r4:deca5800
> [ 183.795864] [<bf120d64>] (fsg_main_thread [usb_f_mass_storage]) from [<c0046cd0>] (kthread+0x14c/0x154)
> [ 183.795870] r10:df785d14 r9:00000000 r8:deca5800 r7:df6c31b8 r6:df70f580 r5:df724000
> [ 183.795873] r4:df6c3180
> [ 183.795881] [<c0046b84>] (kthread) from [<c000a67c>] (ret_from_fork+0x14/0x38)
> [ 183.795887] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0046b84
> [ 183.795889] r4:df70f580
> --------------------------snip-------------------------------------
>
> Fixes: 57943716ff1b ("usb: gadget: composite: set our req->context to cdev")
> Signed-off-by: Florian Faber <faber@faberman.de>
>
> ---
> Change in v4:
> - Short commit hash
> - Fix line wrap
>
> Change in v3:
> - Addes changes
>
> Change in v2:
> - More verbose explanation
> - Added commit hash that introduced the bug
>
> drivers/usb/gadget/composite.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 504c1cbc255d..8d497be4be32 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -2518,6 +2518,7 @@ void usb_composite_setup_continue(struct usb_composite_dev *cdev)
> DBG(cdev, "%s: Completing delayed status\n", __func__);
> req->length = 0;
> req->context = cdev;
> + req->complete = composite_setup_complete;
> value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
> if (value < 0) {
> DBG(cdev, "ep_queue --> %d\n", value);
> --
Does not apply to 5.16-rc1 :(
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] usb: gadget: composite: req->complete not set, using wrong callback for complete
2021-11-17 14:01 ` Greg KH
@ 2021-11-17 14:06 ` Florian Faber
2021-11-17 14:51 ` Greg KH
0 siblings, 1 reply; 15+ messages in thread
From: Florian Faber @ 2021-11-17 14:06 UTC (permalink / raw)
To: Greg KH; +Cc: linux-usb
Greg,
On 11/17/21 15:01, Greg KH wrote:
> On Wed, Oct 13, 2021 at 04:15:13PM +0200, Florian Faber wrote:
>> In usb_composite_setup_continue, req->complete is not set, leaving the
>> previous value untouched. After completion of the ep0 transaction, the
>> UDC would then call whatever complete callback was set previously with
>> the composite cdev as context, leading to all sorts of havoc.
>>
>> A typical call trace looks like this: A setup packet for mass storage,
>> ending up in RNDIS's complete function:
>>
>> ---------------------------snip---------------------------------
>> [ 183.795661] [<bf10b31c>] (rndis_response_complete [usb_f_rndis]) from [<bf0ec024>] (xgs_iproc_ep_enable+0x92c/0xd2c [xgs_iproc_udc])
>> [ 183.795666] r5:df5d73ac r4:df767c80
>> [ 183.795682] [<bf0ebf20>] (xgs_iproc_ep_enable [xgs_iproc_udc]) from [<bf0eca8c>] (xgs_iproc_ep_queue+0x384/0x5bc [xgs_iproc_udc])
>> [ 183.795687] r7:df767cb8 r6:df5d7380 r5:df767c80 r4:df5d73ac
>> [ 183.795706] [<bf0ec708>] (xgs_iproc_ep_queue [xgs_iproc_udc]) from [<c0384fec>] (usb_ep_queue+0x1f0/0x238)
>> [ 183.795713] r10:43425355 r9:df767c80 r8:df767c80 r7:a00f0013 r6:df5d73ac r5:df767c80
>> [ 183.795716] r4:df65dea8
>> [ 183.795743] [<c0384dfc>] (usb_ep_queue) from [<bf0f6910>] (usb_composite_overwrite_options+0x128/0x184 [libcomposite])
>> [ 183.795750] r9:00055302 r8:df767c80 r7:a00f0013 r6:df65df04 r5:df767c80 r4:df65dea8
>> [ 183.795777] [<bf0f68e0>] (usb_composite_overwrite_options [libcomposite]) from [<bf0f69f4>] (usb_composite_setup_continue+0x88/0x138 [libcomposite])
>> [ 183.795782] r7:a00f0013 r6:df65df04 r5:00000000 r4:df65dea8
>> [ 183.795812] [<bf0f696c>] (usb_composite_setup_continue [libcomposite]) from [<bf120cf8>] (fsg_alloc_inst+0xa5c/0xac8 [usb_f_mass_storage])
>> [ 183.795819] r9:00055302 r8:00000003 r7:deca5800 r6:00000001 r5:df595a80 r4:deca5948
>> [ 183.795840] [<bf120a68>] (fsg_alloc_inst [usb_f_mass_storage]) from [<bf120e00>] (fsg_main_thread+0x9c/0x15dc [usb_f_mass_storage])
>> [ 183.795846] r8:df770000 r7:df595a80 r6:deca1cc0 r5:df724000 r4:deca5800
>> [ 183.795864] [<bf120d64>] (fsg_main_thread [usb_f_mass_storage]) from [<c0046cd0>] (kthread+0x14c/0x154)
>> [ 183.795870] r10:df785d14 r9:00000000 r8:deca5800 r7:df6c31b8 r6:df70f580 r5:df724000
>> [ 183.795873] r4:df6c3180
>> [ 183.795881] [<c0046b84>] (kthread) from [<c000a67c>] (ret_from_fork+0x14/0x38)
>> [ 183.795887] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0046b84
>> [ 183.795889] r4:df70f580
>> --------------------------snip-------------------------------------
>>
>> Fixes: 57943716ff1b ("usb: gadget: composite: set our req->context to cdev")
>> Signed-off-by: Florian Faber <faber@faberman.de>
>>
>> ---
>> Change in v4:
>> - Short commit hash
>> - Fix line wrap
>>
>> Change in v3:
>> - Addes changes
>>
>> Change in v2:
>> - More verbose explanation
>> - Added commit hash that introduced the bug
>>
>> drivers/usb/gadget/composite.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>> index 504c1cbc255d..8d497be4be32 100644
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -2518,6 +2518,7 @@ void usb_composite_setup_continue(struct usb_composite_dev *cdev)
>> DBG(cdev, "%s: Completing delayed status\n", __func__);
>> req->length = 0;
>> req->context = cdev;
>> + req->complete = composite_setup_complete;
>> value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
>> if (value < 0) {
>> DBG(cdev, "ep_queue --> %d\n", value);
>> --
>
> Does not apply to 5.16-rc1 :(
The diff against a fresh git pull is exactly the same:
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 504c1cbc255d..8d497be4be32 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -2518,6 +2518,7 @@ void usb_composite_setup_continue(struct usb_composite_dev *cdev)
DBG(cdev, "%s: Completing delayed status\n", __func__);
req->length = 0;
req->context = cdev;
+ req->complete = composite_setup_complete;
value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
if (value < 0) {
DBG(cdev, "ep_queue --> %d\n", value);
What can I do to fix this?
Flo
--
Machines can do the work, so people have time to think.
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v4] usb: gadget: composite: req->complete not set, using wrong callback for complete
2021-11-17 14:06 ` Florian Faber
@ 2021-11-17 14:51 ` Greg KH
0 siblings, 0 replies; 15+ messages in thread
From: Greg KH @ 2021-11-17 14:51 UTC (permalink / raw)
To: Florian Faber; +Cc: linux-usb
On Wed, Nov 17, 2021 at 03:06:23PM +0100, Florian Faber wrote:
> Greg,
>
> On 11/17/21 15:01, Greg KH wrote:
> > On Wed, Oct 13, 2021 at 04:15:13PM +0200, Florian Faber wrote:
> > > In usb_composite_setup_continue, req->complete is not set, leaving the
> > > previous value untouched. After completion of the ep0 transaction, the
> > > UDC would then call whatever complete callback was set previously with
> > > the composite cdev as context, leading to all sorts of havoc.
> > >
> > > A typical call trace looks like this: A setup packet for mass storage,
> > > ending up in RNDIS's complete function:
> > >
> > > ---------------------------snip---------------------------------
> > > [ 183.795661] [<bf10b31c>] (rndis_response_complete [usb_f_rndis]) from [<bf0ec024>] (xgs_iproc_ep_enable+0x92c/0xd2c [xgs_iproc_udc])
> > > [ 183.795666] r5:df5d73ac r4:df767c80
> > > [ 183.795682] [<bf0ebf20>] (xgs_iproc_ep_enable [xgs_iproc_udc]) from [<bf0eca8c>] (xgs_iproc_ep_queue+0x384/0x5bc [xgs_iproc_udc])
> > > [ 183.795687] r7:df767cb8 r6:df5d7380 r5:df767c80 r4:df5d73ac
> > > [ 183.795706] [<bf0ec708>] (xgs_iproc_ep_queue [xgs_iproc_udc]) from [<c0384fec>] (usb_ep_queue+0x1f0/0x238)
> > > [ 183.795713] r10:43425355 r9:df767c80 r8:df767c80 r7:a00f0013 r6:df5d73ac r5:df767c80
> > > [ 183.795716] r4:df65dea8
> > > [ 183.795743] [<c0384dfc>] (usb_ep_queue) from [<bf0f6910>] (usb_composite_overwrite_options+0x128/0x184 [libcomposite])
> > > [ 183.795750] r9:00055302 r8:df767c80 r7:a00f0013 r6:df65df04 r5:df767c80 r4:df65dea8
> > > [ 183.795777] [<bf0f68e0>] (usb_composite_overwrite_options [libcomposite]) from [<bf0f69f4>] (usb_composite_setup_continue+0x88/0x138 [libcomposite])
> > > [ 183.795782] r7:a00f0013 r6:df65df04 r5:00000000 r4:df65dea8
> > > [ 183.795812] [<bf0f696c>] (usb_composite_setup_continue [libcomposite]) from [<bf120cf8>] (fsg_alloc_inst+0xa5c/0xac8 [usb_f_mass_storage])
> > > [ 183.795819] r9:00055302 r8:00000003 r7:deca5800 r6:00000001 r5:df595a80 r4:deca5948
> > > [ 183.795840] [<bf120a68>] (fsg_alloc_inst [usb_f_mass_storage]) from [<bf120e00>] (fsg_main_thread+0x9c/0x15dc [usb_f_mass_storage])
> > > [ 183.795846] r8:df770000 r7:df595a80 r6:deca1cc0 r5:df724000 r4:deca5800
> > > [ 183.795864] [<bf120d64>] (fsg_main_thread [usb_f_mass_storage]) from [<c0046cd0>] (kthread+0x14c/0x154)
> > > [ 183.795870] r10:df785d14 r9:00000000 r8:deca5800 r7:df6c31b8 r6:df70f580 r5:df724000
> > > [ 183.795873] r4:df6c3180
> > > [ 183.795881] [<c0046b84>] (kthread) from [<c000a67c>] (ret_from_fork+0x14/0x38)
> > > [ 183.795887] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0046b84
> > > [ 183.795889] r4:df70f580
> > > --------------------------snip-------------------------------------
> > >
> > > Fixes: 57943716ff1b ("usb: gadget: composite: set our req->context to cdev")
> > > Signed-off-by: Florian Faber <faber@faberman.de>
> > >
> > > ---
> > > Change in v4:
> > > - Short commit hash
> > > - Fix line wrap
> > >
> > > Change in v3:
> > > - Addes changes
> > >
> > > Change in v2:
> > > - More verbose explanation
> > > - Added commit hash that introduced the bug
> > >
> > > drivers/usb/gadget/composite.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> > > index 504c1cbc255d..8d497be4be32 100644
> > > --- a/drivers/usb/gadget/composite.c
> > > +++ b/drivers/usb/gadget/composite.c
> > > @@ -2518,6 +2518,7 @@ void usb_composite_setup_continue(struct usb_composite_dev *cdev)
> > > DBG(cdev, "%s: Completing delayed status\n", __func__);
> > > req->length = 0;
> > > req->context = cdev;
> > > + req->complete = composite_setup_complete;
> > > value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
> > > if (value < 0) {
> > > DBG(cdev, "ep_queue --> %d\n", value);
> > > --
> >
> > Does not apply to 5.16-rc1 :(
>
> The diff against a fresh git pull is exactly the same:
>
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 504c1cbc255d..8d497be4be32 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -2518,6 +2518,7 @@ void usb_composite_setup_continue(struct usb_composite_dev *cdev)
> DBG(cdev, "%s: Completing delayed status\n", __func__);
> req->length = 0;
> req->context = cdev;
> + req->complete = composite_setup_complete;
> value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
> if (value < 0) {
> DBG(cdev, "ep_queue --> %d\n", value);
>
> What can I do to fix this?
Something is wrong somewhere:
$ git co -b x v5.16-rc1
Switched to a new branch 'x'
$ b4 am -t https://lore.kernel.org/r/bfecab97-0495-3fa2-3772-20ed5568224b@faberman.de
Looking up https://lore.kernel.org/r/bfecab97-0495-3fa2-3772-20ed5568224b%40faberman.de
Grabbing thread from lore.kernel.org/all/bfecab97-0495-3fa2-3772-20ed5568224b%40faberman.de/t.mbox.gz
Analyzing 11 messages in the thread
Will use the latest revision: v4
You can pick other revisions using the -vN flag
Checking attestation on all messages, may take a moment...
---
[PATCH v4] usb: gadget: composite: req->complete not set, using wrong callback for complete
---
Total patches: 1
---
Link: https://lore.kernel.org/r/e389b7e4-f8c5-1561-2fbc-e926270fc894@faberman.de
Base: applies clean to current tree
git am ./v4_20211013_faber_usb_gadget_composite_req_complete_not_set_using_wrong_callback_for_complete.mbx
$ patch -p1 < v4_20211013_faber_usb_gadget_composite_req_complete_not_set_using_wrong_callback_for_complete.mbx
checking file drivers/usb/gadget/composite.c
Hunk #1 FAILED at 2518.
1 out of 1 hunk FAILED
Are you sure your email client is working properly?
Ah, that's the problem, your email client is adding extra spaces to the
front of the patch you sent out. Please fix that up, you can verify
this by looking at the patch on lore.kernel.org like I have showed here.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] usb: gadget: composite: req->complete not set, using wrong callback for complete
2021-10-13 14:15 ` [PATCH v4] " Florian Faber
2021-11-17 14:01 ` Greg KH
@ 2021-11-27 5:20 ` Peter Chen
2021-12-01 11:04 ` Florian Faber
1 sibling, 1 reply; 15+ messages in thread
From: Peter Chen @ 2021-11-27 5:20 UTC (permalink / raw)
To: Florian Faber; +Cc: linux-usb, gregkh
On 21-10-13 16:15:13, Florian Faber wrote:
> In usb_composite_setup_continue, req->complete is not set, leaving the
> previous value untouched. After completion of the ep0 transaction, the
> UDC would then call whatever complete callback was set previously with
> the composite cdev as context,
Would you please explain more how ep0's req has changed? EP0's req
should not be called by UDC driver.
> leading to all sorts of havoc.
>
> A typical call trace looks like this: A setup packet for mass storage,
> ending up in RNDIS's complete function:
Sorry, I could not understand your back trace well, would you mind
explaining more? Besides, what's your kernel version?
>
> ---------------------------snip---------------------------------
> [ 183.795661] [<bf10b31c>] (rndis_response_complete [usb_f_rndis]) from [<bf0ec024>] (xgs_iproc_ep_enable+0x92c/0xd2c [xgs_iproc_udc])
> [ 183.795666] r5:df5d73ac r4:df767c80
What is xgs_iproc_udc? It seems a downstream UDC driver.
> [ 183.795682] [<bf0ebf20>] (xgs_iproc_ep_enable [xgs_iproc_udc]) from [<bf0eca8c>] (xgs_iproc_ep_queue+0x384/0x5bc [xgs_iproc_udc])
> [ 183.795687] r7:df767cb8 r6:df5d7380 r5:df767c80 r4:df5d73ac
> [ 183.795706] [<bf0ec708>] (xgs_iproc_ep_queue [xgs_iproc_udc]) from [<c0384fec>] (usb_ep_queue+0x1f0/0x238)
> [ 183.795713] r10:43425355 r9:df767c80 r8:df767c80 r7:a00f0013 r6:df5d73ac r5:df767c80
> [ 183.795716] r4:df65dea8
> [ 183.795743] [<c0384dfc>] (usb_ep_queue) from [<bf0f6910>] (usb_composite_overwrite_options+0x128/0x184 [libcomposite])
How could usb_ep_queue is called in usb_composite_overwrite_options?
> [ 183.795750] r9:00055302 r8:df767c80 r7:a00f0013 r6:df65df04 r5:df767c80 r4:df65dea8
> [ 183.795777] [<bf0f68e0>] (usb_composite_overwrite_options [libcomposite]) from [<bf0f69f4>] (usb_composite_setup_continue+0x88/0x138 [libcomposite])
> [ 183.795782] r7:a00f0013 r6:df65df04 r5:00000000 r4:df65dea8
> [ 183.795812] [<bf0f696c>] (usb_composite_setup_continue [libcomposite]) from [<bf120cf8>] (fsg_alloc_inst+0xa5c/0xac8 [usb_f_mass_storage])
How could usb_composite_setup_continue is called in fsg_alloc_inst? The
usb_composite_setup_continue is usually called at the very late of
enumeration.
> [ 183.795819] r9:00055302 r8:00000003 r7:deca5800 r6:00000001 r5:df595a80 r4:deca5948
> [ 183.795840] [<bf120a68>] (fsg_alloc_inst [usb_f_mass_storage]) from [<bf120e00>] (fsg_main_thread+0x9c/0x15dc [usb_f_mass_storage])
How fsg_alloc_inst is called at fsg_main_thread?
Peter
> [ 183.795846] r8:df770000 r7:df595a80 r6:deca1cc0 r5:df724000 r4:deca5800
> [ 183.795864] [<bf120d64>] (fsg_main_thread [usb_f_mass_storage]) from [<c0046cd0>] (kthread+0x14c/0x154)
> [ 183.795870] r10:df785d14 r9:00000000 r8:deca5800 r7:df6c31b8 r6:df70f580 r5:df724000
> [ 183.795873] r4:df6c3180
> [ 183.795881] [<c0046b84>] (kthread) from [<c000a67c>] (ret_from_fork+0x14/0x38)
> [ 183.795887] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0046b84
> [ 183.795889] r4:df70f580
> --------------------------snip-------------------------------------
>
> Fixes: 57943716ff1b ("usb: gadget: composite: set our req->context to cdev")
> Signed-off-by: Florian Faber <faber@faberman.de>
>
> ---
> Change in v4:
> - Short commit hash
> - Fix line wrap
>
> Change in v3:
> - Addes changes
>
> Change in v2:
> - More verbose explanation
> - Added commit hash that introduced the bug
>
> drivers/usb/gadget/composite.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 504c1cbc255d..8d497be4be32 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -2518,6 +2518,7 @@ void usb_composite_setup_continue(struct usb_composite_dev *cdev)
> DBG(cdev, "%s: Completing delayed status\n", __func__);
> req->length = 0;
> req->context = cdev;
> + req->complete = composite_setup_complete;
> value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
> if (value < 0) {
> DBG(cdev, "ep_queue --> %d\n", value);
> --
--
Thanks,
Peter Chen
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] usb: gadget: composite: req->complete not set, using wrong callback for complete
2021-11-27 5:20 ` Peter Chen
@ 2021-12-01 11:04 ` Florian Faber
2021-12-01 14:12 ` Peter Chen
0 siblings, 1 reply; 15+ messages in thread
From: Florian Faber @ 2021-12-01 11:04 UTC (permalink / raw)
To: Peter Chen; +Cc: linux-usb, gregkh
Peter,
On 11/27/21 06:20, Peter Chen wrote:
> On 21-10-13 16:15:13, Florian Faber wrote:
>> In usb_composite_setup_continue, req->complete is not set, leaving the
>> previous value untouched. After completion of the ep0 transaction, the
>> UDC would then call whatever complete callback was set previously with
>> the composite cdev as context,
>
> Would you please explain more how ep0's req has changed? EP0's req
> should not be called by UDC driver.
I have found this bug by adding a source identifier field to struct usb_request so I could not only detect wether a correct packet was sent to a complete function, but also trace the exact origin. That way I could show that a packet with a gadget's completion callback was generated in composite.c.
I don't understand your question. Of course composite.c's completion function is called from the UDC driver.
>> leading to all sorts of havoc.
>>
>> A typical call trace looks like this: A setup packet for mass storage,
>> ending up in RNDIS's complete function:
>
> Sorry, I could not understand your back trace well, would you mind
> explaining more? Besides, what's your kernel version?
For some reasons, the kernel stack traces on the target are often wrong and manually looking up the addresses in gdb gives the correct location in the source. That might be the case in this trace as well.
4.14.115.
>> ---------------------------snip---------------------------------
>> [ 183.795661] [<bf10b31c>] (rndis_response_complete [usb_f_rndis]) from [<bf0ec024>] (xgs_iproc_ep_enable+0x92c/0xd2c [xgs_iproc_udc])
>> [ 183.795666] r5:df5d73ac r4:df767c80
>
> What is xgs_iproc_udc? It seems a downstream UDC driver.
Yes, embedded system with iproc SoC.
>> [ 183.795682] [<bf0ebf20>] (xgs_iproc_ep_enable [xgs_iproc_udc]) from [<bf0eca8c>] (xgs_iproc_ep_queue+0x384/0x5bc [xgs_iproc_udc])
>> [ 183.795687] r7:df767cb8 r6:df5d7380 r5:df767c80 r4:df5d73ac
>> [ 183.795706] [<bf0ec708>] (xgs_iproc_ep_queue [xgs_iproc_udc]) from [<c0384fec>] (usb_ep_queue+0x1f0/0x238)
>> [ 183.795713] r10:43425355 r9:df767c80 r8:df767c80 r7:a00f0013 r6:df5d73ac r5:df767c80
>> [ 183.795716] r4:df65dea8
>> [ 183.795743] [<c0384dfc>] (usb_ep_queue) from [<bf0f6910>] (usb_composite_overwrite_options+0x128/0x184 [libcomposite])
>
> How could usb_ep_queue is called in usb_composite_overwrite_options?
>> [ 183.795750] r9:00055302 r8:df767c80 r7:a00f0013 r6:df65df04 r5:df767c80 r4:df65dea8
>> [ 183.795777] [<bf0f68e0>] (usb_composite_overwrite_options [libcomposite]) from [<bf0f69f4>] (usb_composite_setup_continue+0x88/0x138 [libcomposite])
>> [ 183.795782] r7:a00f0013 r6:df65df04 r5:00000000 r4:df65dea8
>> [ 183.795812] [<bf0f696c>] (usb_composite_setup_continue [libcomposite]) from [<bf120cf8>] (fsg_alloc_inst+0xa5c/0xac8 [usb_f_mass_storage])
>
> How could usb_composite_setup_continue is called in fsg_alloc_inst? The
> usb_composite_setup_continue is usually called at the very late of
> enumeration.
I don't know what this has to do with this bug. The only relevant question is: Why is the callback function not set in this particular location in composite.c?
>> [ 183.795819] r9:00055302 r8:00000003 r7:deca5800 r6:00000001 r5:df595a80 r4:deca5948
>> [ 183.795840] [<bf120a68>] (fsg_alloc_inst [usb_f_mass_storage]) from [<bf120e00>] (fsg_main_thread+0x9c/0x15dc [usb_f_mass_storage])
>
> How fsg_alloc_inst is called at fsg_main_thread>
> Peter
>
>> [ 183.795846] r8:df770000 r7:df595a80 r6:deca1cc0 r5:df724000 r4:deca5800
>> [ 183.795864] [<bf120d64>] (fsg_main_thread [usb_f_mass_storage]) from [<c0046cd0>] (kthread+0x14c/0x154)
>> [ 183.795870] r10:df785d14 r9:00000000 r8:deca5800 r7:df6c31b8 r6:df70f580 r5:df724000
>> [ 183.795873] r4:df6c3180
>> [ 183.795881] [<c0046b84>] (kthread) from [<c000a67c>] (ret_from_fork+0x14/0x38)
>> [ 183.795887] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0046b84
>> [ 183.795889] r4:df70f580
>> --------------------------snip-------------------------------------
>
>
>>
>> Fixes: 57943716ff1b ("usb: gadget: composite: set our req->context to cdev")
>> Signed-off-by: Florian Faber <faber@faberman.de>
>>
>> ---
>> Change in v4:
>> - Short commit hash
>> - Fix line wrap
>>
>> Change in v3:
>> - Addes changes
>>
>> Change in v2:
>> - More verbose explanation
>> - Added commit hash that introduced the bug
>>
>> drivers/usb/gadget/composite.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>> index 504c1cbc255d..8d497be4be32 100644
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -2518,6 +2518,7 @@ void usb_composite_setup_continue(struct usb_composite_dev *cdev)
>> DBG(cdev, "%s: Completing delayed status\n", __func__);
>> req->length = 0;
>> req->context = cdev;
>> + req->complete = composite_setup_complete;
>> value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
>> if (value < 0) {
>> DBG(cdev, "ep_queue --> %d\n", value);
>> --
>
Flo
--
Machines can do the work, so people have time to think.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4] usb: gadget: composite: req->complete not set, using wrong callback for complete
2021-12-01 11:04 ` Florian Faber
@ 2021-12-01 14:12 ` Peter Chen
0 siblings, 0 replies; 15+ messages in thread
From: Peter Chen @ 2021-12-01 14:12 UTC (permalink / raw)
To: Florian Faber; +Cc: linux-usb, gregkh
On 21-12-01 12:04:52, Florian Faber wrote:
> Peter,
>
> On 11/27/21 06:20, Peter Chen wrote:
> > On 21-10-13 16:15:13, Florian Faber wrote:
> > > In usb_composite_setup_continue, req->complete is not set, leaving the
> > > previous value untouched. After completion of the ep0 transaction, the
> > > UDC would then call whatever complete callback was set previously with
> > > the composite cdev as context,
> >
> > Would you please explain more how ep0's req has changed? EP0's req
> > should not be called by UDC driver.
Sorry, typo. I mean **changed** by UDC driver. Except for composite.c, I only see ep0's req has
changed at f_tcm.c.
>
> I have found this bug by adding a source identifier field to struct usb_request so I could not only detect wether a correct packet was sent to a complete function, but also trace the exact origin. That way I could show that a packet with a gadget's completion callback was generated in composite.c.
Then, have you found how the ep0's req has been changed, and the
req->complete was set by other completion function?
>
> I don't understand your question. Of course composite.c's completion function is called from the UDC driver.
>
> > > leading to all sorts of havoc.
> > >
> > > A typical call trace looks like this: A setup packet for mass storage,
> > > ending up in RNDIS's complete function:
> >
> > Sorry, I could not understand your back trace well, would you mind
> > explaining more? Besides, what's your kernel version?
>
> For some reasons, the kernel stack traces on the target are often wrong and manually looking up the addresses in gdb gives the correct location in the source. That might be the case in this trace as well.
>
> 4.14.115.
This version kernel is old, would you have newer kernel to try this issue?
Peter
>
> > > ---------------------------snip---------------------------------
> > > [ 183.795661] [<bf10b31c>] (rndis_response_complete [usb_f_rndis]) from [<bf0ec024>] (xgs_iproc_ep_enable+0x92c/0xd2c [xgs_iproc_udc])
> > > [ 183.795666] r5:df5d73ac r4:df767c80
> >
> > What is xgs_iproc_udc? It seems a downstream UDC driver.
>
> Yes, embedded system with iproc SoC.
>
> > > [ 183.795682] [<bf0ebf20>] (xgs_iproc_ep_enable [xgs_iproc_udc]) from [<bf0eca8c>] (xgs_iproc_ep_queue+0x384/0x5bc [xgs_iproc_udc])
> > > [ 183.795687] r7:df767cb8 r6:df5d7380 r5:df767c80 r4:df5d73ac
> > > [ 183.795706] [<bf0ec708>] (xgs_iproc_ep_queue [xgs_iproc_udc]) from [<c0384fec>] (usb_ep_queue+0x1f0/0x238)
> > > [ 183.795713] r10:43425355 r9:df767c80 r8:df767c80 r7:a00f0013 r6:df5d73ac r5:df767c80
> > > [ 183.795716] r4:df65dea8
> > > [ 183.795743] [<c0384dfc>] (usb_ep_queue) from [<bf0f6910>] (usb_composite_overwrite_options+0x128/0x184 [libcomposite])
> >
> > How could usb_ep_queue is called in usb_composite_overwrite_options?
> > > [ 183.795750] r9:00055302 r8:df767c80 r7:a00f0013 r6:df65df04 r5:df767c80 r4:df65dea8
> > > [ 183.795777] [<bf0f68e0>] (usb_composite_overwrite_options [libcomposite]) from [<bf0f69f4>] (usb_composite_setup_continue+0x88/0x138 [libcomposite])
> > > [ 183.795782] r7:a00f0013 r6:df65df04 r5:00000000 r4:df65dea8
> > > [ 183.795812] [<bf0f696c>] (usb_composite_setup_continue [libcomposite]) from [<bf120cf8>] (fsg_alloc_inst+0xa5c/0xac8 [usb_f_mass_storage])
> >
> > How could usb_composite_setup_continue is called in fsg_alloc_inst? The
> > usb_composite_setup_continue is usually called at the very late of
> > enumeration.
>
> I don't know what this has to do with this bug. The only relevant question is: Why is the callback function not set in this particular location in composite.c?
>
> > > [ 183.795819] r9:00055302 r8:00000003 r7:deca5800 r6:00000001 r5:df595a80 r4:deca5948
> > > [ 183.795840] [<bf120a68>] (fsg_alloc_inst [usb_f_mass_storage]) from [<bf120e00>] (fsg_main_thread+0x9c/0x15dc [usb_f_mass_storage])
> >
> > How fsg_alloc_inst is called at fsg_main_thread> Peter
> >
> > > [ 183.795846] r8:df770000 r7:df595a80 r6:deca1cc0 r5:df724000 r4:deca5800
> > > [ 183.795864] [<bf120d64>] (fsg_main_thread [usb_f_mass_storage]) from [<c0046cd0>] (kthread+0x14c/0x154)
> > > [ 183.795870] r10:df785d14 r9:00000000 r8:deca5800 r7:df6c31b8 r6:df70f580 r5:df724000
> > > [ 183.795873] r4:df6c3180
> > > [ 183.795881] [<c0046b84>] (kthread) from [<c000a67c>] (ret_from_fork+0x14/0x38)
> > > [ 183.795887] r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c0046b84
> > > [ 183.795889] r4:df70f580
> > > --------------------------snip-------------------------------------
> >
> >
> > >
> > > Fixes: 57943716ff1b ("usb: gadget: composite: set our req->context to cdev")
> > > Signed-off-by: Florian Faber <faber@faberman.de>
> > >
> > > ---
> > > Change in v4:
> > > - Short commit hash
> > > - Fix line wrap
> > >
> > > Change in v3:
> > > - Addes changes
> > >
> > > Change in v2:
> > > - More verbose explanation
> > > - Added commit hash that introduced the bug
> > >
> > > drivers/usb/gadget/composite.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> > > index 504c1cbc255d..8d497be4be32 100644
> > > --- a/drivers/usb/gadget/composite.c
> > > +++ b/drivers/usb/gadget/composite.c
> > > @@ -2518,6 +2518,7 @@ void usb_composite_setup_continue(struct usb_composite_dev *cdev)
> > > DBG(cdev, "%s: Completing delayed status\n", __func__);
> > > req->length = 0;
> > > req->context = cdev;
> > > + req->complete = composite_setup_complete;
> > > value = composite_ep0_queue(cdev, req, GFP_ATOMIC);
> > > if (value < 0) {
> > > DBG(cdev, "ep_queue --> %d\n", value);
> > > --
> >
>
> Flo
> --
> Machines can do the work, so people have time to think.
--
Thanks,
Peter Chen
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-12-01 14:12 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-19 19:41 [PATCH] usb: gadget: composite: req->complete not set, using wrong callback for complete Florian Faber
2021-09-20 4:58 ` Greg KH
2021-09-20 5:46 ` Florian Faber
2021-09-20 6:24 ` Greg KH
2021-10-13 8:41 ` [PATCH v2] " Florian Faber
2021-10-13 8:48 ` Greg KH
2021-10-13 8:54 ` [PATCH v3] " Florian Faber
2021-10-13 12:05 ` Greg KH
2021-10-13 14:15 ` [PATCH v4] " Florian Faber
2021-11-17 14:01 ` Greg KH
2021-11-17 14:06 ` Florian Faber
2021-11-17 14:51 ` Greg KH
2021-11-27 5:20 ` Peter Chen
2021-12-01 11:04 ` Florian Faber
2021-12-01 14:12 ` Peter Chen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.