[2.5] generic_usercopy() function (resend, forgot the patches)

RFC In review Michael Hunold 6 participants 8 messages May 23, 2003 – May 30, 2003

Subsystems (7)

Media (V4L/DVB) Library code Cadet FM/am radio receiver driver Maxiradio FM radio receiver driver +3 more subsystems

lore

Share & tools

lore

Michael Hunold

author

2003-05-23 09:24 UTC #0 lore raw

RFC 1/1 [2.5] generic_usercopy() function (resend, forgot the patches)

Hello all,

especially the Video4Linux-driver rely intensively on the video_usercopy() function, which handles the copying of userspace arguments of ioctls with a simply callback mechanism.

Recently the dvb-core has been added which needs the same function. Because of the fact that the core is independent of Video4Linux, the code was duplicated to a dvb_usercopy() function.

In order to prevent this code duplication, introducing a generic_usercopy() function to lib/ is one possibilty.

The appended 4 patches do the following:

01-introduce.diff:
- remove video_usercopy() from videodev.c
- add generic_usercopy() to "lib/usecopy.c" and update the build system

02-video.diff:
- change all users of video_usercopy() to use generic_usercopy() instead

03-radio.diff:
- change all users of video_usercopy() to use generic_usercopy() instead

04-dvb.diff
- remove dvb_usercopy() from the dvb core and fix it to use generic_usercopy() instead.

The diffs are against 2.5.69.

Comments are very appreciated. 8-)
Is there a possibility to get this into the kernel?

CU
Michael.

drivers/media/video/videodev.c +1 ~~-62~~

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/video/videodev.c linux-2.5.69.usercopy/drivers/media/video/videodev.c
@@ -29,6 +29,7 @@
 #include <linux/kmod.h>
 #include <linux/slab.h>
 #include <linux/devfs_fs_kernel.h>
+#include <linux/usercopy.h>
 #include <asm/uaccess.h>
 #include <asm/system.h>
 #include <asm/semaphore.h>

@@ -109,67 +109,6 @@
 }
 
 /*
- * helper function -- handles userspace copying for ioctl arguments
- */
-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))
-{
-	char	sbuf[128];
-	void    *mbuf = NULL;
-	void	*parg = NULL;
-	int	err  = -EINVAL;
-
-	/*  Copy arguments into temp kernel buffer  */
-	switch (_IOC_DIR(cmd)) {
-	case _IOC_NONE:
-		parg = (void *)arg;
-		break;
-	case _IOC_READ: /* some v4l ioctls are marked wrong ... */
-	case _IOC_WRITE:
-	case (_IOC_WRITE | _IOC_READ):
-		if (_IOC_SIZE(cmd) <= sizeof(sbuf)) {
-			parg = sbuf;
-		} else {
-			/* too big to allocate from stack */
-			mbuf = kmalloc(_IOC_SIZE(cmd),GFP_KERNEL);
-			if (NULL == mbuf)
-				return -ENOMEM;
-			parg = mbuf;
-		}
-		
-		err = -EFAULT;
-		if (copy_from_user(parg, (void *)arg, _IOC_SIZE(cmd)))
-			goto out;
-		break;
-	}
-
-	/* call driver */
-	err = func(inode, file, cmd, parg);
-	if (err == -ENOIOCTLCMD)
-		err = -EINVAL;
-	if (err < 0)
-		goto out;
-
-	/*  Copy results into user buffer  */
-	switch (_IOC_DIR(cmd))
-	{
-	case _IOC_READ:
-	case (_IOC_WRITE | _IOC_READ):
-		if (copy_to_user((void *)arg, parg, _IOC_SIZE(cmd)))
-			err = -EFAULT;
-		break;
-	}
-
-out:
-	if (mbuf)
-		kfree(mbuf);
-	return err;
-}
-
-/*
  * open/release helper functions -- handle exclusive opens
  */
 extern int video_exclusive_open(struct inode *inode, struct file *file)

@@ -506,7 +445,6 @@
 EXPORT_SYMBOL(video_register_device);
 EXPORT_SYMBOL(video_unregister_device);
 EXPORT_SYMBOL(video_devdata);
-EXPORT_SYMBOL(video_usercopy);
 EXPORT_SYMBOL(video_exclusive_open);
 EXPORT_SYMBOL(video_exclusive_release);

