videodev fixups / generic usercopy helper

PATCH New Gerd Knorr 1 participant 1 message Mar 18, 2002

Subsystems (2)

Media (V4L/DVB) USB

lore

Share & tools

lore

Gerd Knorr

author

2002-03-18 10:50 UTC #0 lore raw

PATCH 1/1 videodev fixups / generic usercopy helper

Hi,

I've just noticed that a hole left in the recent changes which should allow the usb v4l drivers to unregister with open file handles. The drivers itself handle it just fine, but video_generic_ioctl() will barf when called on unregistered devices. Oops.

One way to fix this is to expect drivers call the helper function and pass a pointer for the function doing the actual work, i.e. handle it this way:

driver_ioctl(inode,file,cmd,userptr)

	-> video_usercopy(inode,file,cmd,userptr,func)
		copy_from_user(...)
		-> func(inode,file,cmd,kernelptr);
		copy_to_user(...)

Patch against 2.5.7-pre2 below. It updates videodev.[ch] and adapts usbvideo.c to show how the driver changes will look like.

Note that this change makes the usercopy helper function a very generic one, it probably could be used for other drivers to (as long as the API has sane magic numbers based on _IO*(...) defines) as there is no video4linux-related stuff in there any more. So we might think of renaming it an moving it to some more central place (fs/ioctl.c maybe).

Comments?

Gerd

==============================[ cut here ]============================== ===== drivers/media/video/videodev.c 1.11 vs edited =====

drivers/media/video/videodev.c +7 -9

@@ -107,20 +107,18 @@
 }
 
 /*
- * ioctl helper function -- handles userspace copying
+ * helper function -- handles userspace copying for ioctl arguments
  */
 int
-video_generic_ioctl(struct inode *inode, struct file *file,
-		    unsigned int cmd, unsigned long arg)
+video_usercopy(struct inode *inode, struct file *file,
+	       unsigned int cmd, unsigned long arg,
+	       int (*func)(struct inode *inode, struct file *file,
+			   unsigned int cmd, void *arg))
 {
-	struct  video_device *vfl = video_devdata(file);
 	char	sbuf[128];
 	void    *mbuf = NULL;
 	void	*parg = NULL;
 	int	err  = -EINVAL;
-	
-	if (vfl->kernel_ioctl == NULL)
-		return -EINVAL;
 
 	/*  Copy arguments into temp kernel buffer  */
 	switch (_IOC_DIR(cmd)) {

@@ -147,7 +145,7 @@
 	}
 
 	/* call driver */
-	err = vfl->kernel_ioctl(inode, file, cmd, parg);
+	err = func(inode, file, cmd, parg);
 	if (err == -ENOIOCTLCMD)
 		err = -EINVAL;
 	if (err < 0)

@@ -512,7 +510,7 @@
 EXPORT_SYMBOL(video_register_device);
 EXPORT_SYMBOL(video_unregister_device);
 EXPORT_SYMBOL(video_devdata);
-EXPORT_SYMBOL(video_generic_ioctl);
+EXPORT_SYMBOL(video_usercopy);
 EXPORT_SYMBOL(video_exclusive_open);
 EXPORT_SYMBOL(video_exclusive_release);

===== drivers/usb/usbvideo.c 1.13 vs edited =====

drivers/usb/usbvideo.c +9 -4

@@ -1020,7 +1020,7 @@
 	release:  usbvideo_v4l_close,
 	read:     usbvideo_v4l_read,
 	mmap:     usbvideo_v4l_mmap,
-	ioctl:    video_generic_ioctl,
+	ioctl:    usbvideo_v4l_ioctl,
 	llseek:   no_llseek,
 };
 static struct video_device usbvideo_template = {

@@ -1028,7 +1028,6 @@
 	type:         VID_TYPE_CAPTURE,
 	hardware:     VID_HARDWARE_CPIA,
 	fops:         &usbvideo_fops,
-	kernel_ioctl: usbvideo_v4l_ioctl,
 };
 
 uvd_t *usbvideo_AllocateDevice(usbvideo_t *cams)

@@ -1349,8 +1348,8 @@
  * History:
  * 22-Jan-2000 Corrected VIDIOCSPICT to reject unsupported settings.
  */
-int usbvideo_v4l_ioctl(struct inode *inode, struct file *file,
-		       unsigned int cmd, void *arg)
+static int usbvideo_v4l_do_ioctl(struct inode *inode, struct file *file,
+				 unsigned int cmd, void *arg)
 {
 	uvd_t *uvd = file->private_data;

@@ -1553,6 +1552,12 @@
 			return -ENOIOCTLCMD;
 	}
 	return 0;
+}
+
+int usbvideo_v4l_ioctl(struct inode *inode, struct file *file,
+		       unsigned int cmd, unsigned long arg)
+{
+	return video_usercopy(inode, file, cmd, arg, usbvideo_v4l_do_ioctl);
 }
 
 /*

===== drivers/usb/usbvideo.h 1.8 vs edited =====

drivers/usb/usbvideo.h +1 -1

@@ -350,7 +350,7 @@
 int usbvideo_v4l_close(struct inode *inode, struct file *file);
 int usbvideo_v4l_initialize(struct video_device *dev);
 int usbvideo_v4l_ioctl(struct inode *inode, struct file *file,
-		       unsigned int ioctlnr, void *arg);
+		       unsigned int cmd, unsigned long arg);
 int usbvideo_v4l_mmap(struct file *file, struct vm_area_struct *vma);
 int usbvideo_v4l_open(struct inode *inode, struct file *file);
 int usbvideo_v4l_read(struct file *file, char *buf,

===== include/linux/videodev.h 1.12 vs edited =====

include/linux/videodev.h +4 -4

@@ -37,8 +37,6 @@
  	 * video_generic_ioctl() does the userspace copying of the
  	 * ioctl arguments */
  	struct file_operations *fops;
- 	int (*kernel_ioctl)(struct inode *inode, struct file *file,
- 			    unsigned int cmd, void *arg);
 	void *priv;		/* Used to be 'private' but that upsets C++ */
 
 	/* for videodev.c intenal usage -- don't touch */

@@ -60,8 +58,10 @@
 
 extern int video_exclusive_open(struct inode *inode, struct file *file);
 extern int video_exclusive_release(struct inode *inode, struct file *file);
-extern int video_generic_ioctl(struct inode *inode, struct file *file,
-			       unsigned int cmd, unsigned long arg);
+extern int video_usercopy(struct inode *inode, struct file *file,
+			  unsigned int cmd, unsigned long arg,
+			  int (*func)(struct inode *inode, struct file *file,
+				      unsigned int cmd, void *arg));
 #endif /* __KERNEL__ */
 
 #define VID_TYPE_CAPTURE	1	/* Can capture */