| Message ID | 20230928201904.454229-1-arakesh@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
Hi Avichal Thanks for the patch! On Thu, Sep 28, 2023 at 01:19:04PM -0700, Avichal Rakesh wrote: >Currently, when stopping the stream, uvcg_video_enable immediately >deallocates the usb_requests after calling usb_ep_dequeue. However, >usb_ep_dequeue is asynchronous and it is possible that it a request >that is still in use by the controller. This can lead to some hard >to reproduce crashes with double frees and general memory >inconsistencies. > >This patch sets up some stronger guarantees around when a request should >be deallocated. To that extent, this patch does the following: >1. When stream is stopped only the currently owned uvc_requests are freed > and all in-flight uvc_requests are marked 'abandoned' >2. uvc_video_complete callback is made responsible for freeing up the > abandoned requests. No uvc specific logic is triggered when handling > abandoned requests. > >This ensures that the ownership of uvc_request (and its corresponding >usb_request) is never ambiguous, and uvc_requests are always freed >regardless of when the requests are returned to the gadget driver. > >Other changes in the patch are required to decouple the allocated >uvc_requests from uvc_video that allocated them. Unfortunatly the format of this patch is relly heavy to review. You are changing to much at once. Could you please finegrain your changes in smaller overall topics. Starting with the transition from the one big kcalloc to the chaned list. This patch will be easy to review. After that you could address this issue you are refering to. This will help the review alot. I already saw some changes that are probably even completely unrelated. > >Link: https://lore.kernel.org/7cd81649-2795-45b6-8c10-b7df1055020d@google.com >Suggested-by: Michael Grzeschik <mgr@pengutronix.de> >Signed-off-by: Avichal Rakesh <arakesh@google.com> >--- >v1 -> v2: Switched from a timed wait to free on complete model based > on discussions with Michael. >v3 -> v3: Initialized ureqs at init to prevent accessing invalid > memory when no stream is created. > > drivers/usb/gadget/function/uvc.h | 4 +- > drivers/usb/gadget/function/uvc_video.c | 183 +++++++++++++++++------- > 2 files changed, 134 insertions(+), 53 deletions(-) > >diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h >index 989bc6b4e93d..e69cfb7cced1 100644 >--- a/drivers/usb/gadget/function/uvc.h >+++ b/drivers/usb/gadget/function/uvc.h >@@ -81,6 +81,8 @@ struct uvc_request { > struct sg_table sgt; > u8 header[UVCG_REQUEST_HEADER_LEN]; > struct uvc_buffer *last_buf; >+ struct list_head list; >+ bool is_abandoned; > }; > > struct uvc_video { >@@ -102,7 +104,7 @@ struct uvc_video { > > /* Requests */ > unsigned int req_size; >- struct uvc_request *ureq; >+ struct list_head ureqs; /* all uvc_requests allocated by uvc_video */ > struct list_head req_free; > spinlock_t req_lock; > >diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c >index 70ff88854539..aa5ef63e5540 100644 >--- a/drivers/usb/gadget/function/uvc_video.c >+++ b/drivers/usb/gadget/function/uvc_video.c >@@ -227,6 +227,23 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video, > * Request handling > */ > >+static void uvc_video_free_request(struct uvc_request *ureq, struct usb_ep *ep) >+{ >+ sg_free_table(&ureq->sgt); >+ if (ureq->req && ep) { >+ usb_ep_free_request(ep, ureq->req); >+ ureq->req = NULL; >+ } >+ >+ kfree(ureq->req_buffer); >+ ureq->req_buffer = NULL; >+ >+ if (!list_empty(&ureq->list)) >+ list_del(&ureq->list); >+ >+ kfree(ureq); >+} >+ > static int uvcg_video_ep_queue(struct uvc_video *video, struct usb_request *req) > { > int ret; >@@ -254,7 +271,21 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) > struct uvc_video *video = ureq->video; > struct uvc_video_queue *queue = &video->queue; > struct uvc_device *uvc = video->uvc; >+ struct uvc_buffer *last_buf; > unsigned long flags; >+ bool is_abandoned; >+ >+ spin_lock_irqsave(&video->req_lock, flags); >+ is_abandoned = ureq->is_abandoned; >+ last_buf = ureq->last_buf; >+ ureq->last_buf = NULL; >+ spin_unlock_irqrestore(&video->req_lock, flags); >+ >+ if (is_abandoned) { >+ uvcg_dbg(&video->uvc->func, "Freeing abandoned request\n"); >+ uvc_video_free_request(ureq, ep); >+ return; >+ } > > switch (req->status) { > case 0: >@@ -277,15 +308,27 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) > uvcg_queue_cancel(queue, 0); > } > >- if (ureq->last_buf) { >- uvcg_complete_buffer(&video->queue, ureq->last_buf); >- ureq->last_buf = NULL; >+ if (last_buf) { >+ spin_lock_irqsave(&video->queue.irqlock, flags); >+ uvcg_complete_buffer(&video->queue, last_buf); >+ spin_unlock_irqrestore(&video->queue.irqlock, flags); > } This change seems unrelated to the rest of the patch. > >+ /* >+ * request might have been abandoned while being processed. >+ * do a last minute check before queueing the request back. >+ */ > spin_lock_irqsave(&video->req_lock, flags); >- list_add_tail(&req->list, &video->req_free); >+ is_abandoned = ureq->is_abandoned; >+ if (!is_abandoned) >+ list_add_tail(&req->list, &video->req_free); > spin_unlock_irqrestore(&video->req_lock, flags); > >+ if (is_abandoned) { >+ uvc_video_free_request(ureq, ep); >+ return; >+ } >+ This whole check of is_abondoned is really hard to read and understand. Could it be somehow simplified? > if (uvc->state == UVC_STATE_STREAMING) > queue_work(video->async_wq, &video->pump); > } >@@ -293,25 +336,10 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) > static int > uvc_video_free_requests(struct uvc_video *video) > { >- unsigned int i; >- >- if (video->ureq) { >- for (i = 0; i < video->uvc_num_requests; ++i) { >- sg_free_table(&video->ureq[i].sgt); >+ struct uvc_request *ureq, *temp; > >- if (video->ureq[i].req) { >- usb_ep_free_request(video->ep, video->ureq[i].req); >- video->ureq[i].req = NULL; >- } >- >- if (video->ureq[i].req_buffer) { >- kfree(video->ureq[i].req_buffer); >- video->ureq[i].req_buffer = NULL; >- } >- } >- >- kfree(video->ureq); >- video->ureq = NULL; >+ list_for_each_entry_safe(ureq, temp, &video->ureqs, list) { >+ uvc_video_free_request(ureq, video->ep); I thought we wanted to iterate over the req_free list in this function? > } > > INIT_LIST_HEAD(&video->req_free); >@@ -322,6 +350,7 @@ uvc_video_free_requests(struct uvc_video *video) > static int > uvc_video_alloc_requests(struct uvc_video *video) > { >+ struct uvc_request *ureq; > unsigned int req_size; > unsigned int i; > int ret = -ENOMEM; >@@ -332,35 +361,36 @@ uvc_video_alloc_requests(struct uvc_video *video) > * max_t(unsigned int, video->ep->maxburst, 1) > * (video->ep->mult); > >- video->ureq = kcalloc(video->uvc_num_requests, sizeof(struct uvc_request), GFP_KERNEL); >- if (video->ureq == NULL) >- return -ENOMEM; >- >- for (i = 0; i < video->uvc_num_requests; ++i) { >- video->ureq[i].req_buffer = kmalloc(req_size, GFP_KERNEL); >- if (video->ureq[i].req_buffer == NULL) >+ INIT_LIST_HEAD(&video->ureqs); >+ for (i = 0; i < video->uvc_num_requests; i++) { >+ ureq = kzalloc(sizeof(struct uvc_request), GFP_KERNEL); >+ if (ureq == NULL) > goto error; >+ INIT_LIST_HEAD(&ureq->list); >+ list_add_tail(&ureq->list, &video->ureqs); >+ } > >- video->ureq[i].req = usb_ep_alloc_request(video->ep, GFP_KERNEL); >- if (video->ureq[i].req == NULL) >+ list_for_each_entry(ureq, &video->ureqs, list) { >+ ureq->req_buffer = kmalloc(req_size, GFP_KERNEL); >+ if (ureq->req_buffer == NULL) > goto error; >- >- video->ureq[i].req->buf = video->ureq[i].req_buffer; >- video->ureq[i].req->length = 0; >- video->ureq[i].req->complete = uvc_video_complete; >- video->ureq[i].req->context = &video->ureq[i]; >- video->ureq[i].video = video; >- video->ureq[i].last_buf = NULL; >- >- list_add_tail(&video->ureq[i].req->list, &video->req_free); >+ ureq->req = usb_ep_alloc_request(video->ep, GFP_KERNEL); >+ if (ureq->req == NULL) >+ goto error; >+ ureq->req->buf = ureq->req_buffer; >+ ureq->req->length = 0; >+ ureq->req->complete = uvc_video_complete; >+ ureq->req->context = ureq; >+ ureq->video = video; >+ ureq->last_buf = NULL; >+ list_add_tail(&ureq->req->list, &video->req_free); > /* req_size/PAGE_SIZE + 1 for overruns and + 1 for header */ >- sg_alloc_table(&video->ureq[i].sgt, >+ sg_alloc_table(&ureq->sgt, > DIV_ROUND_UP(req_size - UVCG_REQUEST_HEADER_LEN, > PAGE_SIZE) + 2, GFP_KERNEL); > } Please move these changes to an separate patch. > > video->req_size = req_size; >- > return 0; > > error: >@@ -484,12 +514,68 @@ static void uvcg_video_pump(struct work_struct *work) > return; > } > >+/* >+ * Disable video stream. This ensures that any inflight usb requests are marked >+ * for clean up and video buffers are dropped up before returning. >+ */ >+static void uvcg_video_disable(struct uvc_video *video) >+{ >+ struct uvc_buffer *buf, *tmp_buf; >+ struct uvc_request *ureq, *temp; >+ struct list_head buf_list; /* track in-flight video buffers */ >+ struct usb_request *req; >+ unsigned long flags; >+ >+ cancel_work_sync(&video->pump); >+ uvcg_queue_cancel(&video->queue, 0); >+ >+ INIT_LIST_HEAD(&buf_list); >+ spin_lock_irqsave(&video->req_lock, flags); >+ /* abandon all usb requests */ >+ list_for_each_entry_safe(ureq, temp, &video->ureqs, list) { >+ ureq->is_abandoned = true; >+ if (ureq->last_buf) { >+ list_add(&ureq->last_buf->queue, &buf_list); >+ ureq->last_buf = NULL; >+ } >+ list_del_init(&ureq->list); >+ if (ureq->req) >+ usb_ep_dequeue(video->ep, ureq->req); >+ } >+ /* >+ * re-add uvc_requests currently owned by the gadget to >+ * video->ureqs to be deallocated >+ */ >+ list_for_each_entry(req, &video->req_free, list) { >+ ureq = req->context; >+ list_add_tail(&ureq->list, &video->ureqs); >+ } So what you actually do is moving the requests from the free list to the ureqs list. Would it not be possible to separate the cleanup on two locations instead? >+ spin_unlock_irqrestore(&video->req_lock, flags); >+ >+ /* >+ * drop abandoned uvc_buffers, as the completion handler >+ * no longer will >+ */ >+ if (!list_empty(&buf_list)) { >+ spin_lock_irqsave(&video->queue.irqlock, flags); >+ list_for_each_entry_safe(buf, tmp_buf, >+ &buf_list, queue) { >+ video->queue.flags |= UVC_QUEUE_DROP_INCOMPLETE; >+ uvcg_complete_buffer(&video->queue, buf); >+ list_del(&buf->queue); >+ } >+ spin_unlock_irqrestore(&video->queue.irqlock, flags); >+ } >+ >+ uvc_video_free_requests(video); >+ uvcg_queue_enable(&video->queue, 0); >+} >+ > /* > * Enable or disable the video stream. > */ > int uvcg_video_enable(struct uvc_video *video, int enable) > { >- unsigned int i; > int ret; > > if (video->ep == NULL) { >@@ -499,15 +585,7 @@ int uvcg_video_enable(struct uvc_video *video, int enable) > } > > if (!enable) { >- cancel_work_sync(&video->pump); >- uvcg_queue_cancel(&video->queue, 0); >- >- for (i = 0; i < video->uvc_num_requests; ++i) >- if (video->ureq && video->ureq[i].req) >- usb_ep_dequeue(video->ep, video->ureq[i].req); >- >- uvc_video_free_requests(video); >- uvcg_queue_enable(&video->queue, 0); >+ uvcg_video_disable(video); > return 0; > } > >@@ -536,6 +614,7 @@ int uvcg_video_enable(struct uvc_video *video, int enable) > */ > int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc) > { >+ INIT_LIST_HEAD(&video->ureqs); > INIT_LIST_HEAD(&video->req_free); > spin_lock_init(&video->req_lock); > INIT_WORK(&video->pump, uvcg_video_pump); >-- >2.42.0.582.g8ccd20d70d-goog Regards, Michael
Thank you for the review, Michael! Moved the split up patches to https://lore.kernel.org/20230930184821.310143-1-arakesh@google.com/. Please continue the test/review there! On 9/28/23 14:04, Michael Grzeschik wrote: > Hi Avichal > > Thanks for the patch! > > On Thu, Sep 28, 2023 at 01:19:04PM -0700, Avichal Rakesh wrote: >> Currently, when stopping the stream, uvcg_video_enable immediately >> deallocates the usb_requests after calling usb_ep_dequeue. However, >> usb_ep_dequeue is asynchronous and it is possible that it a request >> that is still in use by the controller. This can lead to some hard >> to reproduce crashes with double frees and general memory >> inconsistencies. >> >> This patch sets up some stronger guarantees around when a request should >> be deallocated. To that extent, this patch does the following: >> 1. When stream is stopped only the currently owned uvc_requests are freed >> and all in-flight uvc_requests are marked 'abandoned' >> 2. uvc_video_complete callback is made responsible for freeing up the >> abandoned requests. No uvc specific logic is triggered when handling >> abandoned requests. >> >> This ensures that the ownership of uvc_request (and its corresponding >> usb_request) is never ambiguous, and uvc_requests are always freed >> regardless of when the requests are returned to the gadget driver. >> >> Other changes in the patch are required to decouple the allocated >> uvc_requests from uvc_video that allocated them. > > > Unfortunatly the format of this patch is relly heavy to review. You are > changing to much at once. Could you please finegrain your changes in > smaller overall topics. > > Starting with the transition from the one big kcalloc to the chaned > list. This patch will be easy to review. > > After that you could address this issue you are refering to. > > This will help the review alot. I already saw some changes that are > probably even completely unrelated. That is fair, sent out https://lore.kernel.org/20230930184821.310143-1-arakesh@google.com/ to split up patches as a separate thread, because the chains here are already getting difficult to follow. > > >> <snip> >> - if (ureq->last_buf) { >> - uvcg_complete_buffer(&video->queue, ureq->last_buf); >> - ureq->last_buf = NULL; >> + if (last_buf) { >> + spin_lock_irqsave(&video->queue.irqlock, flags); >> + uvcg_complete_buffer(&video->queue, last_buf); >> + spin_unlock_irqrestore(&video->queue.irqlock, flags); >> } > > This change seems unrelated to the rest of the patch. Oh, yeah I suppose this is not strictly related. Considering how this patch tightens up the threading guarantees, it seemed fair to add the locks for a function that said " called with &queue_irqlock held.. " as its singular comment. Okay to remove it though. > >> >> + /* >> + * request might have been abandoned while being processed. >> + * do a last minute check before queueing the request back. >> + */ >> spin_lock_irqsave(&video->req_lock, flags); >> - list_add_tail(&req->list, &video->req_free); >> + is_abandoned = ureq->is_abandoned; >> + if (!is_abandoned) >> + list_add_tail(&req->list, &video->req_free); >> spin_unlock_irqrestore(&video->req_lock, flags); >> >> + if (is_abandoned) { >> + uvc_video_free_request(ureq, ep); >> + return; >> + } >> + > > This whole check of is_abondoned is really hard to read and understand. > Could it be somehow simplified? Let me know if the split up patches are any easier to parse. We need this because we aren't synchronizing the whole complete callback, and we can't rely on the old state of is_abandoned flag to queue the request back up. So we need to get the current value of is_abandoned, and queue the request back iff the request isn't currently abandoned (and we can guaratee that it won't be abandoned while we're queueing it back up). > >> if (uvc->state == UVC_STATE_STREAMING) >> queue_work(video->async_wq, &video->pump); >> } >> @@ -293,25 +336,10 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) >> static int >> uvc_video_free_requests(struct uvc_video *video) >> { >> - unsigned int i; >> - >> - if (video->ureq) { >> - for (i = 0; i < video->uvc_num_requests; ++i) { >> - sg_free_table(&video->ureq[i].sgt); >> + struct uvc_request *ureq, *temp; >> >> - if (video->ureq[i].req) { >> - usb_ep_free_request(video->ep, video->ureq[i].req); >> - video->ureq[i].req = NULL; >> - } >> - >> - if (video->ureq[i].req_buffer) { >> - kfree(video->ureq[i].req_buffer); >> - video->ureq[i].req_buffer = NULL; >> - } >> - } >> - >> - kfree(video->ureq); >> - video->ureq = NULL; >> + list_for_each_entry_safe(ureq, temp, &video->ureqs, list) { >> + uvc_video_free_request(ureq, video->ep); > > I thought we wanted to iterate over the req_free list in this function? Not here. This function frees all the requests owned by uvc gadget driver, which is held in ureqs. We abandon the inflight requests, and removed them from ureqs before cleaning up. > >> } >> >> INIT_LIST_HEAD(&video->req_free); >> @@ -322,6 +350,7 @@ uvc_video_free_requests(struct uvc_video *video) >> static int >> uvc_video_alloc_requests(struct uvc_video *video) >> { >> + struct uvc_request *ureq; >> unsigned int req_size; >> unsigned int i; >> int ret = -ENOMEM; >> @@ -332,35 +361,36 @@ uvc_video_alloc_requests(struct uvc_video *video) >> * max_t(unsigned int, video->ep->maxburst, 1) >> * (video->ep->mult); >> >> - video->ureq = kcalloc(video->uvc_num_requests, sizeof(struct uvc_request), GFP_KERNEL); >> - if (video->ureq == NULL) >> - return -ENOMEM; >> - >> - for (i = 0; i < video->uvc_num_requests; ++i) { >> - video->ureq[i].req_buffer = kmalloc(req_size, GFP_KERNEL); >> - if (video->ureq[i].req_buffer == NULL) >> + INIT_LIST_HEAD(&video->ureqs); >> + for (i = 0; i < video->uvc_num_requests; i++) { >> + ureq = kzalloc(sizeof(struct uvc_request), GFP_KERNEL); >> + if (ureq == NULL) >> goto error; >> + INIT_LIST_HEAD(&ureq->list); >> + list_add_tail(&ureq->list, &video->ureqs); >> + } >> >> - video->ureq[i].req = usb_ep_alloc_request(video->ep, GFP_KERNEL); >> - if (video->ureq[i].req == NULL) >> + list_for_each_entry(ureq, &video->ureqs, list) { >> + ureq->req_buffer = kmalloc(req_size, GFP_KERNEL); >> + if (ureq->req_buffer == NULL) >> goto error; >> - >> - video->ureq[i].req->buf = video->ureq[i].req_buffer; >> - video->ureq[i].req->length = 0; >> - video->ureq[i].req->complete = uvc_video_complete; >> - video->ureq[i].req->context = &video->ureq[i]; >> - video->ureq[i].video = video; >> - video->ureq[i].last_buf = NULL; >> - >> - list_add_tail(&video->ureq[i].req->list, &video->req_free); >> + ureq->req = usb_ep_alloc_request(video->ep, GFP_KERNEL); >> + if (ureq->req == NULL) >> + goto error; >> + ureq->req->buf = ureq->req_buffer; >> + ureq->req->length = 0; >> + ureq->req->complete = uvc_video_complete; >> + ureq->req->context = ureq; >> + ureq->video = video; >> + ureq->last_buf = NULL; >> + list_add_tail(&ureq->req->list, &video->req_free); >> /* req_size/PAGE_SIZE + 1 for overruns and + 1 for header */ >> - sg_alloc_table(&video->ureq[i].sgt, >> + sg_alloc_table(&ureq->sgt, >> DIV_ROUND_UP(req_size - UVCG_REQUEST_HEADER_LEN, >> PAGE_SIZE) + 2, GFP_KERNEL); >> } > > Please move these changes to an separate patch. Yep, done! >> >> video->req_size = req_size; >> - >> return 0; >> >> error: >> @@ -484,12 +514,68 @@ static void uvcg_video_pump(struct work_struct *work) >> return; >> } >> >> +/* >> + * Disable video stream. This ensures that any inflight usb requests are marked >> + * for clean up and video buffers are dropped up before returning. >> + */ >> +static void uvcg_video_disable(struct uvc_video *video) >> +{ >> + struct uvc_buffer *buf, *tmp_buf; >> + struct uvc_request *ureq, *temp; >> + struct list_head buf_list; /* track in-flight video buffers */ >> + struct usb_request *req; >> + unsigned long flags; >> + >> + cancel_work_sync(&video->pump); >> + uvcg_queue_cancel(&video->queue, 0); >> + >> + INIT_LIST_HEAD(&buf_list); >> + spin_lock_irqsave(&video->req_lock, flags); >> + /* abandon all usb requests */ >> + list_for_each_entry_safe(ureq, temp, &video->ureqs, list) { >> + ureq->is_abandoned = true; >> + if (ureq->last_buf) { >> + list_add(&ureq->last_buf->queue, &buf_list); >> + ureq->last_buf = NULL; >> + } >> + list_del_init(&ureq->list); >> + if (ureq->req) >> + usb_ep_dequeue(video->ep, ureq->req); >> + } >> + /* >> + * re-add uvc_requests currently owned by the gadget to >> + * video->ureqs to be deallocated >> + */ >> + list_for_each_entry(req, &video->req_free, list) { >> + ureq = req->context; >> + list_add_tail(&ureq->list, &video->ureqs); >> + } > So what you actually do is moving the requests from the free list > to the ureqs list. Would it not be possible to separate the cleanup > on two locations instead? Rather than thinking of it as "moving the requests from req_free to ureqs", it might be easier to reason it as: removing all uvc_requests from ureqs that are associated with inflight usb_requests. We remove those uvc_requests by using req_free but we could've done that in some other way (happy to take recommendations if there is a simpler way of doing so). > >> + spin_unlock_irqrestore(&video->req_lock, flags); >> + >> + /* Regards, Avi.
diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h index 989bc6b4e93d..e69cfb7cced1 100644 --- a/drivers/usb/gadget/function/uvc.h +++ b/drivers/usb/gadget/function/uvc.h @@ -81,6 +81,8 @@ struct uvc_request { struct sg_table sgt; u8 header[UVCG_REQUEST_HEADER_LEN]; struct uvc_buffer *last_buf; + struct list_head list; + bool is_abandoned; }; struct uvc_video { @@ -102,7 +104,7 @@ struct uvc_video { /* Requests */ unsigned int req_size; - struct uvc_request *ureq; + struct list_head ureqs; /* all uvc_requests allocated by uvc_video */ struct list_head req_free; spinlock_t req_lock; diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c index 70ff88854539..aa5ef63e5540 100644 --- a/drivers/usb/gadget/function/uvc_video.c +++ b/drivers/usb/gadget/function/uvc_video.c @@ -227,6 +227,23 @@ uvc_video_encode_isoc(struct usb_request *req, struct uvc_video *video, * Request handling */ +static void uvc_video_free_request(struct uvc_request *ureq, struct usb_ep *ep) +{ + sg_free_table(&ureq->sgt); + if (ureq->req && ep) { + usb_ep_free_request(ep, ureq->req); + ureq->req = NULL; + } + + kfree(ureq->req_buffer); + ureq->req_buffer = NULL; + + if (!list_empty(&ureq->list)) + list_del(&ureq->list); + + kfree(ureq); +} + static int uvcg_video_ep_queue(struct uvc_video *video, struct usb_request *req) { int ret; @@ -254,7 +271,21 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) struct uvc_video *video = ureq->video; struct uvc_video_queue *queue = &video->queue; struct uvc_device *uvc = video->uvc; + struct uvc_buffer *last_buf; unsigned long flags; + bool is_abandoned; + + spin_lock_irqsave(&video->req_lock, flags); + is_abandoned = ureq->is_abandoned; + last_buf = ureq->last_buf; + ureq->last_buf = NULL; + spin_unlock_irqrestore(&video->req_lock, flags); + + if (is_abandoned) { + uvcg_dbg(&video->uvc->func, "Freeing abandoned request\n"); + uvc_video_free_request(ureq, ep); + return; + } switch (req->status) { case 0: @@ -277,15 +308,27 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) uvcg_queue_cancel(queue, 0); } - if (ureq->last_buf) { - uvcg_complete_buffer(&video->queue, ureq->last_buf); - ureq->last_buf = NULL; + if (last_buf) { + spin_lock_irqsave(&video->queue.irqlock, flags); + uvcg_complete_buffer(&video->queue, last_buf); + spin_unlock_irqrestore(&video->queue.irqlock, flags); } + /* + * request might have been abandoned while being processed. + * do a last minute check before queueing the request back. + */ spin_lock_irqsave(&video->req_lock, flags); - list_add_tail(&req->list, &video->req_free); + is_abandoned = ureq->is_abandoned; + if (!is_abandoned) + list_add_tail(&req->list, &video->req_free); spin_unlock_irqrestore(&video->req_lock, flags); + if (is_abandoned) { + uvc_video_free_request(ureq, ep); + return; + } + if (uvc->state == UVC_STATE_STREAMING) queue_work(video->async_wq, &video->pump); } @@ -293,25 +336,10 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req) static int uvc_video_free_requests(struct uvc_video *video) { - unsigned int i; - - if (video->ureq) { - for (i = 0; i < video->uvc_num_requests; ++i) { - sg_free_table(&video->ureq[i].sgt); + struct uvc_request *ureq, *temp; - if (video->ureq[i].req) { - usb_ep_free_request(video->ep, video->ureq[i].req); - video->ureq[i].req = NULL; - } - - if (video->ureq[i].req_buffer) { - kfree(video->ureq[i].req_buffer); - video->ureq[i].req_buffer = NULL; - } - } - - kfree(video->ureq); - video->ureq = NULL; + list_for_each_entry_safe(ureq, temp, &video->ureqs, list) { + uvc_video_free_request(ureq, video->ep); } INIT_LIST_HEAD(&video->req_free); @@ -322,6 +350,7 @@ uvc_video_free_requests(struct uvc_video *video) static int uvc_video_alloc_requests(struct uvc_video *video) { + struct uvc_request *ureq; unsigned int req_size; unsigned int i; int ret = -ENOMEM; @@ -332,35 +361,36 @@ uvc_video_alloc_requests(struct uvc_video *video) * max_t(unsigned int, video->ep->maxburst, 1) * (video->ep->mult); - video->ureq = kcalloc(video->uvc_num_requests, sizeof(struct uvc_request), GFP_KERNEL); - if (video->ureq == NULL) - return -ENOMEM; - - for (i = 0; i < video->uvc_num_requests; ++i) { - video->ureq[i].req_buffer = kmalloc(req_size, GFP_KERNEL); - if (video->ureq[i].req_buffer == NULL) + INIT_LIST_HEAD(&video->ureqs); + for (i = 0; i < video->uvc_num_requests; i++) { + ureq = kzalloc(sizeof(struct uvc_request), GFP_KERNEL); + if (ureq == NULL) goto error; + INIT_LIST_HEAD(&ureq->list); + list_add_tail(&ureq->list, &video->ureqs); + } - video->ureq[i].req = usb_ep_alloc_request(video->ep, GFP_KERNEL); - if (video->ureq[i].req == NULL) + list_for_each_entry(ureq, &video->ureqs, list) { + ureq->req_buffer = kmalloc(req_size, GFP_KERNEL); + if (ureq->req_buffer == NULL) goto error; - - video->ureq[i].req->buf = video->ureq[i].req_buffer; - video->ureq[i].req->length = 0; - video->ureq[i].req->complete = uvc_video_complete; - video->ureq[i].req->context = &video->ureq[i]; - video->ureq[i].video = video; - video->ureq[i].last_buf = NULL; - - list_add_tail(&video->ureq[i].req->list, &video->req_free); + ureq->req = usb_ep_alloc_request(video->ep, GFP_KERNEL); + if (ureq->req == NULL) + goto error; + ureq->req->buf = ureq->req_buffer; + ureq->req->length = 0; + ureq->req->complete = uvc_video_complete; + ureq->req->context = ureq; + ureq->video = video; + ureq->last_buf = NULL; + list_add_tail(&ureq->req->list, &video->req_free); /* req_size/PAGE_SIZE + 1 for overruns and + 1 for header */ - sg_alloc_table(&video->ureq[i].sgt, + sg_alloc_table(&ureq->sgt, DIV_ROUND_UP(req_size - UVCG_REQUEST_HEADER_LEN, PAGE_SIZE) + 2, GFP_KERNEL); } video->req_size = req_size; - return 0; error: @@ -484,12 +514,68 @@ static void uvcg_video_pump(struct work_struct *work) return; } +/* + * Disable video stream. This ensures that any inflight usb requests are marked + * for clean up and video buffers are dropped up before returning. + */ +static void uvcg_video_disable(struct uvc_video *video) +{ + struct uvc_buffer *buf, *tmp_buf; + struct uvc_request *ureq, *temp; + struct list_head buf_list; /* track in-flight video buffers */ + struct usb_request *req; + unsigned long flags; + + cancel_work_sync(&video->pump); + uvcg_queue_cancel(&video->queue, 0); + + INIT_LIST_HEAD(&buf_list); + spin_lock_irqsave(&video->req_lock, flags); + /* abandon all usb requests */ + list_for_each_entry_safe(ureq, temp, &video->ureqs, list) { + ureq->is_abandoned = true; + if (ureq->last_buf) { + list_add(&ureq->last_buf->queue, &buf_list); + ureq->last_buf = NULL; + } + list_del_init(&ureq->list); + if (ureq->req) + usb_ep_dequeue(video->ep, ureq->req); + } + /* + * re-add uvc_requests currently owned by the gadget to + * video->ureqs to be deallocated + */ + list_for_each_entry(req, &video->req_free, list) { + ureq = req->context; + list_add_tail(&ureq->list, &video->ureqs); + } + spin_unlock_irqrestore(&video->req_lock, flags); + + /* + * drop abandoned uvc_buffers, as the completion handler + * no longer will + */ + if (!list_empty(&buf_list)) { + spin_lock_irqsave(&video->queue.irqlock, flags); + list_for_each_entry_safe(buf, tmp_buf, + &buf_list, queue) { + video->queue.flags |= UVC_QUEUE_DROP_INCOMPLETE; + uvcg_complete_buffer(&video->queue, buf); + list_del(&buf->queue); + } + spin_unlock_irqrestore(&video->queue.irqlock, flags); + } + + uvc_video_free_requests(video); + uvcg_queue_enable(&video->queue, 0); +} + /* * Enable or disable the video stream. */ int uvcg_video_enable(struct uvc_video *video, int enable) { - unsigned int i; int ret; if (video->ep == NULL) { @@ -499,15 +585,7 @@ int uvcg_video_enable(struct uvc_video *video, int enable) } if (!enable) { - cancel_work_sync(&video->pump); - uvcg_queue_cancel(&video->queue, 0); - - for (i = 0; i < video->uvc_num_requests; ++i) - if (video->ureq && video->ureq[i].req) - usb_ep_dequeue(video->ep, video->ureq[i].req); - - uvc_video_free_requests(video); - uvcg_queue_enable(&video->queue, 0); + uvcg_video_disable(video); return 0; } @@ -536,6 +614,7 @@ int uvcg_video_enable(struct uvc_video *video, int enable) */ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc) { + INIT_LIST_HEAD(&video->ureqs); INIT_LIST_HEAD(&video->req_free); spin_lock_init(&video->req_lock); INIT_WORK(&video->pump, uvcg_video_pump);
Currently, when stopping the stream, uvcg_video_enable immediately deallocates the usb_requests after calling usb_ep_dequeue. However, usb_ep_dequeue is asynchronous and it is possible that it a request that is still in use by the controller. This can lead to some hard to reproduce crashes with double frees and general memory inconsistencies. This patch sets up some stronger guarantees around when a request should be deallocated. To that extent, this patch does the following: 1. When stream is stopped only the currently owned uvc_requests are freed and all in-flight uvc_requests are marked 'abandoned' 2. uvc_video_complete callback is made responsible for freeing up the abandoned requests. No uvc specific logic is triggered when handling abandoned requests. This ensures that the ownership of uvc_request (and its corresponding usb_request) is never ambiguous, and uvc_requests are always freed regardless of when the requests are returned to the gadget driver. Other changes in the patch are required to decouple the allocated uvc_requests from uvc_video that allocated them. Link: https://lore.kernel.org/7cd81649-2795-45b6-8c10-b7df1055020d@google.com Suggested-by: Michael Grzeschik <mgr@pengutronix.de> Signed-off-by: Avichal Rakesh <arakesh@google.com> --- v1 -> v2: Switched from a timed wait to free on complete model based on discussions with Michael. v3 -> v3: Initialized ureqs at init to prevent accessing invalid memory when no stream is created. drivers/usb/gadget/function/uvc.h | 4 +- drivers/usb/gadget/function/uvc_video.c | 183 +++++++++++++++++------- 2 files changed, 134 insertions(+), 53 deletions(-) -- 2.42.0.582.g8ccd20d70d-goog