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

[Hans Verkuil]

Hi Arnd,

On 06/10/2020 17:14, Arnd Bergmann wrote:
> On Fri, Oct 2, 2020 at 4:32 PM Hans Verkuil <hverkuil@xxxxxxxxx> 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!

Without the compliance and regression tests it would be impossible to get this
right, it is *so* hard to verify just by looking at the code. That's always been
an issue with this compat code.

Luckily, with the test-media script in v4l-utils it is fairly easy to get
almost complete coverage. Ping me on irc if you are interested in testing this
series yourself, it's not too difficult to do. All the issues mentioned above
were all found by this test. Sometimes it failed in fairly obscure ways and
it took quite some time to figure out the underlying cause. The 'typo: p64 -> b64'
in particular wasn't easy to find: I must have read over that typo a dozen times :-)

> 
> 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(). 

Sorry, I should have mentioned that.

> 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@xxxxxxxxx>
> |    Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> |    Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> 
> I could split that out into a separate patch if you prefer.

No, don't split it. I prefer to have an as-clean-as-possible series to ease reviewing.

> 
>> @@ -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.

You're probably right. I should remember to do a test compilation next
time with CONFIG_COMPAT_32BIT_TIME set to n.

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

Looking forward to that.

Regards,

	Hans