include/linux/usercopy.h +12 -0

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/include/linux/usercopy.h linux-2.5.69.usercopy/include/linux/usercopy.h
@@ -0,0 +1,12 @@
+#ifndef _LINUX_USERCOPY_H
+#define _LINUX_USERCOPY_H
+
+#include <linux/types.h>
+
+int
+generic_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 /* _LINUX_USERCOPY_H */

include/linux/videodev.h +1 -0

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/include/linux/videodev.h linux-2.5.69.usercopy/include/linux/videodev.h
@@ -20,6 +20,7 @@
 
 #include <linux/poll.h>
 #include <linux/mm.h>
+#include <linux/usercopy.h>
 
 struct video_device
 {

lib/Kconfig +9 -0

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/lib/Kconfig linux-2.5.69.usercopy/lib/Kconfig
@@ -12,6 +12,15 @@
 	  kernel tree does. Such modules that use library CRC32 functions
 	  require M here.
 
+config USERCOPY
+	tristate "Generic usercopy function"
+	help
+	  This option is provided for the case where no in-kernel-tree
+	  modules require the generic usercopy function for copying 
+	  ioctl arguments from user space to kernel space, but a module
+	  built outside the kernel tree does. Such modules that use
+	  this function require M here.
+
 #
 # Do we need the compression support?
 #

lib/Makefile +4 -0

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/lib/Makefile linux-2.5.69.usercopy/lib/Makefile
@@ -21,6 +21,7 @@
 endif
 
 obj-$(CONFIG_CRC32)	+= crc32.o
+obj-$(CONFIG_USERCOPY)	+= usercopy.o
 
 obj-$(CONFIG_ZLIB_INFLATE) += zlib_inflate/
 obj-$(CONFIG_ZLIB_DEFLATE) += zlib_deflate/

@@ -29,6 +30,9 @@
 include $(TOPDIR)/drivers/usb/Makefile.lib
 include $(TOPDIR)/fs/Makefile.lib
 include $(TOPDIR)/net/bluetooth/bnep/Makefile.lib
+include $(TOPDIR)/drivers/media/video/Makefile.lib
+include $(TOPDIR)/drivers/media/common/Makefile.lib
+include $(TOPDIR)/drivers/media/dvb/dvb-core/Makefile.lib
 
 host-progs := gen_crc32table
 clean-files := crc32table.h

lib/usercopy.c +71 -0

71 lines not on this page · raw message

drivers/media/video/Makefile.lib +11 -0

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/video/Makefile.lib linux-2.5.69.usercopy/drivers/media/video/Makefile.lib
@@ -0,0 +1,11 @@
+# These drivers all require usercopy.o
+obj-$(CONFIG_VIDEODEV)		+= usercopy.o
+obj-$(CONFIG_VIDEO_SAA7134)	+= usercopy.o
+obj-$(CONFIG_VIDEO_BT848)	+= usercopy.o
+obj-$(CONFIG_VIDEO_PMS)	+= usercopy.o
+obj-$(CONFIG_VIDEO_W9966)	+= usercopy.o
+obj-$(CONFIG_VIDEO_BWQCAM)	+= usercopy.o
+obj-$(CONFIG_VIDEO_CQCAM)	+= usercopy.o
+obj-$(CONFIG_VIDEO_MEYE)	+= usercopy.o
+obj-$(CONFIG_VIDEO_CPIA)	+= usercopy.o
+obj-$(CONFIG_VIDEO_SAA5249)	+= usercopy.o

drivers/media/video/bttv-driver.c +2 -2

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/video/bttv-driver.c linux-2.5.69.usercopy/drivers/media/video/bttv-driver.c
@@ -2592,7 +2592,7 @@
 		bttv_switch_type(fh,V4L2_BUF_TYPE_VBI_CAPTURE);
 		return fh->lines * 2 * 2048;
 	default:
-		return video_usercopy(inode, file, cmd, arg, bttv_do_ioctl);
+		return generic_usercopy(inode, file, cmd, arg, bttv_do_ioctl);
 	}
 }

@@ -2903,7 +2903,7 @@
 static int radio_ioctl(struct inode *inode, struct file *file,
 		       unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, radio_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, radio_do_ioctl);
 }
 
 static struct file_operations radio_fops =

