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