[PATCH v1 0/2] usb: gadget: uvc: stability fixes when stopping streams

[PATCH v1 0/2] usb: gadget: uvc: stability fixes when stopping streams

[Avichal Rakesh]

We have been seeing two main bugs when stopping stream:
1. attempting to queue usb_requests on a disabled usb endpoint, and
2. use-after-free problems for inflight requests

Avichal Rakesh (2):
  usb: gadget: uvc: prevent use of disabled endpoint
  usb: gadget: uvc: prevent de-allocating inflight usb_requests

 drivers/usb/gadget/function/f_uvc.c     | 11 ++++----
 drivers/usb/gadget/function/f_uvc.h     |  2 +-
 drivers/usb/gadget/function/uvc.h       |  5 +++-
 drivers/usb/gadget/function/uvc_v4l2.c  | 21 ++++++++++++---
 drivers/usb/gadget/function/uvc_video.c | 34 +++++++++++++++++++++++--
 5 files changed, 60 insertions(+), 13 deletions(-)

--
2.42.0.283.g2d96d420d3-goog

[PATCH v1 1/2] usb: gadget: uvc: prevent use of disabled endpoint

[Avichal Rakesh]

Currently the set_alt callback immediately disables the endpoint and queues
the v4l2 streamoff event. However, as the streamoff event is processed
asynchronously, it is possible that the video_pump thread attempts to queue
requests to an already disabled endpoint.

This change moves disabling usb endpoint to the end of streamoff event
callback. As the endpoint's state can no longer be used, video_pump is
now guarded by uvc->state as well. To be consistent with the actual
streaming state, uvc->state is now toggled between CONNECTED and STREAMING
from the v4l2 event callback only.

Link: https://lore.kernel.org/20230615171558.GK741@pendragon.ideasonboard.com/
Link: https://lore.kernel.org/20230531085544.253363-1-dan.scally@ideasonboard.com/
Signed-off-by: Avichal Rakesh <arakesh@google.com>
---
 drivers/usb/gadget/function/f_uvc.c     | 11 +++++------
 drivers/usb/gadget/function/f_uvc.h     |  2 +-
 drivers/usb/gadget/function/uvc.h       |  2 +-
 drivers/usb/gadget/function/uvc_v4l2.c  | 21 ++++++++++++++++++---
 drivers/usb/gadget/function/uvc_video.c |  3 ++-
 5 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
index faa398109431..75c9f9a3f884 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -263,10 +263,13 @@ uvc_function_setup(struct usb_function *f, const struct usb_ctrlrequest *ctrl)
 	return 0;
 }
 
-void uvc_function_setup_continue(struct uvc_device *uvc)
+void uvc_function_setup_continue(struct uvc_device *uvc, int disable_ep)
 {
 	struct usb_composite_dev *cdev = uvc->func.config->cdev;
 
+	if (disable_ep && uvc->video.ep) {
+		usb_ep_disable(uvc->video.ep);
+	}
 	usb_composite_setup_continue(cdev);
 }
 
@@ -337,15 +340,11 @@ uvc_function_set_alt(struct usb_function *f, unsigned interface, unsigned alt)
 		if (uvc->state != UVC_STATE_STREAMING)
 			return 0;
 
-		if (uvc->video.ep)
-			usb_ep_disable(uvc->video.ep);
-
 		memset(&v4l2_event, 0, sizeof(v4l2_event));
 		v4l2_event.type = UVC_EVENT_STREAMOFF;
 		v4l2_event_queue(&uvc->vdev, &v4l2_event);
 
-		uvc->state = UVC_STATE_CONNECTED;
-		return 0;
+		return USB_GADGET_DELAYED_STATUS;
 
 	case 1:
 		if (uvc->state != UVC_STATE_CONNECTED)
diff --git a/drivers/usb/gadget/function/f_uvc.h b/drivers/usb/gadget/function/f_uvc.h
index 1db972d4beeb..e7f9f13f14dc 100644
--- a/drivers/usb/gadget/function/f_uvc.h
+++ b/drivers/usb/gadget/function/f_uvc.h
@@ -11,7 +11,7 @@
 
 struct uvc_device;
 
-void uvc_function_setup_continue(struct uvc_device *uvc);
+void uvc_function_setup_continue(struct uvc_device *uvc, int disale_ep);
 
 void uvc_function_connect(struct uvc_device *uvc);
 
diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index 6751de8b63ad..989bc6b4e93d 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -177,7 +177,7 @@ struct uvc_file_handle {
  * Functions
  */
 
-extern void uvc_function_setup_continue(struct uvc_device *uvc);
+extern void uvc_function_setup_continue(struct uvc_device *uvc, int disable_ep);
 extern void uvc_function_connect(struct uvc_device *uvc);
 extern void uvc_function_disconnect(struct uvc_device *uvc);
 
diff --git a/drivers/usb/gadget/function/uvc_v4l2.c b/drivers/usb/gadget/function/uvc_v4l2.c
index 3f0a9795c0d4..3d3469883ed0 100644
--- a/drivers/usb/gadget/function/uvc_v4l2.c
+++ b/drivers/usb/gadget/function/uvc_v4l2.c
@@ -451,7 +451,7 @@ uvc_v4l2_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	 * Complete the alternate setting selection setup phase now that
 	 * userspace is ready to provide video frames.
 	 */
-	uvc_function_setup_continue(uvc);
+	uvc_function_setup_continue(uvc, 0);
 	uvc->state = UVC_STATE_STREAMING;
 
 	return 0;
@@ -463,11 +463,19 @@ uvc_v4l2_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
 	struct video_device *vdev = video_devdata(file);
 	struct uvc_device *uvc = video_get_drvdata(vdev);
 	struct uvc_video *video = &uvc->video;
+	int ret = 0;
 
 	if (type != video->queue.queue.type)
 		return -EINVAL;
 
-	return uvcg_video_enable(video, 0);
+	uvc->state = UVC_STATE_CONNECTED;
+	ret = uvcg_video_enable(video, 0);
+	if (ret < 0) {
+		return ret;
+	}
+
+	uvc_function_setup_continue(uvc, 1);
+	return 0;
 }
 
 static int