drivers/media/video/bw-qcam.c +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/video/bw-qcam.c linux-2.5.69.usercopy/drivers/media/video/bw-qcam.c
@@ -855,7 +855,7 @@
 static int qcam_ioctl(struct inode *inode, struct file *file,
 		     unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, qcam_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, qcam_do_ioctl);
 }
 
 static int qcam_read(struct file *file, char *buf,

drivers/media/video/c-qcam.c +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/video/c-qcam.c linux-2.5.69.usercopy/drivers/media/video/c-qcam.c
@@ -665,7 +665,7 @@
 static int qcam_ioctl(struct inode *inode, struct file *file,
 		     unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, qcam_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, qcam_do_ioctl);
 }
 
 static int qcam_read(struct file *file, char *buf,

drivers/media/video/cpia.c +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/video/cpia.c linux-2.5.69.usercopy/drivers/media/video/cpia.c
@@ -3740,7 +3740,7 @@
 static int cpia_ioctl(struct inode *inode, struct file *file,
 		     unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, cpia_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, cpia_do_ioctl);
 }

drivers/media/video/meye.c +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/video/meye.c linux-2.5.69.usercopy/drivers/media/video/meye.c
@@ -1169,7 +1169,7 @@
 static int meye_ioctl(struct inode *inode, struct file *file,
 		     unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, meye_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, meye_do_ioctl);
 }
 
 static int meye_mmap(struct file *file, struct vm_area_struct *vma) {

drivers/media/video/pms.c +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/video/pms.c linux-2.5.69.usercopy/drivers/media/video/pms.c
@@ -858,7 +858,7 @@
 static int pms_ioctl(struct inode *inode, struct file *file,
 		     unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, pms_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, pms_do_ioctl);
 }
 
 static int pms_read(struct file *file, char *buf,

drivers/media/video/saa5249.c +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/video/saa5249.c linux-2.5.69.usercopy/drivers/media/video/saa5249.c
@@ -602,7 +602,7 @@
 	int err;
 	
 	down(&t->lock);
-	err = video_usercopy(inode,file,cmd,arg,do_saa5249_ioctl);
+	err = generic_usercopy(inode,file,cmd,arg,do_saa5249_ioctl);
 	up(&t->lock);
 	return err;
 }

drivers/media/video/saa7134/saa7134-ts.c +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/video/saa7134/saa7134-ts.c linux-2.5.69.usercopy/drivers/media/video/saa7134/saa7134-ts.c
@@ -371,7 +371,7 @@
 static int ts_ioctl(struct inode *inode, struct file *file,
 		     unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, ts_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, ts_do_ioctl);
 }

drivers/media/video/saa7134/saa7134-video.c +2 -2

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/video/saa7134/saa7134-video.c linux-2.5.69.usercopy/drivers/media/video/saa7134/saa7134-video.c
@@ -1815,7 +1815,7 @@
 static int video_ioctl(struct inode *inode, struct file *file,
 		       unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, video_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, video_do_ioctl);
 }
 
 static int radio_do_ioctl(struct inode *inode, struct file *file,

@@ -1916,7 +1916,7 @@
 static int radio_ioctl(struct inode *inode, struct file *file,
 		       unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, radio_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, radio_do_ioctl);
 }
 
 static struct file_operations video_fops =

drivers/media/video/w9966.c +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/video/w9966.c linux-2.5.69.usercopy/drivers/media/video/w9966.c
@@ -863,7 +863,7 @@
 static int w9966_v4l_ioctl(struct inode *inode, struct file *file,
 			   unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, w9966_v4l_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, w9966_v4l_do_ioctl);
 }
 
 // Capture data

drivers/media/common/Makefile.lib +1 -0

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/common/Makefile.lib linux-2.5.69.usercopy/drivers/media/common/Makefile.lib
@@ -0,0 +1 @@
+obj-$(CONFIG_VIDEO_SAA7146)		+= usercopy.o

drivers/media/common/saa7146_fops.c +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/common/saa7146_fops.c linux-2.5.69.usercopy/drivers/media/common/saa7146_fops.c
@@ -259,7 +259,7 @@
 /*
 	DEB_EE(("inode:%p, file:%p, cmd:%d, arg:%li\n",inode, file, cmd, arg));
 */
-	return video_usercopy(inode, file, cmd, arg, saa7146_video_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, saa7146_video_do_ioctl);
 }
 
 static int fops_mmap(struct file *file, struct vm_area_struct * vma)

