| Message ID | 20191111203835.2260382-7-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/8] media: documentation: fix video_event description | expand |
On Mon, Nov 25, 2019 at 3:57 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > On 11/11/19 9:38 PM, Arnd Bergmann wrote: > > 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. This is the video_get_user() function, so the input data comes from user space and gets copied into the kernel, which has to check each field for validity already, so I think this is safe regardless of the padding (which exists before the 64-bit timestamp on 32-bit architectures). The fields match because the definition of all members other than the timeval is the same. On the other hand, I agree it's not obvious from the code why this is correct. I've changed my copy to this version below now, do you like that better? 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, .memory = vb32.memory, .m.userptr = vb32.usercopy, .length = vb32.length, .request_fd = vb32.request_fd, }; if (cmd == VIDIOC_QUERYBUF_TIME32) memset(&vb->length, 0, sizeof(*vb) - offsetof(struct v4l2_buffer, length)); This way, all padding is zeroed out, and it's obvious to human readers that each field gets set in the correct location. > > + 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. This is my new version: 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, .memory = vb->memory, .m.userptr = vb->usercopy, .length = vb->length, .request_fd = vb->request_fd, }; if (copy_to_user(arg, &vb32, sizeof(vb32))) return -EFAULT; > > __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. I was trying hard to avoid adding a modern version of timeval, because all new code should be encouraged to use __kernel_timespec instead. There are not many users of timeval in the uapi, and this is the last one after the others all got invididual treatment. Usually what I would do is to have a kernel-internal type based on timespec or u64, and then define three uapi types: old native (based on __kernel_old_timeval), old compat (using old_timeval32) and the new type with 64-bit time_t. The problem with v4l2_buffer is that it includes another compat-incompatible field (m.userptr) and that it's passed between kernel functions, so then I'd probably need five variants of it in total, and it would slow down the common case (64-bit native) because it would require an extra copy. I can try a few more things here, but I don't expect to find anything much better than this. > > +#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? done. > > #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. done. Arnd
On 11/26/19 2:50 PM, Arnd Bergmann wrote: > On Mon, Nov 25, 2019 at 3:57 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: >> On 11/11/19 9:38 PM, Arnd Bergmann wrote: > >>> 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. > > This is the video_get_user() function, so the input data comes from user > space and gets copied into the kernel, which has to check each field for > validity already, so I think this is safe regardless of the padding (which > exists before the 64-bit timestamp on 32-bit architectures). The fields > match because the definition of all members other than the timeval is > the same. > > On the other hand, I agree it's not obvious from the code why this > is correct. I've changed my copy to this version below now, do you like > that better? > > 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, > .memory = vb32.memory, > .m.userptr = vb32.usercopy, > .length = vb32.length, > .request_fd = vb32.request_fd, > }; > > if (cmd == VIDIOC_QUERYBUF_TIME32) > memset(&vb->length, 0, sizeof(*vb) - > offsetof(struct v4l2_buffer, length)); > > This way, all padding is zeroed out, and it's obvious to human > readers that each field gets set in the correct location. > >>> + 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. > > This is my new version: > > 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, > .memory = vb->memory, > .m.userptr = vb->usercopy, > .length = vb->length, > .request_fd = vb->request_fd, > }; That looks clean. > > if (copy_to_user(arg, &vb32, sizeof(vb32))) > return -EFAULT; > >>> __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. > > I was trying hard to avoid adding a modern version of timeval, because > all new code should be encouraged to use __kernel_timespec instead. > > There are not many users of timeval in the uapi, and this is the last one > after the others all got invididual treatment. > > Usually what I would do is to have a kernel-internal type based > on timespec or u64, and then define three uapi types: > old native (based on __kernel_old_timeval), old compat (using > old_timeval32) and the new type with 64-bit time_t. > > The problem with v4l2_buffer is that it includes another > compat-incompatible field (m.userptr) and that it's passed > between kernel functions, so then I'd probably need five variants > of it in total, and it would slow down the common case (64-bit > native) because it would require an extra copy. > > I can try a few more things here, but I don't expect to find anything > much better than this. How about something like this in videodev2.h: Split off the ugly kernel timeval definition in a separate struct: #ifdef __KERNEL__ /* match glibc timeval64 format */ 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 Then use that in the struct v4l2_buffer definition: struct v4l2_buffer { ... #ifdef __KERNEL__ struct __kernel_v4l2_timeval timestamp; #else struct timeval timestamp; #endif That keeps struct v4l2_buffer fairly clean. And it also makes it possible to have a bit more extensive documentation for the struct __kernel_v4l2_timeval without polluting the actual struct v4l2_buffer definition. The videodev2.h header is something users of the API look at a lot and having this really ugly kernel timestamp in there is not acceptably IMHO. But splitting it off should work. > >>> +#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? > > done. > >>> #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. > > done. > > Arnd > Regards, Hans
On Tue, Nov 26, 2019 at 3:15 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > > Then use that in the struct v4l2_buffer definition: > > struct v4l2_buffer { > ... > #ifdef __KERNEL__ > struct __kernel_v4l2_timeval timestamp; > #else > struct timeval timestamp; > #endif > > That keeps struct v4l2_buffer fairly clean. And it also makes it > possible to have a bit more extensive documentation for the > struct __kernel_v4l2_timeval without polluting the actual struct > v4l2_buffer definition. Yes, good idea. I've added this version now: #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 I briefly considered using #else #define __kernel_v4l2_timeval timeval to avoid the second #ifdef, but went back to your version again for clarify. > The videodev2.h header is something users of the API look at a > lot and having this really ugly kernel timestamp in there is > not acceptably IMHO. But splitting it off should work. Do you also mean moving it into a separate header file, or just outside of struct v4l2_buffer? Since it's hidden in #ifdef __KERNEL__, it could be moved to media/ioctl.h or elsewhere. Arnd
On 11/26/19 4:17 PM, Arnd Bergmann wrote: > On Tue, Nov 26, 2019 at 3:15 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: >> >> Then use that in the struct v4l2_buffer definition: >> >> struct v4l2_buffer { >> ... >> #ifdef __KERNEL__ >> struct __kernel_v4l2_timeval timestamp; >> #else >> struct timeval timestamp; >> #endif >> >> That keeps struct v4l2_buffer fairly clean. And it also makes it >> possible to have a bit more extensive documentation for the >> struct __kernel_v4l2_timeval without polluting the actual struct >> v4l2_buffer definition. > > Yes, good idea. I've added this version now: > > #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 > > I briefly considered using #else #define __kernel_v4l2_timeval timeval > to avoid the second #ifdef, but went back to your version again > for clarify. > >> The videodev2.h header is something users of the API look at a >> lot and having this really ugly kernel timestamp in there is >> not acceptably IMHO. But splitting it off should work. > > Do you also mean moving it into a separate header file, or > just outside of struct v4l2_buffer? Since it's hidden in #ifdef > __KERNEL__, it could be moved to media/ioctl.h or elsewhere. I've thought about that, but that risks having to change drivers since they would now have to include another header to get the right timeval definition. In the end I don't think it is worth the effort. I think it is best to define __kernel_v4l2_timeval just before the struct v4l2_requestbuffers definition rather than before the struct v4l2_buffer. That way it doesn't interfere with the userspace structs for the buffer API. Regards, Hans > > Arnd >
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)); + + 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)); + + 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; +#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 #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) #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)
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(-) -- 2.20.0