From: Hans Verkuil <hverkuil@xs4all.nl>
To: Arnd Bergmann <arnd@arndb.de>
Cc: Linux Media Mailing List <linux-media@vger.kernel.org>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
y2038 Mailman List <y2038@lists.linaro.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5 6/8] media: v4l2-core: fix v4l2_buffer handling for time64 ABI
Date: Fri, 13 Dec 2019 16:32:58 +0100 [thread overview]
Message-ID: <81bb5da1-6b84-8473-4ada-c174f43bbae2@xs4all.nl> (raw)
In-Reply-To: <CAK8P3a1TvFCJf8t9T1yOXjsp088s9dbEOKLVDPinfwJe2B-27g@mail.gmail.com>
On 12/13/19 4:08 PM, Arnd Bergmann wrote:
> On Thu, Dec 12, 2019 at 4:43 PM Hans Verkuil <hverkuil@xs4all.nl> 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,
>
next prev parent reply other threads:[~2019-12-13 20:38 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-26 16:18 [PATCH v5 0/8] y2038 safety in v4l2 Arnd Bergmann
2019-11-26 16:18 ` [PATCH v5 1/8] media: documentation: fix video_event description Arnd Bergmann
2019-11-26 16:18 ` [PATCH v5 2/8] media: v4l2: abstract timeval handling in v4l2_buffer Arnd Bergmann
2019-11-26 16:18 ` [PATCH v5 3/8] media: v4l2-core: compat: ignore native command codes Arnd Bergmann
2019-11-26 16:18 ` [PATCH v5 4/8] media: v4l2-core: split out data copy from video_usercopy Arnd Bergmann
2019-11-26 16:18 ` [PATCH v5 5/8] media: v4l2-core: fix VIDIOC_DQEVENT for time64 ABI Arnd Bergmann
2019-11-26 16:18 ` [PATCH v5 6/8] media: v4l2-core: fix v4l2_buffer handling " Arnd Bergmann
2019-12-12 15:43 ` Hans Verkuil
2019-12-13 15:08 ` Arnd Bergmann
2019-12-13 15:32 ` Hans Verkuil [this message]
2019-12-13 15:38 ` Arnd Bergmann
2019-12-14 11:27 ` Hans Verkuil
2019-12-14 21:44 ` Arnd Bergmann
2019-12-15 17:26 ` Hans Verkuil
2019-12-16 9:29 ` Arnd Bergmann
2019-12-16 10:28 ` Hans Verkuil
2019-12-19 23:26 ` Zach van Rijn
2019-11-26 16:18 ` [PATCH v5 7/8] media: v4l2-core: fix compat VIDIOC_DQEVENT " Arnd Bergmann
2019-11-26 16:18 ` [PATCH v5 8/8] media: v4l2-core: fix compat v4l2_buffer handling " Arnd Bergmann
2019-11-26 19:55 ` [PATCH v5.1 4/8] media: v4l2-core: split out data copy from video_usercopy Arnd Bergmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=81bb5da1-6b84-8473-4ada-c174f43bbae2@xs4all.nl \
--to=hverkuil@xs4all.nl \
--cc=arnd@arndb.de \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=y2038@lists.linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).