[PATCH 0/3] usb: gadget: uvc: restart fixes

[PATCH 0/3] usb: gadget: uvc: restart fixes

[Michael Grzeschik]

This series is improving the stability of the usb uvc gadget driver. On
the unconditional event of a crash or intentional stop while using the
uvc v4l2 userspace device and streaming to the host, the setup was
sometimes running into use after free cases. We fix that.

Michael Grzeschik (3):
  usb: gadget: uvc: stop pump thread on video disable
  usb: gadget: uvc: cleanup request when not in correct state
  usb: gadget: uvc: rework pump worker to avoid while loop

 drivers/usb/gadget/function/uvc_video.c | 31 ++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 6 deletions(-)

-- 
2.39.2

[PATCH 1/3] usb: gadget: uvc: stop pump thread on video disable

[Michael Grzeschik]

Since the uvc-video gadget driver is using the v4l2 interface,
the streamon and streamoff can be triggered at any times. To ensure
that the pump worker will be closed as soon the userspace is
calling streamoff we synchronize the state of the gadget ensuring
the pump worker to bail out.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/usb/gadget/function/uvc_video.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 91af3b1ef0d412..4b6e854e30c58c 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -384,13 +384,14 @@ static void uvcg_video_pump(struct work_struct *work)
 	struct uvc_video_queue *queue = &video->queue;
 	/* video->max_payload_size is only set when using bulk transfer */
 	bool is_bulk = video->max_payload_size;
+	struct uvc_device *uvc = video->uvc;
 	struct usb_request *req = NULL;
 	struct uvc_buffer *buf;
 	unsigned long flags;
 	bool buf_done;
 	int ret;
 
-	while (video->ep->enabled) {
+	while (video->ep->enabled && uvc->state == UVC_STATE_STREAMING) {
 		/*
 		 * Retrieve the first available USB request, protected by the
 		 * request lock.
@@ -498,6 +499,8 @@ int uvcg_video_enable(struct uvc_video *video, int enable)
 	}
 
 	if (!enable) {
+		uvc->state = UVC_STATE_CONNECTED;
+
 		cancel_work_sync(&video->pump);
 		uvcg_queue_cancel(&video->queue, 0);
 
@@ -523,6 +526,8 @@ int uvcg_video_enable(struct uvc_video *video, int enable)
 		video->encode = video->queue.use_sg ?
 			uvc_video_encode_isoc_sg : uvc_video_encode_isoc;
 
+	uvc->state = UVC_STATE_STREAMING;
+
 	video->req_int_count = 0;
 
 	queue_work(video->async_wq, &video->pump);
-- 
2.39.2

[PATCH 2/3] usb: gadget: uvc: cleanup request when not in correct state

[Michael Grzeschik]

The uvc_video_enable function of the uvc-gadget driver is dequeing and
immediately deallocs all requests on its disable codepath. This is not
save since the dequeue function is async and does not ensure that the
requests are left unlinked in the controller driver.

By adding the ep_free_request into the completion path of the requests
we ensure that the request will be properly deallocated.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/usb/gadget/function/uvc_video.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 4b6e854e30c58c..52e3666b51f743 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -256,6 +256,12 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
 	struct uvc_device *uvc = video->uvc;
 	unsigned long flags;
 
+	if (uvc->state == UVC_STATE_CONNECTED) {
+		usb_ep_free_request(video->ep, ureq->req);
+		ureq->req = NULL;
+		return;
+	}
+
 	switch (req->status) {
 	case 0:
 		break;
-- 
2.39.2

[PATCH 3/3] usb: gadget: uvc: rework pump worker to avoid while loop

[Michael Grzeschik]

The uvc_video_enable function is calling cancel_work_sync which will be
blocking as long as new requests will be queued with the while loop. To
ensure an earlier stop in the pumping loop in this particular case we
rework the worker to requeue itself on every requests. Since the worker
is already running prioritized, the scheduling overhad did not have real
impact on the performance.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/usb/gadget/function/uvc_video.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
index 52e3666b51f743..bfda91ca73ddc6 100644
--- a/drivers/usb/gadget/function/uvc_video.c
+++ b/drivers/usb/gadget/function/uvc_video.c
@@ -397,7 +397,7 @@ static void uvcg_video_pump(struct work_struct *work)
 	bool buf_done;
 	int ret;
 
-	while (video->ep->enabled && uvc->state == UVC_STATE_STREAMING) {
+	if (video->ep->enabled && uvc->state == UVC_STATE_STREAMING) {
 		/*
 		 * Retrieve the first available USB request, protected by the
 		 * request lock.
@@ -409,6 +409,11 @@ static void uvcg_video_pump(struct work_struct *work)
 		}
 		req = list_first_entry(&video->req_free, struct usb_request,
 					list);
+		if (!req) {
+			spin_unlock_irqrestore(&video->req_lock, flags);
+			return;
+		}
+
 		list_del(&req->list);
 		spin_unlock_irqrestore(&video->req_lock, flags);
 
@@ -437,7 +442,7 @@ static void uvcg_video_pump(struct work_struct *work)
 			 * further.
 			 */
 			spin_unlock_irqrestore(&queue->irqlock, flags);
-			break;
+			goto out;
 		}
 
 		/*
@@ -470,20 +475,23 @@ static void uvcg_video_pump(struct work_struct *work)
 		/* Queue the USB request */
 		ret = uvcg_video_ep_queue(video, req);
 		spin_unlock_irqrestore(&queue->irqlock, flags);
-
 		if (ret < 0) {
 			uvcg_queue_cancel(queue, 0);
-			break;
+			goto out;
 		}
 
 		/* Endpoint now owns the request */
 		req = NULL;
 		video->req_int_count++;
+	} else {
+		return;
 	}
 
-	if (!req)
-		return;
+	if (uvc->state == UVC_STATE_STREAMING)
+		queue_work(video->async_wq, &video->pump);
 
+	return;
+out:
 	spin_lock_irqsave(&video->req_lock, flags);
 	list_add_tail(&req->list, &video->req_free);
 	spin_unlock_irqrestore(&video->req_lock, flags);
-- 
2.39.2

Re: [PATCH 1/3] usb: gadget: uvc: stop pump thread on video disable

[kernel test robot]

Hi Michael,

kernel test robot noticed the following build errors:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on usb/usb-next usb/usb-linus westeri-thunderbolt/next media-tree/master linus/master v6.6-rc1 next-20230911]
[cannot apply to sailus-media-tree/streams]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Michael-Grzeschik/usb-gadget-uvc-stop-pump-thread-on-video-disable/20230911-082623
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20230911002451.2860049-2-m.grzeschik%40pengutronix.de
patch subject: [PATCH 1/3] usb: gadget: uvc: stop pump thread on video disable
config: x86_64-randconfig-005-20230911 (https://download.01.org/0day-ci/archive/20230911/202309111200.k58A3yiK-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230911/202309111200.k58A3yiK-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309111200.k58A3yiK-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/usb/gadget/function/uvc_video.c:502:3: error: use of undeclared identifier 'uvc'
                   uvc->state = UVC_STATE_CONNECTED;
                   ^
   drivers/usb/gadget/function/uvc_video.c:529:2: error: use of undeclared identifier 'uvc'
           uvc->state = UVC_STATE_STREAMING;
           ^
   2 errors generated.


vim +/uvc +502 drivers/usb/gadget/function/uvc_video.c

   486	
   487	/*
   488	 * Enable or disable the video stream.
   489	 */
   490	int uvcg_video_enable(struct uvc_video *video, int enable)
   491	{
   492		unsigned int i;
   493		int ret;
   494	
   495		if (video->ep == NULL) {
   496			uvcg_info(&video->uvc->func,
   497				  "Video enable failed, device is uninitialized.\n");
   498			return -ENODEV;
   499		}
   500	
   501		if (!enable) {
 > 502			uvc->state = UVC_STATE_CONNECTED;
   503	
   504			cancel_work_sync(&video->pump);
   505			uvcg_queue_cancel(&video->queue, 0);
   506	
   507			for (i = 0; i < video->uvc_num_requests; ++i)
   508				if (video->ureq && video->ureq[i].req)
   509					usb_ep_dequeue(video->ep, video->ureq[i].req);
   510	
   511			uvc_video_free_requests(video);
   512			uvcg_queue_enable(&video->queue, 0);
   513			return 0;
   514		}
   515	
   516		if ((ret = uvcg_queue_enable(&video->queue, 1)) < 0)
   517			return ret;
   518	
   519		if ((ret = uvc_video_alloc_requests(video)) < 0)
   520			return ret;
   521	
   522		if (video->max_payload_size) {
   523			video->encode = uvc_video_encode_bulk;
   524			video->payload_size = 0;
   525		} else
   526			video->encode = video->queue.use_sg ?
   527				uvc_video_encode_isoc_sg : uvc_video_encode_isoc;
   528	
   529		uvc->state = UVC_STATE_STREAMING;
   530	
   531		video->req_int_count = 0;
   532	
   533		queue_work(video->async_wq, &video->pump);
   534	
   535		return ret;
   536	}
   537	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/3] usb: gadget: uvc: stop pump thread on video disable

[kernel test robot]

Hi Michael,

kernel test robot noticed the following build errors:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on usb/usb-next usb/usb-linus westeri-thunderbolt/next media-tree/master linus/master v6.6-rc1 next-20230911]
[cannot apply to sailus-media-tree/streams]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Michael-Grzeschik/usb-gadget-uvc-stop-pump-thread-on-video-disable/20230911-082623
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20230911002451.2860049-2-m.grzeschik%40pengutronix.de
patch subject: [PATCH 1/3] usb: gadget: uvc: stop pump thread on video disable
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20230911/202309111506.64B9KHI7-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230911/202309111506.64B9KHI7-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309111506.64B9KHI7-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/usb/gadget/function/uvc_video.c: In function 'uvcg_video_enable':
>> drivers/usb/gadget/function/uvc_video.c:502:17: error: 'uvc' undeclared (first use in this function)
     502 |                 uvc->state = UVC_STATE_CONNECTED;
         |                 ^~~
   drivers/usb/gadget/function/uvc_video.c:502:17: note: each undeclared identifier is reported only once for each function it appears in


vim +/uvc +502 drivers/usb/gadget/function/uvc_video.c

   486	
   487	/*
   488	 * Enable or disable the video stream.
   489	 */
   490	int uvcg_video_enable(struct uvc_video *video, int enable)
   491	{
   492		unsigned int i;
   493		int ret;
   494	
   495		if (video->ep == NULL) {
   496			uvcg_info(&video->uvc->func,
   497				  "Video enable failed, device is uninitialized.\n");
   498			return -ENODEV;
   499		}
   500	
   501		if (!enable) {
 > 502			uvc->state = UVC_STATE_CONNECTED;
   503	
   504			cancel_work_sync(&video->pump);
   505			uvcg_queue_cancel(&video->queue, 0);
   506	
   507			for (i = 0; i < video->uvc_num_requests; ++i)
   508				if (video->ureq && video->ureq[i].req)
   509					usb_ep_dequeue(video->ep, video->ureq[i].req);
   510	
   511			uvc_video_free_requests(video);
   512			uvcg_queue_enable(&video->queue, 0);
   513			return 0;
   514		}
   515	
   516		if ((ret = uvcg_queue_enable(&video->queue, 1)) < 0)
   517			return ret;
   518	
   519		if ((ret = uvc_video_alloc_requests(video)) < 0)
   520			return ret;
   521	
   522		if (video->max_payload_size) {
   523			video->encode = uvc_video_encode_bulk;
   524			video->payload_size = 0;
   525		} else
   526			video->encode = video->queue.use_sg ?
   527				uvc_video_encode_isoc_sg : uvc_video_encode_isoc;
   528	
   529		uvc->state = UVC_STATE_STREAMING;
   530	
   531		video->req_int_count = 0;
   532	
   533		queue_work(video->async_wq, &video->pump);
   534	
   535		return ret;
   536	}
   537	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/3] usb: gadget: uvc: cleanup request when not in correct state

[Avichal Rakesh]

Hey Michael,

