Web lists-archives.com

[Spca50x-devs] gspcav2-0.0.28 locking audit results

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.


-> usb_lock interruptible
-> always called with queue_lock held

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

-> 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.

-> queue_lock + usb_lock, interruptible

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

-> usb_lock, interruptible, no check, BAD!

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

-> 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

-> 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.

-> queue_lock + usb_lock through gspca_init_transfer, interruptible

-> queue_lock + usb_lock through gspca_stream_off, interruptible, no check,

-> usb_lock, interruptible, no check, BAD!

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

-> queue_lock, interruptible

-> queue_lock, interruptible

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

-> 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.

-> 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).

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

-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
-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.

-> 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



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. 
Spca50x-devs mailing list