@@ -500,6 +508,14 @@ uvc_v4l2_subscribe_event(struct v4l2_fh *fh,
 static void uvc_v4l2_disable(struct uvc_device *uvc)
 {
 	uvc_function_disconnect(uvc);
+	if (uvc->state == UVC_STATE_STREAMING) {
+		/*
+		 * Drop uvc->state to CONNECTED if it was streaming before.
+		 * This ensures that the usb_requests are no longer queued
+		 * to the controller.
+		 */
+		uvc->state = UVC_STATE_CONNECTED;
+	}
 	uvcg_video_enable(&uvc->video, 0);
 	uvcg_free_buffers(&uvc->video.queue);
 	uvc->func_connected = false;
@@ -647,4 +663,3 @@ const struct v4l2_file_operations uvc_v4l2_fops = {
 	.get_unmapped_area = uvcg_v4l2_get_unmapped_area,
 #endif
 };
-
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 91af3b1ef0d4..70ff88854539 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -382,6 +382,7 @@ static void uvcg_video_pump(struct work_struct *work)
 {
 	struct uvc_video *video = container_of(work, struct uvc_video, pump);
 	struct uvc_video_queue *queue = &video->queue;
+	struct uvc_device *uvc = video->uvc;
 	/* video->max_payload_size is only set when using bulk transfer */
 	bool is_bulk = video->max_payload_size;
 	struct usb_request *req = NULL;
@@ -390,7 +391,7 @@ static void uvcg_video_pump(struct work_struct *work)
 	bool buf_done;
 	int ret;
 
-	while (video->ep->enabled) {
+	while (uvc->state == UVC_STATE_STREAMING && video->ep->enabled) {
 		/*
 		 * Retrieve the first available USB request, protected by the
 		 * request lock.
-- 
2.42.0.283.g2d96d420d3-goog

[PATCH v1 2/2] usb: gadget: uvc: prevent de-allocating inflight usb_requests

[Avichal Rakesh]

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 deallocates an
inflight request. The gadget drivers should wait until the complete
callbacks before assuming ownership of the request.

This patch adds a simple request counting mechanism to track how many
requests are currently owned by the driver. Now when stopping the stream,
uvcg_video_enable waits for all the complete callbacks to come through
before deallocating the usb_requests.

Signed-off-by: Avichal Rakesh <arakesh@google.com>
---
 drivers/usb/gadget/function/uvc.h       |  3 +++
 drivers/usb/gadget/function/uvc_video.c | 31 ++++++++++++++++++++++++-
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h
index 989bc6b4e93d..e40e702a7074 100644
--- a/drivers/usb/gadget/function/uvc.h
+++ b/drivers/usb/gadget/function/uvc.h
@@ -104,8 +104,11 @@ struct uvc_video {
 	unsigned int req_size;
 	struct uvc_request *ureq;
 	struct list_head req_free;
+	unsigned int req_free_count; /* number of requests in req_free */
 	spinlock_t req_lock;

+	wait_queue_head_t req_free_queue;
+
 	unsigned int req_int_count;

 	void (*encode) (struct usb_request *req, struct uvc_video *video,
diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 70ff88854539..3ea7d52df80d 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -284,10 +284,18 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)

 	spin_lock_irqsave(&video->req_lock, flags);
 	list_add_tail(&req->list, &video->req_free);
+	video->req_free_count++;
 	spin_unlock_irqrestore(&video->req_lock, flags);

-	if (uvc->state == UVC_STATE_STREAMING)
+	if (uvc->state == UVC_STATE_STREAMING) {
 		queue_work(video->async_wq, &video->pump);
+	} else if (video->req_free_count == video->req_size) {
+		/*
+		 * Wake up thread waiting for all requests to be returned to
+		 * the gadget driver.
+		 */
+		wake_up_interruptible(&video->req_free_queue);
+	}
 }

 static int
@@ -316,6 +324,7 @@ uvc_video_free_requests(struct uvc_video *video)

 	INIT_LIST_HEAD(&video->req_free);
 	video->req_size = 0;
+	video->req_free_count = 0;
 	return 0;
 }

@@ -360,6 +369,7 @@ uvc_video_alloc_requests(struct uvc_video *video)
 	}

 	video->req_size = req_size;
+	video->req_free_count = req_size; /* all requests are currently free */

 	return 0;

@@ -404,6 +414,7 @@ static void uvcg_video_pump(struct work_struct *work)
 		req = list_first_entry(&video->req_free, struct usb_request,
 					list);
 		list_del(&req->list);
+		video->req_free_count--;
 		spin_unlock_irqrestore(&video->req_lock, flags);

 		/*
@@ -480,6 +491,7 @@ static void uvcg_video_pump(struct work_struct *work)

 	spin_lock_irqsave(&video->req_lock, flags);
 	list_add_tail(&req->list, &video->req_free);
+	video->req_free_count++;
 	spin_unlock_irqrestore(&video->req_lock, flags);
 	return;
 }
@@ -506,6 +518,22 @@ int uvcg_video_enable(struct uvc_video *video, int enable)
 			if (video->ureq && video->ureq[i].req)
 				usb_ep_dequeue(video->ep, video->ureq[i].req);

+		/*
+		 * Wait 500ms for the usb_requests to be given back to the
+		 * gadget driver. This ensures that we don't accidentally
+		 * reference de-allocated usb_requests in the complete callback.
+		 */
+		if (video->req_free_count != video->req_size) {
+			uvcg_info(&video->uvc->func,
+					"Waiting 500ms for usb_request complete callbacks.\n");
+			ret = wait_event_interruptible_timeout(
+					video->req_free_queue,
+					video->req_free_count == video->req_size,
+					msecs_to_jiffies(500));
+			uvcg_info(&video->uvc->func,
+					"Done waiting for complete callbacks: %d\n", ret);
+		}
+
 		uvc_video_free_requests(video);
 		uvcg_queue_enable(&video->queue, 0);
 		return 0;
@@ -538,6 +566,7 @@ int uvcg_video_init(struct uvc_video *video, struct uvc_device *uvc)
 {
 	INIT_LIST_HEAD(&video->req_free);
 	spin_lock_init(&video->req_lock);
+	init_waitqueue_head(&video->req_free_queue);
 	INIT_WORK(&video->pump, uvcg_video_pump);

 	/* Allocate a work queue for asynchronous video pump handler. */
--
2.42.0.283.g2d96d420d3-goog

[PATCH v2 2/2] usb: gadget: uvc: prevent de-allocating inflight usb_requests

[Avichal Rakesh]

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.

 drivers/usb/gadget/function/uvc.h       |   4 +-
 drivers/usb/gadget/function/uvc_video.c | 182 +++++++++++++++++-------
 2 files changed, 133 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..e999bf5bd458 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;
 	}

--
2.42.0.459.ge4e396fd5e-goog

[PATCH v3 2/2] usb: gadget: uvc: prevent de-allocating inflight usb_requests

[Avichal Rakesh]

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(-)

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);
--
2.42.0.582.g8ccd20d70d-goog

Re: [PATCH v3 2/2] usb: gadget: uvc: prevent de-allocating inflight usb_requests

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 12380 bytes --]

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

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 2/2] usb: gadget: uvc: prevent de-allocating inflight usb_requests

