Re: [PATCH v5 6/8] media: v4l2-core: fix v4l2_buffer handling for time64 ABI

[Hans Verkuil]

On 12/13/19 4:08 PM, Arnd Bergmann wrote:
> On Thu, Dec 12, 2019 at 4:43 PM Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>>
>> On 11/26/19 5:18 PM, Arnd Bergmann wrote:
>>>
>>>       switch (cmd) {
>>> +#ifdef COMPAT_32BIT_TIME
>>
>> COMPAT_32BIT_TIME -> CONFIG_COMPAT_32BIT_TIME
> 
> Fixed.
> 
>>> +             *vb = (struct v4l2_buffer) {
>>> +                     .index          = vb32.index,
>>> +                     .type           = vb32.type,
>>> +                     .bytesused      = vb32.bytesused,
>>> +                     .flags          = vb32.flags,
>>> +                     .field          = vb32.field,
>>> +                     .timestamp.tv_sec       = vb32.timestamp.tv_sec,
>>> +                     .timestamp.tv_usec      = vb32.timestamp.tv_usec,
>>> +                     .timecode       = vb32.timecode,
>>
>> You forgot to copy sequence.
>>
>>> +                     .memory         = vb32.memory,
>>> +                     .m.userptr      = vb32.m.usercopy,
>>
>> usercopy -> userptr
> 
> Fixed.
> 
>>> +                     .length         = vb32.length,
>>> +                     .request_fd     = vb32.request_fd,
>>> +             };
>>> +
>>> +             if (cmd == VIDIOC_QUERYBUF_TIME32)
>>> +                     memset(&vb->length, 0, sizeof(*vb) -
>>> +                            offsetof(struct v4l2_buffer, length));
>>
>> It's from the field AFTER vb->length that this needs to be zeroed. It's best to
>> use the CLEAR_AFTER_FIELD macro here.
> 
> I'm a bit lost about this one: the fields that are not explicitly
> uninitialized here are already set to zero by the assignment
> above. Should this simply be a
> 
>                 if (cmd == VIDIOC_QUERYBUF_TIME32)
>                        vb->request_fd = 0;

Yes, you are correct. That's much simpler.

> 
> then? I don't remember where that memset() originally came
> from or why request_fd has to be cleared here.
> 
>>> @@ -3100,6 +3141,30 @@ static int video_put_user(void __user *arg, void *parg, unsigned int cmd)
>>>                       return -EFAULT;
>>>               break;
>>>       }
>>> +     case VIDIOC_QUERYBUF_TIME32:
>>> +     case VIDIOC_QBUF_TIME32:
>>> +     case VIDIOC_DQBUF_TIME32:
>>> +     case VIDIOC_PREPARE_BUF_TIME32: {
>>> +             struct v4l2_buffer *vb = parg;
>>> +             struct v4l2_buffer_time32 vb32 = {
>>> +                     .index          = vb->index,
>>> +                     .type           = vb->type,
>>> +                     .bytesused      = vb->bytesused,
>>> +                     .flags          = vb->flags,
>>> +                     .field          = vb->field,
>>> +                     .timestamp.tv_sec       = vb->timestamp.tv_sec,
>>> +                     .timestamp.tv_usec      = vb->timestamp.tv_usec,
>>> +                     .timecode       = vb->timecode,
>>
>> You forgot to copy sequence.
> 
> Fixed.
> 
>> With these changes this patch series passed both the 64 and 32 bit compliance
>> tests (in fact, all the issues mentioned above were found with these compliance
>> tests).
> 
> Yay compliance tests!
> 
>> I am unable to test with musl since v4l2-ctl and v4l2-compliance are C++ programs,
>> and there doesn't appear to be an easy way to compile a C++ program with musl.
>>
>> If you happen to have a test environment where you can compile C++ with musl,
>> then let me know and I can give instructions on how to run the compliance tests.
>>
>> If you can't test that, then I can merge this regardless, and hope for the best
>> once the Y2038 fixes end up in glibc. But ideally I'd like to have this tested.
> 
> I've heard good things about the prebuilt toolchains from http://musl.cc/.
> These seems to come with a libstdc++, but I have not tried that myself.

I'll see if I can give those a spin, but if I can't get it to work quickly,
then I don't plan on spending much time on it.

Regards,

	Hans

> 
> I've folded the change below into this patch in my y2038-v4l2-v6 branch
> but have not been able to update the copy on git.kernel.org yet because of
> server-side issues today.
> 
>           Arnd
> 
> 8<-----
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> b/drivers/media/v4l2-core/v4l2-ioctl.c
> index c416870a3166..667225712343 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -3055,7 +3055,7 @@ static int video_get_user(void __user *arg, void
> *parg, unsigned int cmd,
>         }
> 
>         switch (cmd) {
> -#ifdef COMPAT_32BIT_TIME
> +#ifdef CONFIG_COMPAT_32BIT_TIME
>         case VIDIOC_QUERYBUF_TIME32:
>         case VIDIOC_QBUF_TIME32:
>         case VIDIOC_DQBUF_TIME32:
> @@ -3075,15 +3075,15 @@ static int video_get_user(void __user *arg,
> void *parg, unsigned int cmd,
>                         .timestamp.tv_sec       = vb32.timestamp.tv_sec,
>                         .timestamp.tv_usec      = vb32.timestamp.tv_usec,
>                         .timecode       = vb32.timecode,
> +                       .sequence       = vb32.sequence,
>                         .memory         = vb32.memory,
> -                       .m.userptr      = vb32.m.usercopy,
> +                       .m.userptr      = vb32.m.userptr,
>                         .length         = vb32.length,
>                         .request_fd     = vb32.request_fd,
>                 };
> 
>                 if (cmd == VIDIOC_QUERYBUF_TIME32)
> -                       memset(&vb->length, 0, sizeof(*vb) -
> -                              offsetof(struct v4l2_buffer, length));
> +                       vb->request_fd = 0;
> 
>                 break;
>         }
> @@ -3155,6 +3155,7 @@ static int video_put_user(void __user *arg, void
> *parg, unsigned int cmd)
>                         .timestamp.tv_sec       = vb->timestamp.tv_sec,
>                         .timestamp.tv_usec      = vb->timestamp.tv_usec,
>                         .timecode       = vb->timecode,
> +                       .sequence       = vb->sequence,
>                         .memory         = vb->memory,
>                         .m.userptr      = vb->m.userptr,
>                         .length         = vb->length,
>