From: taehyun cho <[email protected]>
Endpoint complete function in u_serial can be executed
when 'gs_port' is Null. This situation happens when
'dwc3_gadget_pullup' returns ETIMEDOUT. The reason why
ETIMEDOUT is returned is that whole system is out of order
including interrupt regardless of USB.
pc : __lock_acquire+0x54/0x5ec
lr : lock_acquire+0xe8/0x198
sp : ffffffc03914b9d0
x29: ffffffc03914b9d0 x28: ffffff895f13b680
x27: 0000000000000000 x26: 0000000000000000
x25: 00000000000003c8 x24: 0000000000000080
x23: ffffffc010a8f650 x22: 0000000000000000
x21: 0000000000000000 x20: 0000000000000000
x19: ffffffc010a8f650 x18: ffffffc02d70a0b0
x17: 0000000000000000 x16: 00000000000229e0
x15: 0000000000000004 x14: 00000000000004f2
x13: ffffffc0120fe178 x12: 0000000000000003
x11: 00000000ffffffff x10: 0000000100000001
x9 : 0000000000000001 x8 : 00000000000003c8
x7 : 0000000000000000 x6 : ffffffc010a8f650
x5 : 0000000000000000 x4 : 0000000000000080
x3 : 0000000000000000 x2 : 0000000000000000
x1 : 0000000000000000 x0 : 00000000000003c8
Call trace:
__lock_acquire+0x54/0x5ec
lock_acquire+0xe8/0x198
_raw_spin_lock+0x70/0x88
gs_read_complete+0x48/0xac
usb_gadget_giveback_request+0x48/0x80
dwc3_gadget_giveback+0xcc/0xe8
dwc3_remove_requests+0xa8/0x188
__dwc3_gadget_ep_disable+0x98/0x110
dwc3_gadget_ep_disable+0x50/0xbc
usb_ep_disable+0x44/0x94
gserial_disconnect+0xc0/0x250
acm_free_func+0x30/0x48
usb_put_function+0x34/0x68
config_usb_cfg_unlink+0xdc/0xf8
configfs_unlink+0x144/0x264
vfs_unlink+0x134/0x218
do_unlinkat+0x13c/0x2a0
__arm64_sys_unlinkat+0x48/0x60
el0_svc_common.llvm.10277270529376503802+0xb8/0x1b4
do_el0_svc+0x24/0x8c
el0_svc+0x10/0x1c
el0_sync_handler+0x68/0xac
el0_sync+0x18c/0x1c0
Signed-off-by: taehyun cho <[email protected]>
---
drivers/usb/gadget/function/u_serial.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
index 6f68cbe..af08a18 100644
--- a/drivers/usb/gadget/function/u_serial.c
+++ b/drivers/usb/gadget/function/u_serial.c
@@ -450,6 +450,15 @@ static void gs_read_complete(struct usb_ep *ep, struct usb_request *req)
{
struct gs_port *port = ep->driver_data;
+ /*
+ * Port became NULL when 'dwc3_gadget_pullup' returns ETIMEDOUT.
+ * Return here to avoid panic.
+ */
+ if (!port) {
+ pr_err("%s, failed to get port\n", __func__);
+ return;
+ }
+
/* Queue all received data until the tty layer is ready for it. */
spin_lock(&port->port_lock);
list_add_tail(&req->list, &port->read_queue);
@@ -461,6 +470,15 @@ static void gs_write_complete(struct usb_ep *ep, struct usb_request *req)
{
struct gs_port *port = ep->driver_data;
+ /*
+ * port became NULL when 'dwc3_gadget_pullup' returns ETIMEDOUT.
+ * Return here to avoid panic.
+ */
+ if (!port) {
+ pr_err("%s, failed to get port\n", __func__);
+ return;
+ }
+
spin_lock(&port->port_lock);
list_add(&req->list, &port->write_pool);
port->write_started--;
--
2.7.4
On Fri, Aug 06, 2021 at 04:53:26PM +0900, Daehwan Jung wrote:
> From: taehyun cho <[email protected]>
>
> Endpoint complete function in u_serial can be executed
> when 'gs_port' is Null. This situation happens when
> 'dwc3_gadget_pullup' returns ETIMEDOUT. The reason why
> ETIMEDOUT is returned is that whole system is out of order
> including interrupt regardless of USB.
>
> pc : __lock_acquire+0x54/0x5ec
> lr : lock_acquire+0xe8/0x198
> sp : ffffffc03914b9d0
> x29: ffffffc03914b9d0 x28: ffffff895f13b680
> x27: 0000000000000000 x26: 0000000000000000
> x25: 00000000000003c8 x24: 0000000000000080
> x23: ffffffc010a8f650 x22: 0000000000000000
> x21: 0000000000000000 x20: 0000000000000000
> x19: ffffffc010a8f650 x18: ffffffc02d70a0b0
> x17: 0000000000000000 x16: 00000000000229e0
> x15: 0000000000000004 x14: 00000000000004f2
> x13: ffffffc0120fe178 x12: 0000000000000003
> x11: 00000000ffffffff x10: 0000000100000001
> x9 : 0000000000000001 x8 : 00000000000003c8
> x7 : 0000000000000000 x6 : ffffffc010a8f650
> x5 : 0000000000000000 x4 : 0000000000000080
> x3 : 0000000000000000 x2 : 0000000000000000
> x1 : 0000000000000000 x0 : 00000000000003c8
> Call trace:
> __lock_acquire+0x54/0x5ec
> lock_acquire+0xe8/0x198
> _raw_spin_lock+0x70/0x88
> gs_read_complete+0x48/0xac
> usb_gadget_giveback_request+0x48/0x80
> dwc3_gadget_giveback+0xcc/0xe8
> dwc3_remove_requests+0xa8/0x188
> __dwc3_gadget_ep_disable+0x98/0x110
> dwc3_gadget_ep_disable+0x50/0xbc
> usb_ep_disable+0x44/0x94
> gserial_disconnect+0xc0/0x250
> acm_free_func+0x30/0x48
> usb_put_function+0x34/0x68
> config_usb_cfg_unlink+0xdc/0xf8
> configfs_unlink+0x144/0x264
> vfs_unlink+0x134/0x218
> do_unlinkat+0x13c/0x2a0
> __arm64_sys_unlinkat+0x48/0x60
> el0_svc_common.llvm.10277270529376503802+0xb8/0x1b4
> do_el0_svc+0x24/0x8c
> el0_svc+0x10/0x1c
> el0_sync_handler+0x68/0xac
> el0_sync+0x18c/0x1c0
>
> Signed-off-by: taehyun cho <[email protected]>
> ---
> drivers/usb/gadget/function/u_serial.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
> index 6f68cbe..af08a18 100644
> --- a/drivers/usb/gadget/function/u_serial.c
> +++ b/drivers/usb/gadget/function/u_serial.c
> @@ -450,6 +450,15 @@ static void gs_read_complete(struct usb_ep *ep, struct usb_request *req)
> {
> struct gs_port *port = ep->driver_data;
>
> + /*
> + * Port became NULL when 'dwc3_gadget_pullup' returns ETIMEDOUT.
> + * Return here to avoid panic.
> + */
> + if (!port) {
> + pr_err("%s, failed to get port\n", __func__);
> + return;
> + }
> +
What prevents port from being null right after checking this? Where is
the lock to prevent this?
> /* Queue all received data until the tty layer is ready for it. */
> spin_lock(&port->port_lock);
> list_add_tail(&req->list, &port->read_queue);
> @@ -461,6 +470,15 @@ static void gs_write_complete(struct usb_ep *ep, struct usb_request *req)
> {
> struct gs_port *port = ep->driver_data;
>
> + /*
> + * port became NULL when 'dwc3_gadget_pullup' returns ETIMEDOUT.
> + * Return here to avoid panic.
> + */
> + if (!port) {
> + pr_err("%s, failed to get port\n", __func__);
> + return;
> + }
Same here, where is the lock?
And why report an error, what can a user do about it?
thanks,
greg k-h
On Wed, Sep 08, 2021 at 04:54:13PM +0900, 정대환 wrote:
<snip>
Please do not send html email, it is rejected by the mailing lists.
Please fix your email client and resend.
thanks,
greg k-h
On Wed, Sep 08, 2021 at 07:21:29PM +0900, Jung Daehwan wrote:
> On Fri, Aug 06, 2021 at 05:13:34PM +0200, Greg Kroah-Harman wrote:
> > On Fri, Aug 06, 2021 at 04:53:26PM +0900, Daehwan Jung wrote:
> > > From: taehyun cho <[email protected]>
> > >
> > > Endpoint complete function in u_serial can be executed when 'gs_port'
> > > is Null. This situation happens when 'dwc3_gadget_pullup' returns
> > > ETIMEDOUT. The reason why ETIMEDOUT is returned is that whole system
> > > is out of order including interrupt regardless of USB.
> > >
> > > pc : __lock_acquire+0x54/0x5ec
> > > lr : lock_acquire+0xe8/0x198
> > > sp : ffffffc03914b9d0
> > > x29: ffffffc03914b9d0 x28: ffffff895f13b680
> > > x27: 0000000000000000 x26: 0000000000000000
> > > x25: 00000000000003c8 x24: 0000000000000080
> > > x23: ffffffc010a8f650 x22: 0000000000000000
> > > x21: 0000000000000000 x20: 0000000000000000
> > > x19: ffffffc010a8f650 x18: ffffffc02d70a0b0
> > > x17: 0000000000000000 x16: 00000000000229e0
> > > x15: 0000000000000004 x14: 00000000000004f2
> > > x13: ffffffc0120fe178 x12: 0000000000000003
> > > x11: 00000000ffffffff x10: 0000000100000001
> > > x9 : 0000000000000001 x8 : 00000000000003c8
> > > x7 : 0000000000000000 x6 : ffffffc010a8f650
> > > x5 : 0000000000000000 x4 : 0000000000000080
> > > x3 : 0000000000000000 x2 : 0000000000000000
> > > x1 : 0000000000000000 x0 : 00000000000003c8 Call trace:
> > > __lock_acquire+0x54/0x5ec
> > > lock_acquire+0xe8/0x198
> > > _raw_spin_lock+0x70/0x88
> > > gs_read_complete+0x48/0xac
> > > usb_gadget_giveback_request+0x48/0x80
> > > dwc3_gadget_giveback+0xcc/0xe8
> > > dwc3_remove_requests+0xa8/0x188
> > > __dwc3_gadget_ep_disable+0x98/0x110
> > > dwc3_gadget_ep_disable+0x50/0xbc
> > > usb_ep_disable+0x44/0x94
> > > gserial_disconnect+0xc0/0x250
> > > acm_free_func+0x30/0x48
> > > usb_put_function+0x34/0x68
> > > config_usb_cfg_unlink+0xdc/0xf8
> > > configfs_unlink+0x144/0x264
> > > vfs_unlink+0x134/0x218
> > > do_unlinkat+0x13c/0x2a0
> > > __arm64_sys_unlinkat+0x48/0x60
> > > el0_svc_common.llvm.10277270529376503802+0xb8/0x1b4
> > > do_el0_svc+0x24/0x8c
> > > el0_svc+0x10/0x1c
> > > el0_sync_handler+0x68/0xac
> > > el0_sync+0x18c/0x1c0
> > >
> > > Signed-off-by: taehyun cho <[email protected]>
> > > ---
> > > drivers/usb/gadget/function/u_serial.c | 18 ++++++++++++++++++
> > > 1 file changed, 18 insertions(+)
> > >
> > > diff --git a/drivers/usb/gadget/function/u_serial.c
> > > b/drivers/usb/gadget/function/u_serial.c
> > > index 6f68cbe..af08a18 100644
> > > --- a/drivers/usb/gadget/function/u_serial.c
> > > +++ b/drivers/usb/gadget/function/u_serial.c
> > > @@ -450,6 +450,15 @@ static void gs_read_complete(struct usb_ep *ep,
> > > struct usb_request *req) {
> > > struct gs_port *port = ep->driver_data;
> > >
> > > + /*
> > > + * Port became NULL when 'dwc3_gadget_pullup' returns ETIMEDOUT.
> > > + * Return here to avoid panic.
> > > + */
> > > + if (!port) {
> > > + pr_err("%s, failed to get port\n", __func__);
> > > + return;
> > > + }
> > > +
> >
> spin_lock(&port->port_lock);
> ...
> spin_unlock(&port->port_lock);
> > What prevents port from being null right after checking this? Where is the
> > lock to prevent this?
> >
> It tries to get lock first in gs_read_complete/gs_write_complete like above.
> That's why the panic occured during getting lock but this issue is not related
> to lock. We just want to prevent doing something after port becomes null.
I do not understand, you are not protecting anything here, what happens
if port becomes NULL right after checking it and before the lock?
Either this needs to be tested like this, or it does not at all. This
change does not really fix the issue.
> > > /* Queue all received data until the tty layer is ready for it. */
> > > spin_lock(&port->port_lock);
> > > list_add_tail(&req->list, &port->read_queue); @@ -461,6 +470,15 @@
> > > static void gs_write_complete(struct usb_ep *ep, struct usb_request
> > > *req) {
> > > struct gs_port *port = ep->driver_data;
> > >
> > > + /*
> > > + * port became NULL when 'dwc3_gadget_pullup' returns ETIMEDOUT.
> > > + * Return here to avoid panic.
> > > + */
> > > + if (!port) {
> > > + pr_err("%s, failed to get port\n", __func__);
> > > + return;
> > > + }
> >
> > Same here, where is the lock?
> >
> > And why report an error, what can a user do about it?
> >
>
> It could happen to access null pointer and occur whole system panic.
Again, what is setting that pointer to NULL and why isn't it caught
before this? Shouldn't everything be properly torn down and the
completions finished _before_ the port is set to NULL?
If not, then fix that issue, as this change will not fix the real
problem here, only delay it.
thanks,
greg k-h