drivers/media/radio/Makefile.lib +15 -0

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/radio/Makefile.lib linux-2.5.69.usercopy/drivers/media/radio/Makefile.lib
@@ -0,0 +1,15 @@
+# These drivers all require usercopy.o
+obj-$(CONFIG_RADIO_GEMTEK)		+= usercopy.o
+obj-$(CONFIG_RADIO_GEMTEK_PCI)		+= usercopy.o
+obj-$(CONFIG_RADIO_MIROPCM20)		+= usercopy.o
+obj-$(CONFIG_RADIO_RTRACK)		+= usercopy.o
+obj-$(CONFIG_RADIO_RTRACK2)		+= usercopy.o
+obj-$(CONFIG_RADIO_TYPHOON)		+= usercopy.o
+obj-$(CONFIG_RADIO_TERRATEC)		+= usercopy.o
+obj-$(CONFIG_RADIO_CADET)		+= usercopy.o
+obj-$(CONFIG_RADIO_TRUST)		+= usercopy.o
+obj-$(CONFIG_RADIO_MAESTRO)		+= usercopy.o
+obj-$(CONFIG_RADIO_SF16FMI)		+= usercopy.o
+obj-$(CONFIG_RADIO_MAXIRADIO)		+= usercopy.o
+obj-$(CONFIG_RADIO_ZOLTRIX)		+= usercopy.o
+obj-$(CONFIG_RADIO_AZTECH)		+= usercopy.o

drivers/media/radio/miropcm20-radio.c +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/radio/miropcm20-radio.c linux-2.5.69.usercopy/drivers/media/radio/miropcm20-radio.c
@@ -213,7 +213,7 @@
 static int pcm20_ioctl(struct inode *inode, struct file *file,
 		       unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, pcm20_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, pcm20_do_ioctl);
 }
 
 static struct pcm20_device pcm20_unit = {

drivers/media/radio/radio-aimslab.c +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/radio/radio-aimslab.c linux-2.5.69.usercopy/drivers/media/radio/radio-aimslab.c
@@ -294,7 +294,7 @@
 static int rt_ioctl(struct inode *inode, struct file *file,
 		    unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, rt_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, rt_do_ioctl);
 }
 
 static struct rt_device rtrack_unit;

drivers/media/radio/radio-aztech.c +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/radio/radio-aztech.c linux-2.5.69.usercopy/drivers/media/radio/radio-aztech.c
@@ -246,7 +246,7 @@
 static int az_ioctl(struct inode *inode, struct file *file,
 		    unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, az_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, az_do_ioctl);
 }
 
 static struct az_device aztech_unit;

drivers/media/radio/radio-cadet.c +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/radio/radio-cadet.c linux-2.5.69.usercopy/drivers/media/radio/radio-cadet.c
@@ -471,7 +471,7 @@
 static int cadet_ioctl(struct inode *inode, struct file *file,
 		       unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, cadet_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, cadet_do_ioctl);
 }
 
 static int cadet_open(struct inode *inode, struct file *file)

drivers/media/radio/radio-gemtek-pci.c +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/radio/radio-gemtek-pci.c linux-2.5.69.usercopy/drivers/media/radio/radio-gemtek-pci.c
@@ -275,7 +275,7 @@
 static int gemtek_pci_ioctl(struct inode *inode, struct file *file,
 			    unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, gemtek_pci_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, gemtek_pci_do_ioctl);
 }
 
 enum {

drivers/media/radio/radio-gemtek.c +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/radio/radio-gemtek.c linux-2.5.69.usercopy/drivers/media/radio/radio-gemtek.c
@@ -223,7 +223,7 @@
 static int gemtek_ioctl(struct inode *inode, struct file *file,
 			unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, gemtek_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, gemtek_do_ioctl);
 }
 
 static struct gemtek_device gemtek_unit;

drivers/media/radio/radio-maestro.c +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/radio/radio-maestro.c linux-2.5.69.usercopy/drivers/media/radio/radio-maestro.c
@@ -265,7 +265,7 @@
 	int ret;
 
 	down(&card->lock);
-	ret = video_usercopy(inode, file, cmd, arg, radio_function);
+	ret = generic_usercopy(inode, file, cmd, arg, radio_function);
 	up(&card->lock);
 	return ret;
 }

drivers/media/radio/radio-maxiradio.c +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/radio/radio-maxiradio.c linux-2.5.69.usercopy/drivers/media/radio/radio-maxiradio.c
@@ -274,7 +274,7 @@
 	int ret;
 	
 	down(&card->lock);
