Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

[media] uvcvideo: Fix open/close race condition

Maintaining the users count using an atomic variable makes sure that
access to the counter won't be racy, but doesn't serialize access to the
operations protected by the counter. This creates a race condition that
could result in the status URB being submitted multiple times.
Use a mutex to protect the users count and serialize access to the
status start and stop operations.

Reported-by: Shawn Nematbakhsh <shawnn@chromium.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

authored by

Laurent Pinchart and committed by
Mauro Carvalho Chehab
17706f56 c2a273b2

+32 -33
+17 -6
drivers/media/usb/uvc/uvc_driver.c
··· 1836 1836 INIT_LIST_HEAD(&dev->chains); 1837 1837 INIT_LIST_HEAD(&dev->streams); 1838 1838 atomic_set(&dev->nstreams, 0); 1839 - atomic_set(&dev->users, 0); 1840 1839 atomic_set(&dev->nmappings, 0); 1840 + mutex_init(&dev->lock); 1841 1841 1842 1842 dev->udev = usb_get_dev(udev); 1843 1843 dev->intf = usb_get_intf(intf); ··· 1950 1950 1951 1951 /* Controls are cached on the fly so they don't need to be saved. */ 1952 1952 if (intf->cur_altsetting->desc.bInterfaceSubClass == 1953 - UVC_SC_VIDEOCONTROL) 1954 - return uvc_status_suspend(dev); 1953 + UVC_SC_VIDEOCONTROL) { 1954 + mutex_lock(&dev->lock); 1955 + if (dev->users) 1956 + uvc_status_stop(dev); 1957 + mutex_unlock(&dev->lock); 1958 + return 0; 1959 + } 1955 1960 1956 1961 list_for_each_entry(stream, &dev->streams, list) { 1957 1962 if (stream->intf == intf) ··· 1978 1973 1979 1974 if (intf->cur_altsetting->desc.bInterfaceSubClass == 1980 1975 UVC_SC_VIDEOCONTROL) { 1981 - if (reset) { 1982 - int ret = uvc_ctrl_resume_device(dev); 1976 + int ret = 0; 1983 1977 1978 + if (reset) { 1979 + ret = uvc_ctrl_resume_device(dev); 1984 1980 if (ret < 0) 1985 1981 return ret; 1986 1982 } 1987 1983 1988 - return uvc_status_resume(dev); 1984 + mutex_lock(&dev->lock); 1985 + if (dev->users) 1986 + ret = uvc_status_start(dev, GFP_NOIO); 1987 + mutex_unlock(&dev->lock); 1988 + 1989 + return ret; 1989 1990 } 1990 1991 1991 1992 list_for_each_entry(stream, &dev->streams, list) {
+2 -19
drivers/media/usb/uvc/uvc_status.c
··· 206 206 uvc_input_cleanup(dev); 207 207 } 208 208 209 - int uvc_status_start(struct uvc_device *dev) 209 + int uvc_status_start(struct uvc_device *dev, gfp_t flags) 210 210 { 211 211 if (dev->int_urb == NULL) 212 212 return 0; 213 213 214 - return usb_submit_urb(dev->int_urb, GFP_KERNEL); 214 + return usb_submit_urb(dev->int_urb, flags); 215 215 } 216 216 217 217 void uvc_status_stop(struct uvc_device *dev) 218 218 { 219 219 usb_kill_urb(dev->int_urb); 220 220 } 221 - 222 - int uvc_status_suspend(struct uvc_device *dev) 223 - { 224 - if (atomic_read(&dev->users)) 225 - usb_kill_urb(dev->int_urb); 226 - 227 - return 0; 228 - } 229 - 230 - int uvc_status_resume(struct uvc_device *dev) 231 - { 232 - if (dev->int_urb == NULL || atomic_read(&dev->users) == 0) 233 - return 0; 234 - 235 - return usb_submit_urb(dev->int_urb, GFP_NOIO); 236 - } 237 -
+10 -4
drivers/media/usb/uvc/uvc_v4l2.c
··· 498 498 return -ENOMEM; 499 499 } 500 500 501 - if (atomic_inc_return(&stream->dev->users) == 1) { 502 - ret = uvc_status_start(stream->dev); 501 + mutex_lock(&stream->dev->lock); 502 + if (stream->dev->users == 0) { 503 + ret = uvc_status_start(stream->dev, GFP_KERNEL); 503 504 if (ret < 0) { 504 - atomic_dec(&stream->dev->users); 505 + mutex_unlock(&stream->dev->lock); 505 506 usb_autopm_put_interface(stream->dev->intf); 506 507 kfree(handle); 507 508 return ret; 508 509 } 509 510 } 511 + 512 + stream->dev->users++; 513 + mutex_unlock(&stream->dev->lock); 510 514 511 515 v4l2_fh_init(&handle->vfh, stream->vdev); 512 516 v4l2_fh_add(&handle->vfh); ··· 542 538 kfree(handle); 543 539 file->private_data = NULL; 544 540 545 - if (atomic_dec_return(&stream->dev->users) == 0) 541 + mutex_lock(&stream->dev->lock); 542 + if (--stream->dev->users == 0) 546 543 uvc_status_stop(stream->dev); 544 + mutex_unlock(&stream->dev->lock); 547 545 548 546 usb_autopm_put_interface(stream->dev->intf); 549 547 return 0;
+3 -4
drivers/media/usb/uvc/uvcvideo.h
··· 514 514 char name[32]; 515 515 516 516 enum uvc_device_state state; 517 - atomic_t users; 517 + struct mutex lock; /* Protects users */ 518 + unsigned int users; 518 519 atomic_t nmappings; 519 520 520 521 /* Video control interface */ ··· 661 660 /* Status */ 662 661 extern int uvc_status_init(struct uvc_device *dev); 663 662 extern void uvc_status_cleanup(struct uvc_device *dev); 664 - extern int uvc_status_start(struct uvc_device *dev); 663 + extern int uvc_status_start(struct uvc_device *dev, gfp_t flags); 665 664 extern void uvc_status_stop(struct uvc_device *dev); 666 - extern int uvc_status_suspend(struct uvc_device *dev); 667 - extern int uvc_status_resume(struct uvc_device *dev); 668 665 669 666 /* Controls */ 670 667 extern const struct v4l2_subscribed_event_ops uvc_ctrl_sub_ev_ops;