From: Hans Verkuil <hverkuil@xs4all.nl>
To: Arnd Bergmann <arnd@arndb.de>,
linux-media@vger.kernel.org,
Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: y2038@lists.linaro.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 6/8] media: v4l2-core: fix v4l2_buffer handling for time64 ABI
Date: Thu, 12 Dec 2019 16:43:55 +0100 [thread overview]
Message-ID: <09c664fd-87fb-4fac-f104-9afbe7d33aa2@xs4all.nl> (raw)
In-Reply-To: <20191126161824.337724-7-arnd@arndb.de>
On 11/26/19 5:18 PM, Arnd Bergmann wrote:
> The v4l2_buffer structure contains a 'struct timeval' member that is
> defined by the user space C library, creating an ABI incompatibility
> when that gets updated to a 64-bit time_t.
>
> As in v4l2_event, handle this with a special case in video_put_user()
> and video_get_user() to replace the memcpy there.
>
> Since the structure also contains a pointer, there are now two
> native versions (on 32-bit systems) as well as two compat versions
> (on 64-bit systems), which unfortunately complicates the compat
> handler quite a bit.
>
> Duplicating the existing handlers for the new types is a safe
> conversion for now, but unfortunately this may turn into a
> maintenance burden later. A larger-scale rework of the
> compat code might be a better alternative, but is out of scope
> of the y2038 work.
>
> Sparc64 needs a special case because of their special suseconds_t
> definition.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> drivers/media/v4l2-core/v4l2-ioctl.c | 73 ++++++++++++++++++++++++++--
> include/media/v4l2-ioctl.h | 30 ++++++++++++
> include/uapi/linux/videodev2.h | 23 +++++++++
> 3 files changed, 122 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 96aafb659783..4d611a847462 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -474,10 +474,10 @@ static void v4l_print_buffer(const void *arg, bool write_only)
> const struct v4l2_plane *plane;
> int i;
>
> - pr_cont("%02ld:%02d:%02d.%08ld index=%d, type=%s, request_fd=%d, flags=0x%08x, field=%s, sequence=%d, memory=%s",
> - p->timestamp.tv_sec / 3600,
> - (int)(p->timestamp.tv_sec / 60) % 60,
> - (int)(p->timestamp.tv_sec % 60),
> + pr_cont("%02d:%02d:%02d.%09ld index=%d, type=%s, request_fd=%d, flags=0x%08x, field=%s, sequence=%d, memory=%s",
> + (int)p->timestamp.tv_sec / 3600,
> + ((int)p->timestamp.tv_sec / 60) % 60,
> + ((int)p->timestamp.tv_sec % 60),
> (long)p->timestamp.tv_usec,
> p->index,
> prt_names(p->type, v4l2_type_names), p->request_fd,
> @@ -3029,6 +3029,14 @@ static unsigned int video_translate_cmd(unsigned int cmd)
> #ifdef CONFIG_COMPAT_32BIT_TIME
> case VIDIOC_DQEVENT_TIME32:
> return VIDIOC_DQEVENT;
> + case VIDIOC_QUERYBUF_TIME32:
> + return VIDIOC_QUERYBUF;
> + case VIDIOC_QBUF_TIME32:
> + return VIDIOC_QBUF;
> + case VIDIOC_DQBUF_TIME32:
> + return VIDIOC_DQBUF;
> + case VIDIOC_PREPARE_BUF_TIME32:
> + return VIDIOC_PREPARE_BUF;
> #endif
> }
>
> @@ -3047,6 +3055,39 @@ static int video_get_user(void __user *arg, void *parg, unsigned int cmd,
> }
>
> switch (cmd) {
> +#ifdef COMPAT_32BIT_TIME
COMPAT_32BIT_TIME -> CONFIG_COMPAT_32BIT_TIME
> + case VIDIOC_QUERYBUF_TIME32:
> + case VIDIOC_QBUF_TIME32:
> + case VIDIOC_DQBUF_TIME32:
> + case VIDIOC_PREPARE_BUF_TIME32: {
> + struct v4l2_buffer_time32 vb32;
> + struct v4l2_buffer *vb = parg;
> +
> + if (copy_from_user(&vb32, arg, sizeof(vb32)))
> + return -EFAULT;
> +
> + *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
> + .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.
> +
> + break;
> + }
> +#endif
> default:
> /*
> * In some cases, only a few fields are used as input,
> @@ -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.
> + .memory = vb->memory,
> + .m.userptr = vb->m.userptr,
> + .length = vb->length,
> + .request_fd = vb->request_fd,
> + };
> +
> + if (copy_to_user(arg, &vb32, sizeof(vb32)))
> + return -EFAULT;
> + break;
> + }
> #endif
> default:
> /* Copy results into user buffer */
> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> index 05c1ec93a911..86878fba332b 100644
> --- a/include/media/v4l2-ioctl.h
> +++ b/include/media/v4l2-ioctl.h
> @@ -749,4 +749,34 @@ struct v4l2_event_time32 {
>
> #define VIDIOC_DQEVENT_TIME32 _IOR('V', 89, struct v4l2_event_time32)
>
> +struct v4l2_buffer_time32 {
> + __u32 index;
> + __u32 type;
> + __u32 bytesused;
> + __u32 flags;
> + __u32 field;
> + struct old_timeval32 timestamp;
> + struct v4l2_timecode timecode;
> + __u32 sequence;
> +
> + /* memory location */
> + __u32 memory;
> + union {
> + __u32 offset;
> + unsigned long userptr;
> + struct v4l2_plane *planes;
> + __s32 fd;
> + } m;
> + __u32 length;
> + __u32 reserved2;
> + union {
> + __s32 request_fd;
> + __u32 reserved;
> + };
> +};
> +#define VIDIOC_QUERYBUF_TIME32 _IOWR('V', 9, struct v4l2_buffer_time32)
> +#define VIDIOC_QBUF_TIME32 _IOWR('V', 15, struct v4l2_buffer_time32)
> +#define VIDIOC_DQBUF_TIME32 _IOWR('V', 17, struct v4l2_buffer_time32)
> +#define VIDIOC_PREPARE_BUF_TIME32 _IOWR('V', 93, struct v4l2_buffer_time32)
> +
> #endif /* _V4L2_IOCTL_H */
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index caf156d45842..5f9357dcb060 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -912,6 +912,25 @@ struct v4l2_jpegcompression {
> /*
> * M E M O R Y - M A P P I N G B U F F E R S
> */
> +
> +#ifdef __KERNEL__
> +/*
> + * This corresponds to the user space version of timeval
> + * for 64-bit time_t. sparc64 is different from everyone
> + * else, using the microseconds in the wrong half of the
> + * second 64-bit word.
> + */
> +struct __kernel_v4l2_timeval {
> + long long tv_sec;
> +#if defined(__sparc__) && defined(__arch64__)
> + int tv_usec;
> + int __pad;
> +#else
> + long long tv_usec;
> +#endif
> +};
> +#endif
> +
> struct v4l2_requestbuffers {
> __u32 count;
> __u32 type; /* enum v4l2_buf_type */
> @@ -997,7 +1016,11 @@ struct v4l2_buffer {
> __u32 bytesused;
> __u32 flags;
> __u32 field;
> +#ifdef __KERNEL__
> + struct __kernel_v4l2_timeval timestamp;
> +#else
> struct timeval timestamp;
> +#endif
> struct v4l2_timecode timecode;
> __u32 sequence;
>
>
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).
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.
Regards,
Hans
next prev parent reply other threads:[~2019-12-12 15:44 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 [this message]
2019-12-13 15:08 ` Arnd Bergmann
2019-12-13 15:32 ` Hans Verkuil
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=09c664fd-87fb-4fac-f104-9afbe7d33aa2@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).