-	ret = video_usercopy(inode, file, cmd, arg, radio_function);
+	ret = generic_usercopy(inode, file, cmd, arg, radio_function);
 	up(&card->lock);
 	return ret;
 }

drivers/media/radio/radio-rtrack2.c +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/radio/radio-rtrack2.c linux-2.5.69.usercopy/drivers/media/radio/radio-rtrack2.c
@@ -189,7 +189,7 @@
 static int rt_ioctl(struct inode *inode, struct file *file,
 		    unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, rt_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, rt_do_ioctl);
 }
 
 static struct rt_device rtrack2_unit;

drivers/media/radio/radio-sf16fmi.c +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/radio/radio-sf16fmi.c linux-2.5.69.usercopy/drivers/media/radio/radio-sf16fmi.c
@@ -217,7 +217,7 @@
 static int fmi_ioctl(struct inode *inode, struct file *file,
 		     unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, fmi_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, fmi_do_ioctl);
 }
 
 static struct fmi_device fmi_unit;

drivers/media/radio/radio-terratec.c +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/radio/radio-terratec.c linux-2.5.69.usercopy/drivers/media/radio/radio-terratec.c
@@ -266,7 +266,7 @@
 static int tt_ioctl(struct inode *inode, struct file *file,
 		    unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, tt_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, tt_do_ioctl);
 }
 
 static struct tt_device terratec_unit;

drivers/media/radio/radio-trust.c +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/radio/radio-trust.c linux-2.5.69.usercopy/drivers/media/radio/radio-trust.c
@@ -247,7 +247,7 @@
 static int tr_ioctl(struct inode *inode, struct file *file,
 		    unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, tr_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, tr_do_ioctl);
 }
 
 static struct file_operations trust_fops = {

drivers/media/radio/radio-typhoon.c +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/radio/radio-typhoon.c linux-2.5.69.usercopy/drivers/media/radio/radio-typhoon.c
@@ -246,7 +246,7 @@
 static int typhoon_ioctl(struct inode *inode, struct file *file,
 			 unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, typhoon_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, typhoon_do_ioctl);
 }
 
 static struct typhoon_device typhoon_unit =

drivers/media/radio/radio-zoltrix.c +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/radio/radio-zoltrix.c linux-2.5.69.usercopy/drivers/media/radio/radio-zoltrix.c
@@ -313,7 +313,7 @@
 static int zol_ioctl(struct inode *inode, struct file *file,
 		     unsigned int cmd, unsigned long arg)
 {
-	return video_usercopy(inode, file, cmd, arg, zol_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, zol_do_ioctl);
 }
 
 static struct zol_device zoltrix_unit;

drivers/media/dvb/dvb-core/Makefile.lib +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/dvb/dvb-core/Makefile.lib linux-2.5.69.usercopy/drivers/media/dvb/dvb-core/Makefile.lib
@@ -1 +1 @@
-obj-$(CONFIG_DVB_CORE)		+= crc32.o
+obj-$(CONFIG_DVB_CORE)		+= crc32.o usercopy.o

drivers/media/dvb/dvb-core/dmxdev.c +2 -2

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/dvb/dvb-core/dmxdev.c linux-2.5.69.usercopy/drivers/media/dvb/dvb-core/dmxdev.c
@@ -983,7 +983,7 @@
 static int dvb_demux_ioctl(struct inode *inode, struct file *file,
 			   unsigned int cmd, unsigned long arg)
 {
-	return dvb_usercopy(inode, file, cmd, arg, dvb_demux_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, dvb_demux_do_ioctl);
 }

@@ -1064,7 +1064,7 @@
 static int dvb_dvr_ioctl(struct inode *inode, struct file *file,
 			 unsigned int cmd, unsigned long arg)
 {
-	return dvb_usercopy(inode, file, cmd, arg, dvb_dvr_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, dvb_dvr_do_ioctl);
 }

drivers/media/dvb/dvb-core/dvb_ksyms.c +0 ~~-64~~

70 lines not on this page · raw message