On 9/10/23 17:24, Michael Grzeschik wrote:
> The uvc_video_enable function of the uvc-gadget driver is dequeing and
> immediately deallocs all requests on its disable codepath. This is not
> save since the dequeue function is async and does not ensure that the
> requests are left unlinked in the controller driver.
> 
> By adding the ep_free_request into the completion path of the requests
> we ensure that the request will be properly deallocated.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>  drivers/usb/gadget/function/uvc_video.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
> index 4b6e854e30c58c..52e3666b51f743 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -256,6 +256,12 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
>  	struct uvc_device *uvc = video->uvc;
>  	unsigned long flags;
>  
> +	if (uvc->state == UVC_STATE_CONNECTED) {
> +		usb_ep_free_request(video->ep, ureq->req);
nit: You can probably just call usb_ep_free_request with req instead of ureq->req.
> +		ureq->req = NULL;
> +		return;
> +	}
> +
>  	switch (req->status) {
>  	case 0:
>  		break;

Perhaps I am missing something here, but I am not sure how this alone 
fixes the use-after-free issue. uvcg_video_enable still deallocates 
_all_ usb_requests right after calling usb_ep_dequeue, so it is still 
possible that an unreturned request is deallocated, and now it is 
possible that the complete callback accesses a deallocated ureq :(

Regards,
Avi

Re: [PATCH 2/3] usb: gadget: uvc: cleanup request when not in correct state

[Michael Grzeschik]

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

Hi Avichal

On Mon, Sep 11, 2023 at 09:52:22PM -0700, Avichal Rakesh wrote:
>On 9/10/23 17:24, Michael Grzeschik wrote:
>> The uvc_video_enable function of the uvc-gadget driver is dequeing and
>> immediately deallocs all requests on its disable codepath. This is not
>> save since the dequeue function is async and does not ensure that the
>> requests are left unlinked in the controller driver.
>>
>> By adding the ep_free_request into the completion path of the requests
>> we ensure that the request will be properly deallocated.
>>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> ---
>>  drivers/usb/gadget/function/uvc_video.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
>> index 4b6e854e30c58c..52e3666b51f743 100644
>> --- a/drivers/usb/gadget/function/uvc_video.c
>> +++ b/drivers/usb/gadget/function/uvc_video.c
>> @@ -256,6 +256,12 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
>>  	struct uvc_device *uvc = video->uvc;
>>  	unsigned long flags;
>>
>> +	if (uvc->state == UVC_STATE_CONNECTED) {
>> +		usb_ep_free_request(video->ep, ureq->req);
>nit: You can probably just call usb_ep_free_request with req instead of ureq->req.

Thanks, thats a good point.

>> +		ureq->req = NULL;
>> +		return;
>> +	}
>> +
>>  	switch (req->status) {
>>  	case 0:
>>  		break;
>
>Perhaps I am missing something here, but I am not sure how this alone
>fixes the use-after-free issue. uvcg_video_enable still deallocates
>_all_ usb_requests right after calling usb_ep_dequeue, so it is still
>possible that an unreturned request is deallocated, and now it is
>possible that the complete callback accesses a deallocated ureq :(

Since the issue I saw was usually coming from the list_del_entry_valid check in
the list_del_entry of the giveback function, the issue was probably just not
triggered anymore as the complete function did exit early.

So this fix alone is actually bogus without a second patch I had in the stack.
The second patch I am refering should change the actual overall issue:

https://lore.kernel.org/linux-usb/20230915233113.2903645-1-m.grzeschik@pengutronix.de/T/#u

This early list_del and this patch here should ensure that the
concurrent functions are not handling already freed memory.

Thanks,
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 2/3] usb: gadget: uvc: cleanup request when not in correct state

[Avichal Rakesh]

On 9/15/23 16:32, Michael Grzeschik wrote:
> Hi Avichal
> 
> On Mon, Sep 11, 2023 at 09:52:22PM -0700, Avichal Rakesh wrote:
>> On 9/10/23 17:24, Michael Grzeschik wrote:
>>> The uvc_video_enable function of the uvc-gadget driver is dequeing and
>>> immediately deallocs all requests on its disable codepath. This is not
>>> save since the dequeue function is async and does not ensure that the
>>> requests are left unlinked in the controller driver.
>>>
>>> By adding the ep_free_request into the completion path of the requests
>>> we ensure that the request will be properly deallocated.
>>>
>>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>> ---
>>>  drivers/usb/gadget/function/uvc_video.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
>>> index 4b6e854e30c58c..52e3666b51f743 100644
>>> --- a/drivers/usb/gadget/function/uvc_video.c
>>> +++ b/drivers/usb/gadget/function/uvc_video.c
>>> @@ -256,6 +256,12 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
>>>      struct uvc_device *uvc = video->uvc;
>>>      unsigned long flags;
>>>
>>> +    if (uvc->state == UVC_STATE_CONNECTED) {
>>> +        usb_ep_free_request(video->ep, ureq->req);
>> nit: You can probably just call usb_ep_free_request with req instead of ureq->req.
> 
> Thanks, thats a good point.
> 
>>> +        ureq->req = NULL;
>>> +        return;
>>> +    }
>>> +
>>>      switch (req->status) {
>>>      case 0:
>>>          break;
>>
>> Perhaps I am missing something here, but I am not sure how this alone
>> fixes the use-after-free issue. uvcg_video_enable still deallocates
>> _all_ usb_requests right after calling usb_ep_dequeue, so it is still
>> possible that an unreturned request is deallocated, and now it is
>> possible that the complete callback accesses a deallocated ureq :(
> 
> Since the issue I saw was usually coming from the list_del_entry_valid check in
> the list_del_entry of the giveback function, the issue was probably just not
> triggered anymore as the complete function did exit early.
> 
> So this fix alone is actually bogus without a second patch I had in the stack.
> The second patch I am refering should change the actual overall issue:
> 
> https://lore.kernel.org/linux-usb/20230915233113.2903645-1-m.grzeschik@pengutronix.de/T/#u
> 
> This early list_del and this patch here should ensure that the
> concurrent functions are not handling already freed memory.

Oh, the patch linked above is interesting. It effectively force removes the dwc3_request
from whatever list it belongs to? So if DWC3's interrupt handler is delayed past 
UVC gadget's ep_free_request call, then it won't see the requests in its cancelled 
list at all. However, this setup is still prone to errors. For example, there is now
a chance that gadget_ep_free_request is called twice for one request. A scheduling
like the following might cause double kfree:

1. uvcg_video_enable calls usb_ep_dequeue for all usb_requests
2. While the usb_ep_dequeues are being processed, dwc3's interrupt handler starts 
   calling the complete callbacks.
3. The complete callback calls gadget_ep_free_request (calling kfree as a result)
4. Meanwhile, uvcg_video_enable has moved to uvc_video_free_requests which also
   calls gadget_ep_free_request (calling kfree).

There is currently (even in your patches) no synchronization between calls to 
gadget_ep_free_request via complete callback and uvcg_video_enable, which will 
inevitably call usb_ep_free_request twice for one request. 

Does that make sense, or am I misunderstanding some part of the patch?

- Avi.

Re: [PATCH 2/3] usb: gadget: uvc: cleanup request when not in correct state

[Michael Grzeschik]

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

On Fri, Sep 15, 2023 at 07:41:05PM -0700, Avichal Rakesh wrote:
>On 9/15/23 16:32, Michael Grzeschik wrote:
>> On Mon, Sep 11, 2023 at 09:52:22PM -0700, Avichal Rakesh wrote:
>>> On 9/10/23 17:24, Michael Grzeschik wrote:
>>>> The uvc_video_enable function of the uvc-gadget driver is dequeing and
>>>> immediately deallocs all requests on its disable codepath. This is not
>>>> save since the dequeue function is async and does not ensure that the
>>>> requests are left unlinked in the controller driver.
>>>>
>>>> By adding the ep_free_request into the completion path of the requests
>>>> we ensure that the request will be properly deallocated.
>>>>
>>>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>>> ---
>>>>  drivers/usb/gadget/function/uvc_video.c | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
>>>> index 4b6e854e30c58c..52e3666b51f743 100644
>>>> --- a/drivers/usb/gadget/function/uvc_video.c
>>>> +++ b/drivers/usb/gadget/function/uvc_video.c
>>>> @@ -256,6 +256,12 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
>>>>      struct uvc_device *uvc = video->uvc;
>>>>      unsigned long flags;
>>>>
>>>> +    if (uvc->state == UVC_STATE_CONNECTED) {
>>>> +        usb_ep_free_request(video->ep, ureq->req);
>>> nit: You can probably just call usb_ep_free_request with req instead of ureq->req.
>>
>> Thanks, thats a good point.
>>
>>>> +        ureq->req = NULL;
>>>> +        return;
>>>> +    }
>>>> +
>>>>      switch (req->status) {
>>>>      case 0:
>>>>          break;
>>>
>>> Perhaps I am missing something here, but I am not sure how this alone
>>> fixes the use-after-free issue. uvcg_video_enable still deallocates
>>> _all_ usb_requests right after calling usb_ep_dequeue, so it is still
>>> possible that an unreturned request is deallocated, and now it is
>>> possible that the complete callback accesses a deallocated ureq :(
>>
>> Since the issue I saw was usually coming from the list_del_entry_valid check in
>> the list_del_entry of the giveback function, the issue was probably just not
>> triggered anymore as the complete function did exit early.
>>
>> So this fix alone is actually bogus without a second patch I had in the stack.
>> The second patch I am refering should change the actual overall issue:
>>
>> https://lore.kernel.org/linux-usb/20230915233113.2903645-1-m.grzeschik@pengutronix.de/T/#u
>>
>> This early list_del and this patch here should ensure that the
>> concurrent functions are not handling already freed memory.
>
>Oh, the patch linked above is interesting. It effectively force removes the dwc3_request
>from whatever list it belongs to? So if DWC3's interrupt handler is delayed past
>UVC gadget's ep_free_request call, then it won't see the requests in its cancelled
>list at all. However, this setup is still prone to errors. For example, there is now
>a chance that gadget_ep_free_request is called twice for one request. A scheduling
>like the following might cause double kfree:
>
>1. uvcg_video_enable calls usb_ep_dequeue for all usb_requests
>2. While the usb_ep_dequeues are being processed, dwc3's interrupt handler starts
>   calling the complete callbacks.
>3. The complete callback calls gadget_ep_free_request (calling kfree as a result)
>4. Meanwhile, uvcg_video_enable has moved to uvc_video_free_requests which also
>   calls gadget_ep_free_request (calling kfree).
>
>There is currently (even in your patches) no synchronization between calls to
>gadget_ep_free_request via complete callback and uvcg_video_enable, which will
>inevitably call usb_ep_free_request twice for one request.
>
>Does that make sense, or am I misunderstanding some part of the patch?

The overall concept is correct. But in detail the
uvc_video_free_requests is checking that video->ureq[i].req is not NULL.

With our previous call of ep_free_request in the complete handler, the
ureq->req pointer in focus was already set to NULL. So the
uvc_video_free_requests function will skip that extra free.

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 2/3] usb: gadget: uvc: cleanup request when not in correct state

[Avichal Rakesh]

On 9/16/23 16:23, Michael Grzeschik wrote:
> On Fri, Sep 15, 2023 at 07:41:05PM -0700, Avichal Rakesh wrote:
>> On 9/15/23 16:32, Michael Grzeschik wrote:
>>> On Mon, Sep 11, 2023 at 09:52:22PM -0700, Avichal Rakesh wrote:
>>>> On 9/10/23 17:24, Michael Grzeschik wrote:
>>>>> The uvc_video_enable function of the uvc-gadget driver is dequeing and
>>>>> immediately deallocs all requests on its disable codepath. This is not
>>>>> save since the dequeue function is async and does not ensure that the
>>>>> requests are left unlinked in the controller driver.
>>>>>
>>>>> By adding the ep_free_request into the completion path of the requests
>>>>> we ensure that the request will be properly deallocated.
>>>>>
>>>>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>>>> ---
>>>>>  drivers/usb/gadget/function/uvc_video.c | 6 ++++++
>>>>>  1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
>>>>> index 4b6e854e30c58c..52e3666b51f743 100644
>>>>> --- a/drivers/usb/gadget/function/uvc_video.c
>>>>> +++ b/drivers/usb/gadget/function/uvc_video.c
>>>>> @@ -256,6 +256,12 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
>>>>>      struct uvc_device *uvc = video->uvc;
>>>>>      unsigned long flags;
>>>>>
>>>>> +    if (uvc->state == UVC_STATE_CONNECTED) {
>>>>> +        usb_ep_free_request(video->ep, ureq->req);
>>>> nit: You can probably just call usb_ep_free_request with req instead of ureq->req.
>>>
>>> Thanks, thats a good point.
>>>
>>>>> +        ureq->req = NULL;
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>>      switch (req->status) {
>>>>>      case 0:
>>>>>          break;
>>>>
>>>> Perhaps I am missing something here, but I am not sure how this alone
>>>> fixes the use-after-free issue. uvcg_video_enable still deallocates
>>>> _all_ usb_requests right after calling usb_ep_dequeue, so it is still
>>>> possible that an unreturned request is deallocated, and now it is
>>>> possible that the complete callback accesses a deallocated ureq :(
>>>
>>> Since the issue I saw was usually coming from the list_del_entry_valid check in
>>> the list_del_entry of the giveback function, the issue was probably just not
>>> triggered anymore as the complete function did exit early.
>>>
>>> So this fix alone is actually bogus without a second patch I had in the stack.
>>> The second patch I am refering should change the actual overall issue:
>>>
>>> https://lore.kernel.org/linux-usb/20230915233113.2903645-1-m.grzeschik@pengutronix.de/T/#u
>>>
>>> This early list_del and this patch here should ensure that the
>>> concurrent functions are not handling already freed memory.
>>
>> Oh, the patch linked above is interesting. It effectively force removes the dwc3_request
>> from whatever list it belongs to? So if DWC3's interrupt handler is delayed past
>> UVC gadget's ep_free_request call, then it won't see the requests in its cancelled
>> list at all. However, this setup is still prone to errors. For example, there is now
>> a chance that gadget_ep_free_request is called twice for one request. A scheduling
>> like the following might cause double kfree:
>>
>> 1. uvcg_video_enable calls usb_ep_dequeue for all usb_requests
>> 2. While the usb_ep_dequeues are being processed, dwc3's interrupt handler starts
>>   calling the complete callbacks.
>> 3. The complete callback calls gadget_ep_free_request (calling kfree as a result)
>> 4. Meanwhile, uvcg_video_enable has moved to uvc_video_free_requests which also
>>   calls gadget_ep_free_request (calling kfree).
>>
>> There is currently (even in your patches) no synchronization between calls to
>> gadget_ep_free_request via complete callback and uvcg_video_enable, which will
>> inevitably call usb_ep_free_request twice for one request.
>>
>> Does that make sense, or am I misunderstanding some part of the patch?
> 
> The overall concept is correct. But in detail the
> uvc_video_free_requests is checking that video->ureq[i].req is not NULL.
> 
> With our previous call of ep_free_request in the complete handler, the
> ureq->req pointer in focus was already set to NULL. So the
> uvc_video_free_requests function will skip that extra free.
> 

Is there any form of synchronization between uvc_video_request and the 
complete callback? As I see it, the dwc3 interrupt thread and the v4l2 
ioctl thread (which calls uvcg_video_enable) are fully independent, so
the calls made by them are free to be interleaved arbitrarily, so an
interleaving like this is technically possible:

+------+------------------------------------+---------------------------------------------+
| time |            ioctl_thread            |            dwc3 interrupt handler           |
+======+====================================+=============================================+
|   1  | -uvc_v4l2_streamoff                |                                             |
|   2  | |-uvcg_video_enable                |                                             |
|   3  | ||-usb_ep_dequeue                  |                                             |
|   4  | ||                                 | -dwc3_process_event_buf                     |
|   5  | ||-uvc_video_free_requests         | |                                           |
|   6  | |||                                | |-dwc3_gadget_ep_cleanup_cancelled_requests |
|   7  | |||                                | ||-dwc3_gadget_giveback                     |
|   8  | |||                                | |||-uvc_video_complete                      |
|   9  | |||-check ureq->req != NULL [true] | ||||                                        |
|  10  | ||||-usb_ep_free_request           | ||||                                        |
|  11  | |||||-dwc3_ep_free_request         | ||||                                        |
|  12  | ||||||-kfree [first call]          | ||||                                        |
|  13  | ||||                               | ||||-usb_ep_free_request                    |
|  14  | ||||                               | |||||-dwc3_ep_free_request                  |
|  15  | ||||                               | ||||||-kfree [second call]                  |
|  16  | ||||                               | ||||-set ureq->req = NULL                   |
|  17  | ||||-set ureq->req = NULL          |                                             |
+------+------------------------------------+---------------------------------------------+

A situation like this means that dwc3_ep_free_request can be called
twice for a particular usb_request. This is obviously low probability,
but a race condition here means we'll start seeing very vague and hard 
to repro crashes or memory inconsistencies when using the uvc gadget.

I do apologize if I've missed something obvious with your changes that 
prevents such interleaving. I don't currently see any locking or 
other synchronization mechanism in your changes. Is there something
in dwc3 that prevents this situation?

Best Regards,
Avi. 

Re: [PATCH 2/3] usb: gadget: uvc: cleanup request when not in correct state

[Michael Grzeschik]

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

On Mon, Sep 18, 2023 at 12:02:11PM -0700, Avichal Rakesh wrote:
>On 9/16/23 16:23, Michael Grzeschik wrote:
>> On Fri, Sep 15, 2023 at 07:41:05PM -0700, Avichal Rakesh wrote:
>>> On 9/15/23 16:32, Michael Grzeschik wrote:
>>>> On Mon, Sep 11, 2023 at 09:52:22PM -0700, Avichal Rakesh wrote:
>>>>> On 9/10/23 17:24, Michael Grzeschik wrote:
>>>>>> The uvc_video_enable function of the uvc-gadget driver is dequeing and
>>>>>> immediately deallocs all requests on its disable codepath. This is not
>>>>>> save since the dequeue function is async and does not ensure that the
>>>>>> requests are left unlinked in the controller driver.
>>>>>>
>>>>>> By adding the ep_free_request into the completion path of the requests
>>>>>> we ensure that the request will be properly deallocated.
>>>>>>
>>>>>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>>>>> ---
>>>>>>  drivers/usb/gadget/function/uvc_video.c | 6 ++++++
>>>>>>  1 file changed, 6 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
>>>>>> index 4b6e854e30c58c..52e3666b51f743 100644
>>>>>> --- a/drivers/usb/gadget/function/uvc_video.c
>>>>>> +++ b/drivers/usb/gadget/function/uvc_video.c
>>>>>> @@ -256,6 +256,12 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
>>>>>>      struct uvc_device *uvc = video->uvc;
>>>>>>      unsigned long flags;
>>>>>>
>>>>>> +    if (uvc->state == UVC_STATE_CONNECTED) {
>>>>>> +        usb_ep_free_request(video->ep, ureq->req);
>>>>> nit: You can probably just call usb_ep_free_request with req instead of ureq->req.
>>>>
>>>> Thanks, thats a good point.
>>>>
>>>>>> +        ureq->req = NULL;
>>>>>> +        return;
>>>>>> +    }
>>>>>> +
>>>>>>      switch (req->status) {
>>>>>>      case 0:
>>>>>>          break;
>>>>>
>>>>> Perhaps I am missing something here, but I am not sure how this alone
>>>>> fixes the use-after-free issue. uvcg_video_enable still deallocates
>>>>> _all_ usb_requests right after calling usb_ep_dequeue, so it is still
>>>>> possible that an unreturned request is deallocated, and now it is
>>>>> possible that the complete callback accesses a deallocated ureq :(
>>>>
>>>> Since the issue I saw was usually coming from the list_del_entry_valid check in
>>>> the list_del_entry of the giveback function, the issue was probably just not
>>>> triggered anymore as the complete function did exit early.
>>>>
>>>> So this fix alone is actually bogus without a second patch I had in the stack.
>>>> The second patch I am refering should change the actual overall issue:
>>>>
>>>> https://lore.kernel.org/linux-usb/20230915233113.2903645-1-m.grzeschik@pengutronix.de/T/#u
>>>>
>>>> This early list_del and this patch here should ensure that the
>>>> concurrent functions are not handling already freed memory.
>>>
>>> Oh, the patch linked above is interesting. It effectively force removes the dwc3_request
>>> from whatever list it belongs to? So if DWC3's interrupt handler is delayed past
>>> UVC gadget's ep_free_request call, then it won't see the requests in its cancelled
>>> list at all. However, this setup is still prone to errors. For example, there is now
>>> a chance that gadget_ep_free_request is called twice for one request. A scheduling
>>> like the following might cause double kfree:
>>>
>>> 1. uvcg_video_enable calls usb_ep_dequeue for all usb_requests
>>> 2. While the usb_ep_dequeues are being processed, dwc3's interrupt handler starts
>>>   calling the complete callbacks.
>>> 3. The complete callback calls gadget_ep_free_request (calling kfree as a result)
>>> 4. Meanwhile, uvcg_video_enable has moved to uvc_video_free_requests which also
>>>   calls gadget_ep_free_request (calling kfree).
>>>
>>> There is currently (even in your patches) no synchronization between calls to
>>> gadget_ep_free_request via complete callback and uvcg_video_enable, which will
>>> inevitably call usb_ep_free_request twice for one request.
>>>
>>> Does that make sense, or am I misunderstanding some part of the patch?
>>
>> The overall concept is correct. But in detail the
>> uvc_video_free_requests is checking that video->ureq[i].req is not NULL.
>>
>> With our previous call of ep_free_request in the complete handler, the
>> ureq->req pointer in focus was already set to NULL. So the
>> uvc_video_free_requests function will skip that extra free.
>>
>
>Is there any form of synchronization between uvc_video_request and the
>complete callback? As I see it, the dwc3 interrupt thread and the v4l2
>ioctl thread (which calls uvcg_video_enable) are fully independent, so
>the calls made by them are free to be interleaved arbitrarily, so an
>interleaving like this is technically possible:
>
>+------+------------------------------------+---------------------------------------------+
>| time |            ioctl_thread            |            dwc3 interrupt handler           |
>+======+====================================+=============================================+
>|   1  | -uvc_v4l2_streamoff                |                                             |
>|   2  | |-uvcg_video_enable                |                                             |
>|   3  | ||-usb_ep_dequeue                  |                                             |
>|   4  | ||                                 | -dwc3_process_event_buf                     |
>|   5  | ||-uvc_video_free_requests         | |                                           |
>|   6  | |||                                | |-dwc3_gadget_ep_cleanup_cancelled_requests |
>|   7  | |||                                | ||-dwc3_gadget_giveback                     |
>|   8  | |||                                | |||-uvc_video_complete                      |
>|   9  | |||-check ureq->req != NULL [true] | ||||                                        |
>|  10  | ||||-usb_ep_free_request           | ||||                                        |
>|  11  | |||||-dwc3_ep_free_request         | ||||                                        |
>|  12  | ||||||-kfree [first call]          | ||||                                        |
>|  13  | ||||                               | ||||-usb_ep_free_request                    |
>|  14  | ||||                               | |||||-dwc3_ep_free_request                  |
>|  15  | ||||                               | ||||||-kfree [second call]                  |
>|  16  | ||||                               | ||||-set ureq->req = NULL                   |
>|  17  | ||||-set ureq->req = NULL          |                                             |
>+------+------------------------------------+---------------------------------------------+
>
>A situation like this means that dwc3_ep_free_request can be called
>twice for a particular usb_request. This is obviously low probability,
>but a race condition here means we'll start seeing very vague and hard
>to repro crashes or memory inconsistencies when using the uvc gadget.
>
>I do apologize if I've missed something obvious with your changes that
>prevents such interleaving. I don't currently see any locking or
>other synchronization mechanism in your changes. Is there something
>in dwc3 that prevents this situation?

I think you have pointed it out totally clear. This is obviously the
case. It just did not trigger here. But the window is there and has to
be locked in some way.

For now we have two options to solve it.

1) Trying to avoid this double code path of the complete callback and
uvc_video_free_requests. This is what your patches are already doing.

But for now I am not so pleased with the timeout concept by waiting for
the complete interrupt to be called. This is also a shot in the dark as
the latency depends on the scheduler and the amount of potential
requests that are being handled.

2) Locking both codepathes around the resource in question so the issue
is avoided.

However, I am also not a fried of many locks.

Perhaps it is possible to use a combination of wait_for_completion in
the uvc_video_free_requests and a complete callback in
uvc_video_complete for those requests that are not listed in the
req_free list.

What do you think?

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 2/3] usb: gadget: uvc: cleanup request when not in correct state

[Avichal Rakesh]

On 9/18/23 14:43, Michael Grzeschik wrote:
> On Mon, Sep 18, 2023 at 12:02:11PM -0700, Avichal Rakesh wrote:
>> On 9/16/23 16:23, Michael Grzeschik wrote:
>>> On Fri, Sep 15, 2023 at 07:41:05PM -0700, Avichal Rakesh wrote:
>>>> On 9/15/23 16:32, Michael Grzeschik wrote:
>>>>> On Mon, Sep 11, 2023 at 09:52:22PM -0700, Avichal Rakesh wrote:
>>>>>> On 9/10/23 17:24, Michael Grzeschik wrote:
>>>>>>> The uvc_video_enable function of the uvc-gadget driver is dequeing and
>>>>>>> immediately deallocs all requests on its disable codepath. This is not
>>>>>>> save since the dequeue function is async and does not ensure that the
>>>>>>> requests are left unlinked in the controller driver.
>>>>>>>
>>>>>>> By adding the ep_free_request into the completion path of the requests
>>>>>>> we ensure that the request will be properly deallocated.
>>>>>>>
>>>>>>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>>>>>> ---
>>>>>>>  drivers/usb/gadget/function/uvc_video.c | 6 ++++++
>>>>>>>  1 file changed, 6 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
>>>>>>> index 4b6e854e30c58c..52e3666b51f743 100644
>>>>>>> --- a/drivers/usb/gadget/function/uvc_video.c
>>>>>>> +++ b/drivers/usb/gadget/function/uvc_video.c
>>>>>>> @@ -256,6 +256,12 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
>>>>>>>      struct uvc_device *uvc = video->uvc;
>>>>>>>      unsigned long flags;
>>>>>>>
>>>>>>> +    if (uvc->state == UVC_STATE_CONNECTED) {
>>>>>>> +        usb_ep_free_request(video->ep, ureq->req);
>>>>>> nit: You can probably just call usb_ep_free_request with req instead of ureq->req.
>>>>>
>>>>> Thanks, thats a good point.
>>>>>
>>>>>>> +        ureq->req = NULL;
>>>>>>> +        return;
>>>>>>> +    }
>>>>>>> +
>>>>>>>      switch (req->status) {
>>>>>>>      case 0:
>>>>>>>          break;
>>>>>>
>>>>>> Perhaps I am missing something here, but I am not sure how this alone
>>>>>> fixes the use-after-free issue. uvcg_video_enable still deallocates
>>>>>> _all_ usb_requests right after calling usb_ep_dequeue, so it is still
>>>>>> possible that an unreturned request is deallocated, and now it is
>>>>>> possible that the complete callback accesses a deallocated ureq :(
>>>>>
>>>>> Since the issue I saw was usually coming from the list_del_entry_valid check in
>>>>> the list_del_entry of the giveback function, the issue was probably just not
>>>>> triggered anymore as the complete function did exit early.
>>>>>
>>>>> So this fix alone is actually bogus without a second patch I had in the stack.
>>>>> The second patch I am refering should change the actual overall issue:
>>>>>
>>>>> https://lore.kernel.org/linux-usb/20230915233113.2903645-1-m.grzeschik@pengutronix.de/T/#u
>>>>>
>>>>> This early list_del and this patch here should ensure that the
>>>>> concurrent functions are not handling already freed memory.
>>>>
>>>> Oh, the patch linked above is interesting. It effectively force removes the dwc3_request
>>>> from whatever list it belongs to? So if DWC3's interrupt handler is delayed past
>>>> UVC gadget's ep_free_request call, then it won't see the requests in its cancelled
>>>> list at all. However, this setup is still prone to errors. For example, there is now
>>>> a chance that gadget_ep_free_request is called twice for one request. A scheduling
>>>> like the following might cause double kfree:
>>>>
>>>> 1. uvcg_video_enable calls usb_ep_dequeue for all usb_requests
>>>> 2. While the usb_ep_dequeues are being processed, dwc3's interrupt handler starts
>>>>   calling the complete callbacks.
>>>> 3. The complete callback calls gadget_ep_free_request (calling kfree as a result)
>>>> 4. Meanwhile, uvcg_video_enable has moved to uvc_video_free_requests which also
>>>>   calls gadget_ep_free_request (calling kfree).
>>>>
>>>> There is currently (even in your patches) no synchronization between calls to
>>>> gadget_ep_free_request via complete callback and uvcg_video_enable, which will
>>>> inevitably call usb_ep_free_request twice for one request.
>>>>
>>>> Does that make sense, or am I misunderstanding some part of the patch?
>>>
>>> The overall concept is correct. But in detail the
>>> uvc_video_free_requests is checking that video->ureq[i].req is not NULL.
>>>
>>> With our previous call of ep_free_request in the complete handler, the
>>> ureq->req pointer in focus was already set to NULL. So the
>>> uvc_video_free_requests function will skip that extra free.
>>>
>>
>> Is there any form of synchronization between uvc_video_request and the
>> complete callback? As I see it, the dwc3 interrupt thread and the v4l2
>> ioctl thread (which calls uvcg_video_enable) are fully independent, so
>> the calls made by them are free to be interleaved arbitrarily, so an
>> interleaving like this is technically possible:
>>
>> +------+------------------------------------+---------------------------------------------+
>> | time |            ioctl_thread            |            dwc3 interrupt handler           |
>> +======+====================================+=============================================+
>> |   1  | -uvc_v4l2_streamoff                |                                             |
>> |   2  | |-uvcg_video_enable                |                                             |
>> |   3  | ||-usb_ep_dequeue                  |                                             |
>> |   4  | ||                                 | -dwc3_process_event_buf                     |
>> |   5  | ||-uvc_video_free_requests         | |                                           |
>> |   6  | |||                                | |-dwc3_gadget_ep_cleanup_cancelled_requests |
>> |   7  | |||                                | ||-dwc3_gadget_giveback                     |
>> |   8  | |||                                | |||-uvc_video_complete                      |
>> |   9  | |||-check ureq->req != NULL [true] | ||||                                        |
>> |  10  | ||||-usb_ep_free_request           | ||||                                        |
>> |  11  | |||||-dwc3_ep_free_request         | ||||                                        |
>> |  12  | ||||||-kfree [first call]          | ||||                                        |
>> |  13  | ||||                               | ||||-usb_ep_free_request                    |
>> |  14  | ||||                               | |||||-dwc3_ep_free_request                  |
>> |  15  | ||||                               | ||||||-kfree [second call]                  |
>> |  16  | ||||                               | ||||-set ureq->req = NULL                   |
>> |  17  | ||||-set ureq->req = NULL          |                                             |
>> +------+------------------------------------+---------------------------------------------+
>>
>> A situation like this means that dwc3_ep_free_request can be called
>> twice for a particular usb_request. This is obviously low probability,
>> but a race condition here means we'll start seeing very vague and hard
>> to repro crashes or memory inconsistencies when using the uvc gadget.
>>
>> I do apologize if I've missed something obvious with your changes that
>> prevents such interleaving. I don't currently see any locking or
>> other synchronization mechanism in your changes. Is there something
>> in dwc3 that prevents this situation?
> 
> I think you have pointed it out totally clear. This is obviously the
> case. It just did not trigger here. But the window is there and has to
> be locked in some way.
> 
> For now we have two options to solve it.
> 
> 1) Trying to avoid this double code path of the complete callback and
> uvc_video_free_requests. This is what your patches are already doing.
> 
> But for now I am not so pleased with the timeout concept by waiting for
> the complete interrupt to be called. This is also a shot in the dark as
> the latency depends on the scheduler and the amount of potential
> requests that are being handled.

I agree, a timeout is not the most elegant of solutions and given a
weird enough scheduler, will run into issues as well.

> 
> 2) Locking both codepathes around the resource in question so the issue
> is avoided.
> 
> However, I am also not a fried of many locks.
> 
> Perhaps it is possible to use a combination of wait_for_completion in
> the uvc_video_free_requests and a complete callback in
> uvc_video_complete for those requests that are not listed in the
> req_free list.
> 
> What do you think?
> 
There might be a way that builds on your idea of cleaning up in the complete callback. 
It would rely on having a uvc_requests that aren't bulk allocated, which may have a
performance impact. 

I am imagining something like the following:
  1. Instead of allocating a bulk of uvc_requests, we allocate them
     one at a time and add them to uvc_video.ureq
  2. uvc_video.ureq becomes a list_head containing all the individual
     requests
  3. We add a sentinel flag in uvc_request that says the request is
     now stale. This flag is protected by uvc_video->req_lock
  4. uvc_video_complete looks at  this flag to deallocate both
     usb_request and uvc_request.
  5. uvcg_video_enable looks something like the following:
       uvcg_video_enable(...) {
         ...
         lock(req_lock);
         forall (uvc_requests->ureqs) {ureq->stale = true}
         unlock(req_lock);
         usb_ep_dequeue all reqs

         uvc_video_free_requests(...)
         ...
       }
  6. uvc_video_complete looks something like:
       uvc_video_complete(...) {
         // at the start
         lock(req_lock)
         is_stale = ureq->stale;
         unlock(req_lock);

         if (is_stale) {
           usb_ep_free_request();
           dealloc corresponding uvc_request();
           return;
         }

         ...

         lock(req_lock);
         // possible that request became stale while we were handling stuff
         if (!ureq->stale) {
           list_add_tail(&req->list, &video->req_free);
         } else {
           usb_ep_free_request();
           dealloc corresponding uvc_request();
         }
         unlock(req_lock);
       }
  7. uvc_video_free_requests can freely dealloc usb_requests/uvc_requests in 
     req_free because we can be certain that uvc_video_complete won't modify 
     it once requests have been marked stale, and the stale requests in flight 
     will be cleaned up by the complete callback.

Effectively, we freeze the state of req_free before dequeuing, and all
inflight requests are considered the responsibility of the complete handler 
from that point onwards. The gadget is only responsible for freeing requests it 
currently owns.

I think this should ensure that we never have a situation where the ownership of the
requests are undefined, and only one thread is responsible for freeing any given request.

Hope that makes sense!

- Avi.

Re: [PATCH 2/3] usb: gadget: uvc: cleanup request when not in correct state

[Avichal Rakesh]

On 9/18/23 16:40, Avichal Rakesh wrote:
> 
> 
> On 9/18/23 14:43, Michael Grzeschik wrote:
>> On Mon, Sep 18, 2023 at 12:02:11PM -0700, Avichal Rakesh wrote:
>>> On 9/16/23 16:23, Michael Grzeschik wrote:
>>>> On Fri, Sep 15, 2023 at 07:41:05PM -0700, Avichal Rakesh wrote:
>>>>> On 9/15/23 16:32, Michael Grzeschik wrote:
>>>>>> On Mon, Sep 11, 2023 at 09:52:22PM -0700, Avichal Rakesh wrote:
>>>>>>> On 9/10/23 17:24, Michael Grzeschik wrote:
>>>>>>>> The uvc_video_enable function of the uvc-gadget driver is dequeing and
>>>>>>>> immediately deallocs all requests on its disable codepath. This is not
>>>>>>>> save since the dequeue function is async and does not ensure that the
>>>>>>>> requests are left unlinked in the controller driver.
>>>>>>>>
>>>>>>>> ...
>>>
>>> I do apologize if I've missed something obvious with your changes that
>>> prevents such interleaving. I don't currently see any locking or
>>> other synchronization mechanism in your changes. Is there something
>>> in dwc3 that prevents this situation?
>>
>> I think you have pointed it out totally clear. This is obviously the
>> case. It just did not trigger here. But the window is there and has to
>> be locked in some way.
>>
>> For now we have two options to solve it.
>>
>> 1) Trying to avoid this double code path of the complete callback and
>> uvc_video_free_requests. This is what your patches are already doing.
>>
>> But for now I am not so pleased with the timeout concept by waiting for
>> the complete interrupt to be called. This is also a shot in the dark as
>> the latency depends on the scheduler and the amount of potential
>> requests that are being handled.
> 
> I agree, a timeout is not the most elegant of solutions and given a
> weird enough scheduler, will run into issues as well

.
Thinking about this some more: I still agree that a timeout seems arbitrary
and will (rightly) lead to questions along the lines of "If we can safely move
on after 500ms, why not safely move on immediately?". 

However, to me it seems more reasonable to put an indefinite wait. The uvc gadget
can wait forever for the complete callbacks to come through. This basically says
that until the usb controller returns the usb_requests back to uvc gadget, it 
can't guarantee a consistent memory state as it is responsible for managing the
usb_requests it has allocated. 

Of course there is no such thing as an indefinite wait, the watchdog will 
eventually kick in to reap the subsystem (potentially causing a kernel panic).
But it seems reasonable to say that if the usb controller is unable to 
return the usb_requests in however long it takes the watchdog to bite, 
it has no business running uvc gadget anyway.

My changes in https://lore.kernel.org/20230912041910.726442-2-arakesh@google.com
will ensure that no other control request comes in while uvc gadget is waiting,
and it should be relatively trivial to update
https://lore.kernel.org/20230912041910.726442-3-arakesh@google.com to wait 
indefinitely.

Laurent and Dan, does an indefinite wait seem more reasonable than an arbitrary wait,
or would something like the suggestion in previous email be better?

> 
>>
>> 2) Locking both codepathes around the resource in question so the issue
>> is avoided.
>>
>> However, I am also not a fried of many locks.
>>
>> Perhaps it is possible to use a combination of wait_for_completion in
>> the uvc_video_free_requests and a complete callback in
>> uvc_video_complete for those requests that are not listed in the
>> req_free list.
>>
>> What do you think?
>>
> There might be a way that builds on your idea of cleaning up in the complete callback. 
> It would rely on having a uvc_requests that aren't bulk allocated, which may have a
> performance impact. 
> 
> I am imagining something like the following:
>   1. Instead of allocating a bulk of uvc_requests, we allocate them
>      one at a time and add them to uvc_video.ureq
>   2. uvc_video.ureq becomes a list_head containing all the individual
>      requests
>   3. We add a sentinel flag in uvc_request that says the request is
>      now stale. This flag is protected by uvc_video->req_lock
>   4. uvc_video_complete looks at  this flag to deallocate both
>      usb_request and uvc_request.
>   5. uvcg_video_enable looks something like the following:
>        uvcg_video_enable(...) {
>          ...
>          lock(req_lock);
>          forall (uvc_requests->ureqs) {ureq->stale = true}
>          unlock(req_lock);
>          usb_ep_dequeue all reqs
> 
>          uvc_video_free_requests(...)
>          ...
>        }
>   6. uvc_video_complete looks something like:
>        uvc_video_complete(...) {
>          // at the start
>          lock(req_lock)
>          is_stale = ureq->stale;
>          unlock(req_lock);
> 
>          if (is_stale) {
>            usb_ep_free_request();
>            dealloc corresponding uvc_request();
>            return;
>          }
> 
>          ...
> 
>          lock(req_lock);
>          // possible that request became stale while we were handling stuff
>          if (!ureq->stale) {
>            list_add_tail(&req->list, &video->req_free);
>          } else {
>            usb_ep_free_request();
>            dealloc corresponding uvc_request();
>          }
>          unlock(req_lock);
>        }
>   7. uvc_video_free_requests can freely dealloc usb_requests/uvc_requests in 
>      req_free because we can be certain that uvc_video_complete won't modify 
>      it once requests have been marked stale, and the stale requests in flight 
>      will be cleaned up by the complete callback.
> 
> Effectively, we freeze the state of req_free before dequeuing, and all
> inflight requests are considered the responsibility of the complete handler 
> from that point onwards. The gadget is only responsible for freeing requests it 
> currently owns.
> 
> I think this should ensure that we never have a situation where the ownership of the
> requests are undefined, and only one thread is responsible for freeing any given request.
> 
> Hope that makes sense!
> 

- Avi.

Re: [PATCH 2/3] usb: gadget: uvc: cleanup request when not in correct state

[Michael Grzeschik]

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

On Mon, Sep 18, 2023 at 04:40:07PM -0700, Avichal Rakesh wrote:
>
>
>On 9/18/23 14:43, Michael Grzeschik wrote:
>> On Mon, Sep 18, 2023 at 12:02:11PM -0700, Avichal Rakesh wrote:
>>> On 9/16/23 16:23, Michael Grzeschik wrote:
>>>> On Fri, Sep 15, 2023 at 07:41:05PM -0700, Avichal Rakesh wrote:
>>>>> On 9/15/23 16:32, Michael Grzeschik wrote:
>>>>>> On Mon, Sep 11, 2023 at 09:52:22PM -0700, Avichal Rakesh wrote:
>>>>>>> On 9/10/23 17:24, Michael Grzeschik wrote:
>>>>>>>> The uvc_video_enable function of the uvc-gadget driver is dequeing and
>>>>>>>> immediately deallocs all requests on its disable codepath. This is not
>>>>>>>> save since the dequeue function is async and does not ensure that the
>>>>>>>> requests are left unlinked in the controller driver.
>>>>>>>>
>>>>>>>> By adding the ep_free_request into the completion path of the requests
>>>>>>>> we ensure that the request will be properly deallocated.
>>>>>>>>
>>>>>>>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>>>>>>> ---
>>>>>>>>  drivers/usb/gadget/function/uvc_video.c | 6 ++++++
>>>>>>>>  1 file changed, 6 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
>>>>>>>> index 4b6e854e30c58c..52e3666b51f743 100644
>>>>>>>> --- a/drivers/usb/gadget/function/uvc_video.c
>>>>>>>> +++ b/drivers/usb/gadget/function/uvc_video.c
>>>>>>>> @@ -256,6 +256,12 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
>>>>>>>>      struct uvc_device *uvc = video->uvc;
>>>>>>>>      unsigned long flags;
>>>>>>>>
>>>>>>>> +    if (uvc->state == UVC_STATE_CONNECTED) {
>>>>>>>> +        usb_ep_free_request(video->ep, ureq->req);
>>>>>>> nit: You can probably just call usb_ep_free_request with req instead of ureq->req.
>>>>>>
>>>>>> Thanks, thats a good point.
>>>>>>
>>>>>>>> +        ureq->req = NULL;
>>>>>>>> +        return;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>>      switch (req->status) {
>>>>>>>>      case 0:
>>>>>>>>          break;
>>>>>>>
>>>>>>> Perhaps I am missing something here, but I am not sure how this alone
>>>>>>> fixes the use-after-free issue. uvcg_video_enable still deallocates
>>>>>>> _all_ usb_requests right after calling usb_ep_dequeue, so it is still
>>>>>>> possible that an unreturned request is deallocated, and now it is
>>>>>>> possible that the complete callback accesses a deallocated ureq :(
>>>>>>
>>>>>> Since the issue I saw was usually coming from the list_del_entry_valid check in
>>>>>> the list_del_entry of the giveback function, the issue was probably just not
>>>>>> triggered anymore as the complete function did exit early.
>>>>>>
>>>>>> So this fix alone is actually bogus without a second patch I had in the stack.
>>>>>> The second patch I am refering should change the actual overall issue:
>>>>>>
>>>>>> https://lore.kernel.org/linux-usb/20230915233113.2903645-1-m.grzeschik@pengutronix.de/T/#u
>>>>>>
>>>>>> This early list_del and this patch here should ensure that the
>>>>>> concurrent functions are not handling already freed memory.
>>>>>
>>>>> Oh, the patch linked above is interesting. It effectively force removes the dwc3_request
>>>>> from whatever list it belongs to? So if DWC3's interrupt handler is delayed past
>>>>> UVC gadget's ep_free_request call, then it won't see the requests in its cancelled
>>>>> list at all. However, this setup is still prone to errors. For example, there is now
>>>>> a chance that gadget_ep_free_request is called twice for one request. A scheduling
>>>>> like the following might cause double kfree:
>>>>>
>>>>> 1. uvcg_video_enable calls usb_ep_dequeue for all usb_requests
>>>>> 2. While the usb_ep_dequeues are being processed, dwc3's interrupt handler starts
>>>>>   calling the complete callbacks.
>>>>> 3. The complete callback calls gadget_ep_free_request (calling kfree as a result)
>>>>> 4. Meanwhile, uvcg_video_enable has moved to uvc_video_free_requests which also
>>>>>   calls gadget_ep_free_request (calling kfree).
>>>>>
>>>>> There is currently (even in your patches) no synchronization between calls to
>>>>> gadget_ep_free_request via complete callback and uvcg_video_enable, which will
>>>>> inevitably call usb_ep_free_request twice for one request.
>>>>>
>>>>> Does that make sense, or am I misunderstanding some part of the patch?
>>>>
>>>> The overall concept is correct. But in detail the
>>>> uvc_video_free_requests is checking that video->ureq[i].req is not NULL.
>>>>
>>>> With our previous call of ep_free_request in the complete handler, the
>>>> ureq->req pointer in focus was already set to NULL. So the
>>>> uvc_video_free_requests function will skip that extra free.
>>>>
>>>
>>> Is there any form of synchronization between uvc_video_request and the
>>> complete callback? As I see it, the dwc3 interrupt thread and the v4l2
>>> ioctl thread (which calls uvcg_video_enable) are fully independent, so
>>> the calls made by them are free to be interleaved arbitrarily, so an
>>> interleaving like this is technically possible:
>>>
>>> +------+------------------------------------+---------------------------------------------+
>>> | time |            ioctl_thread            |            dwc3 interrupt handler           |
>>> +======+====================================+=============================================+
>>> |   1  | -uvc_v4l2_streamoff                |                                             |
>>> |   2  | |-uvcg_video_enable                |                                             |
>>> |   3  | ||-usb_ep_dequeue                  |                                             |
>>> |   4  | ||                                 | -dwc3_process_event_buf                     |
>>> |   5  | ||-uvc_video_free_requests         | |                                           |
>>> |   6  | |||                                | |-dwc3_gadget_ep_cleanup_cancelled_requests |
>>> |   7  | |||                                | ||-dwc3_gadget_giveback                     |
>>> |   8  | |||                                | |||-uvc_video_complete                      |
>>> |   9  | |||-check ureq->req != NULL [true] | ||||                                        |
>>> |  10  | ||||-usb_ep_free_request           | ||||                                        |
>>> |  11  | |||||-dwc3_ep_free_request         | ||||                                        |
>>> |  12  | ||||||-kfree [first call]          | ||||                                        |
>>> |  13  | ||||                               | ||||-usb_ep_free_request                    |
>>> |  14  | ||||                               | |||||-dwc3_ep_free_request                  |
>>> |  15  | ||||                               | ||||||-kfree [second call]                  |
>>> |  16  | ||||                               | ||||-set ureq->req = NULL                   |
>>> |  17  | ||||-set ureq->req = NULL          |                                             |
>>> +------+------------------------------------+---------------------------------------------+
>>>
>>> A situation like this means that dwc3_ep_free_request can be called
>>> twice for a particular usb_request. This is obviously low probability,
>>> but a race condition here means we'll start seeing very vague and hard
>>> to repro crashes or memory inconsistencies when using the uvc gadget.
>>>
>>> I do apologize if I've missed something obvious with your changes that
>>> prevents such interleaving. I don't currently see any locking or
>>> other synchronization mechanism in your changes. Is there something
>>> in dwc3 that prevents this situation?
>>
>> I think you have pointed it out totally clear. This is obviously the
>> case. It just did not trigger here. But the window is there and has to
>> be locked in some way.
>>
>> For now we have two options to solve it.
>>
>> 1) Trying to avoid this double code path of the complete callback and
>> uvc_video_free_requests. This is what your patches are already doing.
>>
>> But for now I am not so pleased with the timeout concept by waiting for
>> the complete interrupt to be called. This is also a shot in the dark as
>> the latency depends on the scheduler and the amount of potential
>> requests that are being handled.
>
>I agree, a timeout is not the most elegant of solutions and given a
>weird enough scheduler, will run into issues as well.
>
>>
>> 2) Locking both codepathes around the resource in question so the issue
>> is avoided.
>>
>> However, I am also not a fried of many locks.
>>
>> Perhaps it is possible to use a combination of wait_for_completion in
>> the uvc_video_free_requests and a complete callback in
>> uvc_video_complete for those requests that are not listed in the
>> req_free list.
>>
>> What do you think?
>>
>There might be a way that builds on your idea of cleaning up in the complete callback.
>It would rely on having a uvc_requests that aren't bulk allocated, which may have a
>performance impact.

Since the allocation will only be done once, this performance impact is
should not be critical.

>I am imagining something like the following:
>  1. Instead of allocating a bulk of uvc_requests, we allocate them
>     one at a time and add them to uvc_video.ureq
>  2. uvc_video.ureq becomes a list_head containing all the individual
>     requests
>  3. We add a sentinel flag in uvc_request that says the request is
>     now stale. This flag is protected by uvc_video->req_lock
>  4. uvc_video_complete looks at  this flag to deallocate both
>     usb_request and uvc_request.
>  5. uvcg_video_enable looks something like the following:
>       uvcg_video_enable(...) {
>         ...
>         lock(req_lock);
>         forall (uvc_requests->ureqs) {ureq->stale = true}
>         unlock(req_lock);
>         usb_ep_dequeue all reqs
>
>         uvc_video_free_requests(...)
>         ...
>       }
>  6. uvc_video_complete looks something like:
>       uvc_video_complete(...) {
>         // at the start
>         lock(req_lock)
>         is_stale = ureq->stale;
>         unlock(req_lock);
>
>         if (is_stale) {
>           usb_ep_free_request();
>           dealloc corresponding uvc_request();
>           return;
>         }
>
>         ...
>
>         lock(req_lock);
>         // possible that request became stale while we were handling stuff
>         if (!ureq->stale) {
>           list_add_tail(&req->list, &video->req_free);
>         } else {
>           usb_ep_free_request();
>           dealloc corresponding uvc_request();
>         }
>         unlock(req_lock);
>       }
>  7. uvc_video_free_requests can freely dealloc usb_requests/uvc_requests in
>     req_free because we can be certain that uvc_video_complete won't modify
>     it once requests have been marked stale, and the stale requests in flight
>     will be cleaned up by the complete callback.
>
>Effectively, we freeze the state of req_free before dequeuing, and all
>inflight requests are considered the responsibility of the complete handler
>from that point onwards. The gadget is only responsible for freeing requests it
>currently owns.
>
>I think this should ensure that we never have a situation where the ownership of the
>requests are undefined, and only one thread is responsible for freeing any given request.
>
>Hope that makes sense!

So you found a way to secure this also with the already available
req_lock then. Nice!

Also what you suggest is to move from the array model we currently have
to dynamic allocation in a linked list.

I would suggest some more adaptions.

Keep to allocate all requests dynamicaly as you suggest instead of the
bulk array.

Rewrite the uvc_video_free_requests to iterate over the video->req_free
list instead of all available requests to take care of all requests
that are truely freed.

Take this patch we started this thread with and expand it to
clean up not only the usb_request but also the uvc_request
like you suggested in your pseudo code.

Since we check for UVC_STATE_CONNECTED already in the comlete handler
this is a superset of your stale flag anyway. And every request
that is currently in flight is not part of the req_free list, which
makes the uvc_video_free_requests function free to run without making
no harm.

Does this sound better?

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 2/3] usb: gadget: uvc: cleanup request when not in correct state

[Avichal Rakesh]

On 9/19/23 12:13, Michael Grzeschik wrote:
> On Mon, Sep 18, 2023 at 04:40:07PM -0700, Avichal Rakesh wrote:
>>
>>
>> On 9/18/23 14:43, Michael Grzeschik wrote:
>>> On Mon, Sep 18, 2023 at 12:02:11PM -0700, Avichal Rakesh wrote:
>>>> On 9/16/23 16:23, Michael Grzeschik wrote:
>>>>> On Fri, Sep 15, 2023 at 07:41:05PM -0700, Avichal Rakesh wrote:
>>>>>> On 9/15/23 16:32, Michael Grzeschik wrote:
>>>>>>> On Mon, Sep 11, 2023 at 09:52:22PM -0700, Avichal Rakesh wrote:
>>>>>>>> On 9/10/23 17:24, Michael Grzeschik wrote:
>>>>>>>>> The uvc_video_enable function of the uvc-gadget driver is dequeing and
>>>>>>>>> immediately deallocs all requests on its disable codepath. This is not
>>>>>>>>> save since the dequeue function is async and does not ensure that the
>>>>>>>>> requests are left unlinked in the controller driver.
>>>>>>>>>
>>>>>>>>> By adding the ep_free_request into the completion path of the requests
>>>>>>>>> we ensure that the request will be properly deallocated.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>>>>>>>> ---
>>>>>>>>>  drivers/usb/gadget/function/uvc_video.c | 6 ++++++
>>>>>>>>>  1 file changed, 6 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
>>>>>>>>> index 4b6e854e30c58c..52e3666b51f743 100644
>>>>>>>>> --- a/drivers/usb/gadget/function/uvc_video.c
>>>>>>>>> +++ b/drivers/usb/gadget/function/uvc_video.c
>>>>>>>>> @@ -256,6 +256,12 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
>>>>>>>>>      struct uvc_device *uvc = video->uvc;
>>>>>>>>>      unsigned long flags;
>>>>>>>>>
>>>>>>>>> +    if (uvc->state == UVC_STATE_CONNECTED) {
>>>>>>>>> +        usb_ep_free_request(video->ep, ureq->req);
>>>>>>>> nit: You can probably just call usb_ep_free_request with req instead of ureq->req.
>>>>>>>
>>>>>>> Thanks, thats a good point.
>>>>>>>
>>>>>>>>> +        ureq->req = NULL;
>>>>>>>>> +        return;
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>>      switch (req->status) {
>>>>>>>>>      case 0:
>>>>>>>>>          break;
>>>>>>>>
>>>>>>>> Perhaps I am missing something here, but I am not sure how this alone
>>>>>>>> fixes the use-after-free issue. uvcg_video_enable still deallocates
>>>>>>>> _all_ usb_requests right after calling usb_ep_dequeue, so it is still
>>>>>>>> possible that an unreturned request is deallocated, and now it is
>>>>>>>> possible that the complete callback accesses a deallocated ureq :(
>>>>>>>
>>>>>>> Since the issue I saw was usually coming from the list_del_entry_valid check in
>>>>>>> the list_del_entry of the giveback function, the issue was probably just not
>>>>>>> triggered anymore as the complete function did exit early.
>>>>>>>
>>>>>>> So this fix alone is actually bogus without a second patch I had in the stack.
>>>>>>> The second patch I am refering should change the actual overall issue:
>>>>>>>
>>>>>>> https://lore.kernel.org/linux-usb/20230915233113.2903645-1-m.grzeschik@pengutronix.de/T/#u
>>>>>>>
>>>>>>> This early list_del and this patch here should ensure that the
>>>>>>> concurrent functions are not handling already freed memory.
>>>>>>
>>>>>> Oh, the patch linked above is interesting. It effectively force removes the dwc3_request
>>>>>> from whatever list it belongs to? So if DWC3's interrupt handler is delayed past
>>>>>> UVC gadget's ep_free_request call, then it won't see the requests in its cancelled
>>>>>> list at all. However, this setup is still prone to errors. For example, there is now
>>>>>> a chance that gadget_ep_free_request is called twice for one request. A scheduling
>>>>>> like the following might cause double kfree:
>>>>>>
>>>>>> 1. uvcg_video_enable calls usb_ep_dequeue for all usb_requests
>>>>>> 2. While the usb_ep_dequeues are being processed, dwc3's interrupt handler starts
>>>>>>   calling the complete callbacks.
>>>>>> 3. The complete callback calls gadget_ep_free_request (calling kfree as a result)
>>>>>> 4. Meanwhile, uvcg_video_enable has moved to uvc_video_free_requests which also
>>>>>>   calls gadget_ep_free_request (calling kfree).
>>>>>>
>>>>>> There is currently (even in your patches) no synchronization between calls to
>>>>>> gadget_ep_free_request via complete callback and uvcg_video_enable, which will
>>>>>> inevitably call usb_ep_free_request twice for one request.
>>>>>>
>>>>>> Does that make sense, or am I misunderstanding some part of the patch?
>>>>>
>>>>> The overall concept is correct. But in detail the
>>>>> uvc_video_free_requests is checking that video->ureq[i].req is not NULL.
>>>>>
>>>>> With our previous call of ep_free_request in the complete handler, the
>>>>> ureq->req pointer in focus was already set to NULL. So the
>>>>> uvc_video_free_requests function will skip that extra free.
>>>>>
>>>>
>>>> Is there any form of synchronization between uvc_video_request and the
>>>> complete callback? As I see it, the dwc3 interrupt thread and the v4l2
>>>> ioctl thread (which calls uvcg_video_enable) are fully independent, so
>>>> the calls made by them are free to be interleaved arbitrarily, so an
>>>> interleaving like this is technically possible:
>>>>
>>>> +------+------------------------------------+---------------------------------------------+
>>>> | time |            ioctl_thread            |            dwc3 interrupt handler           |
>>>> +======+====================================+=============================================+
>>>> |   1  | -uvc_v4l2_streamoff                |                                             |
>>>> |   2  | |-uvcg_video_enable                |                                             |
>>>> |   3  | ||-usb_ep_dequeue                  |                                             |
>>>> |   4  | ||                                 | -dwc3_process_event_buf                     |
>>>> |   5  | ||-uvc_video_free_requests         | |                                           |
>>>> |   6  | |||                                | |-dwc3_gadget_ep_cleanup_cancelled_requests |
>>>> |   7  | |||                                | ||-dwc3_gadget_giveback                     |
>>>> |   8  | |||                                | |||-uvc_video_complete                      |
>>>> |   9  | |||-check ureq->req != NULL [true] | ||||                                        |
>>>> |  10  | ||||-usb_ep_free_request           | ||||                                        |
>>>> |  11  | |||||-dwc3_ep_free_request         | ||||                                        |
>>>> |  12  | ||||||-kfree [first call]          | ||||                                        |
>>>> |  13  | ||||                               | ||||-usb_ep_free_request                    |
>>>> |  14  | ||||                               | |||||-dwc3_ep_free_request                  |
>>>> |  15  | ||||                               | ||||||-kfree [second call]                  |
>>>> |  16  | ||||                               | ||||-set ureq->req = NULL                   |
>>>> |  17  | ||||-set ureq->req = NULL          |                                             |
>>>> +------+------------------------------------+---------------------------------------------+
>>>>
>>>> A situation like this means that dwc3_ep_free_request can be called
>>>> twice for a particular usb_request. This is obviously low probability,
>>>> but a race condition here means we'll start seeing very vague and hard
>>>> to repro crashes or memory inconsistencies when using the uvc gadget.
>>>>
>>>> I do apologize if I've missed something obvious with your changes that
>>>> prevents such interleaving. I don't currently see any locking or
>>>> other synchronization mechanism in your changes. Is there something
>>>> in dwc3 that prevents this situation?
>>>
>>> I think you have pointed it out totally clear. This is obviously the
>>> case. It just did not trigger here. But the window is there and has to
>>> be locked in some way.
>>>
>>> For now we have two options to solve it.
>>>
>>> 1) Trying to avoid this double code path of the complete callback and
>>> uvc_video_free_requests. This is what your patches are already doing.
>>>
>>> But for now I am not so pleased with the timeout concept by waiting for
>>> the complete interrupt to be called. This is also a shot in the dark as
>>> the latency depends on the scheduler and the amount of potential
>>> requests that are being handled.
>>
>> I agree, a timeout is not the most elegant of solutions and given a
>> weird enough scheduler, will run into issues as well.
>>
>>>
>>> 2) Locking both codepathes around the resource in question so the issue
>>> is avoided.
>>>
>>> However, I am also not a fried of many locks.
>>>
>>> Perhaps it is possible to use a combination of wait_for_completion in
>>> the uvc_video_free_requests and a complete callback in
>>> uvc_video_complete for those requests that are not listed in the
>>> req_free list.
>>>
>>> What do you think?
>>>
>> There might be a way that builds on your idea of cleaning up in the complete callback.
>> It would rely on having a uvc_requests that aren't bulk allocated, which may have a
>> performance impact.
> 
> Since the allocation will only be done once, this performance impact is
> should not be critical.
> 
>> I am imagining something like the following:
>>  1. Instead of allocating a bulk of uvc_requests, we allocate them
>>     one at a time and add them to uvc_video.ureq
>>  2. uvc_video.ureq becomes a list_head containing all the individual
>>     requests
>>  3. We add a sentinel flag in uvc_request that says the request is
>>     now stale. This flag is protected by uvc_video->req_lock
>>  4. uvc_video_complete looks at  this flag to deallocate both
>>     usb_request and uvc_request.
>>  5. uvcg_video_enable looks something like the following:
>>       uvcg_video_enable(...) {
>>         ...
>>         lock(req_lock);
>>         forall (uvc_requests->ureqs) {ureq->stale = true}
>>         unlock(req_lock);
>>         usb_ep_dequeue all reqs
>>
>>         uvc_video_free_requests(...)
>>         ...
>>       }
>>  6. uvc_video_complete looks something like:
>>       uvc_video_complete(...) {
>>         // at the start
>>         lock(req_lock)
>>         is_stale = ureq->stale;
>>         unlock(req_lock);
>>
>>         if (is_stale) {
>>           usb_ep_free_request();
>>           dealloc corresponding uvc_request();
>>           return;
>>         }
>>
>>         ...
>>
>>         lock(req_lock);
>>         // possible that request became stale while we were handling stuff
>>         if (!ureq->stale) {
>>           list_add_tail(&req->list, &video->req_free);
>>         } else {
>>           usb_ep_free_request();
>>           dealloc corresponding uvc_request();
>>         }
>>         unlock(req_lock);
>>       }
>>  7. uvc_video_free_requests can freely dealloc usb_requests/uvc_requests in
>>     req_free because we can be certain that uvc_video_complete won't modify
>>     it once requests have been marked stale, and the stale requests in flight
>>     will be cleaned up by the complete callback.
>>
>> Effectively, we freeze the state of req_free before dequeuing, and all
>> inflight requests are considered the responsibility of the complete handler
>> from that point onwards. The gadget is only responsible for freeing requests it
>> currently owns.
>>
>> I think this should ensure that we never have a situation where the ownership of the
>> requests are undefined, and only one thread is responsible for freeing any given request.
>>
>> Hope that makes sense!
> 
> So you found a way to secure this also with the already available
> req_lock then. Nice!
> 
> Also what you suggest is to move from the array model we currently have
> to dynamic allocation in a linked list.
> 
> I would suggest some more adaptions.
> 
> Keep to allocate all requests dynamicaly as you suggest instead of the
> bulk array.
> 
> Rewrite the uvc_video_free_requests to iterate over the video->req_free
> list instead of all available requests to take care of all requests
> that are truely freed.
> 
> Take this patch we started this thread with and expand it to
> clean up not only the usb_request but also the uvc_request
> like you suggested in your pseudo code.
> 
> Since we check for UVC_STATE_CONNECTED already in the comlete handler
> this is a superset of your stale flag anyway. And every request
> that is currently in flight is not part of the req_free list, which
> makes the uvc_video_free_requests function free to run without making
> no harm.

The downside of freeing based on UVC_STATE_CONNECTED and why it might be 
problematic is that without any other synchronization method, the complete
callback can be arbitrarily delayed for a given usb_request.

A STREAMOFF quickly followed by a STREAMON, might set uvc->state to 
UVC_STATE_STREAMING before the controller has had a chance to return the 
stale requests. This won't cause any functional issues AFAICT, but will 
cause a memory "leak" of sorts where every successive quick 
STREAMOFF-->STREAMON will lead to some extra usb_requests sticking around.
They'll eventually get freed, but it doesn't seem very responsible to increase
the memory load unless required.

The stale flag ensures that this situation never happens and even if the 
complete callbacks comes back well after  the new STREAMON event, we correctly
free the associated usb_request and uvc_request.

Hope that makes sense!

- Avi.

Re: [PATCH 2/3] usb: gadget: uvc: cleanup request when not in correct state

[Michael Grzeschik]

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

On Tue, Sep 19, 2023 at 12:55:02PM -0700, Avichal Rakesh wrote:
>On 9/19/23 12:13, Michael Grzeschik wrote:
>> On Mon, Sep 18, 2023 at 04:40:07PM -0700, Avichal Rakesh wrote:
>>>
>>>
>>> On 9/18/23 14:43, Michael Grzeschik wrote:
>>>> On Mon, Sep 18, 2023 at 12:02:11PM -0700, Avichal Rakesh wrote:
>>>>> On 9/16/23 16:23, Michael Grzeschik wrote:
>>>>>> On Fri, Sep 15, 2023 at 07:41:05PM -0700, Avichal Rakesh wrote:
>>>>>>> On 9/15/23 16:32, Michael Grzeschik wrote:
>>>>>>>> On Mon, Sep 11, 2023 at 09:52:22PM -0700, Avichal Rakesh wrote:
>>>>>>>>> On 9/10/23 17:24, Michael Grzeschik wrote:
>>>>>>>>>> The uvc_video_enable function of the uvc-gadget driver is dequeing and
>>>>>>>>>> immediately deallocs all requests on its disable codepath. This is not
>>>>>>>>>> save since the dequeue function is async and does not ensure that the
>>>>>>>>>> requests are left unlinked in the controller driver.
>>>>>>>>>>
>>>>>>>>>> By adding the ep_free_request into the completion path of the requests
>>>>>>>>>> we ensure that the request will be properly deallocated.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>>>>>>>>> ---
>>>>>>>>>>  drivers/usb/gadget/function/uvc_video.c | 6 ++++++
>>>>>>>>>>  1 file changed, 6 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
>>>>>>>>>> index 4b6e854e30c58c..52e3666b51f743 100644
>>>>>>>>>> --- a/drivers/usb/gadget/function/uvc_video.c
>>>>>>>>>> +++ b/drivers/usb/gadget/function/uvc_video.c
>>>>>>>>>> @@ -256,6 +256,12 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
>>>>>>>>>>      struct uvc_device *uvc = video->uvc;
>>>>>>>>>>      unsigned long flags;
>>>>>>>>>>
>>>>>>>>>> +    if (uvc->state == UVC_STATE_CONNECTED) {
>>>>>>>>>> +        usb_ep_free_request(video->ep, ureq->req);
>>>>>>>>> nit: You can probably just call usb_ep_free_request with req instead of ureq->req.
>>>>>>>>
>>>>>>>> Thanks, thats a good point.
>>>>>>>>
>>>>>>>>>> +        ureq->req = NULL;
>>>>>>>>>> +        return;
>>>>>>>>>> +    }
>>>>>>>>>> +
>>>>>>>>>>      switch (req->status) {
>>>>>>>>>>      case 0:
>>>>>>>>>>          break;
>>>>>>>>>
>>>>>>>>> Perhaps I am missing something here, but I am not sure how this alone
>>>>>>>>> fixes the use-after-free issue. uvcg_video_enable still deallocates
>>>>>>>>> _all_ usb_requests right after calling usb_ep_dequeue, so it is still
>>>>>>>>> possible that an unreturned request is deallocated, and now it is
>>>>>>>>> possible that the complete callback accesses a deallocated ureq :(
>>>>>>>>
>>>>>>>> Since the issue I saw was usually coming from the list_del_entry_valid check in
>>>>>>>> the list_del_entry of the giveback function, the issue was probably just not
>>>>>>>> triggered anymore as the complete function did exit early.
>>>>>>>>
>>>>>>>> So this fix alone is actually bogus without a second patch I had in the stack.
>>>>>>>> The second patch I am refering should change the actual overall issue:
>>>>>>>>
>>>>>>>> https://lore.kernel.org/linux-usb/20230915233113.2903645-1-m.grzeschik@pengutronix.de/T/#u
>>>>>>>>
>>>>>>>> This early list_del and this patch here should ensure that the
>>>>>>>> concurrent functions are not handling already freed memory.
>>>>>>>
>>>>>>> Oh, the patch linked above is interesting. It effectively force removes the dwc3_request
>>>>>>> from whatever list it belongs to? So if DWC3's interrupt handler is delayed past
>>>>>>> UVC gadget's ep_free_request call, then it won't see the requests in its cancelled
>>>>>>> list at all. However, this setup is still prone to errors. For example, there is now
>>>>>>> a chance that gadget_ep_free_request is called twice for one request. A scheduling
>>>>>>> like the following might cause double kfree:
>>>>>>>
>>>>>>> 1. uvcg_video_enable calls usb_ep_dequeue for all usb_requests
>>>>>>> 2. While the usb_ep_dequeues are being processed, dwc3's interrupt handler starts
>>>>>>>   calling the complete callbacks.
>>>>>>> 3. The complete callback calls gadget_ep_free_request (calling kfree as a result)
>>>>>>> 4. Meanwhile, uvcg_video_enable has moved to uvc_video_free_requests which also
>>>>>>>   calls gadget_ep_free_request (calling kfree).
>>>>>>>
>>>>>>> There is currently (even in your patches) no synchronization between calls to
>>>>>>> gadget_ep_free_request via complete callback and uvcg_video_enable, which will
>>>>>>> inevitably call usb_ep_free_request twice for one request.
>>>>>>>
>>>>>>> Does that make sense, or am I misunderstanding some part of the patch?
>>>>>>
>>>>>> The overall concept is correct. But in detail the
>>>>>> uvc_video_free_requests is checking that video->ureq[i].req is not NULL.
>>>>>>
>>>>>> With our previous call of ep_free_request in the complete handler, the
>>>>>> ureq->req pointer in focus was already set to NULL. So the
>>>>>> uvc_video_free_requests function will skip that extra free.
>>>>>>
>>>>>
>>>>> Is there any form of synchronization between uvc_video_request and the
>>>>> complete callback? As I see it, the dwc3 interrupt thread and the v4l2
>>>>> ioctl thread (which calls uvcg_video_enable) are fully independent, so
>>>>> the calls made by them are free to be interleaved arbitrarily, so an
>>>>> interleaving like this is technically possible:
>>>>>
>>>>> +------+------------------------------------+---------------------------------------------+
>>>>> | time |            ioctl_thread            |            dwc3 interrupt handler           |
>>>>> +======+====================================+=============================================+
>>>>> |   1  | -uvc_v4l2_streamoff                |                                             |
>>>>> |   2  | |-uvcg_video_enable                |                                             |
>>>>> |   3  | ||-usb_ep_dequeue                  |                                             |
>>>>> |   4  | ||                                 | -dwc3_process_event_buf                     |
>>>>> |   5  | ||-uvc_video_free_requests         | |                                           |
>>>>> |   6  | |||                                | |-dwc3_gadget_ep_cleanup_cancelled_requests |
>>>>> |   7  | |||                                | ||-dwc3_gadget_giveback                     |
>>>>> |   8  | |||                                | |||-uvc_video_complete                      |
>>>>> |   9  | |||-check ureq->req != NULL [true] | ||||                                        |
>>>>> |  10  | ||||-usb_ep_free_request           | ||||                                        |
>>>>> |  11  | |||||-dwc3_ep_free_request         | ||||                                        |
>>>>> |  12  | ||||||-kfree [first call]          | ||||                                        |
>>>>> |  13  | ||||                               | ||||-usb_ep_free_request                    |
>>>>> |  14  | ||||                               | |||||-dwc3_ep_free_request                  |
>>>>> |  15  | ||||                               | ||||||-kfree [second call]                  |
>>>>> |  16  | ||||                               | ||||-set ureq->req = NULL                   |
>>>>> |  17  | ||||-set ureq->req = NULL          |                                             |
>>>>> +------+------------------------------------+---------------------------------------------+
>>>>>
>>>>> A situation like this means that dwc3_ep_free_request can be called
>>>>> twice for a particular usb_request. This is obviously low probability,
>>>>> but a race condition here means we'll start seeing very vague and hard
>>>>> to repro crashes or memory inconsistencies when using the uvc gadget.
>>>>>
>>>>> I do apologize if I've missed something obvious with your changes that
>>>>> prevents such interleaving. I don't currently see any locking or
>>>>> other synchronization mechanism in your changes. Is there something
>>>>> in dwc3 that prevents this situation?
>>>>
>>>> I think you have pointed it out totally clear. This is obviously the
>>>> case. It just did not trigger here. But the window is there and has to
>>>> be locked in some way.
>>>>
>>>> For now we have two options to solve it.
>>>>
>>>> 1) Trying to avoid this double code path of the complete callback and
>>>> uvc_video_free_requests. This is what your patches are already doing.
>>>>
>>>> But for now I am not so pleased with the timeout concept by waiting for
>>>> the complete interrupt to be called. This is also a shot in the dark as
>>>> the latency depends on the scheduler and the amount of potential
>>>> requests that are being handled.
>>>
>>> I agree, a timeout is not the most elegant of solutions and given a
>>> weird enough scheduler, will run into issues as well.
>>>
>>>>
>>>> 2) Locking both codepathes around the resource in question so the issue
>>>> is avoided.
>>>>
>>>> However, I am also not a fried of many locks.
>>>>
>>>> Perhaps it is possible to use a combination of wait_for_completion in
>>>> the uvc_video_free_requests and a complete callback in
>>>> uvc_video_complete for those requests that are not listed in the
>>>> req_free list.
>>>>
>>>> What do you think?
>>>>
>>> There might be a way that builds on your idea of cleaning up in the complete callback.
>>> It would rely on having a uvc_requests that aren't bulk allocated, which may have a
>>> performance impact.
>>
>> Since the allocation will only be done once, this performance impact is
>> should not be critical.
>>
>>> I am imagining something like the following:
>>>  1. Instead of allocating a bulk of uvc_requests, we allocate them
>>>     one at a time and add them to uvc_video.ureq
>>>  2. uvc_video.ureq becomes a list_head containing all the individual
>>>     requests
>>>  3. We add a sentinel flag in uvc_request that says the request is
>>>     now stale. This flag is protected by uvc_video->req_lock
>>>  4. uvc_video_complete looks at  this flag to deallocate both
>>>     usb_request and uvc_request.
>>>  5. uvcg_video_enable looks something like the following:
>>>       uvcg_video_enable(...) {
>>>         ...
>>>         lock(req_lock);
>>>         forall (uvc_requests->ureqs) {ureq->stale = true}
>>>         unlock(req_lock);
>>>         usb_ep_dequeue all reqs
>>>
>>>         uvc_video_free_requests(...)
>>>         ...
>>>       }
>>>  6. uvc_video_complete looks something like:
>>>       uvc_video_complete(...) {
>>>         // at the start
>>>         lock(req_lock)
>>>         is_stale = ureq->stale;
>>>         unlock(req_lock);
>>>
>>>         if (is_stale) {
>>>           usb_ep_free_request();
>>>           dealloc corresponding uvc_request();
>>>           return;
>>>         }
>>>
>>>         ...
>>>
>>>         lock(req_lock);
>>>         // possible that request became stale while we were handling stuff
>>>         if (!ureq->stale) {
>>>           list_add_tail(&req->list, &video->req_free);
>>>         } else {
>>>           usb_ep_free_request();
>>>           dealloc corresponding uvc_request();
>>>         }
>>>         unlock(req_lock);
>>>       }
>>>  7. uvc_video_free_requests can freely dealloc usb_requests/uvc_requests in
>>>     req_free because we can be certain that uvc_video_complete won't modify
>>>     it once requests have been marked stale, and the stale requests in flight
>>>     will be cleaned up by the complete callback.
>>>
>>> Effectively, we freeze the state of req_free before dequeuing, and all
>>> inflight requests are considered the responsibility of the complete handler
>>> from that point onwards. The gadget is only responsible for freeing requests it
>>> currently owns.
>>>
>>> I think this should ensure that we never have a situation where the ownership of the
>>> requests are undefined, and only one thread is responsible for freeing any given request.
>>>
>>> Hope that makes sense!
>>
>> So you found a way to secure this also with the already available
>> req_lock then. Nice!
>>
>> Also what you suggest is to move from the array model we currently have
>> to dynamic allocation in a linked list.
>>
>> I would suggest some more adaptions.
>>
>> Keep to allocate all requests dynamicaly as you suggest instead of the
>> bulk array.
>>
>> Rewrite the uvc_video_free_requests to iterate over the video->req_free
>> list instead of all available requests to take care of all requests
>> that are truely freed.
>>
>> Take this patch we started this thread with and expand it to
>> clean up not only the usb_request but also the uvc_request
>> like you suggested in your pseudo code.
>>
>> Since we check for UVC_STATE_CONNECTED already in the comlete handler
>> this is a superset of your stale flag anyway. And every request
>> that is currently in flight is not part of the req_free list, which
>> makes the uvc_video_free_requests function free to run without making
>> no harm.
>
>The downside of freeing based on UVC_STATE_CONNECTED and why it might be
>problematic is that without any other synchronization method, the complete
>callback can be arbitrarily delayed for a given usb_request.
>
>A STREAMOFF quickly followed by a STREAMON, might set uvc->state to
>UVC_STATE_STREAMING before the controller has had a chance to return the
>stale requests. This won't cause any functional issues AFAICT, but will
>cause a memory "leak" of sorts where every successive quick
>STREAMOFF-->STREAMON will lead to some extra usb_requests sticking around.
>They'll eventually get freed, but it doesn't seem very responsible to increase
>the memory load unless required.
>
>The stale flag ensures that this situation never happens and even if the
>complete callbacks comes back well after  the new STREAMON event, we correctly
>free the associated usb_request and uvc_request.

In that case we could use stale then, but I would suggest also keeping
the change the functionality of uvc_video_free_requests aswell to loop
over the requests in req_free 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 2/3] usb: gadget: uvc: cleanup request when not in correct state

[Avichal Rakesh]

On 9/19/23 13:07, Michael Grzeschik wrote:
> On Tue, Sep 19, 2023 at 12:55:02PM -0700, Avichal Rakesh wrote:
>> On 9/19/23 12:13, Michael Grzeschik wrote:
>>> On Mon, Sep 18, 2023 at 04:40:07PM -0700, Avichal Rakesh wrote:
>>>>
>>>>
>>>> On 9/18/23 14:43, Michael Grzeschik wrote:
>>>>> On Mon, Sep 18, 2023 at 12:02:11PM -0700, Avichal Rakesh wrote:
>>>>>> On 9/16/23 16:23, Michael Grzeschik wrote:
>>>>>>> On Fri, Sep 15, 2023 at 07:41:05PM -0700, Avichal Rakesh wrote:
>>>>>>>> On 9/15/23 16:32, Michael Grzeschik wrote:
>>>>>>>>> On Mon, Sep 11, 2023 at 09:52:22PM -0700, Avichal Rakesh wrote:
>>>>>>>>>> On 9/10/23 17:24, Michael Grzeschik wrote:
>>>>>>>>>>> The uvc_video_enable function of the uvc-gadget driver is dequeing and
>>>>>>>>>>> immediately deallocs all requests on its disable codepath. This is not
>>>>>>>>>>> save since the dequeue function is async and does not ensure that the
>>>>>>>>>>> requests are left unlinked in the controller driver.
>>>>>>>>>>>
>>>>>>>>>>> By adding the ep_free_request into the completion path of the requests
>>>>>>>>>>> we ensure that the request will be properly deallocated.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>>>>>>>>>> ---
>>>>>>>>>>>  drivers/usb/gadget/function/uvc_video.c | 6 ++++++
>>>>>>>>>>>  1 file changed, 6 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
>>>>>>>>>>> index 4b6e854e30c58c..52e3666b51f743 100644
>>>>>>>>>>> --- a/drivers/usb/gadget/function/uvc_video.c
>>>>>>>>>>> +++ b/drivers/usb/gadget/function/uvc_video.c
>>>>>>>>>>> @@ -256,6 +256,12 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
>>>>>>>>>>>      struct uvc_device *uvc = video->uvc;
>>>>>>>>>>>      unsigned long flags;
>>>>>>>>>>>
>>>>>>>>>>> +    if (uvc->state == UVC_STATE_CONNECTED) {
>>>>>>>>>>> +        usb_ep_free_request(video->ep, ureq->req);
>>>>>>>>>> nit: You can probably just call usb_ep_free_request with req instead of ureq->req.
>>>>>>>>>
>>>>>>>>> Thanks, thats a good point.
>>>>>>>>>
>>>>>>>>>>> +        ureq->req = NULL;
>>>>>>>>>>> +        return;
>>>>>>>>>>> +    }
>>>>>>>>>>> +
>>>>>>>>>>>      switch (req->status) {
>>>>>>>>>>>      case 0:
>>>>>>>>>>>          break;
>>>>>>>>>>
>>>>>>>>>> Perhaps I am missing something here, but I am not sure how this alone
>>>>>>>>>> fixes the use-after-free issue. uvcg_video_enable still deallocates
>>>>>>>>>> _all_ usb_requests right after calling usb_ep_dequeue, so it is still
>>>>>>>>>> possible that an unreturned request is deallocated, and now it is
>>>>>>>>>> possible that the complete callback accesses a deallocated ureq :(
>>>>>>>>>
>>>>>>>>> Since the issue I saw was usually coming from the list_del_entry_valid check in
>>>>>>>>> the list_del_entry of the giveback function, the issue was probably just not
>>>>>>>>> triggered anymore as the complete function did exit early.
>>>>>>>>>
>>>>>>>>> So this fix alone is actually bogus without a second patch I had in the stack.
>>>>>>>>> The second patch I am refering should change the actual overall issue:
>>>>>>>>>
>>>>>>>>> https://lore.kernel.org/linux-usb/20230915233113.2903645-1-m.grzeschik@pengutronix.de/T/#u
>>>>>>>>>
>>>>>>>>> This early list_del and this patch here should ensure that the
>>>>>>>>> concurrent functions are not handling already freed memory.
>>>>>>>>
>>>>>>>> Oh, the patch linked above is interesting. It effectively force removes the dwc3_request
>>>>>>>> from whatever list it belongs to? So if DWC3's interrupt handler is delayed past
>>>>>>>> UVC gadget's ep_free_request call, then it won't see the requests in its cancelled
>>>>>>>> list at all. However, this setup is still prone to errors. For example, there is now
>>>>>>>> a chance that gadget_ep_free_request is called twice for one request. A scheduling
>>>>>>>> like the following might cause double kfree:
>>>>>>>>
>>>>>>>> 1. uvcg_video_enable calls usb_ep_dequeue for all usb_requests
>>>>>>>> 2. While the usb_ep_dequeues are being processed, dwc3's interrupt handler starts
>>>>>>>>   calling the complete callbacks.
>>>>>>>> 3. The complete callback calls gadget_ep_free_request (calling kfree as a result)
>>>>>>>> 4. Meanwhile, uvcg_video_enable has moved to uvc_video_free_requests which also
>>>>>>>>   calls gadget_ep_free_request (calling kfree).
>>>>>>>>
>>>>>>>> There is currently (even in your patches) no synchronization between calls to
>>>>>>>> gadget_ep_free_request via complete callback and uvcg_video_enable, which will
>>>>>>>> inevitably call usb_ep_free_request twice for one request.
>>>>>>>>
>>>>>>>> Does that make sense, or am I misunderstanding some part of the patch?
>>>>>>>
>>>>>>> The overall concept is correct. But in detail the
>>>>>>> uvc_video_free_requests is checking that video->ureq[i].req is not NULL.
>>>>>>>
>>>>>>> With our previous call of ep_free_request in the complete handler, the
>>>>>>> ureq->req pointer in focus was already set to NULL. So the
>>>>>>> uvc_video_free_requests function will skip that extra free.
>>>>>>>
>>>>>>
>>>>>> Is there any form of synchronization between uvc_video_request and the
>>>>>> complete callback? As I see it, the dwc3 interrupt thread and the v4l2
>>>>>> ioctl thread (which calls uvcg_video_enable) are fully independent, so
>>>>>> the calls made by them are free to be interleaved arbitrarily, so an
>>>>>> interleaving like this is technically possible:
>>>>>>
>>>>>> +------+------------------------------------+---------------------------------------------+
>>>>>> | time |            ioctl_thread            |            dwc3 interrupt handler           |
>>>>>> +======+====================================+=============================================+
>>>>>> |   1  | -uvc_v4l2_streamoff                |                                             |
>>>>>> |   2  | |-uvcg_video_enable                |                                             |
>>>>>> |   3  | ||-usb_ep_dequeue                  |                                             |
>>>>>> |   4  | ||                                 | -dwc3_process_event_buf                     |
>>>>>> |   5  | ||-uvc_video_free_requests         | |                                           |
>>>>>> |   6  | |||                                | |-dwc3_gadget_ep_cleanup_cancelled_requests |
>>>>>> |   7  | |||                                | ||-dwc3_gadget_giveback                     |
>>>>>> |   8  | |||                                | |||-uvc_video_complete                      |
>>>>>> |   9  | |||-check ureq->req != NULL [true] | ||||                                        |
>>>>>> |  10  | ||||-usb_ep_free_request           | ||||                                        |
>>>>>> |  11  | |||||-dwc3_ep_free_request         | ||||                                        |
>>>>>> |  12  | ||||||-kfree [first call]          | ||||                                        |
>>>>>> |  13  | ||||                               | ||||-usb_ep_free_request                    |
>>>>>> |  14  | ||||                               | |||||-dwc3_ep_free_request                  |
>>>>>> |  15  | ||||                               | ||||||-kfree [second call]                  |
>>>>>> |  16  | ||||                               | ||||-set ureq->req = NULL                   |
>>>>>> |  17  | ||||-set ureq->req = NULL          |                                             |
>>>>>> +------+------------------------------------+---------------------------------------------+
>>>>>>
>>>>>> A situation like this means that dwc3_ep_free_request can be called
>>>>>> twice for a particular usb_request. This is obviously low probability,
>>>>>> but a race condition here means we'll start seeing very vague and hard
>>>>>> to repro crashes or memory inconsistencies when using the uvc gadget.
>>>>>>
>>>>>> I do apologize if I've missed something obvious with your changes that
>>>>>> prevents such interleaving. I don't currently see any locking or
>>>>>> other synchronization mechanism in your changes. Is there something
>>>>>> in dwc3 that prevents this situation?
>>>>>
>>>>> I think you have pointed it out totally clear. This is obviously the
>>>>> case. It just did not trigger here. But the window is there and has to
>>>>> be locked in some way.
>>>>>
>>>>> For now we have two options to solve it.
>>>>>
>>>>> 1) Trying to avoid this double code path of the complete callback and
>>>>> uvc_video_free_requests. This is what your patches are already doing.
>>>>>
>>>>> But for now I am not so pleased with the timeout concept by waiting for
>>>>> the complete interrupt to be called. This is also a shot in the dark as
>>>>> the latency depends on the scheduler and the amount of potential
>>>>> requests that are being handled.
>>>>
>>>> I agree, a timeout is not the most elegant of solutions and given a
>>>> weird enough scheduler, will run into issues as well.
>>>>
>>>>>
>>>>> 2) Locking both codepathes around the resource in question so the issue
>>>>> is avoided.
>>>>>
>>>>> However, I am also not a fried of many locks.
>>>>>
>>>>> Perhaps it is possible to use a combination of wait_for_completion in
>>>>> the uvc_video_free_requests and a complete callback in
>>>>> uvc_video_complete for those requests that are not listed in the
>>>>> req_free list.
>>>>>
>>>>> What do you think?
>>>>>
>>>> There might be a way that builds on your idea of cleaning up in the complete callback.
>>>> It would rely on having a uvc_requests that aren't bulk allocated, which may have a
>>>> performance impact.
>>>
>>> Since the allocation will only be done once, this performance impact is
>>> should not be critical.
>>>
>>>> I am imagining something like the following:
>>>>  1. Instead of allocating a bulk of uvc_requests, we allocate them
>>>>     one at a time and add them to uvc_video.ureq
>>>>  2. uvc_video.ureq becomes a list_head containing all the individual
>>>>     requests
>>>>  3. We add a sentinel flag in uvc_request that says the request is
>>>>     now stale. This flag is protected by uvc_video->req_lock
>>>>  4. uvc_video_complete looks at  this flag to deallocate both
>>>>     usb_request and uvc_request.
>>>>  5. uvcg_video_enable looks something like the following:
>>>>       uvcg_video_enable(...) {
>>>>         ...
>>>>         lock(req_lock);
>>>>         forall (uvc_requests->ureqs) {ureq->stale = true}
>>>>         unlock(req_lock);
>>>>         usb_ep_dequeue all reqs
>>>>
>>>>         uvc_video_free_requests(...)
>>>>         ...
>>>>       }
>>>>  6. uvc_video_complete looks something like:
>>>>       uvc_video_complete(...) {
>>>>         // at the start
>>>>         lock(req_lock)
>>>>         is_stale = ureq->stale;
>>>>         unlock(req_lock);
>>>>
>>>>         if (is_stale) {
>>>>           usb_ep_free_request();
>>>>           dealloc corresponding uvc_request();
>>>>           return;
>>>>         }
>>>>
>>>>         ...
>>>>
>>>>         lock(req_lock);
>>>>         // possible that request became stale while we were handling stuff
>>>>         if (!ureq->stale) {
>>>>           list_add_tail(&req->list, &video->req_free);
>>>>         } else {
>>>>           usb_ep_free_request();
>>>>           dealloc corresponding uvc_request();
>>>>         }
>>>>         unlock(req_lock);
>>>>       }
>>>>  7. uvc_video_free_requests can freely dealloc usb_requests/uvc_requests in
>>>>     req_free because we can be certain that uvc_video_complete won't modify
>>>>     it once requests have been marked stale, and the stale requests in flight
>>>>     will be cleaned up by the complete callback.
>>>>
>>>> Effectively, we freeze the state of req_free before dequeuing, and all
>>>> inflight requests are considered the responsibility of the complete handler
>>>> from that point onwards. The gadget is only responsible for freeing requests it
>>>> currently owns.
>>>>
>>>> I think this should ensure that we never have a situation where the ownership of the
>>>> requests are undefined, and only one thread is responsible for freeing any given request.
>>>>
>>>> Hope that makes sense!
>>>
>>> So you found a way to secure this also with the already available
>>> req_lock then. Nice!
>>>
>>> Also what you suggest is to move from the array model we currently have
>>> to dynamic allocation in a linked list.
>>>
>>> I would suggest some more adaptions.
>>>
>>> Keep to allocate all requests dynamicaly as you suggest instead of the
>>> bulk array.
>>>
>>> Rewrite the uvc_video_free_requests to iterate over the video->req_free
>>> list instead of all available requests to take care of all requests
>>> that are truely freed.
>>>
>>> Take this patch we started this thread with and expand it to
>>> clean up not only the usb_request but also the uvc_request
>>> like you suggested in your pseudo code.
>>>
>>> Since we check for UVC_STATE_CONNECTED already in the comlete handler
>>> this is a superset of your stale flag anyway. And every request
>>> that is currently in flight is not part of the req_free list, which
>>> makes the uvc_video_free_requests function free to run without making
>>> no harm.
>>
>> The downside of freeing based on UVC_STATE_CONNECTED and why it might be
>> problematic is that without any other synchronization method, the complete
>> callback can be arbitrarily delayed for a given usb_request.
>>
>> A STREAMOFF quickly followed by a STREAMON, might set uvc->state to
>> UVC_STATE_STREAMING before the controller has had a chance to return the
>> stale requests. This won't cause any functional issues AFAICT, but will
>> cause a memory "leak" of sorts where every successive quick
>> STREAMOFF-->STREAMON will lead to some extra usb_requests sticking around.
>> They'll eventually get freed, but it doesn't seem very responsible to increase
>> the memory load unless required.
>>
>> The stale flag ensures that this situation never happens and even if the
>> complete callbacks comes back well after  the new STREAMON event, we correctly
>> free the associated usb_request and uvc_request.
> 
> In that case we could use stale then, but I would suggest also keeping
> the change the functionality of uvc_video_free_requests aswell to loop
> over the requests in req_free list.

Agreed, uvc_video_free_requests should only free the requests in
req_free. 

Just to clear any confusion: are you working on incorporating these changes
into your patchset, or do you want me to include them in 
https://lore.kernel.org/20230912041910.726442-3-arakesh@google.com/ 
instead?

Regards,
Avi.

Re: [PATCH 2/3] usb: gadget: uvc: cleanup request when not in correct state

[Michael Grzeschik]

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

On Tue, Sep 19, 2023 at 01:22:42PM -0700, Avichal Rakesh wrote:
>
>
>On 9/19/23 13:07, Michael Grzeschik wrote:
>> On Tue, Sep 19, 2023 at 12:55:02PM -0700, Avichal Rakesh wrote:
>>> On 9/19/23 12:13, Michael Grzeschik wrote:
>>>> On Mon, Sep 18, 2023 at 04:40:07PM -0700, Avichal Rakesh wrote:
>>>>>
>>>>>
>>>>> On 9/18/23 14:43, Michael Grzeschik wrote:
>>>>>> On Mon, Sep 18, 2023 at 12:02:11PM -0700, Avichal Rakesh wrote:
>>>>>>> On 9/16/23 16:23, Michael Grzeschik wrote:
>>>>>>>> On Fri, Sep 15, 2023 at 07:41:05PM -0700, Avichal Rakesh wrote:
>>>>>>>>> On 9/15/23 16:32, Michael Grzeschik wrote:
>>>>>>>>>> On Mon, Sep 11, 2023 at 09:52:22PM -0700, Avichal Rakesh wrote:
>>>>>>>>>>> On 9/10/23 17:24, Michael Grzeschik wrote:
>>>>>>>>>>>> The uvc_video_enable function of the uvc-gadget driver is dequeing and
>>>>>>>>>>>> immediately deallocs all requests on its disable codepath. This is not
>>>>>>>>>>>> save since the dequeue function is async and does not ensure that the
>>>>>>>>>>>> requests are left unlinked in the controller driver.
>>>>>>>>>>>>
>>>>>>>>>>>> By adding the ep_free_request into the completion path of the requests
>>>>>>>>>>>> we ensure that the request will be properly deallocated.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>>>>>>>>>>> ---
>>>>>>>>>>>>  drivers/usb/gadget/function/uvc_video.c | 6 ++++++
>>>>>>>>>>>>  1 file changed, 6 insertions(+)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/usb/gadget/function/uvc_video.c b/drivers/usb/gadget/function/uvc_video.c
>>>>>>>>>>>> index 4b6e854e30c58c..52e3666b51f743 100644
>>>>>>>>>>>> --- a/drivers/usb/gadget/function/uvc_video.c
>>>>>>>>>>>> +++ b/drivers/usb/gadget/function/uvc_video.c
>>>>>>>>>>>> @@ -256,6 +256,12 @@ uvc_video_complete(struct usb_ep *ep, struct usb_request *req)
>>>>>>>>>>>>      struct uvc_device *uvc = video->uvc;
>>>>>>>>>>>>      unsigned long flags;
>>>>>>>>>>>>
>>>>>>>>>>>> +    if (uvc->state == UVC_STATE_CONNECTED) {
>>>>>>>>>>>> +        usb_ep_free_request(video->ep, ureq->req);
>>>>>>>>>>> nit: You can probably just call usb_ep_free_request with req instead of ureq->req.
>>>>>>>>>>
>>>>>>>>>> Thanks, thats a good point.
>>>>>>>>>>
>>>>>>>>>>>> +        ureq->req = NULL;
>>>>>>>>>>>> +        return;
>>>>>>>>>>>> +    }
>>>>>>>>>>>> +
>>>>>>>>>>>>      switch (req->status) {
>>>>>>>>>>>>      case 0:
>>>>>>>>>>>>          break;
>>>>>>>>>>>
>>>>>>>>>>> Perhaps I am missing something here, but I am not sure how this alone
>>>>>>>>>>> fixes the use-after-free issue. uvcg_video_enable still deallocates
>>>>>>>>>>> _all_ usb_requests right after calling usb_ep_dequeue, so it is still
>>>>>>>>>>> possible that an unreturned request is deallocated, and now it is
>>>>>>>>>>> possible that the complete callback accesses a deallocated ureq :(
>>>>>>>>>>
>>>>>>>>>> Since the issue I saw was usually coming from the list_del_entry_valid check in
>>>>>>>>>> the list_del_entry of the giveback function, the issue was probably just not
>>>>>>>>>> triggered anymore as the complete function did exit early.
>>>>>>>>>>
>>>>>>>>>> So this fix alone is actually bogus without a second patch I had in the stack.
>>>>>>>>>> The second patch I am refering should change the actual overall issue:
>>>>>>>>>>
>>>>>>>>>> https://lore.kernel.org/linux-usb/20230915233113.2903645-1-m.grzeschik@pengutronix.de/T/#u
>>>>>>>>>>
>>>>>>>>>> This early list_del and this patch here should ensure that the
>>>>>>>>>> concurrent functions are not handling already freed memory.
>>>>>>>>>
>>>>>>>>> Oh, the patch linked above is interesting. It effectively force removes the dwc3_request
>>>>>>>>> from whatever list it belongs to? So if DWC3's interrupt handler is delayed past
>>>>>>>>> UVC gadget's ep_free_request call, then it won't see the requests in its cancelled
>>>>>>>>> list at all. However, this setup is still prone to errors. For example, there is now
>>>>>>>>> a chance that gadget_ep_free_request is called twice for one request. A scheduling
>>>>>>>>> like the following might cause double kfree:
>>>>>>>>>
>>>>>>>>> 1. uvcg_video_enable calls usb_ep_dequeue for all usb_requests
>>>>>>>>> 2. While the usb_ep_dequeues are being processed, dwc3's interrupt handler starts
>>>>>>>>>   calling the complete callbacks.
>>>>>>>>> 3. The complete callback calls gadget_ep_free_request (calling kfree as a result)
>>>>>>>>> 4. Meanwhile, uvcg_video_enable has moved to uvc_video_free_requests which also
>>>>>>>>>   calls gadget_ep_free_request (calling kfree).
>>>>>>>>>
>>>>>>>>> There is currently (even in your patches) no synchronization between calls to
>>>>>>>>> gadget_ep_free_request via complete callback and uvcg_video_enable, which will
>>>>>>>>> inevitably call usb_ep_free_request twice for one request.
>>>>>>>>>
>>>>>>>>> Does that make sense, or am I misunderstanding some part of the patch?
>>>>>>>>
>>>>>>>> The overall concept is correct. But in detail the
>>>>>>>> uvc_video_free_requests is checking that video->ureq[i].req is not NULL.
>>>>>>>>
>>>>>>>> With our previous call of ep_free_request in the complete handler, the
>>>>>>>> ureq->req pointer in focus was already set to NULL. So the
>>>>>>>> uvc_video_free_requests function will skip that extra free.
>>>>>>>>
>>>>>>>
>>>>>>> Is there any form of synchronization between uvc_video_request and the
>>>>>>> complete callback? As I see it, the dwc3 interrupt thread and the v4l2
>>>>>>> ioctl thread (which calls uvcg_video_enable) are fully independent, so
>>>>>>> the calls made by them are free to be interleaved arbitrarily, so an
>>>>>>> interleaving like this is technically possible:
>>>>>>>
>>>>>>> +------+------------------------------------+---------------------------------------------+
>>>>>>> | time |            ioctl_thread            |            dwc3 interrupt handler           |
>>>>>>> +======+====================================+=============================================+
>>>>>>> |   1  | -uvc_v4l2_streamoff                |                                             |
>>>>>>> |   2  | |-uvcg_video_enable                |                                             |
>>>>>>> |   3  | ||-usb_ep_dequeue                  |                                             |
>>>>>>> |   4  | ||                                 | -dwc3_process_event_buf                     |
>>>>>>> |   5  | ||-uvc_video_free_requests         | |                                           |
>>>>>>> |   6  | |||                                | |-dwc3_gadget_ep_cleanup_cancelled_requests |
>>>>>>> |   7  | |||                                | ||-dwc3_gadget_giveback                     |
>>>>>>> |   8  | |||                                | |||-uvc_video_complete                      |
>>>>>>> |   9  | |||-check ureq->req != NULL [true] | ||||                                        |
>>>>>>> |  10  | ||||-usb_ep_free_request           | ||||                                        |
>>>>>>> |  11  | |||||-dwc3_ep_free_request         | ||||                                        |
>>>>>>> |  12  | ||||||-kfree [first call]          | ||||                                        |
>>>>>>> |  13  | ||||                               | ||||-usb_ep_free_request                    |
>>>>>>> |  14  | ||||                               | |||||-dwc3_ep_free_request                  |
>>>>>>> |  15  | ||||                               | ||||||-kfree [second call]                  |
>>>>>>> |  16  | ||||                               | ||||-set ureq->req = NULL                   |
>>>>>>> |  17  | ||||-set ureq->req = NULL          |                                             |
>>>>>>> +------+------------------------------------+---------------------------------------------+
>>>>>>>
>>>>>>> A situation like this means that dwc3_ep_free_request can be called
>>>>>>> twice for a particular usb_request. This is obviously low probability,
>>>>>>> but a race condition here means we'll start seeing very vague and hard
>>>>>>> to repro crashes or memory inconsistencies when using the uvc gadget.
>>>>>>>
>>>>>>> I do apologize if I've missed something obvious with your changes that
>>>>>>> prevents such interleaving. I don't currently see any locking or
>>>>>>> other synchronization mechanism in your changes. Is there something
>>>>>>> in dwc3 that prevents this situation?
>>>>>>
>>>>>> I think you have pointed it out totally clear. This is obviously the
>>>>>> case. It just did not trigger here. But the window is there and has to
>>>>>> be locked in some way.
>>>>>>
>>>>>> For now we have two options to solve it.
>>>>>>
>>>>>> 1) Trying to avoid this double code path of the complete callback and
>>>>>> uvc_video_free_requests. This is what your patches are already doing.
>>>>>>
>>>>>> But for now I am not so pleased with the timeout concept by waiting for
>>>>>> the complete interrupt to be called. This is also a shot in the dark as
>>>>>> the latency depends on the scheduler and the amount of potential
>>>>>> requests that are being handled.
>>>>>
>>>>> I agree, a timeout is not the most elegant of solutions and given a
>>>>> weird enough scheduler, will run into issues as well.
>>>>>
>>>>>>
>>>>>> 2) Locking both codepathes around the resource in question so the issue
>>>>>> is avoided.
>>>>>>
>>>>>> However, I am also not a fried of many locks.
>>>>>>
>>>>>> Perhaps it is possible to use a combination of wait_for_completion in
>>>>>> the uvc_video_free_requests and a complete callback in
>>>>>> uvc_video_complete for those requests that are not listed in the
>>>>>> req_free list.
>>>>>>
>>>>>> What do you think?
>>>>>>
>>>>> There might be a way that builds on your idea of cleaning up in the complete callback.
>>>>> It would rely on having a uvc_requests that aren't bulk allocated, which may have a
>>>>> performance impact.
>>>>
>>>> Since the allocation will only be done once, this performance impact is
>>>> should not be critical.
>>>>
>>>>> I am imagining something like the following:
>>>>>  1. Instead of allocating a bulk of uvc_requests, we allocate them
>>>>>     one at a time and add them to uvc_video.ureq
>>>>>  2. uvc_video.ureq becomes a list_head containing all the individual
>>>>>     requests
>>>>>  3. We add a sentinel flag in uvc_request that says the request is
>>>>>     now stale. This flag is protected by uvc_video->req_lock
>>>>>  4. uvc_video_complete looks at  this flag to deallocate both
>>>>>     usb_request and uvc_request.
>>>>>  5. uvcg_video_enable looks something like the following:
>>>>>       uvcg_video_enable(...) {
>>>>>         ...
>>>>>         lock(req_lock);
>>>>>         forall (uvc_requests->ureqs) {ureq->stale = true}
>>>>>         unlock(req_lock);
>>>>>         usb_ep_dequeue all reqs
>>>>>
>>>>>         uvc_video_free_requests(...)
>>>>>         ...
>>>>>       }
>>>>>  6. uvc_video_complete looks something like:
>>>>>       uvc_video_complete(...) {
>>>>>         // at the start
>>>>>         lock(req_lock)
>>>>>         is_stale = ureq->stale;
>>>>>         unlock(req_lock);
>>>>>
>>>>>         if (is_stale) {
>>>>>           usb_ep_free_request();
>>>>>           dealloc corresponding uvc_request();
>>>>>           return;
>>>>>         }
>>>>>
>>>>>         ...
>>>>>
>>>>>         lock(req_lock);
>>>>>         // possible that request became stale while we were handling stuff
>>>>>         if (!ureq->stale) {
>>>>>           list_add_tail(&req->list, &video->req_free);
>>>>>         } else {
>>>>>           usb_ep_free_request();
>>>>>           dealloc corresponding uvc_request();
>>>>>         }
>>>>>         unlock(req_lock);
>>>>>       }
>>>>>  7. uvc_video_free_requests can freely dealloc usb_requests/uvc_requests in
>>>>>     req_free because we can be certain that uvc_video_complete won't modify
>>>>>     it once requests have been marked stale, and the stale requests in flight
>>>>>     will be cleaned up by the complete callback.
>>>>>
>>>>> Effectively, we freeze the state of req_free before dequeuing, and all
>>>>> inflight requests are considered the responsibility of the complete handler
>>>>> from that point onwards. The gadget is only responsible for freeing requests it
>>>>> currently owns.
>>>>>
>>>>> I think this should ensure that we never have a situation where the ownership of the
>>>>> requests are undefined, and only one thread is responsible for freeing any given request.
>>>>>
>>>>> Hope that makes sense!
>>>>
>>>> So you found a way to secure this also with the already available
>>>> req_lock then. Nice!
>>>>
>>>> Also what you suggest is to move from the array model we currently have
>>>> to dynamic allocation in a linked list.
>>>>
>>>> I would suggest some more adaptions.
>>>>
>>>> Keep to allocate all requests dynamicaly as you suggest instead of the
>>>> bulk array.
>>>>
>>>> Rewrite the uvc_video_free_requests to iterate over the video->req_free
>>>> list instead of all available requests to take care of all requests
>>>> that are truely freed.
>>>>
>>>> Take this patch we started this thread with and expand it to
>>>> clean up not only the usb_request but also the uvc_request
>>>> like you suggested in your pseudo code.
>>>>
>>>> Since we check for UVC_STATE_CONNECTED already in the comlete handler
>>>> this is a superset of your stale flag anyway. And every request
>>>> that is currently in flight is not part of the req_free list, which
>>>> makes the uvc_video_free_requests function free to run without making
>>>> no harm.
>>>
>>> The downside of freeing based on UVC_STATE_CONNECTED and why it might be
>>> problematic is that without any other synchronization method, the complete
>>> callback can be arbitrarily delayed for a given usb_request.
>>>
>>> A STREAMOFF quickly followed by a STREAMON, might set uvc->state to
>>> UVC_STATE_STREAMING before the controller has had a chance to return the
>>> stale requests. This won't cause any functional issues AFAICT, but will
>>> cause a memory "leak" of sorts where every successive quick
>>> STREAMOFF-->STREAMON will lead to some extra usb_requests sticking around.
>>> They'll eventually get freed, but it doesn't seem very responsible to increase
>>> the memory load unless required.
>>>
>>> The stale flag ensures that this situation never happens and even if the
>>> complete callbacks comes back well after  the new STREAMON event, we correctly
>>> free the associated usb_request and uvc_request.
>>
>> In that case we could use stale then, but I would suggest also keeping
>> the change the functionality of uvc_video_free_requests aswell to loop
>> over the requests in req_free list.
>
>Agreed, uvc_video_free_requests should only free the requests in
>req_free.
>
>Just to clear any confusion: are you working on incorporating these changes
>into your patchset, or do you want me to include them in
>https://lore.kernel.org/20230912041910.726442-3-arakesh@google.com/
>instead?

As I am busy on a different topic at the moment, and you have suggested
the main walkthrough for the solution, it would be great if you could
come up with the proper patch.

But it would be great to find my Suggested-by in the patches. :)

Thanks!
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 2/3] usb: gadget: uvc: cleanup request when not in correct state

[Avichal Rakesh]

On 9/19/23 14:16, Michael Grzeschik wrote:
> On Tue, Sep 19, 2023 at 01:22:42PM -0700, Avichal Rakesh wrote:
>>
>>
>> On 9/19/23 13:07, Michael Grzeschik wrote:
>>> On Tue, Sep 19, 2023 at 12:55:02PM -0700, Avichal Rakesh wrote:
>>>> On 9/19/23 12:13, Michael Grzeschik wrote:
>>>>> On Mon, Sep 18, 2023 at 04:40:07PM -0700, Avichal Rakesh wrote:
>>>>>>
>>>>>>
>>>>>> On 9/18/23 14:43, Michael Grzeschik wrote:
>>>>>>> On Mon, Sep 18, 2023 at 12:02:11PM -0700, Avichal Rakesh wrote:
>>>>>>>> On 9/16/23 16:23, Michael Grzeschik wrote:
>>>>>>>>> On Fri, Sep 15, 2023 at 07:41:05PM -0700, Avichal Rakesh wrote:
>>>>>>>>>> On 9/15/23 16:32, Michael Grzeschik wrote:
>>>>>>>>>>> On Mon, Sep 11, 2023 at 09:52:22PM -0700, Avichal Rakesh wrote:
>>>>>>>>>>>> On 9/10/23 17:24, Michael Grzeschik wrote:
>>>>>>>>>>>>> The uvc_video_enable function of the uvc-gadget driver is dequeing and
>>>>>>>>>>>>> immediately deallocs all requests on its disable codepath. This is not
>>>>>>>>>>>>> save since the dequeue function is async and does not ensure that the
>>>>>>>>>>>>> requests are left unlinked in the controller driver.
>>>>>>>>>>>>>
>>>>>>>>>>>>> <snip>
>>
>> Agreed, uvc_video_free_requests should only free the requests in
>> req_free.
>>
>> Just to clear any confusion: are you working on incorporating these changes
>> into your patchset, or do you want me to include them in
>> https://lore.kernel.org/20230912041910.726442-3-arakesh@google.com/
>> instead?
> 
> As I am busy on a different topic at the moment, and you have suggested
> the main walkthrough for the solution, it would be great if you could
> come up with the proper patch.
> 
> But it would be great to find my Suggested-by in the patches. :)

Just sent out https://lore.kernel.org/20230920200335.63709-1-arakesh@google.com
with the changes discussed in this thread. The patch should work without
requiring any changes to dwc3. 

I didn't run into any crashes when testing the changes locally, but if you can,
I'd appreciate you testing the patches on your crash-prone setup as my setup's
crash rate was pretty low to begin with.

Thank you!

- Avi.