| Message ID | 20200830150443.167286-5-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: uvcvideo: Fix race conditions | expand |
Hi Guenter,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v5.9-rc3 next-20200828]
[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]
url: https://github.com/0day-ci/linux/commits/Guenter-Roeck/media-uvcvideo-Fix-race-conditions/20200830-230715
base: git://linuxtv.org/media_tree.git master
config: x86_64-randconfig-s031-20200901 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.2-191-g10164920-dirty
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/media/usb/uvc/uvc_queue.c:402:21: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __poll_t [usertype] ret @@ got int @@
>> drivers/media/usb/uvc/uvc_queue.c:402:21: sparse: expected restricted __poll_t [usertype] ret
>> drivers/media/usb/uvc/uvc_queue.c:402:21: sparse: got int
# https://github.com/0day-ci/linux/commit/9128827e77bc077a6fefd3ca886a8466e84f7154
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Guenter-Roeck/media-uvcvideo-Fix-race-conditions/20200830-230715
git checkout 9128827e77bc077a6fefd3ca886a8466e84f7154
vim +402 drivers/media/usb/uvc/uvc_queue.c
393
394 __poll_t uvc_queue_poll(struct uvc_video_queue *queue, struct file *file,
395 poll_table *wait)
396 {
397 struct uvc_streaming *stream = uvc_queue_to_stream(queue);
398 __poll_t ret;
399
400 mutex_lock(&queue->mutex);
401 if (!video_is_registered(&stream->vdev)) {
> 402 ret = -ENODEV;
403 goto unlock;
404 }
405 ret = vb2_poll(&queue->queue, file, wait);
406 unlock:
407 mutex_unlock(&queue->mutex);
408
409 return ret;
410 }
411
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 9/1/20 9:51 AM, kernel test robot wrote: > Hi Guenter, > > I love your patch! Perhaps something to improve: > > [auto build test WARNING on linuxtv-media/master] > [also build test WARNING on v5.9-rc3 next-20200828] > [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] > > url: https://github.com/0day-ci/linux/commits/Guenter-Roeck/media-uvcvideo-Fix-race-conditions/20200830-230715 > base: git://linuxtv.org/media_tree.git master > config: x86_64-randconfig-s031-20200901 (attached as .config) > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 > reproduce: > # apt-get install sparse > # sparse version: v0.6.2-191-g10164920-dirty > # save the attached .config to linux build tree > make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > > > sparse warnings: (new ones prefixed by >>) > >>> drivers/media/usb/uvc/uvc_queue.c:402:21: sparse: sparse: incorrect type in assignment (different base types) @@ expected restricted __poll_t [usertype] ret @@ got int @@ >>> drivers/media/usb/uvc/uvc_queue.c:402:21: sparse: expected restricted __poll_t [usertype] ret >>> drivers/media/usb/uvc/uvc_queue.c:402:21: sparse: got int > > # https://github.com/0day-ci/linux/commit/9128827e77bc077a6fefd3ca886a8466e84f7154 > git remote add linux-review https://github.com/0day-ci/linux > git fetch --no-tags linux-review Guenter-Roeck/media-uvcvideo-Fix-race-conditions/20200830-230715 > git checkout 9128827e77bc077a6fefd3ca886a8466e84f7154 > vim +402 drivers/media/usb/uvc/uvc_queue.c > > 393 > 394 __poll_t uvc_queue_poll(struct uvc_video_queue *queue, struct file *file, > 395 poll_table *wait) > 396 { > 397 struct uvc_streaming *stream = uvc_queue_to_stream(queue); > 398 __poll_t ret; > 399 > 400 mutex_lock(&queue->mutex); > 401 if (!video_is_registered(&stream->vdev)) { > > 402 ret = -ENODEV; Looks like this has to return EPOLLERR. I'll make that change in v2. Guenter > 403 goto unlock; > 404 } > 405 ret = vb2_poll(&queue->queue, file, wait); > 406 unlock: > 407 mutex_unlock(&queue->mutex); > 408 > 409 return ret; > 410 } > 411 > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org >
diff --git a/drivers/media/usb/uvc/uvc_queue.c b/drivers/media/usb/uvc/uvc_queue.c index cd60c6c1749e..c7964177a99d 100644 --- a/drivers/media/usb/uvc/uvc_queue.c +++ b/drivers/media/usb/uvc/uvc_queue.c @@ -358,24 +358,52 @@ 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) { - return vb2_mmap(&queue->queue, vma); + struct uvc_streaming *stream = uvc_queue_to_stream(queue); + int ret; + + mutex_lock(&queue->mutex); + if (!video_is_registered(&stream->vdev)) { + ret = -ENODEV; + goto unlock; + } + ret = vb2_mmap(&queue->queue, vma); +unlock: + mutex_unlock(&queue->mutex); + return ret; } #ifndef CONFIG_MMU unsigned long uvc_queue_get_unmapped_area(struct uvc_video_queue *queue, unsigned long pgoff) { - return vb2_get_unmapped_area(&queue->queue, 0, 0, pgoff, 0); + struct uvc_streaming *stream = uvc_queue_to_stream(queue); + unsigned long ret; + + mutex_lock(&queue->mutex); + if (!video_is_registered(&stream->vdev)) { + ret = -ENODEV; + goto unlock; + } + ret = vb2_get_unmapped_area(&queue->queue, 0, 0, pgoff, 0); +unlock: + mutex_unlock(&queue->mutex); + return ret; } #endif __poll_t uvc_queue_poll(struct uvc_video_queue *queue, struct file *file, poll_table *wait) { + struct uvc_streaming *stream = uvc_queue_to_stream(queue); __poll_t ret; mutex_lock(&queue->mutex); + if (!video_is_registered(&stream->vdev)) { + ret = -ENODEV; + goto unlock; + } ret = vb2_poll(&queue->queue, file, wait); +unlock: mutex_unlock(&queue->mutex); return ret;
uvc queue file operations have no mutex protection against USB disconnect. This is questionable at least for the poll operation, which has to wait for the uvc queue mutex. By the time that mutex has been acquired, is it possible that the video device has been unregistered. Protect all file operations by using the queue mutex to avoid possible race conditions. After acquiring the mutex, check if the video device is still registered, and bail out if not. Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Alan Stern <stern@rowland.harvard.edu> Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl> Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- drivers/media/usb/uvc/uvc_queue.c | 32 +++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-)