drivers/media/dvb/dvb-core/dvb_net.c +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/dvb/dvb-core/dvb_net.c linux-2.5.69.usercopy/drivers/media/dvb/dvb-core/dvb_net.c
@@ -535,7 +535,7 @@
 dvb_net_ioctl(struct inode *inode, struct file *file,
 	      unsigned int cmd, unsigned long arg)
 {
-	return dvb_usercopy(inode, file, cmd, arg, dvb_net_do_ioctl);
+	return generic_usercopy(inode, file, cmd, arg, dvb_net_do_ioctl);
 }
 
 static struct file_operations dvb_net_fops = {

drivers/media/dvb/dvb-core/dvbdev.c +1 -1

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/dvb/dvb-core/dvbdev.c linux-2.5.69.usercopy/drivers/media/dvb/dvb-core/dvbdev.c
@@ -160,7 +160,7 @@
 	if (!dvbdev->kernel_ioctl)
 		return -EINVAL;
 
-	return dvb_usercopy (inode, file, cmd, arg, dvbdev->kernel_ioctl);
+	return generic_usercopy (inode, file, cmd, arg, dvbdev->kernel_ioctl);
 }

drivers/media/dvb/dvb-core/dvbdev.h +2 -5

diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/dvb/dvb-core/dvbdev.h linux-2.5.69.usercopy/drivers/media/dvb/dvb-core/dvbdev.h
@@ -29,6 +29,7 @@
 #include <linux/poll.h>
 #include <linux/devfs_fs_kernel.h>
 #include <linux/list.h>
+#include <linux/usercopy.h>
 
 #define DVB_MAJOR 250

@@ -61,7 +62,7 @@
 	int users;
 	int writers;
 
-        /* don't really need those !? -- FIXME: use video_usercopy  */
+        /* don't really need those !? -- FIXME: use generic_usercopy  */
         int (*kernel_ioctl)(struct inode *inode, struct file *file,
 			    unsigned int cmd, void *arg);

@@ -84,9 +85,5 @@
 extern int dvb_generic_release (struct inode *inode, struct file *file);
 extern int dvb_generic_ioctl (struct inode *inode, struct file *file,
 			      unsigned int cmd, unsigned long arg);
-int dvb_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 /* #ifndef _DVBDEV_H_ */

Christoph Hellwig

2003-05-23 09:35 UTC #1 lore raw

This function is small and very useful, I think it should be included unconditional and the prototype maybe moved to kernel.h.

lib/usercopy.c line 17

Quoted 5 lines

lib/usercopy.c +5 -0

+int
+generic_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))

The name is a bit mislead. maybe ioctl_usercopy? ioctl_uaccess? Also file/inode should go away from the prototype (and the callback). Only file is needed because inode == file->f_dentry->d_inode, and even that one should be just some void *data instead.

lib/usercopy.c line 29

Quoted 11 lines

lib/usercopy.c +11 -0

+	char	sbuf[128];
+	void    *mbuf = NULL;
+	void	*parg = NULL;
+	int	err  = -EINVAL;
+
+	/*  Copy arguments into temp kernel buffer  */
+	switch (_IOC_DIR(cmd)) {
+	case _IOC_NONE:
+		parg = (void *)arg;
+		break;
+	case _IOC_READ: /* some v4l ioctls are marked wrong ... */

That's crap. Please move this workaround to v4l not into generic code.

lib/usercopy.c line 39

Quoted 10 lines

lib/usercopy.c +10 -0

+	case _IOC_WRITE:
+	case (_IOC_WRITE | _IOC_READ):
+		if (_IOC_SIZE(cmd) <= sizeof(sbuf)) {
+			parg = sbuf;
+		} else {
+			/* too big to allocate from stack */
+			mbuf = kmalloc(_IOC_SIZE(cmd),GFP_KERNEL);
+			if (NULL == mbuf)
+				return -ENOMEM;
+			parg = mbuf;

I wonder whether you should just kmalloc always.

lib/usercopy.c line 51

Quoted 4 lines

lib/usercopy.c +4 -0

+	/* call driver */
+	err = func(inode, file, cmd, parg);
+	if (err == -ENOIOCTLCMD)
+		err = -EINVAL;

I don't think this is the right place for this substitution - leave it to the drivers.

Michael Hunold

author

2003-05-23 09:57 UTC #2 lore raw

Hello Christoph,

Quoted 2 lines

This function is small and very useful, I think it should be included unconditional and the prototype maybe moved to kernel.h.

That's ok for me, although I don't know what the exact criteria for adding a function to kernel.h are.

Quoted 8 lines

lib/usercopy.c line 17
Quoted 5 lines
lib/usercopy.c +5 -0
+int
+generic_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))
The name is a bit mislead. maybe ioctl_usercopy? ioctl_uaccess?

