[Spca50x-devs] gspcav2-0.0.28 locking audit results

[Hans de Goede <j.w.r.degoede@xxxxxx>]

Hi Jef & All,

I've done a full audit of all locking in the gspcav2_main code, below is a 
summary of my findings, I'll be working on fixing all this the next couple of 
days as time permits, so expect a few big patches soonish.

In the mean time any feedback on my perceived problems, and suggested fixes it 
much welcome.

---

gspca_init_transfer
-> usb_lock interruptible
-> always called with queue_lock held


gspca_stream_off
-> usb_lock interruptible, no check, BAD!
-> always called with queue_lock held


vidioc_s_fmt_cap
-> queue_lock interruptible, no check, BAD!
-> if already streaming also takes usb lock through
    gspca_stream_off / gspca_init_transfer
-> stops / start the stream if already streaming, but in this case
    the frsz will most likely change, without reallocing the bufs, BAD!
    should return -EBUSY if there are already frames allocated (independent
    of streaming) instead.


dev_open
-> queue_lock + usb_lock, interruptible


dev_close
-> queue_lock + usb_lock through gspca_stream_off, interruptible, no check,
    BAD!
Then:
-> usb_lock, interruptible, no check, BAD!
Then:
-> queue_lock, interruptible, no check, BAD!


vidioc_s_ctrl
-> usb_lock, interruptible, no check, BAD!

vidioc_g_ctrl
-> usb_lock, interruptible, no check, BAD!
Note: the usb_lock isn't always needed here, should be left over to sd code


vidioc_reqbufs
-> queue_lock, interruptible
Note: only takes the lock after calculating the framesize, possible race
with vidioc_s_fmt_cap which can change the frame size


vidioc_querybuf
-> no locking
Should probably take the queuelock, as many queue operations can change the
buffers and since this does a memcpy of the bufferstate, this could cause a
race where it gets part of an old and parts of a new buffer state.


vidioc_streamon
-> queue_lock + usb_lock through gspca_init_transfer, interruptible


vidioc_streamoff
-> queue_lock + usb_lock through gspca_stream_off, interruptible, no check,
    BAD!


vidioc_g_jpegcomp
vidioc_s_jpegcomp
-> usb_lock, interruptible, no check, BAD!


vidioc_s_parm
-> usb_lock, why o why?, interruptible, no check, BAD!


dev_mmap
-> queue_lock, interruptible

dev_poll
-> queue_lock, interruptible


vidioc_dqbuf
-> read_lock, interruptible
BAD protects against multiple readers but not against other queue
manipilating functions such as stream on/off


vidioc_qbuf
-> queue_lock, interruptible
Note: lock should be moved up to also protect the checking for the nframes
and esp for checking if the memory types match.


dev_read
-> no locking, should have locking for checking nframes and streaming
(otherwise there could be potential races). Should perhaps also check
somehow if nframes > 0, if this were not allocated through other means
(application mixing read and mmap modes).


gspca_dev_probe
no locking, non needed, but should set gspca_dev->present to 1 before
registering the v4ldevice


gspca_disconnect
-calls gspca_kill_transfer which isn't needed as all urbs are killed on
  disconnect by the usb core, and if streaming the closing of the device will
  call gspca_kill_transfer freeing the urbs, does some locking surrounding
  gspca_kill_transfer, which is bad as its interruptible, but doesn't check
  returns
-possible race with open between the "while (gspca_dev->users != 0) { ... }
  and the "video_unregister_device(&gspca_dev->vdev);", yes the window is
  small but it is there. This can be fixed by:
  1) not waiting for users at all, instead make gspca_dev refcounted,
     ref up in open, down in close and disconnect, free if ref hits zero
  2) check gspca_dev->present everywhere
  3) disconnect now just becomes a set gspca_dev->present to 0,
     take and release all locks (to make sure everyone has seen that
     gspca_dev->present has become 0), deref and a unregister v4ldev.
  4) using a global mutex to protect the refcount from open/close/disconnect
     races, this must be global so that it can be checked in open while
     racing against disconnect, without checking a mutex in potentially freed
     memory, see the comments explaining this in my usbvideo2 "core" code
  Note: the above requires a v4l2_ioctl wrapper which checks for
  gspca_dev->present before calling v4l2_ioctl, otherwise v4l2_ioctl could be
  called on a unregistered v4ldev, making it rather unhappy! Again for all the
  details see my usbvideo2 "core" code.

  Fixing things this way has the added advantage that a quick disconnect /
  connect due to an usb glitch no longer changes the /dev/videoX number, as
  /dev/videoX gets deregistered immediately rather then waiting for any using
  apps to close it.


misc
-> a struct module * must be added to struct sd_desc, and then its
    usage count must be increased in dev_open, an decreased in dec_close,
    currently someone can rmmod gspca_driver, while /dev/videoX is open!


None lock related notes:
1) The framequeues should be cleared (all buffers back to user state) upon
    a stream off.

2) gspca_init_transfer should call gspca_stream_off rather then
    kill_transfert on error so that the stream also gets stopped at the cam
    side

Regards,

Hans


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
_______________________________________________
Spca50x-devs mailing list
Spca50x-devs@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/spca50x-devs