Re: [PATCH 0/8] media: v4l2: simplify compat ioctl handling

[Arnd Bergmann <arnd@arndb.de>]

On Fri, Oct 2, 2020 at 4:32 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 17/09/2020 17:28, Arnd Bergmann wrote:

> While testing I found a lot of bugs. Below is a patch that sits on top
> of your series and fixes all the bugs:
>
> - put_v4l2_standard32: typo: p64->index -> p64->id
> - get_v4l2_plane32: typo: p64 -> plane32
>                     typo: duplicate bytesused: the 2nd should be 'length'
> - put_v4l2_plane32: typo: duplicate bytesused: the 2nd should be 'length'
> - get_v4l2_buffer32/get_v4l2_buffer32_time32: missing compat_ptr for vb32.m.userptr
> - get_v4l2_buffer32/get_v4l2_buffer32_time32: drop querybuf bool
> - put_v4l2_buffer32/put_v4l2_buffer32_time32: add uintptr_t cast for vb->m.userptr
> - get_v4l2_ext_controls32: typo: error_idx -> request_fd
> - put_v4l2_ext_controls32: typo: error_idx -> request_fd
> - v4l2_compat_translate_cmd: missing case VIDIOC_CREATE_BUFS32
> - v4l2_compat_translate_cmd: #ifdef CONFIG_COMPAT_32BIT_TIME for
>   case VIDIOC_DQEVENT32_TIME32 and return VIDIOC_DQEVENT
> - v4l2_compat_put_user: #ifdef CONFIG_COMPAT_32BIT_TIME for case VIDIOC_DQEVENT32_TIME32
> - v4l2_compat_get_array_args: typo: p64 -> b64
> - v4l2_compat_put_array_args: typo: p64 -> b64
> - video_get_user: make sure INFO_FL_CLEAR_MASK is honored, also when in_compat_syscall()
> - video_usercopy: err = v4l2_compat_put_array_args overwrote the original ioctl err value.
>   Handle this correctly.
>
> I've tested this as well with the y2038 compat mode (i686 with 64-bit time_t)
> and that too works fine.

I'm not too surprised that there were bugs, but I am a little shocked
at how much
I got wrong in the end. Thanks a lot for testing my series and fixing all of the
above!

I've carefully studied your changes and folded them into my series now.
Most of the changes were obvious in hindsight, just two things to comment on:

>  #ifdef CONFIG_COMPAT_32BIT_TIME
>  static int get_v4l2_buffer32_time32(struct v4l2_buffer *vb,
> -                                   struct v4l2_buffer32_time32 __user *arg,
> -                                   bool querybuf)
> +                                   struct v4l2_buffer32_time32 __user *arg)
>  {
>         struct v4l2_buffer32_time32 vb32;
>
> @@ -489,8 +484,6 @@ static int get_v4l2_buffer32_time32(struct v4l2_buffer *vb,
>         if (V4L2_TYPE_IS_MULTIPLANAR(vb->type))
>                 vb->m.planes = (void __force *)
>                                 compat_ptr(vb32.m.planes);
> -       if (querybuf)
> -               vb->request_fd = 0;
>
>         return 0;

It took me too long to understand what you changed here, as this depends
on your rewrite of video_get_user(). The new version definitely looks
cleaner. After folding in the video_get_user() changes, I've amended
that changelog of the "media: v4l2: prepare compat-ioctl rework" commit
with:

|    [...]
|    provide a location for reading and writing user space data from inside
|    of the generic video_usercopy() helper.
|
|    Hans Verkuil rewrote the video_get_user() function here to simplify
|    the zeroing of the extra input fields and fixed a couple of bugs in
|    the original implementation.
|
|    Co-developed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
|    Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
|    Signed-off-by: Arnd Bergmann <arnd@arndb.de>

I could split that out into a separate patch if you prefer.

> @@ -1025,8 +1020,10 @@ int v4l2_compat_put_user(void __user *arg, void *parg, unsigned int cmd)
>  #ifdef CONFIG_X86_64
>         case VIDIOC_DQEVENT32:
>                 return put_v4l2_event32(parg, arg);
> +#ifdef CONFIG_COMPAT_32BIT_TIME
>         case VIDIOC_DQEVENT32_TIME32:
>                 return put_v4l2_event32_time32(parg, arg);
> +#endif
>  #endif
>         }
>         return 0;

I think this change requires adding another #ifdef around the
put_v4l2_event32_time32() definition, to avoid an "unused function"
warning. The #ifdef was already missing in the original version, but I
agree it makes sense to add it.

As you suggested earlier, I will resend the fixed series after -rc1
is out.

       Arnd