| Message ID | 20191111203835.2260382-1-arnd@arndb.de |
|---|---|
| Headers | show |
| Series | y2038 safety in v4l2 | expand |
On 11/11/19 9:38 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 | 57 ++++++++++++++++++++++++++-- > include/uapi/linux/videodev2.h | 45 ++++++++++++++++++++++ > 2 files changed, 98 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > index 1de939d11628..4ae1bcaec3fa 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, > @@ -3014,6 +3014,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 > } > > @@ -3032,6 +3040,30 @@ static int video_get_user(void __user *arg, void *parg, unsigned int cmd, > } > > switch (cmd) { > +#ifdef 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; > + > + memcpy(vb, &vb32, offsetof(struct v4l2_buffer, timestamp)); > + vb->timestamp.tv_sec = vb32.timestamp.tv_sec; > + vb->timestamp.tv_usec = vb32.timestamp.tv_usec; > + memcpy(&vb->timecode, &vb32.timecode, > + sizeof(*vb) - offsetof(struct v4l2_buffer, timecode)); I have similar concerns as with dqevent about whether this memcpy is the right approach. Unless you can prove with a utility like pahole that this memcpy is safe. > + > + if (cmd == VIDIOC_QUERYBUF_TIME32) > + memset(&vb->length, 0, sizeof(*vb) - > + offsetof(struct v4l2_buffer, length)); > + > + break; > + } > +#endif > default: > /* > * In some cases, only a few fields are used as input, > @@ -3080,6 +3112,23 @@ 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_time32 vb32; > + struct v4l2_buffer *vb = parg; > + > + memcpy(&vb32, vb, offsetof(struct v4l2_buffer, timestamp)); > + vb32.timestamp.tv_sec = vb->timestamp.tv_sec; > + vb32.timestamp.tv_usec = vb->timestamp.tv_usec; > + memcpy(&vb32.timecode, &vb->timecode, > + sizeof(*vb) - offsetof(struct v4l2_buffer, timecode)); Ditto. > + > + if (copy_to_user(arg, &vb32, sizeof(vb32))) > + return -EFAULT; > + break; > + } > #endif > default: > /* Copy results into user buffer */ > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index 1d2553d4ed5b..f05c54d63f96 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -990,7 +990,47 @@ struct v4l2_buffer { > __u32 bytesused; > __u32 flags; > __u32 field; > +#ifdef __KERNEL__ > + /* match glibc timeval64 format */ > + struct { > + long long tv_sec; > +# if defined(__sparc__) && defined(__arch64__) > + int tv_usec; > + int __pad; > +# else > + long long tv_usec; > +# endif > + } timestamp; Ewww! Are there more places where this is needed? If so, then I very much prefer that a __kernel_timeval struct is defined somewhere, with appropriate comments. > +#else > struct timeval timestamp; > +#endif > + 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; > + }; > +}; > + > +#ifdef __KERNEL__ > +struct v4l2_buffer_time32 { > + __u32 index; > + __u32 type; > + __u32 bytesused; > + __u32 flags; > + __u32 field; > + struct old_timeval32 timestamp; > struct v4l2_timecode timecode; > __u32 sequence; > > @@ -1009,6 +1049,7 @@ struct v4l2_buffer { > __u32 reserved; > }; > }; > +#endif Can this be moved to v4l2-ioctls.h? > > #ifndef __KERNEL__ > /** > @@ -2446,12 +2487,15 @@ struct v4l2_create_buffers { > #define VIDIOC_S_FMT _IOWR('V', 5, struct v4l2_format) > #define VIDIOC_REQBUFS _IOWR('V', 8, struct v4l2_requestbuffers) > #define VIDIOC_QUERYBUF _IOWR('V', 9, struct v4l2_buffer) > +#define VIDIOC_QUERYBUF_TIME32 _IOWR('V', 9, struct v4l2_buffer_time32) And all these should be moved there as well. > #define VIDIOC_G_FBUF _IOR('V', 10, struct v4l2_framebuffer) > #define VIDIOC_S_FBUF _IOW('V', 11, struct v4l2_framebuffer) > #define VIDIOC_OVERLAY _IOW('V', 14, int) > #define VIDIOC_QBUF _IOWR('V', 15, struct v4l2_buffer) > +#define VIDIOC_QBUF_TIME32 _IOWR('V', 15, struct v4l2_buffer_time32) > #define VIDIOC_EXPBUF _IOWR('V', 16, struct v4l2_exportbuffer) > #define VIDIOC_DQBUF _IOWR('V', 17, struct v4l2_buffer) > +#define VIDIOC_DQBUF_TIME32 _IOWR('V', 17, struct v4l2_buffer_time32) > #define VIDIOC_STREAMON _IOW('V', 18, int) > #define VIDIOC_STREAMOFF _IOW('V', 19, int) > #define VIDIOC_G_PARM _IOWR('V', 21, struct v4l2_streamparm) > @@ -2520,6 +2564,7 @@ struct v4l2_create_buffers { > #define VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91, struct v4l2_event_subscription) > #define VIDIOC_CREATE_BUFS _IOWR('V', 92, struct v4l2_create_buffers) > #define VIDIOC_PREPARE_BUF _IOWR('V', 93, struct v4l2_buffer) > +#define VIDIOC_PREPARE_BUF_TIME32 _IOWR('V', 93, struct v4l2_buffer_time32) > #define VIDIOC_G_SELECTION _IOWR('V', 94, struct v4l2_selection) > #define VIDIOC_S_SELECTION _IOWR('V', 95, struct v4l2_selection) > #define VIDIOC_DECODER_CMD _IOWR('V', 96, struct v4l2_decoder_cmd) > Regards, Hans
On 11/11/19 9:38 PM, Arnd Bergmann wrote: > As a preparation for adding 64-bit time_t support in the uapi, > change the drivers to no longer care about the format of the > timestamp field in struct v4l2_buffer. > > The v4l2_timeval_to_ns() function is no longer needed in the > kernel after this, but there may be userspace code relying on > it because it is part of the uapi header. There is indeed userspace code that relies on this. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/media/common/videobuf2/videobuf2-v4l2.c | 4 ++-- > drivers/media/pci/meye/meye.c | 4 ++-- > drivers/media/usb/cpia2/cpia2_v4l.c | 4 ++-- > drivers/media/usb/stkwebcam/stk-webcam.c | 2 +- > drivers/media/usb/usbvision/usbvision-video.c | 4 ++-- > drivers/media/v4l2-core/videobuf-core.c | 4 ++-- > include/linux/videodev2.h | 17 ++++++++++++++++- > include/trace/events/v4l2.h | 2 +- > include/uapi/linux/videodev2.h | 2 ++ > 9 files changed, 30 insertions(+), 13 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c > index 5a9ba3846f0a..9ec710878db6 100644 > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c > @@ -143,7 +143,7 @@ static void __copy_timestamp(struct vb2_buffer *vb, const void *pb) > * and the timecode field and flag if needed. > */ > if (q->copy_timestamp) > - vb->timestamp = v4l2_timeval_to_ns(&b->timestamp); > + vb->timestamp = v4l2_buffer_get_timestamp(b); > vbuf->flags |= b->flags & V4L2_BUF_FLAG_TIMECODE; > if (b->flags & V4L2_BUF_FLAG_TIMECODE) > vbuf->timecode = b->timecode; > @@ -476,7 +476,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb) > > b->flags = vbuf->flags; > b->field = vbuf->field; > - b->timestamp = ns_to_timeval(vb->timestamp); > + v4l2_buffer_set_timestamp(b, vb->timestamp); > b->timecode = vbuf->timecode; > b->sequence = vbuf->sequence; > b->reserved2 = 0; > diff --git a/drivers/media/pci/meye/meye.c b/drivers/media/pci/meye/meye.c > index 0e61c81356ef..3a4c29bc0ba5 100644 > --- a/drivers/media/pci/meye/meye.c > +++ b/drivers/media/pci/meye/meye.c > @@ -1266,7 +1266,7 @@ static int vidioc_querybuf(struct file *file, void *fh, struct v4l2_buffer *buf) > buf->flags |= V4L2_BUF_FLAG_DONE; > > buf->field = V4L2_FIELD_NONE; > - buf->timestamp = ns_to_timeval(meye.grab_buffer[index].ts); > + v4l2_buffer_set_timestamp(buf, meye.grab_buffer[index].ts); > buf->sequence = meye.grab_buffer[index].sequence; > buf->memory = V4L2_MEMORY_MMAP; > buf->m.offset = index * gbufsize; > @@ -1332,7 +1332,7 @@ static int vidioc_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf) > buf->bytesused = meye.grab_buffer[reqnr].size; > buf->flags = V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; > buf->field = V4L2_FIELD_NONE; > - buf->timestamp = ns_to_timeval(meye.grab_buffer[reqnr].ts); > + v4l2_buffer_set_timestamp(buf, meye.grab_buffer[reqnr].ts); > buf->sequence = meye.grab_buffer[reqnr].sequence; > buf->memory = V4L2_MEMORY_MMAP; > buf->m.offset = reqnr * gbufsize; > diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c b/drivers/media/usb/cpia2/cpia2_v4l.c > index 626264a56517..9d3d05125d7b 100644 > --- a/drivers/media/usb/cpia2/cpia2_v4l.c > +++ b/drivers/media/usb/cpia2/cpia2_v4l.c > @@ -800,7 +800,7 @@ static int cpia2_querybuf(struct file *file, void *fh, struct v4l2_buffer *buf) > break; > case FRAME_READY: > buf->bytesused = cam->buffers[buf->index].length; > - buf->timestamp = ns_to_timeval(cam->buffers[buf->index].ts); > + v4l2_buffer_set_timestamp(buf, cam->buffers[buf->index].ts); > buf->sequence = cam->buffers[buf->index].seq; > buf->flags = V4L2_BUF_FLAG_DONE; > break; > @@ -907,7 +907,7 @@ static int cpia2_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf) > buf->flags = V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_DONE > | V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; > buf->field = V4L2_FIELD_NONE; > - buf->timestamp = ns_to_timeval(cam->buffers[buf->index].ts); > + v4l2_buffer_set_timestamp(buf, cam->buffers[buf->index].ts); > buf->sequence = cam->buffers[buf->index].seq; > buf->m.offset = cam->buffers[buf->index].data - cam->frame_buffer; > buf->length = cam->frame_size; > diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c > index 21f90a887485..b22501f76b78 100644 > --- a/drivers/media/usb/stkwebcam/stk-webcam.c > +++ b/drivers/media/usb/stkwebcam/stk-webcam.c > @@ -1125,7 +1125,7 @@ static int stk_vidioc_dqbuf(struct file *filp, > sbuf->v4lbuf.flags &= ~V4L2_BUF_FLAG_QUEUED; > sbuf->v4lbuf.flags |= V4L2_BUF_FLAG_DONE; > sbuf->v4lbuf.sequence = ++dev->sequence; > - sbuf->v4lbuf.timestamp = ns_to_timeval(ktime_get_ns()); > + v4l2_buffer_set_timestamp(&sbuf->v4lbuf, ktime_get_ns()); > > *buf = sbuf->v4lbuf; > return 0; > diff --git a/drivers/media/usb/usbvision/usbvision-video.c b/drivers/media/usb/usbvision/usbvision-video.c > index cdc66adda755..15a423c5deb7 100644 > --- a/drivers/media/usb/usbvision/usbvision-video.c > +++ b/drivers/media/usb/usbvision/usbvision-video.c > @@ -687,7 +687,7 @@ static int vidioc_querybuf(struct file *file, > vb->length = usbvision->curwidth * > usbvision->curheight * > usbvision->palette.bytes_per_pixel; > - vb->timestamp = ns_to_timeval(usbvision->frame[vb->index].ts); > + v4l2_buffer_set_timestamp(vb, usbvision->frame[vb->index].ts); > vb->sequence = usbvision->frame[vb->index].sequence; > return 0; > } > @@ -756,7 +756,7 @@ static int vidioc_dqbuf(struct file *file, void *priv, struct v4l2_buffer *vb) > V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; > vb->index = f->index; > vb->sequence = f->sequence; > - vb->timestamp = ns_to_timeval(f->ts); > + v4l2_buffer_set_timestamp(vb, f->ts); > vb->field = V4L2_FIELD_NONE; > vb->bytesused = f->scanlength; > > diff --git a/drivers/media/v4l2-core/videobuf-core.c b/drivers/media/v4l2-core/videobuf-core.c > index 939fc11cf080..ab650371c151 100644 > --- a/drivers/media/v4l2-core/videobuf-core.c > +++ b/drivers/media/v4l2-core/videobuf-core.c > @@ -364,7 +364,7 @@ static void videobuf_status(struct videobuf_queue *q, struct v4l2_buffer *b, > } > > b->field = vb->field; > - b->timestamp = ns_to_timeval(vb->ts); > + v4l2_buffer_set_timestamp(b, vb->ts); > b->bytesused = vb->size; > b->sequence = vb->field_count >> 1; > } > @@ -578,7 +578,7 @@ int videobuf_qbuf(struct videobuf_queue *q, struct v4l2_buffer *b) > || q->type == V4L2_BUF_TYPE_SDR_OUTPUT) { > buf->size = b->bytesused; > buf->field = b->field; > - buf->ts = v4l2_timeval_to_ns(&b->timestamp); > + buf->ts = v4l2_buffer_get_timestamp(b); > } > break; > case V4L2_MEMORY_USERPTR: > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h > index 16c0ed6c50a7..4086036e37d5 100644 > --- a/include/linux/videodev2.h > +++ b/include/linux/videodev2.h > @@ -56,7 +56,22 @@ > #ifndef __LINUX_VIDEODEV2_H > #define __LINUX_VIDEODEV2_H > > -#include <linux/time.h> /* need struct timeval */ > +#include <linux/time.h> > #include <uapi/linux/videodev2.h> > > +static inline u64 v4l2_buffer_get_timestamp(const struct v4l2_buffer *buf) > +{ > + return buf->timestamp.tv_sec * NSEC_PER_SEC + > + (u32)buf->timestamp.tv_usec * NSEC_PER_USEC; Why the (u32) cast? > +} > + > +static inline void v4l2_buffer_set_timestamp(struct v4l2_buffer *buf, > + u64 timestamp) > +{ > + struct timespec64 ts = ns_to_timespec64(timestamp); > + > + buf->timestamp.tv_sec = ts.tv_sec; > + buf->timestamp.tv_usec = ts.tv_nsec / NSEC_PER_USEC; > +} > + This does not belong in the public header. This is kernel specific, so media/v4l2-common.h would be a good place. Regards, Hans > #endif /* __LINUX_VIDEODEV2_H */ > diff --git a/include/trace/events/v4l2.h b/include/trace/events/v4l2.h > index 83860de120e3..248bc09bfc99 100644 > --- a/include/trace/events/v4l2.h > +++ b/include/trace/events/v4l2.h > @@ -130,7 +130,7 @@ DECLARE_EVENT_CLASS(v4l2_event_class, > __entry->bytesused = buf->bytesused; > __entry->flags = buf->flags; > __entry->field = buf->field; > - __entry->timestamp = timeval_to_ns(&buf->timestamp); > + __entry->timestamp = v4l2_buffer_get_timestamp(buf); > __entry->timecode_type = buf->timecode.type; > __entry->timecode_flags = buf->timecode.flags; > __entry->timecode_frames = buf->timecode.frames; > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index 530638dffd93..74d3d522f3db 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -1010,6 +1010,7 @@ struct v4l2_buffer { > }; > }; > > +#ifndef __KERNEL__ > /** > * v4l2_timeval_to_ns - Convert timeval to nanoseconds > * @ts: pointer to the timeval variable to be converted > @@ -1021,6 +1022,7 @@ static inline __u64 v4l2_timeval_to_ns(const struct timeval *tv) > { > return (__u64)tv->tv_sec * 1000000000ULL + tv->tv_usec * 1000; > } > +#endif > > /* Flags for 'flags' field */ > /* Buffer is mapped (flag) */ >
Hi Arnd, On 11/11/19 9:38 PM, Arnd Bergmann wrote: > I'm in the process of finishing up the last bits on y2038-unsafe > code in the kernel, this series is for v4l2, which has no problem > with overflow, but has multiple ioctls that break with user space > built against a new 32-bit libc. Thank you for working on this. Much appreciated! I've reviewed this v4 series and found a few issues (mostly things that ended up in videodev2.h that shouldn't be there). Once a v5 is posted I'll try to test this more. Is there an easy(-ish) way to test this with a time64 ABI? Do you have such an environment set up for testing? > > I posted similar patches as part of a series back in 2015, the > new version was rewritten from scratch and I double-checked with > the old version to make sure I did not miss anything I had already > taken care of before. > > Hans Verkuil worked on a different patch set in 2017, but this > also did not get to the point of being merged. > > My new version contains compat-ioctl support, which the old one > did not and should be complete, but given its size likely contains > bugs. I did randconfig build tests, but no runtime test, so > careful review as well as testing would be much appreciated. > > With this version, the newly added code takes care of the existing > ABI, while the existing code got moved to the 64-bit time_t > interface and is used internally. This means that testing with > existing binaries should exercise most of the modifications > and if that works and doesn't get shot down in review, we can > probably live without testing the new ABI explicitly. > > I'm not entirely happy with the compat-ioctl implementation that > adds quite a bit of code duplication, but I hope this is > acceptable anyway, as a better implementation would likely > require a larger refactoring of the compat-ioctl file, while > my approach simply adds support for the additional data structure > variants. A cleanup is indeed much more work. This y2038 code is awful, but that's really because the original structs are aligned in the worst possible way. Regards, Hans > > This is a minor update compared to version 3 of this series, > with bugfixes for small mistakes that I found or that were > reported by automated build bots. I updated the tree at [2] > to this version now. > > Arnd > > [1] https://lwn.net/Articles/657754/ > [2] https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=y2038-v4l2 > > Arnd Bergmann (8): > media: documentation: fix video_event description > media: v4l2: abstract timeval handling in v4l2_buffer > media: v4l2-core: compat: ignore native command codes > media: v4l2-core: split out data copy from video_usercopy > media: v4l2-core: fix VIDIOC_DQEVENT for time64 ABI > media: v4l2-core: fix v4l2_buffer handling for time64 ABI > media: v4l2-core: fix compat VIDIOC_DQEVENT for time64 ABI > media: v4l2-core: fix compat v4l2_buffer handling for time64 ABI > > .../media/uapi/dvb/video-get-event.rst | 2 +- > Documentation/media/uapi/dvb/video_types.rst | 2 +- > .../media/common/videobuf2/videobuf2-v4l2.c | 4 +- > drivers/media/pci/meye/meye.c | 4 +- > drivers/media/usb/cpia2/cpia2_v4l.c | 4 +- > drivers/media/usb/stkwebcam/stk-webcam.c | 2 +- > drivers/media/usb/usbvision/usbvision-video.c | 4 +- > drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 470 +++++++++++++++--- > drivers/media/v4l2-core/v4l2-event.c | 5 +- > drivers/media/v4l2-core/v4l2-ioctl.c | 188 +++++-- > drivers/media/v4l2-core/v4l2-subdev.c | 20 +- > drivers/media/v4l2-core/videobuf-core.c | 4 +- > include/linux/videodev2.h | 17 +- > include/trace/events/v4l2.h | 2 +- > include/uapi/linux/videodev2.h | 77 +++ > 15 files changed, 669 insertions(+), 136 deletions(-) >
I'm in the process of finishing up the last bits on y2038-unsafe code in the kernel, this series is for v4l2, which has no problem with overflow, but has multiple ioctls that break with user space built against a new 32-bit libc. I posted similar patches as part of a series back in 2015, the new version was rewritten from scratch and I double-checked with the old version to make sure I did not miss anything I had already taken care of before. Hans Verkuil worked on a different patch set in 2017, but this also did not get to the point of being merged. My new version contains compat-ioctl support, which the old one did not and should be complete, but given its size likely contains bugs. I did randconfig build tests, but no runtime test, so careful review as well as testing would be much appreciated. With this version, the newly added code takes care of the existing ABI, while the existing code got moved to the 64-bit time_t interface and is used internally. This means that testing with existing binaries should exercise most of the modifications and if that works and doesn't get shot down in review, we can probably live without testing the new ABI explicitly. I'm not entirely happy with the compat-ioctl implementation that adds quite a bit of code duplication, but I hope this is acceptable anyway, as a better implementation would likely require a larger refactoring of the compat-ioctl file, while my approach simply adds support for the additional data structure variants. This is a minor update compared to version 3 of this series, with bugfixes for small mistakes that I found or that were reported by automated build bots. I updated the tree at [2] to this version now. Arnd [1] https://lwn.net/Articles/657754/ [2] https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=y2038-v4l2 Arnd Bergmann (8): media: documentation: fix video_event description media: v4l2: abstract timeval handling in v4l2_buffer media: v4l2-core: compat: ignore native command codes media: v4l2-core: split out data copy from video_usercopy media: v4l2-core: fix VIDIOC_DQEVENT for time64 ABI media: v4l2-core: fix v4l2_buffer handling for time64 ABI media: v4l2-core: fix compat VIDIOC_DQEVENT for time64 ABI media: v4l2-core: fix compat v4l2_buffer handling for time64 ABI .../media/uapi/dvb/video-get-event.rst | 2 +- Documentation/media/uapi/dvb/video_types.rst | 2 +- .../media/common/videobuf2/videobuf2-v4l2.c | 4 +- drivers/media/pci/meye/meye.c | 4 +- drivers/media/usb/cpia2/cpia2_v4l.c | 4 +- drivers/media/usb/stkwebcam/stk-webcam.c | 2 +- drivers/media/usb/usbvision/usbvision-video.c | 4 +- drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 470 +++++++++++++++--- drivers/media/v4l2-core/v4l2-event.c | 5 +- drivers/media/v4l2-core/v4l2-ioctl.c | 188 +++++-- drivers/media/v4l2-core/v4l2-subdev.c | 20 +- drivers/media/v4l2-core/videobuf-core.c | 4 +- include/linux/videodev2.h | 17 +- include/trace/events/v4l2.h | 2 +- include/uapi/linux/videodev2.h | 77 +++ 15 files changed, 669 insertions(+), 136 deletions(-) -- 2.20.0 See below for the changes compared to v3: |diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c |index a13e4849df0c..3bbf47d950e0 100644 |--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c |+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c |@@ -500,7 +500,7 @@ struct v4l2_buffer32_time32 { | __u32 bytesused; | __u32 flags; | __u32 field; /* enum v4l2_field */ |- struct compat_timeval timestamp; |+ struct old_timeval32 timestamp; | struct v4l2_timecode timecode; | __u32 sequence; | |@@ -1290,7 +1290,7 @@ struct v4l2_event32_time32 { | } u; | __u32 pending; | __u32 sequence; |- struct compat_timespec timestamp; |+ struct old_timespec32 timestamp; | __u32 id; | __u32 reserved[8]; | }; |@@ -1482,8 +1482,8 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar | case VIDIOC_S_EXT_CTRLS32: ncmd = VIDIOC_S_EXT_CTRLS; break; | case VIDIOC_TRY_EXT_CTRLS32: ncmd = VIDIOC_TRY_EXT_CTRLS; break; | #ifdef CONFIG_X86_64 |- case VIDIOC_DQEVENT32: cmd = VIDIOC_DQEVENT; break; |- case VIDIOC_DQEVENT32_TIME32: cmd = VIDIOC_DQEVENT_TIME32; break; |+ case VIDIOC_DQEVENT32: ncmd = VIDIOC_DQEVENT; break; |+ case VIDIOC_DQEVENT32_TIME32: ncmd = VIDIOC_DQEVENT_TIME32; break; | #endif | case VIDIOC_OVERLAY32: ncmd = VIDIOC_OVERLAY; break; | case VIDIOC_STREAMON32: ncmd = VIDIOC_STREAMON; break; |diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c |index cd9a80960289..ad125cd4eb41 100644 |--- a/drivers/media/v4l2-core/v4l2-ioctl.c |+++ b/drivers/media/v4l2-core/v4l2-ioctl.c |@@ -474,7 +474,7 @@ static void v4l_print_buffer(const void *arg, bool write_only) | const struct v4l2_plane *plane; | int i; | |- pr_cont("%02ld:%02d:%02d.%09ld index=%d, type=%s, request_fd=%d, flags=0x%08x, field=%s, sequence=%d, memory=%s", |+ 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), |@@ -821,7 +821,7 @@ static void v4l_print_event(const void *arg, bool write_only) | const struct v4l2_event *p = arg; | const struct v4l2_event_ctrl *c; | |- pr_cont("type=0x%x, pending=%u, sequence=%u, id=%u, timestamp=%lu.%9.9lu\n", |+ pr_cont("type=0x%x, pending=%u, sequence=%u, id=%u, timestamp=%llu.%9.9llu\n", | p->type, p->pending, p->sequence, p->id, | p->timestamp.tv_sec, p->timestamp.tv_nsec); | switch (p->type) { |diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h |index 481ee3013b50..4086036e37d5 100644 |--- a/include/linux/videodev2.h |+++ b/include/linux/videodev2.h |@@ -62,7 +62,7 @@ | static inline u64 v4l2_buffer_get_timestamp(const struct v4l2_buffer *buf) | { | return buf->timestamp.tv_sec * NSEC_PER_SEC + |- buf->timestamp.tv_usec * NSEC_PER_USEC; |+ (u32)buf->timestamp.tv_usec * NSEC_PER_USEC; | } | | static inline void v4l2_buffer_set_timestamp(struct v4l2_buffer *buf, |diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h |index 4275d6e92dae..ca10828328a5 100644 |--- a/include/uapi/linux/videodev2.h |+++ b/include/uapi/linux/videodev2.h |@@ -1001,7 +1001,12 @@ struct v4l2_buffer { | /* match glibc timeval64 format */ | struct { | long long tv_sec; |+# if defined(__sparc__) && defined(__arch64__) |+ int tv_usec; |+ int __pad; |+# else | long long tv_usec; |+# endif | } timestamp; | #else | struct timeval timestamp; |