| Message ID | 1645507768-77687-1-git-send-email-dh10.jung@samsung.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | aaaba1c86d04dac8e49bf508b492f81506257da3 |
| Headers | show |
| Series | usb: gadget: rndis: add spinlock for rndis response list | expand |
On Tue, Feb 22, 2022 at 02:29:28PM +0900, Daehwan Jung wrote: > There's no lock for rndis response list. It could cause list corruption > if there're two different list_add at the same time like below. > It's better to add in rndis_add_response / rndis_free_response > / rndis_get_next_response to prevent any race condition on response list. > > [ 361.894299] [1: irq/191-dwc3:16979] list_add corruption. > next->prev should be prev (ffffff80651764d0), > but was ffffff883dc36f80. (next=ffffff80651764d0). > > [ 361.904380] [1: irq/191-dwc3:16979] Call trace: > [ 361.904391] [1: irq/191-dwc3:16979] __list_add_valid+0x74/0x90 > [ 361.904401] [1: irq/191-dwc3:16979] rndis_msg_parser+0x168/0x8c0 > [ 361.904409] [1: irq/191-dwc3:16979] rndis_command_complete+0x24/0x84 > [ 361.904417] [1: irq/191-dwc3:16979] usb_gadget_giveback_request+0x20/0xe4 > [ 361.904426] [1: irq/191-dwc3:16979] dwc3_gadget_giveback+0x44/0x60 > [ 361.904434] [1: irq/191-dwc3:16979] dwc3_ep0_complete_data+0x1e8/0x3a0 > [ 361.904442] [1: irq/191-dwc3:16979] dwc3_ep0_interrupt+0x29c/0x3dc > [ 361.904450] [1: irq/191-dwc3:16979] dwc3_process_event_entry+0x78/0x6cc > [ 361.904457] [1: irq/191-dwc3:16979] dwc3_process_event_buf+0xa0/0x1ec > [ 361.904465] [1: irq/191-dwc3:16979] dwc3_thread_interrupt+0x34/0x5c > This is just one context. what about the other contexts? Interested to see the different contexts under which this list is being manipulated. From the f_rndis perspective, this list is touched from setup and disconnect callbacks. so are those two contexts not serialized from the UDC? not saying we don't need lock, but would be good to record that information in the change log. > Fixes: f6281af9d62e ("usb: gadget: rndis: use list_for_each_entry_safe") > Signed-off-by: Daehwan Jung <dh10.jung@samsung.com> > --- > drivers/usb/gadget/function/rndis.c | 8 ++++++++ > drivers/usb/gadget/function/rndis.h | 1 + > 2 files changed, 9 insertions(+) > > diff --git a/drivers/usb/gadget/function/rndis.c b/drivers/usb/gadget/function/rndis.c > index 431d5a7..79fd994 100644 > --- a/drivers/usb/gadget/function/rndis.c > +++ b/drivers/usb/gadget/function/rndis.c > @@ -919,6 +919,7 @@ struct rndis_params *rndis_register(void (*resp_avail)(void *v), void *v) > params->resp_avail = resp_avail; > params->v = v; > INIT_LIST_HEAD(¶ms->resp_queue); > + spin_lock_init(¶ms->resp_lock); > pr_debug("%s: configNr = %d\n", __func__, i); > > return params; > @@ -1012,12 +1013,14 @@ void rndis_free_response(struct rndis_params *params, u8 *buf) > { > rndis_resp_t *r, *n; > > + spin_lock(¶ms->resp_lock); > list_for_each_entry_safe(r, n, ¶ms->resp_queue, list) { > if (r->buf == buf) { > list_del(&r->list); > kfree(r); > } > } > + spin_unlock(¶ms->resp_lock); > } > EXPORT_SYMBOL_GPL(rndis_free_response); Are you sure that this lock is not acquired from any interrupt context from other contexts? Also would it be true for all UDC? some UDC may call setup from hadirq context and disconnect in process context etc or vice versa. we don't want to acquire lock without disabling interrupts in that case. Thanks, Pavan
diff --git a/drivers/usb/gadget/function/rndis.c b/drivers/usb/gadget/function/rndis.c index 431d5a7..79fd994 100644 --- a/drivers/usb/gadget/function/rndis.c +++ b/drivers/usb/gadget/function/rndis.c @@ -919,6 +919,7 @@ struct rndis_params *rndis_register(void (*resp_avail)(void *v), void *v) params->resp_avail = resp_avail; params->v = v; INIT_LIST_HEAD(¶ms->resp_queue); + spin_lock_init(¶ms->resp_lock); pr_debug("%s: configNr = %d\n", __func__, i); return params; @@ -1012,12 +1013,14 @@ void rndis_free_response(struct rndis_params *params, u8 *buf) { rndis_resp_t *r, *n; + spin_lock(¶ms->resp_lock); list_for_each_entry_safe(r, n, ¶ms->resp_queue, list) { if (r->buf == buf) { list_del(&r->list); kfree(r); } } + spin_unlock(¶ms->resp_lock); } EXPORT_SYMBOL_GPL(rndis_free_response); @@ -1027,14 +1030,17 @@ u8 *rndis_get_next_response(struct rndis_params *params, u32 *length) if (!length) return NULL; + spin_lock(¶ms->resp_lock); list_for_each_entry_safe(r, n, ¶ms->resp_queue, list) { if (!r->send) { r->send = 1; *length = r->length; + spin_unlock(¶ms->resp_lock); return r->buf; } } + spin_unlock(¶ms->resp_lock); return NULL; } EXPORT_SYMBOL_GPL(rndis_get_next_response); @@ -1051,7 +1057,9 @@ static rndis_resp_t *rndis_add_response(struct rndis_params *params, u32 length) r->length = length; r->send = 0; + spin_lock(¶ms->resp_lock); list_add_tail(&r->list, ¶ms->resp_queue); + spin_unlock(¶ms->resp_lock); return r; } diff --git a/drivers/usb/gadget/function/rndis.h b/drivers/usb/gadget/function/rndis.h index f6167f7..6206b8b 100644 --- a/drivers/usb/gadget/function/rndis.h +++ b/drivers/usb/gadget/function/rndis.h @@ -174,6 +174,7 @@ typedef struct rndis_params { void (*resp_avail)(void *v); void *v; struct list_head resp_queue; + spinlock_t resp_lock; } rndis_params; /* RNDIS Message parser and other useless functions */
There's no lock for rndis response list. It could cause list corruption if there're two different list_add at the same time like below. It's better to add in rndis_add_response / rndis_free_response / rndis_get_next_response to prevent any race condition on response list. [ 361.894299] [1: irq/191-dwc3:16979] list_add corruption. next->prev should be prev (ffffff80651764d0), but was ffffff883dc36f80. (next=ffffff80651764d0). [ 361.904380] [1: irq/191-dwc3:16979] Call trace: [ 361.904391] [1: irq/191-dwc3:16979] __list_add_valid+0x74/0x90 [ 361.904401] [1: irq/191-dwc3:16979] rndis_msg_parser+0x168/0x8c0 [ 361.904409] [1: irq/191-dwc3:16979] rndis_command_complete+0x24/0x84 [ 361.904417] [1: irq/191-dwc3:16979] usb_gadget_giveback_request+0x20/0xe4 [ 361.904426] [1: irq/191-dwc3:16979] dwc3_gadget_giveback+0x44/0x60 [ 361.904434] [1: irq/191-dwc3:16979] dwc3_ep0_complete_data+0x1e8/0x3a0 [ 361.904442] [1: irq/191-dwc3:16979] dwc3_ep0_interrupt+0x29c/0x3dc [ 361.904450] [1: irq/191-dwc3:16979] dwc3_process_event_entry+0x78/0x6cc [ 361.904457] [1: irq/191-dwc3:16979] dwc3_process_event_buf+0xa0/0x1ec [ 361.904465] [1: irq/191-dwc3:16979] dwc3_thread_interrupt+0x34/0x5c Fixes: f6281af9d62e ("usb: gadget: rndis: use list_for_each_entry_safe") Signed-off-by: Daehwan Jung <dh10.jung@samsung.com> --- drivers/usb/gadget/function/rndis.c | 8 ++++++++ drivers/usb/gadget/function/rndis.h | 1 + 2 files changed, 9 insertions(+)