Messages in this thread | | | Date | Sat, 19 Oct 2013 12:51:08 +0200 | From | Sylwester Nawrocki <> | Subject | Re: [PATCH] videobuf2: Add missing lock held on vb2_fop_relase |
| |
On 10/19/2013 12:22 PM, Ricardo Ribalda Delgado wrote: > On Sat, Oct 19, 2013 at 11:55 AM, Sylwester Nawrocki > <sylvester.nawrocki@gmail.com> wrote: >> > On 10/14/2013 09:41 AM, Ricardo Ribalda Delgado wrote: >>> >> >>> >> vb2_fop_relase does not held the lock although it is modifying the >>> >> queue->owner field. >> > [...] >>> >> diff --git a/drivers/media/v4l2-core/videobuf2-core.c >>> >> b/drivers/media/v4l2-core/videobuf2-core.c >>> >> index 9fc4bab..3a961ee 100644 >>> >> --- a/drivers/media/v4l2-core/videobuf2-core.c >>> >> +++ b/drivers/media/v4l2-core/videobuf2-core.c >>> >> @@ -2588,8 +2588,15 @@ int vb2_fop_release(struct file *file) >>> >> struct video_device *vdev = video_devdata(file); >>> >> >>> >> if (file->private_data == vdev->queue->owner) { >>> >> + struct mutex *lock; >>> >> + >>> >> + lock = vdev->queue->lock ? vdev->queue->lock : vdev->lock; >>> >> + if (lock) >>> >> + mutex_lock(lock); >>> >> vb2_queue_release(vdev->queue); >>> >> vdev->queue->owner = NULL; >>> >> + if (lock) >>> >> + mutex_unlock(lock); >>> >> } >>> >> return v4l2_fh_release(file); >>> >> } >> > >> > >> > It seems you didn't inspect all users of vb2_fop_release(). There are 3 >> > drivers >> > that don't assign vb2_fop_release() to struct v4l2_file_operations directly >> > but >> > instead call it from within its own release() handler. Two of them do call >> > vb2_fop_release() with the video queue lock already held. >> > >> > $ git grep -n vb2_fop_rel -- drivers/media/ >> > >> > drivers/media/platform/exynos4-is/fimc-capture.c:552: ret = >> > vb2_fop_release(file); >> > drivers/media/platform/exynos4-is/fimc-lite.c:549: vb2_fop_release(file); >> > > > Very good catch, thanks! > >> > A rather ugly solution would be to open code the vb2_fop_release() function >> > in those driver, like in below patch (untested). Unless there are better >> > proposals I would queue the patch as below together with the $subject patch >> > upstream. > > IMHO this will lead to the same type of mistakes in the future. > > What about creating a function __vb2_fop_release that does exactly > the same as the original function but with an extra parameter bool > lock_held > > vb2_fop_release will be a wrapper for that funtion with lock_held== false
Hmm, the parameter would be telling whether the lock is already held ? Perhaps we should inverse its meaning and it should indicate whether vb2_fop_release() should be taking the lock internally ? It seems to me more straightforward.
> drivers that overload the fop_release and need to hold the lock will > call the __ function with lock_held= true > > What do you think?
I was also considering this, it's probably better. I'm not sure about exporting functions prefixed with __ though. And the locking becomes less clear with such functions proliferation.
Anyway, I'm in general personally OK with having an additional version like:
__vb2_fop_release(struct file *filp, bool lock);
Regards, Sylwester
| |