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
--- linux-2.5.69/drivers/media/dvb/dvb-core/Makefile.lib 2003-05-06 13:15:32.000000000 +0200
+++ linux-2.5.69.usercopy/drivers/media/dvb/dvb-core/Makefile.lib 2003-05-23 10:42:12.000000000 +0200
@@ -1 +1 @@
-obj-$(CONFIG_DVB_CORE) += crc32.o
+obj-$(CONFIG_DVB_CORE) += crc32.o usercopy.o
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
--- linux-2.5.69/drivers/media/dvb/dvb-core/dmxdev.c 2003-05-06 13:15:32.000000000 +0200
+++ linux-2.5.69.usercopy/drivers/media/dvb/dvb-core/dmxdev.c 2003-05-23 10:17:57.000000000 +0200
@@ -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);
}
diff -uNrwB -x '.*' -x '*.o' -x '*.mod' --new-file linux-2.5.69/drivers/media/dvb/dvb-core/dvb_ksyms.c linux-2.5.69.usercopy/drivers/media/dvb/dvb-core/dvb_ksyms.c
--- linux-2.5.69/drivers/media/dvb/dvb-core/dvb_ksyms.c 2003-05-06 13:15:32.000000000 +0200
+++ linux-2.5.69.usercopy/drivers/media/dvb/dvb-core/dvb_ksyms.c 2003-05-23 10:17:57.000000000 +0200
@@ -8,70 +8,6 @@
#include "dvb_demux.h"
#include "dvb_net.h"
-/* if the miracle happens and "generic_usercopy()" is included into
- the kernel, then this can vanish. please don't make the mistake and
- define this as video_usercopy(). this will introduce a dependecy
- to the v4l "videodev.o" module, which is unnecessary for some
- cards (ie. the budget dvb-cards don't need the v4l module...) */
-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))
-{
- 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 */
- if ((err = func(inode, file, cmd, parg)) == -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;
-}
-EXPORT_SYMBOL(dvb_usercopy);
-
EXPORT_SYMBOL(dvb_dmxdev_init);
EXPORT_SYMBOL(dvb_dmxdev_release);
EXPORT_SYMBOL(dvb_dmx_init);
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
--- linux-2.5.69/drivers/media/dvb/dvb-core/dvb_net.c 2003-05-06 13:15:32.000000000 +0200
+++ linux-2.5.69.usercopy/drivers/media/dvb/dvb-core/dvb_net.c 2003-05-23 10:17:57.000000000 +0200
@@ -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 = {
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
--- linux-2.5.69/drivers/media/dvb/dvb-core/dvbdev.c 2003-05-06 13:16:20.000000000 +0200
+++ linux-2.5.69.usercopy/drivers/media/dvb/dvb-core/dvbdev.c 2003-05-23 10:20:15.000000000 +0200
@@ -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);
}
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
--- linux-2.5.69/drivers/media/dvb/dvb-core/dvbdev.h 2003-05-06 13:16:20.000000000 +0200
+++ linux-2.5.69.usercopy/drivers/media/dvb/dvb-core/dvbdev.h 2003-05-23 10:20:25.000000000 +0200
@@ -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_ */
This function is small and very useful, I think it should be included unconditional
and the prototype maybe moved to kernel.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))
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.
> + 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.
> + 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.
> + /* 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.
Hello Christoph,
> 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.
>>+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.
> 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.
>>+ 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?
>>+ 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.
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.
>>+ /* 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.
On Fri, May 23, 2003 at 12:10:57PM +0200, Michael Hunold wrote:
> > 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 :)
>
> >>+ 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.
> >>+ 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.
>
> 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.
Hi Michael,
On Fri, May 23, 2003 at 11:37:09AM +0200, Michael Hunold wrote:
> 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
> >>+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.
> >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.
> >>+ 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.
> >>+ 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.
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.
> 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
--
sigfault
On Fri, 2003-05-23 at 10:47, Christoph Hellwig wrote:
> > + 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.
--
dwmw2
Michael Hunold wrote:
> /*
>- * 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))
>-{
...
>- if (copy_from_user(parg, (void *)arg, _IOC_SIZE(cmd)))
...
>- 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?