[Avichal Rakesh]

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.

Re: [PATCH v1 0/2] usb: gadget: uvc: stability fixes when stopping streams

[Avichal Rakesh]

On Mon, Sep 11, 2023 at 9:19 PM Avichal Rakesh <arakesh@google.com> wrote:
>
> We have been seeing two main bugs when stopping stream:
> 1. attempting to queue usb_requests on a disabled usb endpoint, and
> 2. use-after-free problems for inflight requests
>
> Avichal Rakesh (2):
>   usb: gadget: uvc: prevent use of disabled endpoint
>   usb: gadget: uvc: prevent de-allocating inflight usb_requests
>
>  drivers/usb/gadget/function/f_uvc.c     | 11 ++++----
>  drivers/usb/gadget/function/f_uvc.h     |  2 +-
>  drivers/usb/gadget/function/uvc.h       |  5 +++-
>  drivers/usb/gadget/function/uvc_v4l2.c  | 21 ++++++++++++---
>  drivers/usb/gadget/function/uvc_video.c | 34 +++++++++++++++++++++++--
>  5 files changed, 60 insertions(+), 13 deletions(-)
>

Bumping this thread up. Laurent, Dan, and Michael could you take a look?

Thank you!
- Avi.

Re: [PATCH v1 0/2] usb: gadget: uvc: stability fixes when stopping streams

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 4481 bytes --]

Hi Avichal

On Thu, Sep 14, 2023 at 04:05:36PM -0700, Avichal Rakesh wrote:
>On Mon, Sep 11, 2023 at 9:19 PM Avichal Rakesh <arakesh@google.com> wrote:
>>
>> We have been seeing two main bugs when stopping stream:
>> 1. attempting to queue usb_requests on a disabled usb endpoint, and
>> 2. use-after-free problems for inflight requests
>>
>> Avichal Rakesh (2):
>>   usb: gadget: uvc: prevent use of disabled endpoint
>>   usb: gadget: uvc: prevent de-allocating inflight usb_requests
>>
>>  drivers/usb/gadget/function/f_uvc.c     | 11 ++++----
>>  drivers/usb/gadget/function/f_uvc.h     |  2 +-
>>  drivers/usb/gadget/function/uvc.h       |  5 +++-
>>  drivers/usb/gadget/function/uvc_v4l2.c  | 21 ++++++++++++---
>>  drivers/usb/gadget/function/uvc_video.c | 34 +++++++++++++++++++++++--
>>  5 files changed, 60 insertions(+), 13 deletions(-)
>>
>
>Bumping this thread up. Laurent, Dan, and Michael could you take a look?

I tested the patches against my setup and it did not help.

In fact I saw two different issues when calling the streamoff event.

One issue was a stalled pipeline after the streamoff from the host came in.
The streaming application did not handle any events anymore.

The second issue was when the streamoff event is triggered sometimes the
following trace is shown, even with your patches applied.


