| Message ID | TYUPR06MB6217549161B67D996EB3DCE3D2942@TYUPR06MB6217.apcprd06.prod.outlook.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v8] usb: gadget: u_serial: Add null pointer check in gs_read_complete & gs_write_complete | expand |
Hi 胡连勤 On Tue, Aug 27, 2024 at 12:19 PM 胡连勤 <hulianqin@vivo.com> wrote: > > From: Lianqin Hu <hulianqin@vivo.com> > > Considering that in some extreme cases, when the unbind operation > is being executed, gserial_disconnect has already cleared gser->ioport, > and gs_read_complete gets called afterwards, which results in accessing > null pointer, add a null pointer check to prevent this situation. > Was better not to get rid of the test case. What config.usb in android does is trigger a gadget reconfiguration on this property change. It means that I expect the android test, try to change gadget and moving from one type to another in a loop. Is that correct? If so will be nice that you describe > Added a static spinlock to prevent gser->ioport from becoming > null after the newly added check. > > Unable to handle kernel NULL pointer dereference at > virtual address 00000000000001a8 > pc : gs_read_complete+0x58/0x240 > lr : usb_gadget_giveback_request+0x40/0x160 > sp : ffffffc00f1539c0 > x29: ffffffc00f1539c0 x28: ffffff8002a30000 x27: 0000000000000000 > x26: ffffff8002a30000 x25: 0000000000000000 x24: ffffff8002a30000 > x23: ffffff8002ff9a70 x22: ffffff898e7a7b00 x21: ffffff803c9af9d8 > x20: ffffff898e7a7b00 x19: 00000000000001a8 x18: ffffffc0099fd098 > x17: 0000000000001000 x16: 0000000080000000 x15: 0000000ac1200000 > x14: 0000000000000003 x13: 000000000000d5e8 x12: 0000000355c314ac > x11: 0000000000000015 x10: 0000000000000012 x9 : 0000000000000008 > x8 : 0000000000000000 x7 : 0000000000000000 x6 : ffffff887cd12000 > x5 : 0000000000000002 x4 : ffffffc00f9b07f0 x3 : ffffffc00f1538d0 > x2 : 0000000000000001 x1 : 0000000000000000 x0 : 00000000000001a8 > Call trace: > gs_read_complete+0x58/0x240 > usb_gadget_giveback_request+0x40/0x160 > dwc3_remove_requests+0x170/0x484 > dwc3_ep0_out_start+0xb0/0x1d4 > __dwc3_gadget_start+0x25c/0x720 > kretprobe_trampoline.cfi_jt+0x0/0x8 > kretprobe_trampoline.cfi_jt+0x0/0x8 > udc_bind_to_driver+0x1d8/0x300 > usb_gadget_probe_driver+0xa8/0x1dc > gadget_dev_desc_UDC_store+0x13c/0x188 > configfs_write_iter+0x160/0x1f4 > vfs_write+0x2d0/0x40c > ksys_write+0x7c/0xf0 > __arm64_sys_write+0x20/0x30 > invoke_syscall+0x60/0x150 > el0_svc_common+0x8c/0xf8 > do_el0_svc+0x28/0xa0 > el0_svc+0x24/0x84 > el0t_64_sync_handler+0x88/0xec > el0t_64_sync+0x1b4/0x1b8 > Code: aa1f03e1 aa1303e0 52800022 2a0103e8 (88e87e62) > ---[ end trace 938847327a739172 ]--- > Kernel panic - not syncing: Oops: Fatal exception > > Fixes: c1dca562be8a ("usb gadget: split out serial core") > Cc: stable@vger.kernel.org > Suggested-by: Prashanth K <quic_prashk@quicinc.com> > Signed-off-by: Lianqin Hu <hulianqin@vivo.com> > --- > v8: Updated patch submission description as suggested in v7 discussion. > v7: Remove code comments. > v6: Update the commit text. > v5: Add the Fixes tag. > v4: CC stable kernel. > v3: Add serial_port_lock protection when checking port pointer. > v2: Optimize code comments. > v1: Delete log printing. > > drivers/usb/gadget/function/u_serial.c | 31 +++++++++++++++++++++----- > 1 file changed, 26 insertions(+), 5 deletions(-) > > diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c > index b394105e55d6..66d918523b3e 100644 > --- a/drivers/usb/gadget/function/u_serial.c > +++ b/drivers/usb/gadget/function/u_serial.c > @@ -452,20 +452,41 @@ static void gs_rx_push(struct work_struct *work) > > static void gs_read_complete(struct usb_ep *ep, struct usb_request *req) > { > - struct gs_port *port = ep->driver_data; > + struct gs_port *port; > + unsigned long flags; > + > + spin_lock_irqsave(&serial_port_lock, flags); > + port = ep->driver_data; > + > + if (!port) { > + spin_unlock_irqrestore(&serial_port_lock, flags); > + return; > + } > > - /* Queue all received data until the tty layer is ready for it. */ > spin_lock(&port->port_lock); > + spin_unlock(&serial_port_lock); > + > + /* Queue all received data until the tty layer is ready for it. */ > list_add_tail(&req->list, &port->read_queue); > schedule_delayed_work(&port->push, 0); > - spin_unlock(&port->port_lock); > + spin_unlock_irqrestore(&port->port_lock, flags); > } > > static void gs_write_complete(struct usb_ep *ep, struct usb_request *req) > { > - struct gs_port *port = ep->driver_data; > + struct gs_port *port; > + unsigned long flags; > + > + spin_lock_irqsave(&serial_port_lock, flags); > + port = ep->driver_data; > + > + if (!port) { > + spin_unlock_irqrestore(&serial_port_lock, flags); > + return; > + } > > spin_lock(&port->port_lock); > + spin_unlock(&serial_port_lock); > list_add(&req->list, &port->write_pool); > port->write_started--; > > @@ -486,7 +507,7 @@ static void gs_write_complete(struct usb_ep *ep, struct usb_request *req) > break; > } > > - spin_unlock(&port->port_lock); > + spin_unlock_irqrestore(&port->port_lock, flags); > } > > static void gs_free_requests(struct usb_ep *ep, struct list_head *head, > -- I can base my review on what was done before for suspend/resume and this implementation looks the same. I don't think that both are optimal but this should avoid race on completion. Michael > 2.39.0 >
Hi Michael: >> Considering that in some extreme cases, when the unbind operation is >> being executed, gserial_disconnect has already cleared gser->ioport, >> and gs_read_complete gets called afterwards, which results in >> accessing null pointer, add a null pointer check to prevent this situation. >> >Was better not to get rid of the test case. What config.usb in android does is trigger a gadget reconfiguration on this property change. >It means that I expect the android test, try to change gadget and moving from one type to another in a loop. >Is that correct? If so will be nice that you describe Yes, it happens when the USB mode is switched. The dmesg log shows that the USB mode is switched from the vivo industrial mode port (diag+adb+serial_cdev+serial_tty+rmnet_ipa) to the mtp mode. In addition, does the above specific USB mode switching information need to be added to the submission description? >> Added a static spinlock to prevent gser->ioport from becoming null >> after the newly added check. >> >> Unable to handle kernel NULL pointer dereference at virtual address >> 00000000000001a8 pc : gs_read_complete+0x58/0x240 lr : >> usb_gadget_giveback_request+0x40/0x160 >> sp : ffffffc00f1539c0 >> x29: ffffffc00f1539c0 x28: ffffff8002a30000 x27: 0000000000000000 >> x26: ffffff8002a30000 x25: 0000000000000000 x24: ffffff8002a30000 >> x23: ffffff8002ff9a70 x22: ffffff898e7a7b00 x21: ffffff803c9af9d8 >> x20: ffffff898e7a7b00 x19: 00000000000001a8 x18: ffffffc0099fd098 >> x17: 0000000000001000 x16: 0000000080000000 x15: 0000000ac1200000 >> x14: 0000000000000003 x13: 000000000000d5e8 x12: 0000000355c314ac >> x11: 0000000000000015 x10: 0000000000000012 x9 : 0000000000000008 >> x8 : 0000000000000000 x7 : 0000000000000000 x6 : ffffff887cd12000 >> x5 : 0000000000000002 x4 : ffffffc00f9b07f0 x3 : ffffffc00f1538d0 >> x2 : 0000000000000001 x1 : 0000000000000000 x0 : 00000000000001a8 Call >> trace: >> gs_read_complete+0x58/0x240 >> usb_gadget_giveback_request+0x40/0x160 >> dwc3_remove_requests+0x170/0x484 >> dwc3_ep0_out_start+0xb0/0x1d4 >> __dwc3_gadget_start+0x25c/0x720 >> kretprobe_trampoline.cfi_jt+0x0/0x8 >> kretprobe_trampoline.cfi_jt+0x0/0x8 >> udc_bind_to_driver+0x1d8/0x300 >> usb_gadget_probe_driver+0xa8/0x1dc >> gadget_dev_desc_UDC_store+0x13c/0x188 >> configfs_write_iter+0x160/0x1f4 >> vfs_write+0x2d0/0x40c >> ksys_write+0x7c/0xf0 >> __arm64_sys_write+0x20/0x30 >> invoke_syscall+0x60/0x150 >> el0_svc_common+0x8c/0xf8 >> do_el0_svc+0x28/0xa0 >> el0_svc+0x24/0x84 >> el0t_64_sync_handler+0x88/0xec >> el0t_64_sync+0x1b4/0x1b8 >> Code: aa1f03e1 aa1303e0 52800022 2a0103e8 (88e87e62) ---[ end trace >> 938847327a739172 ]--- Kernel panic - not syncing: Oops: Fatal >> exception >> >> Fixes: c1dca562be8a ("usb gadget: split out serial core") >> Cc: stable@vger.kernel.org >> Suggested-by: Prashanth K <quic_prashk@quicinc.com> >> Signed-off-by: Lianqin Hu <hulianqin@vivo.com> >> --- >> v8: Updated patch submission description as suggested in v7 discussion. >> v7: Remove code comments. >> v6: Update the commit text. >> v5: Add the Fixes tag. >> v4: CC stable kernel. >> v3: Add serial_port_lock protection when checking port pointer. >> v2: Optimize code comments. >> v1: Delete log printing. >> >> drivers/usb/gadget/function/u_serial.c | 31 >> +++++++++++++++++++++----- >> 1 file changed, 26 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/usb/gadget/function/u_serial.c >> b/drivers/usb/gadget/function/u_serial.c >> index b394105e55d6..66d918523b3e 100644 >> --- a/drivers/usb/gadget/function/u_serial.c >> +++ b/drivers/usb/gadget/function/u_serial.c >> @@ -452,20 +452,41 @@ static void gs_rx_push(struct work_struct *work) >> >> static void gs_read_complete(struct usb_ep *ep, struct usb_request >> *req) { >> - struct gs_port *port = ep->driver_data; >> + struct gs_port *port; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&serial_port_lock, flags); >> + port = ep->driver_data; >> + >> + if (!port) { >> + spin_unlock_irqrestore(&serial_port_lock, flags); >> + return; >> + } >> >> - /* Queue all received data until the tty layer is ready for it. */ >> spin_lock(&port->port_lock); >> + spin_unlock(&serial_port_lock); >> + >> + /* Queue all received data until the tty layer is ready for >> + it. */ >> list_add_tail(&req->list, &port->read_queue); >> schedule_delayed_work(&port->push, 0); >> - spin_unlock(&port->port_lock); >> + spin_unlock_irqrestore(&port->port_lock, flags); >> } >> >> static void gs_write_complete(struct usb_ep *ep, struct usb_request >> *req) { >> - struct gs_port *port = ep->driver_data; >> + struct gs_port *port; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&serial_port_lock, flags); >> + port = ep->driver_data; >> + >> + if (!port) { >> + spin_unlock_irqrestore(&serial_port_lock, flags); >> + return; >> + } >> >> spin_lock(&port->port_lock); >> + spin_unlock(&serial_port_lock); >> list_add(&req->list, &port->write_pool); >> port->write_started--; >> >> @@ -486,7 +507,7 @@ static void gs_write_complete(struct usb_ep *ep, struct usb_request *req) >> break; >> } >> >> - spin_unlock(&port->port_lock); >> + spin_unlock_irqrestore(&port->port_lock, flags); >> } >> >> static void gs_free_requests(struct usb_ep *ep, struct list_head >> *head, >> -- > >I can base my review on what was done before for suspend/resume and this implementation looks the same. >I don't think that both are optimal but this should avoid race on completion. Yes, the current way to resolve contention is consistent with the resume/suspend method. The callbacks gs_read_complete/gs_write_complete/gserial_suspend/gserial_resume are called from udc and can be performed asynchronously. Use locks to resolve contention issues. Thanks
diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c index b394105e55d6..66d918523b3e 100644 --- a/drivers/usb/gadget/function/u_serial.c +++ b/drivers/usb/gadget/function/u_serial.c @@ -452,20 +452,41 @@ static void gs_rx_push(struct work_struct *work) static void gs_read_complete(struct usb_ep *ep, struct usb_request *req) { - struct gs_port *port = ep->driver_data; + struct gs_port *port; + unsigned long flags; + + spin_lock_irqsave(&serial_port_lock, flags); + port = ep->driver_data; + + if (!port) { + spin_unlock_irqrestore(&serial_port_lock, flags); + return; + } - /* Queue all received data until the tty layer is ready for it. */ spin_lock(&port->port_lock); + spin_unlock(&serial_port_lock); + + /* Queue all received data until the tty layer is ready for it. */ list_add_tail(&req->list, &port->read_queue); schedule_delayed_work(&port->push, 0); - spin_unlock(&port->port_lock); + spin_unlock_irqrestore(&port->port_lock, flags); } static void gs_write_complete(struct usb_ep *ep, struct usb_request *req) { - struct gs_port *port = ep->driver_data; + struct gs_port *port; + unsigned long flags; + + spin_lock_irqsave(&serial_port_lock, flags); + port = ep->driver_data; + + if (!port) { + spin_unlock_irqrestore(&serial_port_lock, flags); + return; + } spin_lock(&port->port_lock); + spin_unlock(&serial_port_lock); list_add(&req->list, &port->write_pool); port->write_started--; @@ -486,7 +507,7 @@ static void gs_write_complete(struct usb_ep *ep, struct usb_request *req) break; } - spin_unlock(&port->port_lock); + spin_unlock_irqrestore(&port->port_lock, flags); } static void gs_free_requests(struct usb_ep *ep, struct list_head *head,