Hi Mauro,
On Wednesday 02 May 2012 19:32:23 Mauro Carvalho Chehab wrote:
> Em 02-05-2012 16:13, Sakari Ailus escreveu:
> > Implement compat functions to provide conversion between structs
> > containing enums and those not. The functions are intended to be removed
> > when the support for such old binaries is no longer necessary.
>
> This is not a full review of this patch, as checking the full logic
> will consume some time.
>
> This is just a few points to consider.
[snip]
> > -video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
> > +video_usercopy(struct file *file, unsigned int compat_cmd, unsigned long
> > arg,>
> > v4l2_kioctl func)
> >
> > {
> >
> > char sbuf[128];
> >
> > @@ -2401,6 +3048,7 @@ video_usercopy(struct file *file, unsigned int cmd,
> > unsigned long arg,>
> > size_t array_size = 0;
> > void __user *user_ptr = NULL;
> > void **kernel_ptr = NULL;
> >
> > + unsigned int cmd = get_non_compat_cmd(compat_cmd);
>
> This will put a penalty on archs where sizeof(u32) == sizeof(enum), with
> covers most of the cases.
>
> My suggestion is to, instead, call it at the end of __video_do_ioctl, at
> the "default" clause, with a recursive call to __video_do_ioctl().
Would that work for "has_array_args" ioctls ? video_usercopy() won't copy the
array. The compat code could possibly handle that though.
What about making get_non_compat_cmd() return its argument directly when
sizeof(__u32) == sizeof(enum) ? The performance impact should be negligible.
> It should be noticed that UVC driver and pvrusb2 are the only two drivers
> that don't use __video_do_ioctl(). So, a logic similar to it should be
> implemented there.
>
> > /* Copy arguments into temp kernel buffer */
> > if (_IOC_DIR(cmd) != _IOC_NONE) {
> >
> > @@ -2418,12 +3066,23 @@ video_usercopy(struct file *file, unsigned int
> > cmd, unsigned long arg,>
> > if (_IOC_DIR(cmd) & _IOC_WRITE) {
> >
> > unsigned long n = cmd_input_size(cmd);
> >
> > - if (copy_from_user(parg, (void __user *)arg, n))
> > - goto out;
> > -
> > - /* zero out anything we don't copy from userspace */
> > - if (n < _IOC_SIZE(cmd))
> > - memset((u8 *)parg + n, 0, _IOC_SIZE(cmd) - n);
> > + if (cmd == compat_cmd) {
> > + if (copy_from_user(
> > + parg, (void __user *)arg, n))
> > + goto out;
> > +
> > + /*
> > + * zero out anything we don't copy
> > + * from userspace
> > + */
> > + if (n < _IOC_SIZE(cmd))
> > + memset((u8 *)parg + n, 0,
> > + _IOC_SIZE(cmd) - n);
> > + } else {
> > + if (copy_compat_from_user(compat_cmd, parg,
> > + (void __user *)arg))
> > + goto out;
> > + }
> >
> > } else {
> >
> > /* read-only ioctl */
> > memset(parg, 0, _IOC_SIZE(cmd));
> >
> > @@ -2471,8 +3130,15 @@ out_array_args:
> > switch (_IOC_DIR(cmd)) {
> > case _IOC_READ:
> >
> > case (_IOC_WRITE | _IOC_READ):
> > - if (copy_to_user((void __user *)arg, parg, _IOC_SIZE(cmd)))
> > - err = -EFAULT;
> > + if (cmd == compat_cmd) {
> > + if (copy_to_user((void __user *)arg, parg,
> > + _IOC_SIZE(cmd)))
> > + err = -EFAULT;
> > + } else {
> > + if (copy_compat_to_user(compat_cmd, (void __user *)arg,
> > + parg))
> > + err = -EFAULT;
> > + }
> >
> > break;
> >
> > }
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
