Re: [PATCH] media: v4l2-compat-ioctl32: better name userspace pointers
On Mon, Apr 16, 2018 at 06:00:12PM +0200, Hans Verkuil wrote:
> 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
> > ---
> > 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
>
>
>
> > @@ -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(>data_offset, >data_offset,
> > -sizeof(up->data_offset)))
> > + if (copy_in_user(p32, up32, 2 * sizeof(__u32)) ||
> > + copy_in_user(>data_offset, >data_offset,
> > +sizeof(p32->data_offset)))
> > return -EFAULT;
> >
> > switch (memory) {
> > case V4L2_MEMORY_MMAP:
> > case V4L2_MEMORY_OVERLAY:
> > - if (copy_in_user(>m.mem_offset, >m.mem_offset,
> > + if (copy_in_user(>m.mem_offset, >m.mem_offset,
> > sizeof(up32->m.mem_offset)))
> > return -EFAULT;
> > break;
> > case V4L2_MEMORY_USERPTR:
> > if (get_user(p, >m.userptr) ||
> > - put_user((unsigned long)compat_ptr(p), >m.userptr))
> > + put_user((unsigned long)compat_ptr(p), >m.userptr))
> > return -EFAULT;
> > break;
> > case V4L2_MEMORY_DMABUF:
> > - if (copy_in_user(>m.fd, >m.fd, sizeof(up32->m.fd)))
> > + if (copy_in_user(>m.fd, >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(>data_offset, >data_offset,
> > -sizeof(up->data_offset)))
> > + if (copy_in_user(up32, p32, 2 * sizeof(__u32)) ||
> > + copy_in_user(>data_offset, >data_offset,
> > +sizeof(p32->data_offset)))
> > return -EFAULT;
> >
> > switch (memory) {
> > case V4L2_MEMORY_MMAP:
> > case V4L2_MEMORY_OVERLAY:
> > - if (copy_in_user(>m.mem_offset, >m.mem_offset,
> > -sizeof(up->m.mem_offset)))
> > + if (copy_in_user(>m.mem_offset, >m.mem_offset,
> > +sizeof(p32->m.mem_offset)))
> > return -EFAULT;
> > break;
> > case V4L2_MEMORY_USERPTR:
> > - if (get_user(p, >m.userptr)||
> > + if (get_user(p, >m.userptr)||
> > put_user((compat_ulong_t)ptr_to_compat((void __user *)p),
> >
Re: [PATCH] media: v4l2-compat-ioctl32: better name userspace pointers
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
> ---
> 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
> @@ -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(>data_offset, >data_offset,
> - sizeof(up->data_offset)))
> + if (copy_in_user(p32, up32, 2 * sizeof(__u32)) ||
> + copy_in_user(>data_offset, >data_offset,
> + sizeof(p32->data_offset)))
> return -EFAULT;
>
> switch (memory) {
> case V4L2_MEMORY_MMAP:
> case V4L2_MEMORY_OVERLAY:
> - if (copy_in_user(>m.mem_offset, >m.mem_offset,
> + if (copy_in_user(>m.mem_offset, >m.mem_offset,
>sizeof(up32->m.mem_offset)))
> return -EFAULT;
> break;
> case V4L2_MEMORY_USERPTR:
> if (get_user(p, >m.userptr) ||
> - put_user((unsigned long)compat_ptr(p), >m.userptr))
> + put_user((unsigned long)compat_ptr(p), >m.userptr))
> return -EFAULT;
> break;
> case V4L2_MEMORY_DMABUF:
> - if (copy_in_user(>m.fd, >m.fd, sizeof(up32->m.fd)))
> + if (copy_in_user(>m.fd, >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(>data_offset, >data_offset,
> - sizeof(up->data_offset)))
> + if (copy_in_user(up32, p32, 2 * sizeof(__u32)) ||
> + copy_in_user(>data_offset, >data_offset,
> + sizeof(p32->data_offset)))
> return -EFAULT;
>
> switch (memory) {
> case V4L2_MEMORY_MMAP:
> case V4L2_MEMORY_OVERLAY:
> - if (copy_in_user(>m.mem_offset, >m.mem_offset,
> - sizeof(up->m.mem_offset)))
> + if (copy_in_user(>m.mem_offset, >m.mem_offset,
> + sizeof(p32->m.mem_offset)))
> return -EFAULT;
> break;
> case V4L2_MEMORY_USERPTR:
> - if (get_user(p, >m.userptr)||
> + if (get_user(p, >m.userptr)||
> put_user((compat_ulong_t)ptr_to_compat((void __user *)p),
>>m.userptr))
> return -EFAULT;
> break;
> case V4L2_MEMORY_DMABUF:
> - if (copy_in_user(>m.fd, >m.fd, sizeof(up->m.fd)))
> +