These are both fine IMHO.

Quoted 3 lines

Also file/inode should go away from the prototype (and the callback). Only file is needed because inode == file->f_dentry->d_inode, and even that one should be just some void *data instead.

I only copied the function from videodev.c and renamed it, so please don't blame me for the way stuff is done there. 8-)

Perhaps Gerd Knorr or Alan Cox can comment on your changes -- I'll have to investigate if all of these arguments are used anyway.

Quoted 2 lines

lib/usercopy.c line 29
Quoted 1 line
lib/usercopy.c +1 -0
+	case _IOC_READ: /* some v4l ioctls are marked wrong ... */
That's crap. Please move this workaround to v4l not into generic code.

Is it possible to fix the definitons of the v4l ioctls instead without breaking binary compatiblity?

Quoted 5 lines

lib/usercopy.c +5 -0

+	case _IOC_WRITE:
+	case (_IOC_WRITE | _IOC_READ):
+		if (_IOC_SIZE(cmd) <= sizeof(sbuf)) {
+			parg = sbuf;
+		} else {

Quoted 8 lines

lib/usercopy.c line 39
Quoted 5 lines
lib/usercopy.c +5 -0
+			/* too big to allocate from stack */
+			mbuf = kmalloc(_IOC_SIZE(cmd),GFP_KERNEL);
+			if (NULL == mbuf)
+				return -ENOMEM;
+			parg = mbuf;
I wonder whether you should just kmalloc always.

Since this function is always used in user context and the memory is always freed afterwards, it should be possible to use vmalloc() or a static buffer (what's the maximum size?) instead.

Quoted 8 lines

lib/usercopy.c line 51
Quoted 4 lines
lib/usercopy.c +4 -0
+	/* call driver */
+	err = func(inode, file, cmd, parg);
+	if (err == -ENOIOCTLCMD)
+		err = -EINVAL;
I don't think this is the right place for this substitution - leave it to the drivers.

Agreed.

CU
Michael.

Christoph Hellwig

2003-05-23 10:07 UTC #3 lore raw

Quoted 9 lines · Michael Hunold

Quoted 3 lines
Also file/inode should go away from the prototype (and the callback). Only file is needed because inode == file->f_dentry->d_inode, and even that one should be just some void *data instead.
I only copied the function from videodev.c and renamed it, so please don't blame me for the way stuff is done there. 8-)
Perhaps Gerd Knorr or Alan Cox can comment on your changes -- I'll have to investigate if all of these arguments are used anyway.

I don't blame you. I just think it shouldn't be added to the kernel API without fixing it first :)

Quoted 6 lines

Quoted 3 lines
lib/usercopy.c line 29
Quoted 2 lines
lib/usercopy.c +1 -0
+	case _IOC_READ: /* some v4l ioctls are marked wrong ... */
That's crap. Please move this workaround to v4l not into generic code.
Is it possible to fix the definitons of the v4l ioctls instead without breaking binary compatiblity?

I doubt it. This just means v4l has to work around it's bug in the v4l code, not in common code.

Quoted 14 lines

Quoted 5 lines
Quoted 5 lines
lib/usercopy.c +5 -0
+	case _IOC_WRITE:
+	case (_IOC_WRITE | _IOC_READ):
+		if (_IOC_SIZE(cmd) <= sizeof(sbuf)) {
+			parg = sbuf;
+		} else {
Quoted 8 lines
lib/usercopy.c line 39
Quoted 5 lines
lib/usercopy.c +5 -0
+			/* too big to allocate from stack */
+			mbuf = kmalloc(_IOC_SIZE(cmd),GFP_KERNEL);
+			if (NULL == mbuf)
+				return -ENOMEM;
+			parg = mbuf;
I wonder whether you should just kmalloc always.

Quoted 3 lines

Since this function is always used in user context and the memory is always freed afterwards, it should be possible to use vmalloc() or a static buffer (what's the maximum size?) instead.

Sorry, I meant it's probably not worth using the stack at all. vmalloc() for ioctl buffers sounds like a very bad idea.

Gerd Knorr

2003-05-23 12:07 UTC #4 lore raw

In reply to Michael Hunold

Quoted 5 lines

Quoted 3 lines
Quoted 1 line
+generic_usercopy(struct inode *inode, struct file *file,
The name is a bit mislead. maybe ioctl_usercopy? ioctl_uaccess?
These are both fine IMHO.

I'd pick ioctl_usercopy() because the name is much like the well-known copy_(from|to)_user functions.

Quoted 6 lines

Quoted 3 lines
Also file/inode should go away from the prototype (and the callback). Only file is needed because inode == file->f_dentry->d_inode, and even that one should be just some void *data instead.
I only copied the function from videodev.c and renamed it, so please don't blame me for the way stuff is done there. 8-)

I've just pass through what the fops->ioctl() function is called with. Sure file* is enougth? If so, why the fops->ioctl() function is called with both file and inode?

I think my v4l drivers just need file->priv_data (havn't looked at the code through), so some void *data would work equally well for them.

Quoted 5 lines

Quoted 2 lines
lib/usercopy.c line 29
Quoted 1 line
lib/usercopy.c +1 -0
+	case _IOC_READ: /* some v4l ioctls are marked wrong ... */
That's crap. Please move this workaround to v4l not into generic code.
Is it possible to fix the definitons of the v4l ioctls instead without breaking binary compatiblity?

Not trivial. You'll have to support both old and new (fixed) ioctl numbers, otherwise you'll break existing binaries. Using the new ones internally and mapping the old to the new ones somehow might work.

Quoted 5 lines

lib/usercopy.c +5 -0

+	case _IOC_WRITE:
+	case (_IOC_WRITE | _IOC_READ):
+		if (_IOC_SIZE(cmd) <= sizeof(sbuf)) {
+			parg = sbuf;
+		} else {

Quoted 8 lines

Quoted 8 lines
lib/usercopy.c line 39
Quoted 5 lines
lib/usercopy.c +5 -0
+			/* too big to allocate from stack */
+			mbuf = kmalloc(_IOC_SIZE(cmd),GFP_KERNEL);
+			if (NULL == mbuf)
+				return -ENOMEM;
+			parg = mbuf;
I wonder whether you should just kmalloc always.

Point of implementing it this way is that (a) the kmalloc()/kfree() isn't for free and (b) we shouldn't use plenty of stack memory. So using kmalloc for big chunks and allocate from stack for the small ones looks like a good compromise to me.

Quoted 3 lines

Since this function is always used in user context and the memory is always freed afterwards, it should be possible to use vmalloc() or a static buffer (what's the maximum size?) instead.

static buffer? You are kidding, are you?

Gerd

Signature · 1 line

sigfault

David Woodhouse

2003-05-25 11:10 UTC #5 lore raw

In reply to Christoph Hellwig

Quoted 6 lines · Christoph Hellwig

lib/usercopy.c line 51
Quoted 3 lines
lib/usercopy.c +3 -0
+	err = func(inode, file, cmd, parg);
+	if (err == -ENOIOCTLCMD)
+		err = -EINVAL;
I don't think this is the right place for this substitution - leave it to the drivers.

And make it -ENOTTY. -EINVAL is not correct for unknown ioctls.

Signature · 1 line

dwmw2

Ingo Oeser

2003-05-23 11:14 UTC #6 lore raw

In reply to Michael Hunold

Hi Michael,

Quoted 2 lines · Michael Hunold

In order to prevent this code duplication, introducing a generic_usercopy() function to lib/ is one possibilty.

I like the idea, because whoever invented the IOCTL generation macros forgot exactly this function.

This raised the problem, that some IOCTLs have the wrong numbers due to the author not being able to grok the macros and/or documentation.

Also many authors have problems evaluating their IOCTLs and get directions wrong.

So this code helps these issues.

I just miss variants for the smaller sizes. These should be handled differently and more easily,
so better warn about sizes <= sizeof(void *).

Regards

Ingo Oeser

David Wagner

2003-05-30 00:38 UTC #7 lore raw

In reply to Michael Hunold

Quoted 9 lines · Michael Hunold

/*
- * helper function -- handles userspace copying for ioctl arguments
- */

drivers/media/video/videodev.c +0 -6

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

...

Quoted 1 line

(quoted diff) +0 -1

-		if (copy_from_user(parg, (void *)arg, _IOC_SIZE(cmd)))

...

drivers/media/video/videodev.c line 150

Quoted 1 line

drivers/media/video/videodev.c +0 -1
-	err = func(inode, file, cmd, parg);

...

What about doubly-indirected pointers? i.e., arg is a pointer to a structure that itself contains a pointer? Can this happen?