> 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 */ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Fri Feb 15 2002 - 21:00:32 EST