On 04/16/2018 05:11 PM, Mauro Carvalho Chehab wrote:
> In the past, "up" were an acronym for "user pointer" and "kp" for
> "kernel pointer". However, since a1dfb4c48cc1 ("media:
> v4l2-compat-ioctl32.c: refactor compat ioctl32 logic"), both
> are now __user pointers.
>
> So, the usage of "kp" is really misleading there. So, rename
> both to just "p32" and "p64" everywhere it occurs, in order to
> make peace with this file's namespace.
>
> There are two exceptions to "up/kp" nomenclature: at
> alloc_userspace() and at do_video_ioctl().
>
> There, a new userspace pointer were allocated, in order to store
> the 64 bits version of the ioctl. Those were called as "up_native",
> with is, IMHO, an even worse name, as "native" could mislead of
> being the arguments that were filled from userspace. I almost
> renamed it to just "p64", but, after thinking more about that,
> it sounded better to call it as "new_p64", as this makes clearer
> that this is the data structure that was allocated inside this
> file in order to be used to pass/retrieve data when calling the
> 64-bit ready file->f_op->unlocked_ioctl() function.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxxx>
> ---
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 578 +++++++++++++-------------
> 1 file changed, 289 insertions(+), 289 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index 5c3408bdfd89..064e4a2bdba3 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
<snip>
> @@ -392,31 +392,31 @@ struct v4l2_buffer32 {
> __u32 reserved;
> };
>
> -static int get_v4l2_plane32(struct v4l2_plane __user *up,
> +static int get_v4l2_plane32(struct v4l2_plane __user *p32,
> struct v4l2_plane32 __user *up32,
This is confusing: there is now a p32 and a up32 pointer. In all
fairness, this was already confusing. In this specific case 'up' should
be 'p64' and 'up32' should be 'p32'.
> enum v4l2_memory memory)
> {
> compat_ulong_t p;
>
> - if (copy_in_user(up, up32, 2 * sizeof(__u32)) ||
> - copy_in_user(&up->data_offset, &up32->data_offset,
> - sizeof(up->data_offset)))
> + if (copy_in_user(p32, up32, 2 * sizeof(__u32)) ||
> + copy_in_user(&p32->data_offset, &up32->data_offset,
> + sizeof(p32->data_offset)))
> return -EFAULT;
>
> switch (memory) {
> case V4L2_MEMORY_MMAP:
> case V4L2_MEMORY_OVERLAY:
> - if (copy_in_user(&up->m.mem_offset, &up32->m.mem_offset,
> + if (copy_in_user(&p32->m.mem_offset, &up32->m.mem_offset,
> sizeof(up32->m.mem_offset)))
> return -EFAULT;
> break;
> case V4L2_MEMORY_USERPTR:
> if (get_user(p, &up32->m.userptr) ||
> - put_user((unsigned long)compat_ptr(p), &up->m.userptr))
> + put_user((unsigned long)compat_ptr(p), &p32->m.userptr))
> return -EFAULT;
> break;
> case V4L2_MEMORY_DMABUF:
> - if (copy_in_user(&up->m.fd, &up32->m.fd, sizeof(up32->m.fd)))
> + if (copy_in_user(&p32->m.fd, &up32->m.fd, sizeof(up32->m.fd)))
> return -EFAULT;
> break;
> }
> @@ -424,32 +424,32 @@ static int get_v4l2_plane32(struct v4l2_plane __user *up,
> return 0;
> }
>
> -static int put_v4l2_plane32(struct v4l2_plane __user *up,
> +static int put_v4l2_plane32(struct v4l2_plane __user *p32,
> struct v4l2_plane32 __user *up32,
> enum v4l2_memory memory)
Same here. up -> p64 and up32 -> p32.
> {
> unsigned long p;
>
> - if (copy_in_user(up32, up, 2 * sizeof(__u32)) ||
> - copy_in_user(&up32->data_offset, &up->data_offset,
> - sizeof(up->data_offset)))
> + if (copy_in_user(up32, p32, 2 * sizeof(__u32)) ||
> + copy_in_user(&up32->data_offset, &p32->data_offset,
> + sizeof(p32->data_offset)))
> return -EFAULT;
>
> switch (memory) {
> case V4L2_MEMORY_MMAP:
> case V4L2_MEMORY_OVERLAY:
> - if (copy_in_user(&up32->m.mem_offset, &up->m.mem_offset,
> - sizeof(up->m.mem_offset)))
> + if (copy_in_user(&up32->m.mem_offset, &p32->m.mem_offset,
> + sizeof(p32->m.mem_offset)))
> return -EFAULT;
> break;
> case V4L2_MEMORY_USERPTR:
> - if (get_user(p, &up->m.userptr)||
> + if (get_user(p, &p32->m.userptr)||
> put_user((compat_ulong_t)ptr_to_compat((void __user *)p),
> &up32->m.userptr))
> return -EFAULT;
> break;
> case V4L2_MEMORY_DMABUF:
> - if (copy_in_user(&up32->m.fd, &up->m.fd, sizeof(up->m.fd)))
> + if (copy_in_user(&up32->m.fd, &p32->m.fd, sizeof(p32->m.fd)))
> return -EFAULT;
> break;
> }
> @@ -457,14 +457,14 @@ static int put_v4l2_plane32(struct v4l2_plane __user *up,
> return 0;
> }
>
After fixing these two functions you can add my:
Reviewed-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
One note: this patch does not update variables like uplane and [ku]controls. But
I think it would be better to change those in follow-up patches.
I really like the new p32/p64/new_p64 names. Much more descriptive.
Regards,
Hans
