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(&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)||
> >
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(&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;
>
[PATCH] media: v4l2-compat-ioctl32: better name userspace pointers
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
@@ -56,8 +56,8 @@ struct v4l2_window32 {
__u8global_alpha;
};
-static int get_v4l2_window32(struct v4l2_window __user *kp,
-struct v4l2_window32 __user *up,
+static int get_v4l2_window32(struct v4l2_window __user *p64,
+struct v4l2_window32 __user *p32,
void __user *aux_buf, u32 aux_space)
{
struct v4l2_clip32 __user *uclips;
@@ -65,26 +65,26 @@ static int get_v4l2_window32(struct v4l2_window __user *kp,
compat_caddr_t p;
u32 clipcount;
- if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
- copy_in_user(&kp->w, &up->w, sizeof(up->w)) ||
- assign_in_user(&kp->field, &up->field) ||
- assign_in_user(&kp->chromakey, &up->chromakey) ||
- assign_in_user(&kp->global_alpha, &up->global_alpha) ||
- get_user(clipcount, &up->clipcount) ||
- put_user(clipcount, &kp->clipcount))
+ if (!access_ok(VERIFY_READ, p32, sizeof(*p32)) ||
+ copy_in_user(&p64->w, &p32->w, sizeof(p32->w)) ||
+ assign_in_user(&p64->field, &p32->field) ||
+ assign_in_user(&p64->chromakey, &p32->chromakey) ||
+ assign_in_user(&p64->global_alpha, &p32->global_alpha) ||
+ get_user(clipcount, &p32->clipcount) ||
+ put_user(clipcount, &p64->clipcount))
return -EFAULT;
if (clipcount > 2048)
return -EINVAL;
if (!clipcount)
- return put_user(NULL, &kp->clips);
+ return put_user(NULL, &p64->clips);
- if (get_user(p, &up->clips))
+ if (get_user(p, &p32->clips))
return -EFAULT;
uclips = compat_ptr(p);
if (aux_space < clipcount * sizeof(*kclips))
return -EFAULT;
kclips = aux_buf;
- if (put_user(kclips, &kp->clips))
+ if (put_user(kclips, &p64->clips))
return -EFAULT;
while (clipcount--) {
@@ -98,27 +98,27 @@ static int get_v4l2_window32(struct v4l2_window __user *kp,
return 0;
}
-static int put_v4l2_window32(struct v4l2_window __user *kp,
-struct v4l2_window32 __user *up)
+static int put_v4l2_window32(struct v4l2_window __user *p64,
+struct v4l2_window32 __user *p32)
{
struct v4l2_clip __user *kclips;
struct v4l2_clip32 __user *uclips;
compat_caddr_t p;
u32 clipcount;
- if (copy_in_user(&up->w, &kp->w, sizeof(kp->w)) ||
- assign_in_user(&up->field, &kp->field) ||
- assign_in_user(&up->chromakey, &kp->chromakey) ||
- assign_in_user(&up->global_alpha, &kp->global_alpha) ||
- get_user(clipcount, &kp->clipcount) ||
- put_user(clipcount, &up->clipcount))
+ if (copy_in_user(&p32->w, &p64->w, sizeof(p64->w)) ||
+ assign_in_user(&p32->field, &p64->field) ||
+ assign_in_user(&p32->chromakey, &p64->chromakey) ||
+ assign_in_user(&p32->global_alpha, &p64->global_alpha) ||
+ get_user(clipcount, &p64->clipcount) ||
+ put_user(clipcount, &p32->clipcount))
return -EFAULT;
if (!clipcount)
return 0;
- if (get_user(kclips, &kp->clips))
+ if (get_user(kclips, &p64->clips))
return -EFAULT;
- if (get_user(p, &up->clips))
+ if (get_user(p, &p32->clips))
return -EFAULT;
uclips = compat_
