Here are the patches https://patchwork.linuxtv.org/patch/20668/ https://patchwork.linuxtv.org/patch/20669/ Thanks! On Wed, Nov 6, 2013 at 9:26 AM, Ricardo Ribalda Delgado <ricardo.ribalda@xxxxxxxxx> wrote: > Hello Hans and Sywester > > I have just posted a new patch. I think it fits the suggestions from > both of you, please take a look to it and please post any comment. > > I will also send a patch about the em28xx, to swap the lock order. > > Thanks for your comments > > On Mon, Nov 4, 2013 at 4:19 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote: >> On 11/04/2013 03:24 PM, Sylwester Nawrocki wrote: >>> On 04/11/13 15:12, Hans Verkuil wrote: >>>> On 11/04/2013 02:54 PM, Ricardo Ribalda Delgado wrote: >>>>>> Hello Hans >>>>>> >>>>>> Thanks for your comments. >>>>>> >>>>>> Please take a look to v4 of this patch >>>>>> https://patchwork.linuxtv.org/patch/20529/ >>>>>> >>>>>> On Mon, Nov 4, 2013 at 1:37 PM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote: >>>>>>>> On 11/02/2013 10:53 AM, Ricardo Ribalda Delgado wrote: >>>>>>>>>> From: Ricardo Ribalda <ricardo.ribalda@xxxxxxxxx> >>>>>>>>>> >>>>>>>>>> vb2_fop_relase does not held the lock although it is modifying the >>>>>>>>>> queue->owner field. >>>>>>>>>> >>>>>>>>>> This could lead to race conditions on the vb2_perform_io function >>>>>>>>>> when multiple applications are accessing the video device via >>>>>>>>>> read/write API: >>>>>>>> >>>>>>>> It's also called directly by drivers/media/usb/em28xx/em28xx-video.c! >>>>>>>> >>>>>> >>>>>> em28xx-video does not hold the lock, therefore it can call the normal >>>>>> function. On v2 we made a internal function that should be called if >>>>>> the funciton is called directly by the driver. Please take a look to >>>>>> the old comments. https://patchwork.linuxtv.org/patch/20460/ >>>> >>>> static int em28xx_v4l2_close(struct file *filp) >>>> { >>>> struct em28xx_fh *fh = filp->private_data; >>>> struct em28xx *dev = fh->dev; >>>> int errCode; >>>> >>>> em28xx_videodbg("users=%d\n", dev->users); >>>> >>>> mutex_lock(&dev->lock); >>>> vb2_fop_release(filp); >>>> ... >>>> >>>> vb2_fop_release(filp) will, with your patch, also try to get dev->lock. >>>> >>>> Sylwester's comment re em28xx is incorrect. >>> >>> dev->lock is not used as the video queue lock: >>> >>> $ git grep "lock =" drivers/media/usb/em28xx/ >>> ... >>> drivers/media/usb/em28xx/em28xx-video.c: dev->vdev->queue->lock = &dev->vb_queue_lock; >>> drivers/media/usb/em28xx/em28xx-video.c: dev->vbi_dev->queue->lock = &dev->vb_vbi_queue_lock; >>> >>> There is a separate mutex for the video queue which needs to be acquired >>> independently. >> >> Darn, I missed that one. I was looking for it in em28xx_vdev_init(), which is >> where I expected the queue->lock to be set, if there was any. >> >> That said, wouldn't it be a good idea to swap the order: >> >> vb2_fop_release(filp); >> mutex_lock(&dev->lock); >> >> I don't believe there is a good reason for nesting mutexes here. >> >> Regards, >> >> Hans >> >>> >>> -- >>> Regards, >>> Sylwester >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-media" in >>> the body of a message to majordomo@xxxxxxxxxxxxxxx >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >> > > > > -- > Ricardo Ribalda -- Ricardo Ribalda -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
