> Excellent work. I have no complaints, just a few questions: > > 1. Would it be better to memset the temp buffer in video_generic_ioctl() > rather than in the driver? I've seen so many drivers forget to do this, > and it's a potential (albeit very small) security hole. The wrapper fills the buffer using copy_from_user() -- even for _IOR ioctls because some are labeled wrong -- and the driver needs that data. I can't zero the buffer ... > 2. In skeleton_open(), couldn't the device_data lookup code be replaced > with: > > struct video_device *vdev = video_devdata(file); > struct device_data *dev = vdev->priv; Good point. Yes, that should work. > 3. In skeleton_initdev(), shouldn't... > > dev->vdev = skeleton_template; > > ...be... > > memcpy(&dev->vdev, &skeleton_template, sizeof(skeleton_template); No. It does the same. > 4. Is it safe to keep even 128 bytes on the stack in > video_generic_ioctl()? Consider that devices might spend a relatively > long time blocking on VIDIOCSYNC. With 32 devices in use at once, you'd > be coming dangerously close to a stack overflow. I don't see a overflow can easily happen here. There is one kernel strack _per process_. Calling schedule() will also switch the stack. > IMHO it would be better > to only allocate as much as MCAPTURE and SYNC need, and fall back on > kmalloc for the less time-critical ones (if necessary). struct v4l2_buffer should fit onto the stack too. Gerd -- #define ENOCLUE 125 /* userland programmer induced race condition */