[  104.202689] Unable to handle kernel paging request at virtual address 005bf43a692a5fd5
[  104.235122] Mem abort info:
[  104.238257]   ESR = 0x0000000096000004
[  104.242449]   EC = 0x25: DABT (current EL), IL = 32 bits
[  104.248391]   SET = 0, FnV = 0
[  104.251803]   EA = 0, S1PTW = 0
[  104.255313]   FSC = 0x04: level 0 translation fault
[  104.260765] Data abort info:
[  104.263982]   ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
[  104.270114]   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
[  104.275760]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[  104.281698] [005bf43a692a5fd5] address between user and kernel address ranges
[  104.290042] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
[  104.297060] Dumping ftrace buffer:
[  104.300869]    (ftrace buffer empty)
[  104.304862] Modules linked in: st1232 hantro_vpu v4l2_vp9 v4l2_h264 uio_pdrv_genirq fuse [last unloaded: rockchip_vpu(C)]
[  104.312080] panfrost fde60000.gpu: Panfrost Dump: BO has no sgt, cannot dump
[  104.317137] CPU: 0 PID: 465 Comm: irq/46-dwc3 Tainted: G         C         6.5.0-20230831-2+ #5
[  104.317144] Hardware name: WolfVision PF5 (DT)
[  104.317148] pstate: 604000c9 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[  104.317154] pc : __list_del_entry_valid+0x48/0xe8
[  104.352728] lr : dwc3_gadget_giveback+0x3c/0x1b0
[  104.357893] sp : ffffffc08381bc60
[  104.361593] x29: ffffffc08381bc60 x28: ffffff80047d4000 x27: ffffff80047de440
[  104.369576] x26: 0000000000000000 x25: ffffffc08135b2d0 x24: ffffffc08381bd00
[  104.377559] x23: 00000000ffffff98 x22: ffffff8004204880 x21: ffffff80047d4000
[  104.385541] x20: ffffff800718dea0 x19: ffffff800718dea0 x18: 0000000000000000
[  104.393523] x17: 7461747320687469 x16: 7720646574656c70 x15: 6d6f632074736575
[  104.401504] x14: 716572205356203a x13: 2e3430312d207375 x12: 7461747320687469
[  104.409486] x11: ffffffc0815c98f0 x10: 0000000000000000 x9 : ffffffc0808f4fa0
[  104.417468] x8 : ffffffc082415000 x7 : ffffffc0808f4e2c x6 : ffffffc0823d0928
[  104.425450] x5 : 0000000000000282 x4 : 0000000000000201 x3 : d85bf43a692a5fcd
[  104.433431] x2 : ffffff80047d4048 x1 : ffffff800718dea0 x0 : dead000000000122
[  104.441413] Call trace:
[  104.444142]  __list_del_entry_valid+0x48/0xe8
[  104.449013]  dwc3_gadget_giveback+0x3c/0x1b0
[  104.453786]  dwc3_gadget_ep_cleanup_cancelled_requests+0xe0/0x170
[  104.460599]  dwc3_process_event_buf+0x2a8/0xbb0
[  104.465662]  dwc3_thread_interrupt+0x4c/0x90
[  104.470435]  irq_thread_fn+0x34/0xb8
[  104.474431]  irq_thread+0x1a0/0x290
[  104.478327]  kthread+0x10c/0x120
[  104.481933]  ret_from_fork+0x10/0x20

The error path triggering these list errors are usually in the
dwc3 driver handling the cancelled or completed list.

Regards,
Michael

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 0/2] usb: gadget: uvc: stability fixes when stopping streams

[Avichal Rakesh]

On 9/15/23 16:16, Michael Grzeschik wrote:
> Hi Avichal
> 
> On Thu, Sep 14, 2023 at 04:05:36PM -0700, Avichal Rakesh wrote:
>> On Mon, Sep 11, 2023 at 9:19 PM Avichal Rakesh <arakesh@google.com> wrote:
>>>
>>> We have been seeing two main bugs when stopping stream:
>>> 1. attempting to queue usb_requests on a disabled usb endpoint, and
>>> 2. use-after-free problems for inflight requests
>>>
>>> Avichal Rakesh (2):
>>>   usb: gadget: uvc: prevent use of disabled endpoint
>>>   usb: gadget: uvc: prevent de-allocating inflight usb_requests
>>>
>>>  drivers/usb/gadget/function/f_uvc.c     | 11 ++++----
>>>  drivers/usb/gadget/function/f_uvc.h     |  2 +-
>>>  drivers/usb/gadget/function/uvc.h       |  5 +++-
>>>  drivers/usb/gadget/function/uvc_v4l2.c  | 21 ++++++++++++---
>>>  drivers/usb/gadget/function/uvc_video.c | 34 +++++++++++++++++++++++--
>>>  5 files changed, 60 insertions(+), 13 deletions(-)
>>>
>>
>> Bumping this thread up. Laurent, Dan, and Michael could you take a look?
> 
> I tested the patches against my setup and it did not help.

Thank you for testing the patch, I really appreciate your help in testing the 
patches!

> 
> In fact I saw two different issues when calling the streamoff event.
> 
> One issue was a stalled pipeline after the streamoff from the host came in.
> The streaming application did not handle any events anymore.

