| Message ID | 20200917152823.1241599-1-arnd@arndb.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | media: v4l2: simplify compat ioctl handling | expand |
Hi Arnd, On 17/09/2020 17:28, Arnd Bergmann wrote: > I have a series to remove all uses of compat_alloc_user_space() > and copy_in_user() from the kernel, this is the part of it that > involves the v4l2 compat code. > > The resulting code is significantly shorter and arguably more > readable, but I have not done any testing beyond compilation on > it, so at the minimum this first needs to pass the test suite > for both native and compat users space. > > Given the complexity of the code, I am fairly sure that there > are bugs I missed. > > Please review and test if possible. While testing I found a lot of bugs. Below is a patch that sits on top of your series and fixes all the bugs: - put_v4l2_standard32: typo: p64->index -> p64->id - get_v4l2_plane32: typo: p64 -> plane32 typo: duplicate bytesused: the 2nd should be 'length' - put_v4l2_plane32: typo: duplicate bytesused: the 2nd should be 'length' - get_v4l2_buffer32/get_v4l2_buffer32_time32: missing compat_ptr for vb32.m.userptr - get_v4l2_buffer32/get_v4l2_buffer32_time32: drop querybuf bool - put_v4l2_buffer32/put_v4l2_buffer32_time32: add uintptr_t cast for vb->m.userptr - get_v4l2_ext_controls32: typo: error_idx -> request_fd - put_v4l2_ext_controls32: typo: error_idx -> request_fd - v4l2_compat_translate_cmd: missing case VIDIOC_CREATE_BUFS32 - v4l2_compat_translate_cmd: #ifdef CONFIG_COMPAT_32BIT_TIME for case VIDIOC_DQEVENT32_TIME32 and return VIDIOC_DQEVENT - v4l2_compat_put_user: #ifdef CONFIG_COMPAT_32BIT_TIME for case VIDIOC_DQEVENT32_TIME32 - v4l2_compat_get_array_args: typo: p64 -> b64 - v4l2_compat_put_array_args: typo: p64 -> b64 - video_get_user: make sure INFO_FL_CLEAR_MASK is honored, also when in_compat_syscall() - video_usercopy: err = v4l2_compat_put_array_args overwrote the original ioctl err value. Handle this correctly. I've tested this as well with the y2038 compat mode (i686 with 64-bit time_t) and that too works fine. Regards, Hans Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> --- diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c index d20c36ab01b4..e8c823776de0 100644 --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c @@ -258,7 +258,7 @@ static int put_v4l2_standard32(struct v4l2_standard *p64, struct v4l2_standard32 __user *p32) { if (put_user(p64->index, &p32->index) || - put_user(p64->index, &p32->id) || + put_user(p64->id, &p32->id) || copy_to_user(p32->name, p64->name, sizeof(p32->name)) || copy_to_user(&p32->frameperiod, &p64->frameperiod, sizeof(p32->frameperiod)) || @@ -358,10 +358,10 @@ static int get_v4l2_plane32(struct v4l2_plane *p64, memset(p64, 0, sizeof(*p64)); *p64 = (struct v4l2_plane) { - .bytesused = p64->bytesused, - .length = p64->bytesused, + .bytesused = plane32.bytesused, + .length = plane32.length, .m = m, - .data_offset = p64->data_offset, + .data_offset = plane32.data_offset, }; return 0; @@ -376,7 +376,7 @@ static int put_v4l2_plane32(struct v4l2_plane *p64, memset(&plane32, 0, sizeof(plane32)); plane32 = (struct v4l2_plane32) { .bytesused = p64->bytesused, - .length = p64->bytesused, + .length = p64->length, .data_offset = p64->data_offset, }; @@ -400,8 +400,7 @@ static int put_v4l2_plane32(struct v4l2_plane *p64, } static int get_v4l2_buffer32(struct v4l2_buffer *vb, - struct v4l2_buffer32 __user *arg, - bool querybuf) + struct v4l2_buffer32 __user *arg) { struct v4l2_buffer32 vb32; @@ -431,7 +430,7 @@ static int get_v4l2_buffer32(struct v4l2_buffer *vb, vb->m.offset = vb32.m.offset; break; case V4L2_MEMORY_USERPTR: - vb->m.userptr = vb32.m.userptr; + vb->m.userptr = (unsigned long)compat_ptr(vb32.m.userptr); break; case V4L2_MEMORY_DMABUF: vb->m.fd = vb32.m.fd; @@ -442,16 +441,12 @@ static int get_v4l2_buffer32(struct v4l2_buffer *vb, vb->m.planes = (void __force *) compat_ptr(vb32.m.planes); - if (querybuf) - vb->request_fd = 0; - return 0; } #ifdef CONFIG_COMPAT_32BIT_TIME static int get_v4l2_buffer32_time32(struct v4l2_buffer *vb, - struct v4l2_buffer32_time32 __user *arg, - bool querybuf) + struct v4l2_buffer32_time32 __user *arg) { struct v4l2_buffer32_time32 vb32; @@ -479,7 +474,7 @@ static int get_v4l2_buffer32_time32(struct v4l2_buffer *vb, vb->m.offset = vb32.m.offset; break; case V4L2_MEMORY_USERPTR: - vb->m.userptr = vb32.m.userptr; + vb->m.userptr = (unsigned long)compat_ptr(vb32.m.userptr); break; case V4L2_MEMORY_DMABUF: vb->m.fd = vb32.m.fd; @@ -489,8 +484,6 @@ static int get_v4l2_buffer32_time32(struct v4l2_buffer *vb, if (V4L2_TYPE_IS_MULTIPLANAR(vb->type)) vb->m.planes = (void __force *) compat_ptr(vb32.m.planes); - if (querybuf) - vb->request_fd = 0; return 0; } @@ -524,7 +517,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer *vb, vb32.m.offset = vb->m.offset; break; case V4L2_MEMORY_USERPTR: - vb32.m.userptr = vb->m.userptr; + vb32.m.userptr = (uintptr_t)(vb->m.userptr); break; case V4L2_MEMORY_DMABUF: vb32.m.fd = vb->m.fd; @@ -568,7 +561,7 @@ static int put_v4l2_buffer32_time32(struct v4l2_buffer *vb, vb32.m.offset = vb->m.offset; break; case V4L2_MEMORY_USERPTR: - vb32.m.userptr = vb->m.userptr; + vb32.m.userptr = (uintptr_t)(vb->m.userptr); break; case V4L2_MEMORY_DMABUF: vb32.m.fd = vb->m.fd; @@ -722,7 +715,7 @@ static int get_v4l2_ext_controls32(struct v4l2_ext_controls *p64, .which = ec32.which, .count = ec32.count, .error_idx = ec32.error_idx, - .request_fd = ec32.error_idx, + .request_fd = ec32.request_fd, .reserved[0] = ec32.reserved[0], .controls = (void __force *)compat_ptr(ec32.controls), }; @@ -740,7 +733,7 @@ static int put_v4l2_ext_controls32(struct v4l2_ext_controls *p64, .which = p64->which, .count = p64->count, .error_idx = p64->error_idx, - .request_fd = p64->error_idx, + .request_fd = p64->request_fd, .reserved[0] = p64->reserved[0], .controls = (uintptr_t)p64->controls, }; @@ -910,8 +903,10 @@ unsigned int v4l2_compat_translate_cmd(unsigned int cmd) return VIDIOC_QBUF; case VIDIOC_DQBUF32: return VIDIOC_DQBUF; + case VIDIOC_CREATE_BUFS32: + return VIDIOC_CREATE_BUFS; case VIDIOC_G_EXT_CTRLS32: - return VIDIOC_G_EXT_CTRLS; + return VIDIOC_G_EXT_CTRLS; case VIDIOC_S_EXT_CTRLS32: return VIDIOC_S_EXT_CTRLS; case VIDIOC_TRY_EXT_CTRLS32: @@ -929,8 +924,10 @@ unsigned int v4l2_compat_translate_cmd(unsigned int cmd) #ifdef CONFIG_X86_64 case VIDIOC_DQEVENT32: return VIDIOC_DQEVENT; +#ifdef CONFIG_COMPAT_32BIT_TIME case VIDIOC_DQEVENT32_TIME32: - return VIDIOC_DQEVENT_TIME32; + return VIDIOC_DQEVENT; +#endif #endif } return cmd; @@ -951,15 +948,13 @@ int v4l2_compat_get_user(void __user *arg, void *parg, unsigned int cmd) case VIDIOC_QBUF32_TIME32: case VIDIOC_DQBUF32_TIME32: case VIDIOC_PREPARE_BUF32_TIME32: - return get_v4l2_buffer32_time32(parg, arg, - cmd == VIDIOC_QUERYBUF32_TIME32); + return get_v4l2_buffer32_time32(parg, arg); #endif case VIDIOC_QUERYBUF32: case VIDIOC_QBUF32: case VIDIOC_DQBUF32: case VIDIOC_PREPARE_BUF32: - return get_v4l2_buffer32(parg, arg, - cmd == VIDIOC_QUERYBUF32); + return get_v4l2_buffer32(parg, arg); case VIDIOC_G_EXT_CTRLS32: case VIDIOC_S_EXT_CTRLS32: @@ -1025,8 +1020,10 @@ int v4l2_compat_put_user(void __user *arg, void *parg, unsigned int cmd) #ifdef CONFIG_X86_64 case VIDIOC_DQEVENT32: return put_v4l2_event32(parg, arg); +#ifdef CONFIG_COMPAT_32BIT_TIME case VIDIOC_DQEVENT32_TIME32: return put_v4l2_event32_time32(parg, arg); +#endif #endif } return 0; @@ -1073,9 +1070,10 @@ int v4l2_compat_get_array_args(struct file *file, void *mbuf, struct v4l2_buffer *b64 = arg; struct v4l2_plane *p64 = mbuf; struct v4l2_plane32 __user *p32 = user_ptr; - u32 num_planes = p64->length; if (V4L2_TYPE_IS_MULTIPLANAR(b64->type)) { + u32 num_planes = b64->length; + if (num_planes == 0) return 0; @@ -1161,9 +1159,10 @@ int v4l2_compat_put_array_args(struct file *file, void __user *user_ptr, struct v4l2_buffer *b64 = arg; struct v4l2_plane *p64 = mbuf; struct v4l2_plane32 __user *p32 = user_ptr; - u32 num_planes = p64->length; if (V4L2_TYPE_IS_MULTIPLANAR(b64->type)) { + u32 num_planes = b64->length; + if (num_planes == 0) return 0; diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index 114eaa79b6f5..aacb13b8f0a9 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -3133,7 +3133,8 @@ static int video_get_user(void __user *arg, void *parg, unsigned int real_cmd, unsigned int cmd, bool *always_copy) { - unsigned int n = _IOC_SIZE(cmd); + unsigned int n = _IOC_SIZE(real_cmd); + int err = 0; if (!(_IOC_DIR(cmd) & _IOC_WRITE)) { /* read-only ioctl */ @@ -3141,70 +3142,64 @@ static int video_get_user(void __user *arg, void *parg, return 0; } - if (cmd == real_cmd) { - /* - * In some cases, only a few fields are used as input, - * i.e. when the app sets "index" and then the driver - * fills in the rest of the structure for the thing - * with that index. We only need to copy up the first - * non-input field. - */ - if (v4l2_is_known_ioctl(cmd)) { - u32 flags = v4l2_ioctls[_IOC_NR(cmd)].flags; - - if (flags & INFO_FL_CLEAR_MASK) - n = (flags & INFO_FL_CLEAR_MASK) >> 16; - *always_copy = flags & INFO_FL_ALWAYS_COPY; - } - - if (copy_from_user(parg, (void __user *)arg, n)) - return -EFAULT; + /* + * In some cases, only a few fields are used as input, + * i.e. when the app sets "index" and then the driver + * fills in the rest of the structure for the thing + * with that index. We only need to copy up the first + * non-input field. + */ + if (v4l2_is_known_ioctl(real_cmd)) { + u32 flags = v4l2_ioctls[_IOC_NR(real_cmd)].flags; - /* zero out anything we don't copy from userspace */ - if (n < _IOC_SIZE(cmd)) - memset((u8 *)parg + n, 0, _IOC_SIZE(cmd) - n); - return 0; + if (flags & INFO_FL_CLEAR_MASK) + n = (flags & INFO_FL_CLEAR_MASK) >> 16; + *always_copy = flags & INFO_FL_ALWAYS_COPY; } - if (in_compat_syscall()) - return v4l2_compat_get_user(arg, parg, cmd); - - switch (cmd) { + if (cmd == real_cmd) { + if (copy_from_user(parg, (void __user *)arg, n)) + err = -EFAULT; + } else if (in_compat_syscall()) { + err = v4l2_compat_get_user(arg, parg, cmd); + } else { + switch (cmd) { #ifdef 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, - .sequence = vb32.sequence, - .memory = vb32.memory, - .m.userptr = vb32.m.userptr, - .length = vb32.length, - .request_fd = vb32.request_fd, - }; - - if (cmd == VIDIOC_QUERYBUF_TIME32) - vb->request_fd = 0; - - 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; + + 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, + .sequence = vb32.sequence, + .memory = vb32.memory, + .m.userptr = vb32.m.userptr, + .length = vb32.length, + .request_fd = vb32.request_fd, + }; + break; + } #endif + } } - return 0; + + /* zero out anything we don't copy from userspace */ + if (!err && n < _IOC_SIZE(real_cmd)) + memset((u8 *)parg + n, 0, _IOC_SIZE(real_cmd) - n); + return err; } static int video_put_user(void __user *arg, void *parg, @@ -3357,12 +3352,17 @@ video_usercopy(struct file *file, unsigned int orig_cmd, unsigned long arg, if (has_array_args) { *kernel_ptr = (void __force *)user_ptr; - if (in_compat_syscall()) - err = v4l2_compat_put_array_args(file, user_ptr, mbuf, - array_size, orig_cmd, - parg); - else if (copy_to_user(user_ptr, mbuf, array_size)) + if (in_compat_syscall()) { + int put_err; + + put_err = v4l2_compat_put_array_args(file, user_ptr, mbuf, + array_size, orig_cmd, + parg); + if (put_err) + err = put_err; + } else if (copy_to_user(user_ptr, mbuf, array_size)) { err = -EFAULT; + } goto out_array_args; } /*
On Fri, Oct 2, 2020 at 4:32 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: > On 17/09/2020 17:28, Arnd Bergmann wrote: > While testing I found a lot of bugs. Below is a patch that sits on top > of your series and fixes all the bugs: > > - put_v4l2_standard32: typo: p64->index -> p64->id > - get_v4l2_plane32: typo: p64 -> plane32 > typo: duplicate bytesused: the 2nd should be 'length' > - put_v4l2_plane32: typo: duplicate bytesused: the 2nd should be 'length' > - get_v4l2_buffer32/get_v4l2_buffer32_time32: missing compat_ptr for vb32.m.userptr > - get_v4l2_buffer32/get_v4l2_buffer32_time32: drop querybuf bool > - put_v4l2_buffer32/put_v4l2_buffer32_time32: add uintptr_t cast for vb->m.userptr > - get_v4l2_ext_controls32: typo: error_idx -> request_fd > - put_v4l2_ext_controls32: typo: error_idx -> request_fd > - v4l2_compat_translate_cmd: missing case VIDIOC_CREATE_BUFS32 > - v4l2_compat_translate_cmd: #ifdef CONFIG_COMPAT_32BIT_TIME for > case VIDIOC_DQEVENT32_TIME32 and return VIDIOC_DQEVENT > - v4l2_compat_put_user: #ifdef CONFIG_COMPAT_32BIT_TIME for case VIDIOC_DQEVENT32_TIME32 > - v4l2_compat_get_array_args: typo: p64 -> b64 > - v4l2_compat_put_array_args: typo: p64 -> b64 > - video_get_user: make sure INFO_FL_CLEAR_MASK is honored, also when in_compat_syscall() > - video_usercopy: err = v4l2_compat_put_array_args overwrote the original ioctl err value. > Handle this correctly. > > I've tested this as well with the y2038 compat mode (i686 with 64-bit time_t) > and that too works fine. I'm not too surprised that there were bugs, but I am a little shocked at how much I got wrong in the end. Thanks a lot for testing my series and fixing all of the above! I've carefully studied your changes and folded them into my series now. Most of the changes were obvious in hindsight, just two things to comment on: > #ifdef CONFIG_COMPAT_32BIT_TIME > static int get_v4l2_buffer32_time32(struct v4l2_buffer *vb, > - struct v4l2_buffer32_time32 __user *arg, > - bool querybuf) > + struct v4l2_buffer32_time32 __user *arg) > { > struct v4l2_buffer32_time32 vb32; > > @@ -489,8 +484,6 @@ static int get_v4l2_buffer32_time32(struct v4l2_buffer *vb, > if (V4L2_TYPE_IS_MULTIPLANAR(vb->type)) > vb->m.planes = (void __force *) > compat_ptr(vb32.m.planes); > - if (querybuf) > - vb->request_fd = 0; > > return 0; It took me too long to understand what you changed here, as this depends on your rewrite of video_get_user(). The new version definitely looks cleaner. After folding in the video_get_user() changes, I've amended that changelog of the "media: v4l2: prepare compat-ioctl rework" commit with: | [...] | provide a location for reading and writing user space data from inside | of the generic video_usercopy() helper. | | Hans Verkuil rewrote the video_get_user() function here to simplify | the zeroing of the extra input fields and fixed a couple of bugs in | the original implementation. | | Co-developed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> | Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> | Signed-off-by: Arnd Bergmann <arnd@arndb.de> I could split that out into a separate patch if you prefer. > @@ -1025,8 +1020,10 @@ int v4l2_compat_put_user(void __user *arg, void *parg, unsigned int cmd) > #ifdef CONFIG_X86_64 > case VIDIOC_DQEVENT32: > return put_v4l2_event32(parg, arg); > +#ifdef CONFIG_COMPAT_32BIT_TIME > case VIDIOC_DQEVENT32_TIME32: > return put_v4l2_event32_time32(parg, arg); > +#endif > #endif > } > return 0; I think this change requires adding another #ifdef around the put_v4l2_event32_time32() definition, to avoid an "unused function" warning. The #ifdef was already missing in the original version, but I agree it makes sense to add it. As you suggested earlier, I will resend the fixed series after -rc1 is out. Arnd
Hi Arnd, On 06/10/2020 17:14, Arnd Bergmann wrote: > On Fri, Oct 2, 2020 at 4:32 PM Hans Verkuil <hverkuil@xs4all.nl> wrote: >> On 17/09/2020 17:28, Arnd Bergmann wrote: > >> While testing I found a lot of bugs. Below is a patch that sits on top >> of your series and fixes all the bugs: >> >> - put_v4l2_standard32: typo: p64->index -> p64->id >> - get_v4l2_plane32: typo: p64 -> plane32 >> typo: duplicate bytesused: the 2nd should be 'length' >> - put_v4l2_plane32: typo: duplicate bytesused: the 2nd should be 'length' >> - get_v4l2_buffer32/get_v4l2_buffer32_time32: missing compat_ptr for vb32.m.userptr >> - get_v4l2_buffer32/get_v4l2_buffer32_time32: drop querybuf bool >> - put_v4l2_buffer32/put_v4l2_buffer32_time32: add uintptr_t cast for vb->m.userptr >> - get_v4l2_ext_controls32: typo: error_idx -> request_fd >> - put_v4l2_ext_controls32: typo: error_idx -> request_fd >> - v4l2_compat_translate_cmd: missing case VIDIOC_CREATE_BUFS32 >> - v4l2_compat_translate_cmd: #ifdef CONFIG_COMPAT_32BIT_TIME for >> case VIDIOC_DQEVENT32_TIME32 and return VIDIOC_DQEVENT >> - v4l2_compat_put_user: #ifdef CONFIG_COMPAT_32BIT_TIME for case VIDIOC_DQEVENT32_TIME32 >> - v4l2_compat_get_array_args: typo: p64 -> b64 >> - v4l2_compat_put_array_args: typo: p64 -> b64 >> - video_get_user: make sure INFO_FL_CLEAR_MASK is honored, also when in_compat_syscall() >> - video_usercopy: err = v4l2_compat_put_array_args overwrote the original ioctl err value. >> Handle this correctly. >> >> I've tested this as well with the y2038 compat mode (i686 with 64-bit time_t) >> and that too works fine. > > I'm not too surprised that there were bugs, but I am a little shocked > at how much > I got wrong in the end. Thanks a lot for testing my series and fixing all of the > above! Without the compliance and regression tests it would be impossible to get this right, it is *so* hard to verify just by looking at the code. That's always been an issue with this compat code. Luckily, with the test-media script in v4l-utils it is fairly easy to get almost complete coverage. Ping me on irc if you are interested in testing this series yourself, it's not too difficult to do. All the issues mentioned above were all found by this test. Sometimes it failed in fairly obscure ways and it took quite some time to figure out the underlying cause. The 'typo: p64 -> b64' in particular wasn't easy to find: I must have read over that typo a dozen times :-) > > I've carefully studied your changes and folded them into my series now. > Most of the changes were obvious in hindsight, just two things to comment on: > >> #ifdef CONFIG_COMPAT_32BIT_TIME >> static int get_v4l2_buffer32_time32(struct v4l2_buffer *vb, >> - struct v4l2_buffer32_time32 __user *arg, >> - bool querybuf) >> + struct v4l2_buffer32_time32 __user *arg) >> { >> struct v4l2_buffer32_time32 vb32; >> >> @@ -489,8 +484,6 @@ static int get_v4l2_buffer32_time32(struct v4l2_buffer *vb, >> if (V4L2_TYPE_IS_MULTIPLANAR(vb->type)) >> vb->m.planes = (void __force *) >> compat_ptr(vb32.m.planes); >> - if (querybuf) >> - vb->request_fd = 0; >> >> return 0; > > It took me too long to understand what you changed here, as this depends > on your rewrite of video_get_user(). Sorry, I should have mentioned that. > The new version definitely looks > cleaner. After folding in the video_get_user() changes, I've amended > that changelog of the "media: v4l2: prepare compat-ioctl rework" commit > with: > > | [...] > | provide a location for reading and writing user space data from inside > | of the generic video_usercopy() helper. > | > | Hans Verkuil rewrote the video_get_user() function here to simplify > | the zeroing of the extra input fields and fixed a couple of bugs in > | the original implementation. > | > | Co-developed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > | Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > | Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > I could split that out into a separate patch if you prefer. No, don't split it. I prefer to have an as-clean-as-possible series to ease reviewing. > >> @@ -1025,8 +1020,10 @@ int v4l2_compat_put_user(void __user *arg, void *parg, unsigned int cmd) >> #ifdef CONFIG_X86_64 >> case VIDIOC_DQEVENT32: >> return put_v4l2_event32(parg, arg); >> +#ifdef CONFIG_COMPAT_32BIT_TIME >> case VIDIOC_DQEVENT32_TIME32: >> return put_v4l2_event32_time32(parg, arg); >> +#endif >> #endif >> } >> return 0; > > I think this change requires adding another #ifdef around the > put_v4l2_event32_time32() definition, to avoid an "unused function" > warning. The #ifdef was already missing in the original version, but I > agree it makes sense to add it. You're probably right. I should remember to do a test compilation next time with CONFIG_COMPAT_32BIT_TIME set to n. > > As you suggested earlier, I will resend the fixed series after -rc1 > is out. Looking forward to that. Regards, Hans
Hi Arnd, On 06/10/2020 17:28, Hans Verkuil wrote: > Hi Arnd, > > On 06/10/2020 17:14, Arnd Bergmann wrote: >> >> As you suggested earlier, I will resend the fixed series after -rc1 >> is out. > > Looking forward to that. FYI: v5.10-rc1 was just merged into the media_tree master branch. Once I have the v2 of this series I'll test it and if it is OK make a PR. Thanks! Hans
I have a series to remove all uses of compat_alloc_user_space() and copy_in_user() from the kernel, this is the part of it that involves the v4l2 compat code. The resulting code is significantly shorter and arguably more readable, but I have not done any testing beyond compilation on it, so at the minimum this first needs to pass the test suite for both native and compat users space. Given the complexity of the code, I am fairly sure that there are bugs I missed. Please review and test if possible. Arnd Arnd Bergmann (8): media: v4l2: prepare compat-ioctl rework media: v4l2: remove unneeded compat ioctl handlers media: v4l2: move v4l2_ext_controls conversion media: v4l2: move compat handling for v4l2_buffer media: v4l2: allocate v4l2_clip objects early media: v4l2: convert v4l2_format compat ioctls media: v4l2: remaining compat handlers media: v4l2: remove remaining compat_ioctl drivers/media/common/saa7146/saa7146_video.c | 6 +- drivers/media/pci/bt8xx/bttv-driver.c | 8 +- drivers/media/pci/saa7134/saa7134-video.c | 19 +- .../media/test-drivers/vivid/vivid-vid-cap.c | 18 +- .../media/test-drivers/vivid/vivid-vid-out.c | 18 +- drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 1774 ++++++----------- drivers/media/v4l2-core/v4l2-ioctl.c | 120 +- include/media/v4l2-ioctl.h | 11 + include/uapi/linux/videodev2.h | 2 +- 9 files changed, 729 insertions(+), 1247 deletions(-)