[PATCH] uvcvideo: Don't call vb2 mmap and get_unmapped_area with queue lock held

[PATCH] uvcvideo: Don't call vb2 mmap and get_unmapped_area with queue lock held

[Laurent Pinchart]

videobuf2 has long been subject to AB-BA style deadlocks due to the
queue lock and mmap_sem being taken in different orders for the mmap and
get_unmapped_area operations. The problem has been fixed by making those
two operations callable without taking the queue lock, using an
mmap_lock internal to videobuf2.

The uvcvideo driver still calls the mmap and get_unmapped_area
operations with the queue lock held, resulting in a potential deadlock.
As the operations can now be called without locking the queue, fix it.

Reported-by: Bjørn Mork <bjorn@mork.no>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/usb/uvc/uvc_queue.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

Bjørn, does this fix the circular locking dependency you have reported in
"[v3.19-rc7] possible circular locking dependency in uvc_queue_streamoff" ?
The report mentions involves locks, so I'm not 100% this patch will fix the
issue.

diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c
index 10c554e..87a19f3 100644
--- a/drivers/media/usb/uvc/uvc_queue.c
+++ b/drivers/media/usb/uvc/uvc_queue.c
@@ -306,25 +306,14 @@ int uvc_queue_streamoff(struct uvc_video_queue *queue, enum v4l2_buf_type type)
 
 int uvc_queue_mmap(struct uvc_video_queue *queue, struct vm_area_struct *vma)
 {
-	int ret;
-
-	mutex_lock(&queue->mutex);
-	ret = vb2_mmap(&queue->queue, vma);
-	mutex_unlock(&queue->mutex);
-
-	return ret;
+	return vb2_mmap(&queue->queue, vma);
 }
 
 #ifndef CONFIG_MMU
 unsigned long uvc_queue_get_unmapped_area(struct uvc_video_queue *queue,
 		unsigned long pgoff)
 {
-	unsigned long ret;
-
-	mutex_lock(&queue->mutex);
-	ret = vb2_get_unmapped_area(&queue->queue, 0, 0, pgoff, 0);
-	mutex_unlock(&queue->mutex);
-	return ret;
+	return vb2_get_unmapped_area(&queue->queue, 0, 0, pgoff, 0);
 }
 #endif
 
-- 
Regards,

Laurent Pinchart

Re: [PATCH] uvcvideo: Don't call vb2 mmap and get_unmapped_area with queue lock held

[Bjørn Mork]

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> Bjørn, does this fix the circular locking dependency you have reported in
> "[v3.19-rc7] possible circular locking dependency in uvc_queue_streamoff" ?
> The report mentions involves locks, so I'm not 100% this patch will fix the
> issue.

Sorry, I forgot all about that report after firing it off...  Should
have followed it up with some more details.

Grepping my logs now I cannot find this warning at all after the one I
reported.  I see it once before (while running 3.19-rc6).  So it is
definitely not easily reproducible.  And I have a bad feeling the
trigger might involve completely unrelated USB issues...

In any case, thanks for the patch.  I will test it for a while and let
you know if the same warning shows ut with it.  But based on the rare
occurence, I don't think I ever will be able to positively confirm that
the warning is gone.


Bjørn

Re: [PATCH] uvcvideo: Don't call vb2 mmap and get_unmapped_area with queue lock held

[Bjørn Mork]

Bjørn Mork <bjorn@mork.no> writes:
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
>
>> Bjørn, does this fix the circular locking dependency you have reported in
>> "[v3.19-rc7] possible circular locking dependency in uvc_queue_streamoff" ?
>> The report mentions involves locks, so I'm not 100% this patch will fix the
>> issue.
>
> Sorry, I forgot all about that report after firing it off...  Should
> have followed it up with some more details.
>
> Grepping my logs now I cannot find this warning at all after the one I
> reported.  I see it once before (while running 3.19-rc6).  So it is
> definitely not easily reproducible.  And I have a bad feeling the
> trigger might involve completely unrelated USB issues...
>
> In any case, thanks for the patch.  I will test it for a while and let
> you know if the same warning shows ut with it.  But based on the rare
> occurence, I don't think I ever will be able to positively confirm that
> the warning is gone.

FWIW, I have not seen the warning after applying this patch, so it
appears to fix the problem.  Thanks.

If I'm wrong, then I'm sure Murphy will tell us as soon as I send this
email :-)


Bjørn

Re: [PATCH] uvcvideo: Don't call vb2 mmap and get_unmapped_area with queue lock held

[Laurent Pinchart]

Hi Bjørn,

(it took me half an hour to figure out how to write ø on my keyboard :-))

On Monday 09 March 2015 12:06:36 Bjørn Mork wrote:
> Bjørn Mork <bjorn@mork.no> writes:
> > Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> >> Bjørn, does this fix the circular locking dependency you have reported in
> >> "[v3.19-rc7] possible circular locking dependency in uvc_queue_streamoff"
> >> ? The report mentions involves locks, so I'm not 100% this patch will fix
> >> the issue.
> > 
> > Sorry, I forgot all about that report after firing it off...  Should
> > have followed it up with some more details.
> > 
> > Grepping my logs now I cannot find this warning at all after the one I
> > reported.  I see it once before (while running 3.19-rc6).  So it is
> > definitely not easily reproducible.  And I have a bad feeling the
> > trigger might involve completely unrelated USB issues...
> > 
> > In any case, thanks for the patch.  I will test it for a while and let
> > you know if the same warning shows ut with it.  But based on the rare
> > occurence, I don't think I ever will be able to positively confirm that
> > the warning is gone.
> 
> FWIW, I have not seen the warning after applying this patch, so it
> appears to fix the problem.  Thanks.

You're welcome.

> If I'm wrong, then I'm sure Murphy will tell us as soon as I send this
> email :-)

I'd be happy to prove Murphy wrong for once.

-- 
Regards,

Laurent Pinchart