This sounds like uvc_function_setup_continue was never called, so no more control
events were queued for the userspace to handle. This is a little bit of a shot in
the dark: if you are not using the Laurent's uvc-gadget
(https://git.ideasonboard.org/uvc-gadget.git) on the gadget, could you check that 
your userspace application (on the gadget side) calls VIDIOC_STREAMOFF when 
handling UVC_EVENT_STREAMOFF? 

This is similar to how it should called VIDIOC_STREAMON when handling 
UVC_EVENT_STREAMON. Before my patch, I think UVC gadget driver is functionally fine
with userspace application not calling VIDIOC_STREAMOFF. However, my patch prevents
the host from making any more control requests until the gadget's userspace
application calls VIDIOC_STREAMOFF, which looks like a stalled control ep.

> 
> The second issue was when the streamoff event is triggered sometimes the
> following trace is shown, even with your patches applied.
> 
> 
> [  104.202689] Unable to handle kernel paging request at virtual address 005bf43a692a5fd5
> [  104.235122] Mem abort info:
> [  104.238257]   ESR = 0x0000000096000004
> [  104.242449]   EC = 0x25: DABT (current EL), IL = 32 bits
> [  104.248391]   SET = 0, FnV = 0
> [  104.251803]   EA = 0, S1PTW = 0
> [  104.255313]   FSC = 0x04: level 0 translation fault
> [  104.260765] Data abort info:
> [  104.263982]   ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
> [  104.270114]   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
> [  104.275760]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> [  104.281698] [005bf43a692a5fd5] address between user and kernel address ranges
> [  104.290042] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
> [  104.297060] Dumping ftrace buffer:
> [  104.300869]    (ftrace buffer empty)
> [  104.304862] Modules linked in: st1232 hantro_vpu v4l2_vp9 v4l2_h264 uio_pdrv_genirq fuse [last unloaded: rockchip_vpu(C)]
> [  104.312080] panfrost fde60000.gpu: Panfrost Dump: BO has no sgt, cannot dump
> [  104.317137] CPU: 0 PID: 465 Comm: irq/46-dwc3 Tainted: G         C         6.5.0-20230831-2+ #5
> [  104.317144] Hardware name: WolfVision PF5 (DT)
> [  104.317148] pstate: 604000c9 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [  104.317154] pc : __list_del_entry_valid+0x48/0xe8
> [  104.352728] lr : dwc3_gadget_giveback+0x3c/0x1b0
> [  104.357893] sp : ffffffc08381bc60
> [  104.361593] x29: ffffffc08381bc60 x28: ffffff80047d4000 x27: ffffff80047de440
> [  104.369576] x26: 0000000000000000 x25: ffffffc08135b2d0 x24: ffffffc08381bd00
> [  104.377559] x23: 00000000ffffff98 x22: ffffff8004204880 x21: ffffff80047d4000
> [  104.385541] x20: ffffff800718dea0 x19: ffffff800718dea0 x18: 0000000000000000
> [  104.393523] x17: 7461747320687469 x16: 7720646574656c70 x15: 6d6f632074736575
> [  104.401504] x14: 716572205356203a x13: 2e3430312d207375 x12: 7461747320687469
> [  104.409486] x11: ffffffc0815c98f0 x10: 0000000000000000 x9 : ffffffc0808f4fa0
> [  104.417468] x8 : ffffffc082415000 x7 : ffffffc0808f4e2c x6 : ffffffc0823d0928
> [  104.425450] x5 : 0000000000000282 x4 : 0000000000000201 x3 : d85bf43a692a5fcd
> [  104.433431] x2 : ffffff80047d4048 x1 : ffffff800718dea0 x0 : dead000000000122
> [  104.441413] Call trace:
> [  104.444142]  __list_del_entry_valid+0x48/0xe8
> [  104.449013]  dwc3_gadget_giveback+0x3c/0x1b0
> [  104.453786]  dwc3_gadget_ep_cleanup_cancelled_requests+0xe0/0x170
> [  104.460599]  dwc3_process_event_buf+0x2a8/0xbb0
> [  104.465662]  dwc3_thread_interrupt+0x4c/0x90
> [  104.470435]  irq_thread_fn+0x34/0xb8
> [  104.474431]  irq_thread+0x1a0/0x290
> [  104.478327]  kthread+0x10c/0x120
> [  104.481933]  ret_from_fork+0x10/0x20
> 
> The error path triggering these list errors are usually in the
> dwc3 driver handling the cancelled or completed list.

It looks like we're still freeing un-returned requests :(. If you still have
the setup can you pull the uvc logs to see if waiting for requests to be returned timed 
out? I wonder if dwc3's interrupt handler is being scheduled too late. 500ms seemed
like a reasonable time out to me, but this seems to prove otherwise.

Thank you!
- Avi.

Re: [PATCH v1 0/2] usb: gadget: uvc: stability fixes when stopping streams

[Avichal Rakesh]

On 9/15/23 18:35, Avichal Rakesh wrote:
> 
> 
> On 9/15/23 16:16, Michael Grzeschik wrote:
>> Hi Avichal
>>
>> On Thu, Sep 14, 2023 at 04:05:36PM -0700, Avichal Rakesh wrote:
>>> On Mon, Sep 11, 2023 at 9:19 PM Avichal Rakesh <arakesh@google.com> wrote:
>>>>
>>>> We have been seeing two main bugs when stopping stream:
>>>> 1. attempting to queue usb_requests on a disabled usb endpoint, and
>>>> 2. use-after-free problems for inflight requests
>>>>
>>>> Avichal Rakesh (2):
>>>>   usb: gadget: uvc: prevent use of disabled endpoint
>>>>   usb: gadget: uvc: prevent de-allocating inflight usb_requests
>>>>
>>>>  drivers/usb/gadget/function/f_uvc.c     | 11 ++++----
>>>>  drivers/usb/gadget/function/f_uvc.h     |  2 +-
>>>>  drivers/usb/gadget/function/uvc.h       |  5 +++-
>>>>  drivers/usb/gadget/function/uvc_v4l2.c  | 21 ++++++++++++---
>>>>  drivers/usb/gadget/function/uvc_video.c | 34 +++++++++++++++++++++++--
>>>>  5 files changed, 60 insertions(+), 13 deletions(-)
>>>>
>>>
>>> Bumping this thread up. Laurent, Dan, and Michael could you take a look?
>>
>> I tested the patches against my setup and it did not help.
> 
> Thank you for testing the patch, I really appreciate your help in testing the 
> patches!
> 
>>
>> In fact I saw two different issues when calling the streamoff event.
>>
>> One issue was a stalled pipeline after the streamoff from the host came in.
>> The streaming application did not handle any events anymore.
> 
> This sounds like uvc_function_setup_continue was never called, so no more control
> events were queued for the userspace to handle. This is a little bit of a shot in
> the dark: if you are not using the Laurent's uvc-gadget
> (https://git.ideasonboard.org/uvc-gadget.git) on the gadget, could you check that 
> your userspace application (on the gadget side) calls VIDIOC_STREAMOFF when 
> handling UVC_EVENT_STREAMOFF? 
> 
> This is similar to how it should called VIDIOC_STREAMON when handling 
> UVC_EVENT_STREAMON. Before my patch, I think UVC gadget driver is functionally fine
> with userspace application not calling VIDIOC_STREAMOFF. However, my patch prevents
> the host from making any more control requests until the gadget's userspace
> application calls VIDIOC_STREAMOFF, which looks like a stalled control ep.
> 
>>
>> The second issue was when the streamoff event is triggered sometimes the
>> following trace is shown, even with your patches applied.
>>
>>
>> [  104.202689] Unable to handle kernel paging request at virtual address 005bf43a692a5fd5
>> [  104.235122] Mem abort info:
>> [  104.238257]   ESR = 0x0000000096000004
>> [  104.242449]   EC = 0x25: DABT (current EL), IL = 32 bits
>> [  104.248391]   SET = 0, FnV = 0
>> [  104.251803]   EA = 0, S1PTW = 0
>> [  104.255313]   FSC = 0x04: level 0 translation fault
>> [  104.260765] Data abort info:
>> [  104.263982]   ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000
>> [  104.270114]   CM = 0, WnR = 0, TnD = 0, TagAccess = 0
>> [  104.275760]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
>> [  104.281698] [005bf43a692a5fd5] address between user and kernel address ranges
>> [  104.290042] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
>> [  104.297060] Dumping ftrace buffer:
>> [  104.300869]    (ftrace buffer empty)
>> [  104.304862] Modules linked in: st1232 hantro_vpu v4l2_vp9 v4l2_h264 uio_pdrv_genirq fuse [last unloaded: rockchip_vpu(C)]
>> [  104.312080] panfrost fde60000.gpu: Panfrost Dump: BO has no sgt, cannot dump
>> [  104.317137] CPU: 0 PID: 465 Comm: irq/46-dwc3 Tainted: G         C         6.5.0-20230831-2+ #5
>> [  104.317144] Hardware name: WolfVision PF5 (DT)
>> [  104.317148] pstate: 604000c9 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>> [  104.317154] pc : __list_del_entry_valid+0x48/0xe8
>> [  104.352728] lr : dwc3_gadget_giveback+0x3c/0x1b0
>> [  104.357893] sp : ffffffc08381bc60
>> [  104.361593] x29: ffffffc08381bc60 x28: ffffff80047d4000 x27: ffffff80047de440
>> [  104.369576] x26: 0000000000000000 x25: ffffffc08135b2d0 x24: ffffffc08381bd00
>> [  104.377559] x23: 00000000ffffff98 x22: ffffff8004204880 x21: ffffff80047d4000
>> [  104.385541] x20: ffffff800718dea0 x19: ffffff800718dea0 x18: 0000000000000000
>> [  104.393523] x17: 7461747320687469 x16: 7720646574656c70 x15: 6d6f632074736575
>> [  104.401504] x14: 716572205356203a x13: 2e3430312d207375 x12: 7461747320687469
>> [  104.409486] x11: ffffffc0815c98f0 x10: 0000000000000000 x9 : ffffffc0808f4fa0
>> [  104.417468] x8 : ffffffc082415000 x7 : ffffffc0808f4e2c x6 : ffffffc0823d0928
>> [  104.425450] x5 : 0000000000000282 x4 : 0000000000000201 x3 : d85bf43a692a5fcd
>> [  104.433431] x2 : ffffff80047d4048 x1 : ffffff800718dea0 x0 : dead000000000122
>> [  104.441413] Call trace:
>> [  104.444142]  __list_del_entry_valid+0x48/0xe8
>> [  104.449013]  dwc3_gadget_giveback+0x3c/0x1b0
>> [  104.453786]  dwc3_gadget_ep_cleanup_cancelled_requests+0xe0/0x170
>> [  104.460599]  dwc3_process_event_buf+0x2a8/0xbb0
>> [  104.465662]  dwc3_thread_interrupt+0x4c/0x90
>> [  104.470435]  irq_thread_fn+0x34/0xb8
>> [  104.474431]  irq_thread+0x1a0/0x290
>> [  104.478327]  kthread+0x10c/0x120
>> [  104.481933]  ret_from_fork+0x10/0x20
>>
>> The error path triggering these list errors are usually in the
>> dwc3 driver handling the cancelled or completed list.
> 
> It looks like we're still freeing un-returned requests :(. If you still have
> the setup can you pull the uvc logs to see if waiting for requests to be returned timed 
> out? I wonder if dwc3's interrupt handler is being scheduled too late. 500ms seemed
> like a reasonable time out to me, but this seems to prove otherwise.
> 


Hey Michael, were you able to look into the comments from the previous
email? 

Avi.

Re: [PATCH v1 0/2] usb: gadget: uvc: stability fixes when stopping streams

[Avichal Rakesh]

On 9/19/23 11:24, Avichal Rakesh wrote:
> 
> 
> On 9/15/23 18:35, Avichal Rakesh wrote:
>>
>>
>> On 9/15/23 16:16, Michael Grzeschik wrote:
>>> Hi Avichal
>>>
>>> On Thu, Sep 14, 2023 at 04:05:36PM -0700, Avichal Rakesh wrote:
>>>> On Mon, Sep 11, 2023 at 9:19 PM Avichal Rakesh <arakesh@google.com> wrote:
>>>>>
>>>>> We have been seeing two main bugs when stopping stream:
>>>>> 1. attempting to queue usb_requests on a disabled usb endpoint, and
>>>>> 2. use-after-free problems for inflight requests
>>>>>

>>>
>>> The error path triggering these list errors are usually in the
>>> dwc3 driver handling the cancelled or completed list.
>>
>> It looks like we're still freeing un-returned requests :(. If you still have
>> the setup can you pull the uvc logs to see if waiting for requests to be returned timed 
>> out? I wonder if dwc3's interrupt handler is being scheduled too late. 500ms seemed
>> like a reasonable time out to me, but this seems to prove otherwise.
>>
> 
> 
> Hey Michael, were you able to look into the comments from the previous
> email? 
> 

Bumping the thread up. Laurent, Dan, and Michael: the patches are ready to
review/test. Please take a look when possible. 

Thank you!
Avi.