[PATCH v2 0/4] Improve v4l2-compat-ioctl32 handler getting rid of smatch warnings

[PATCH v2 0/4] Improve v4l2-compat-ioctl32 handler getting rid of smatch warnings

[Mauro Carvalho Chehab]

This series correspond to the last 3 patches of my previous patch series:
	https://www.spinics.net/lists/linux-media/msg132453.html

It contains the compat32 related bits.

Version 2 addresses some comments from Hans.
It adds a new patch better documenting it.

Mauro Carvalho Chehab (4):
  media: v4l2-compat-ioctl32: fix several __user annotations
  media: v4l2-compat-ioctl32: better name userspace pointers
  media: v4l2-compat-ioctl32: simplify casts
  media: v4l2-compat-ioctl32: better document the code

 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 805 ++++++++++++++++----------
 1 file changed, 496 insertions(+), 309 deletions(-)

-- 
2.14.3

[PATCH v2 1/4] media: v4l2-compat-ioctl32: fix several __user annotations

[Mauro Carvalho Chehab]

Smatch report several issues with bad __user annotations:

  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21: warning: incorrect type in argument 1 (different address spaces)
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:    expected void [noderef] <asn:1>*uptr
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:    got void *<noident>
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21: warning: incorrect type in argument 1 (different address spaces)
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:    expected void const volatile [noderef] <asn:1>*<noident>
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:    got struct v4l2_plane [noderef] <asn:1>**<noident>
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13: warning: incorrect type in argument 1 (different address spaces)
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13:    expected void [noderef] <asn:1>*uptr
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13:    got void *[assigned] base
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13: warning: incorrect type in assignment (different address spaces)
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:    expected struct v4l2_ext_control [noderef] <asn:1>*kcontrols
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:    got struct v4l2_ext_control *<noident>
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13: warning: incorrect type in assignment (different address spaces)
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13:    expected unsigned char [usertype] *__pu_val
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13:    got void [noderef] <asn:1>*
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13: warning: incorrect type in argument 1 (different address spaces)
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:    expected void [noderef] <asn:1>*uptr
  drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:    got void *[assigned] edid

Fix them.

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 51 ++++++++++++++++++---------
 1 file changed, 35 insertions(+), 16 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index d03a44d89649..51c7c5ab15ef 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -443,8 +443,8 @@ static int put_v4l2_plane32(struct v4l2_plane __user *up,
 			return -EFAULT;
 		break;
 	case V4L2_MEMORY_USERPTR:
-		if (get_user(p, &up->m.userptr) ||
-		    put_user((compat_ulong_t)ptr_to_compat((__force void *)p),
+		if (get_user(p, &up->m.userptr)||
+		    put_user((compat_ulong_t)ptr_to_compat((void __user *)p),
 			     &up32->m.userptr))
 			return -EFAULT;
 		break;
@@ -587,7 +587,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
 	u32 length;
 	enum v4l2_memory memory;
 	struct v4l2_plane32 __user *uplane32;
-	struct v4l2_plane __user *uplane;
+	struct v4l2_plane *uplane;
 	compat_caddr_t p;
 	int ret;
 
@@ -617,15 +617,22 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
 
 		if (num_planes == 0)
 			return 0;
-
-		if (get_user(uplane, ((__force struct v4l2_plane __user **)&kp->m.planes)))
+		/* We need to define uplane without __user, even though
+		 * it does point to data in userspace here. The reason is
+		 * that v4l2-ioctl.c copies it from userspace to kernelspace,
+		 * so its definition in videodev2.h doesn't have a
+		 * __user markup. Defining uplane with __user causes
+		 * smatch warnings, so instead declare it without __user
+		 * and cast it as a userspace pointer to put_v4l2_plane32().
+		 */
+		if (get_user(uplane, &kp->m.planes))
 			return -EFAULT;
 		if (get_user(p, &up->m.planes))
 			return -EFAULT;
 		uplane32 = compat_ptr(p);
 
 		while (num_planes--) {
-			ret = put_v4l2_plane32(uplane, uplane32, memory);
+			ret = put_v4l2_plane32((void __user *)uplane, uplane32, memory);
 			if (ret)
 				return ret;
 			++uplane;
@@ -675,7 +682,7 @@ static int get_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp,
 
 	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
 	    get_user(tmp, &up->base) ||
-	    put_user((__force void *)compat_ptr(tmp), &kp->base) ||
+	    put_user((void __force *)compat_ptr(tmp), &kp->base) ||
 	    assign_in_user(&kp->capability, &up->capability) ||
 	    assign_in_user(&kp->flags, &up->flags) ||
 	    copy_in_user(&kp->fmt, &up->fmt, sizeof(kp->fmt)))
@@ -690,7 +697,7 @@ static int put_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp,
 
 	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
 	    get_user(base, &kp->base) ||
-	    put_user(ptr_to_compat(base), &up->base) ||
+	    put_user(ptr_to_compat((void __user *)base), &up->base) ||
 	    assign_in_user(&up->capability, &kp->capability) ||
 	    assign_in_user(&up->flags, &kp->flags) ||
 	    copy_in_user(&up->fmt, &kp->fmt, sizeof(kp->fmt)))
@@ -857,11 +864,19 @@ static int put_v4l2_ext_controls32(struct file *file,
 				   struct v4l2_ext_controls32 __user *up)
 {
 	struct v4l2_ext_control32 __user *ucontrols;
-	struct v4l2_ext_control __user *kcontrols;
+	struct v4l2_ext_control *kcontrols;
 	u32 count;
 	u32 n;
 	compat_caddr_t p;
 
+	/*
+	 * We need to define kcontrols without __user, even though it does
+	 * point to data in userspace here. The reason is that v4l2-ioctl.c
+	 * copies it from userspace to kernelspace, so its definition in
+	 * videodev2.h doesn't have a __user markup. Defining kcontrols
+	 * with __user causes smatch warnings, so instead declare it
+	 * without __user and cast it as a userspace pointer where needed.
+	 */
 	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
 	    assign_in_user(&up->which, &kp->which) ||
 	    get_user(count, &kp->count) ||
@@ -883,10 +898,12 @@ static int put_v4l2_ext_controls32(struct file *file,
 		unsigned int size = sizeof(*ucontrols);
 		u32 id;
 
-		if (get_user(id, &kcontrols->id) ||
+		if (get_user(id, (unsigned int __user *)&kcontrols->id) ||
 		    put_user(id, &ucontrols->id) ||
-		    assign_in_user(&ucontrols->size, &kcontrols->size) ||
-		    copy_in_user(&ucontrols->reserved2, &kcontrols->reserved2,
+		    assign_in_user(&ucontrols->size,
+				   (unsigned int __user *)&kcontrols->size) ||
+		    copy_in_user(&ucontrols->reserved2,
+				 (void __user *)&kcontrols->reserved2,
 				 sizeof(ucontrols->reserved2)))
 			return -EFAULT;
 
@@ -898,7 +915,8 @@ static int put_v4l2_ext_controls32(struct file *file,
 		if (ctrl_is_pointer(file, id))
 			size -= sizeof(ucontrols->value64);
 
-		if (copy_in_user(ucontrols, kcontrols, size))
+		if (copy_in_user(ucontrols,
+			         (void __user *)kcontrols, size))
 			return -EFAULT;
 
 		ucontrols++;
@@ -952,9 +970,10 @@ static int get_v4l2_edid32(struct v4l2_edid __user *kp,
 	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
 	    assign_in_user(&kp->pad, &up->pad) ||
 	    assign_in_user(&kp->start_block, &up->start_block) ||
-	    assign_in_user(&kp->blocks, &up->blocks) ||
+	    assign_in_user(&kp->blocks,
+			   (u32 __user *)&up->blocks) ||
 	    get_user(tmp, &up->edid) ||
-	    put_user(compat_ptr(tmp), &kp->edid) ||
+	    put_user((void __force *)compat_ptr(tmp), &kp->edid) ||
 	    copy_in_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
 		return -EFAULT;
 	return 0;
@@ -970,7 +989,7 @@ static int put_v4l2_edid32(struct v4l2_edid __user *kp,
 	    assign_in_user(&up->start_block, &kp->start_block) ||
 	    assign_in_user(&up->blocks, &kp->blocks) ||
 	    get_user(edid, &kp->edid) ||
-	    put_user(ptr_to_compat(edid), &up->edid) ||
+	    put_user(ptr_to_compat((void __user *)edid), &up->edid) ||
 	    copy_in_user(up->reserved, kp->reserved, sizeof(up->reserved)))
 		return -EFAULT;
 	return 0;
-- 
2.14.3

Re: [PATCH v2 1/4] media: v4l2-compat-ioctl32: fix several __user annotations

[Hans Verkuil]

On 04/19/18 18:33, Mauro Carvalho Chehab wrote:
> Smatch report several issues with bad __user annotations:
> 
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21: warning: incorrect type in argument 1 (different address spaces)
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:    expected void [noderef] <asn:1>*uptr
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:    got void *<noident>
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21: warning: incorrect type in argument 1 (different address spaces)
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:    expected void const volatile [noderef] <asn:1>*<noident>
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:    got struct v4l2_plane [noderef] <asn:1>**<noident>
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13: warning: incorrect type in argument 1 (different address spaces)
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13:    expected void [noderef] <asn:1>*uptr
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13:    got void *[assigned] base
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13: warning: incorrect type in assignment (different address spaces)
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:    expected struct v4l2_ext_control [noderef] <asn:1>*kcontrols
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:    got struct v4l2_ext_control *<noident>
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13: warning: incorrect type in assignment (different address spaces)
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13:    expected unsigned char [usertype] *__pu_val
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13:    got void [noderef] <asn:1>*
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13: warning: incorrect type in argument 1 (different address spaces)
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:    expected void [noderef] <asn:1>*uptr
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:    got void *[assigned] edid
> 
> Fix them.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

> ---
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 51 ++++++++++++++++++---------
>  1 file changed, 35 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index d03a44d89649..51c7c5ab15ef 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -443,8 +443,8 @@ static int put_v4l2_plane32(struct v4l2_plane __user *up,
>  			return -EFAULT;
>  		break;
>  	case V4L2_MEMORY_USERPTR:
> -		if (get_user(p, &up->m.userptr) ||
> -		    put_user((compat_ulong_t)ptr_to_compat((__force void *)p),
> +		if (get_user(p, &up->m.userptr)||
> +		    put_user((compat_ulong_t)ptr_to_compat((void __user *)p),
>  			     &up32->m.userptr))
>  			return -EFAULT;
>  		break;
> @@ -587,7 +587,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
>  	u32 length;
>  	enum v4l2_memory memory;
>  	struct v4l2_plane32 __user *uplane32;
> -	struct v4l2_plane __user *uplane;
> +	struct v4l2_plane *uplane;
>  	compat_caddr_t p;
>  	int ret;
>  
> @@ -617,15 +617,22 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
>  
>  		if (num_planes == 0)
>  			return 0;
> -
> -		if (get_user(uplane, ((__force struct v4l2_plane __user **)&kp->m.planes)))
> +		/* We need to define uplane without __user, even though
> +		 * it does point to data in userspace here. The reason is
> +		 * that v4l2-ioctl.c copies it from userspace to kernelspace,
> +		 * so its definition in videodev2.h doesn't have a
> +		 * __user markup. Defining uplane with __user causes
> +		 * smatch warnings, so instead declare it without __user
> +		 * and cast it as a userspace pointer to put_v4l2_plane32().
> +		 */
> +		if (get_user(uplane, &kp->m.planes))
>  			return -EFAULT;
>  		if (get_user(p, &up->m.planes))
>  			return -EFAULT;
>  		uplane32 = compat_ptr(p);
>  
>  		while (num_planes--) {
> -			ret = put_v4l2_plane32(uplane, uplane32, memory);
> +			ret = put_v4l2_plane32((void __user *)uplane, uplane32, memory);
>  			if (ret)
>  				return ret;
>  			++uplane;
> @@ -675,7 +682,7 @@ static int get_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp,
>  
>  	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
>  	    get_user(tmp, &up->base) ||
> -	    put_user((__force void *)compat_ptr(tmp), &kp->base) ||
> +	    put_user((void __force *)compat_ptr(tmp), &kp->base) ||
>  	    assign_in_user(&kp->capability, &up->capability) ||
>  	    assign_in_user(&kp->flags, &up->flags) ||
>  	    copy_in_user(&kp->fmt, &up->fmt, sizeof(kp->fmt)))
> @@ -690,7 +697,7 @@ static int put_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp,
>  
>  	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
>  	    get_user(base, &kp->base) ||
> -	    put_user(ptr_to_compat(base), &up->base) ||
> +	    put_user(ptr_to_compat((void __user *)base), &up->base) ||
>  	    assign_in_user(&up->capability, &kp->capability) ||
>  	    assign_in_user(&up->flags, &kp->flags) ||
>  	    copy_in_user(&up->fmt, &kp->fmt, sizeof(kp->fmt)))
> @@ -857,11 +864,19 @@ static int put_v4l2_ext_controls32(struct file *file,
>  				   struct v4l2_ext_controls32 __user *up)
>  {
>  	struct v4l2_ext_control32 __user *ucontrols;
> -	struct v4l2_ext_control __user *kcontrols;
> +	struct v4l2_ext_control *kcontrols;
>  	u32 count;
>  	u32 n;
>  	compat_caddr_t p;
>  
> +	/*
> +	 * We need to define kcontrols without __user, even though it does
> +	 * point to data in userspace here. The reason is that v4l2-ioctl.c
> +	 * copies it from userspace to kernelspace, so its definition in
> +	 * videodev2.h doesn't have a __user markup. Defining kcontrols
> +	 * with __user causes smatch warnings, so instead declare it
> +	 * without __user and cast it as a userspace pointer where needed.
> +	 */
>  	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
>  	    assign_in_user(&up->which, &kp->which) ||
>  	    get_user(count, &kp->count) ||
> @@ -883,10 +898,12 @@ static int put_v4l2_ext_controls32(struct file *file,
>  		unsigned int size = sizeof(*ucontrols);
>  		u32 id;
>  
> -		if (get_user(id, &kcontrols->id) ||
> +		if (get_user(id, (unsigned int __user *)&kcontrols->id) ||
>  		    put_user(id, &ucontrols->id) ||
> -		    assign_in_user(&ucontrols->size, &kcontrols->size) ||
> -		    copy_in_user(&ucontrols->reserved2, &kcontrols->reserved2,
> +		    assign_in_user(&ucontrols->size,
> +				   (unsigned int __user *)&kcontrols->size) ||
> +		    copy_in_user(&ucontrols->reserved2,
> +				 (void __user *)&kcontrols->reserved2,
>  				 sizeof(ucontrols->reserved2)))
>  			return -EFAULT;
>  
> @@ -898,7 +915,8 @@ static int put_v4l2_ext_controls32(struct file *file,
>  		if (ctrl_is_pointer(file, id))
>  			size -= sizeof(ucontrols->value64);
>  
> -		if (copy_in_user(ucontrols, kcontrols, size))
> +		if (copy_in_user(ucontrols,
> +			         (void __user *)kcontrols, size))
>  			return -EFAULT;
>  
>  		ucontrols++;
> @@ -952,9 +970,10 @@ static int get_v4l2_edid32(struct v4l2_edid __user *kp,
>  	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
>  	    assign_in_user(&kp->pad, &up->pad) ||
>  	    assign_in_user(&kp->start_block, &up->start_block) ||
> -	    assign_in_user(&kp->blocks, &up->blocks) ||
> +	    assign_in_user(&kp->blocks,
> +			   (u32 __user *)&up->blocks) ||
>  	    get_user(tmp, &up->edid) ||
> -	    put_user(compat_ptr(tmp), &kp->edid) ||
> +	    put_user((void __force *)compat_ptr(tmp), &kp->edid) ||
>  	    copy_in_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
>  		return -EFAULT;
>  	return 0;
> @@ -970,7 +989,7 @@ static int put_v4l2_edid32(struct v4l2_edid __user *kp,
>  	    assign_in_user(&up->start_block, &kp->start_block) ||
>  	    assign_in_user(&up->blocks, &kp->blocks) ||
>  	    get_user(edid, &kp->edid) ||
> -	    put_user(ptr_to_compat(edid), &up->edid) ||
> +	    put_user(ptr_to_compat((void __user *)edid), &up->edid) ||
>  	    copy_in_user(up->reserved, kp->reserved, sizeof(up->reserved)))
>  		return -EFAULT;
>  	return 0;
> 

Re: [PATCH v2 1/4] media: v4l2-compat-ioctl32: fix several __user annotations

[Sakari Ailus]

Hi Mauro,

Thanks for the update. Just a few comments below...

On Thu, Apr 19, 2018 at 12:33:29PM -0400, Mauro Carvalho Chehab wrote:
> Smatch report several issues with bad __user annotations:
> 
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21: warning: incorrect type in argument 1 (different address spaces)
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:    expected void [noderef] <asn:1>*uptr
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:447:21:    got void *<noident>
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21: warning: incorrect type in argument 1 (different address spaces)
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:    expected void const volatile [noderef] <asn:1>*<noident>
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:    got struct v4l2_plane [noderef] <asn:1>**<noident>
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13: warning: incorrect type in argument 1 (different address spaces)
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13:    expected void [noderef] <asn:1>*uptr
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:693:13:    got void *[assigned] base
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13: warning: incorrect type in assignment (different address spaces)
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:    expected struct v4l2_ext_control [noderef] <asn:1>*kcontrols
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:871:13:    got struct v4l2_ext_control *<noident>
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13: warning: incorrect type in assignment (different address spaces)
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13:    expected unsigned char [usertype] *__pu_val
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:957:13:    got void [noderef] <asn:1>*
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13: warning: incorrect type in argument 1 (different address spaces)
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:    expected void [noderef] <asn:1>*uptr
>   drivers/media/v4l2-core/v4l2-compat-ioctl32.c:973:13:    got void *[assigned] edid
> 
> Fix them.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> ---
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 51 ++++++++++++++++++---------
>  1 file changed, 35 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index d03a44d89649..51c7c5ab15ef 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -443,8 +443,8 @@ static int put_v4l2_plane32(struct v4l2_plane __user *up,
>  			return -EFAULT;
>  		break;
>  	case V4L2_MEMORY_USERPTR:
> -		if (get_user(p, &up->m.userptr) ||
> -		    put_user((compat_ulong_t)ptr_to_compat((__force void *)p),
> +		if (get_user(p, &up->m.userptr)||

Space before "||", i.e. as it was?

> +		    put_user((compat_ulong_t)ptr_to_compat((void __user *)p),
>  			     &up32->m.userptr))
>  			return -EFAULT;
>  		break;
> @@ -587,7 +587,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
>  	u32 length;
>  	enum v4l2_memory memory;
>  	struct v4l2_plane32 __user *uplane32;
> -	struct v4l2_plane __user *uplane;
> +	struct v4l2_plane *uplane;
>  	compat_caddr_t p;
>  	int ret;
>  
> @@ -617,15 +617,22 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
>  
>  		if (num_planes == 0)
>  			return 0;
> -
> -		if (get_user(uplane, ((__force struct v4l2_plane __user **)&kp->m.planes)))
> +		/* We need to define uplane without __user, even though

/*
 * ...

> +		 * it does point to data in userspace here. The reason is
> +		 * that v4l2-ioctl.c copies it from userspace to kernelspace,
> +		 * so its definition in videodev2.h doesn't have a
> +		 * __user markup. Defining uplane with __user causes
> +		 * smatch warnings, so instead declare it without __user
> +		 * and cast it as a userspace pointer to put_v4l2_plane32().
> +		 */
> +		if (get_user(uplane, &kp->m.planes))

This line looks much better indeed...

>  			return -EFAULT;
>  		if (get_user(p, &up->m.planes))
>  			return -EFAULT;
>  		uplane32 = compat_ptr(p);
>  
>  		while (num_planes--) {
> -			ret = put_v4l2_plane32(uplane, uplane32, memory);
> +			ret = put_v4l2_plane32((void __user *)uplane, uplane32, memory);

Over 80; please wrap.

>  			if (ret)
>  				return ret;
>  			++uplane;
> @@ -675,7 +682,7 @@ static int get_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp,
>  
>  	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
>  	    get_user(tmp, &up->base) ||
> -	    put_user((__force void *)compat_ptr(tmp), &kp->base) ||
> +	    put_user((void __force *)compat_ptr(tmp), &kp->base) ||
>  	    assign_in_user(&kp->capability, &up->capability) ||
>  	    assign_in_user(&kp->flags, &up->flags) ||
>  	    copy_in_user(&kp->fmt, &up->fmt, sizeof(kp->fmt)))
> @@ -690,7 +697,7 @@ static int put_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp,
>  
>  	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
>  	    get_user(base, &kp->base) ||
> -	    put_user(ptr_to_compat(base), &up->base) ||
> +	    put_user(ptr_to_compat((void __user *)base), &up->base) ||
>  	    assign_in_user(&up->capability, &kp->capability) ||
>  	    assign_in_user(&up->flags, &kp->flags) ||
>  	    copy_in_user(&up->fmt, &kp->fmt, sizeof(kp->fmt)))
> @@ -857,11 +864,19 @@ static int put_v4l2_ext_controls32(struct file *file,
>  				   struct v4l2_ext_controls32 __user *up)
>  {
>  	struct v4l2_ext_control32 __user *ucontrols;
> -	struct v4l2_ext_control __user *kcontrols;
> +	struct v4l2_ext_control *kcontrols;
>  	u32 count;
>  	u32 n;
>  	compat_caddr_t p;
>  
> +	/*
> +	 * We need to define kcontrols without __user, even though it does
> +	 * point to data in userspace here. The reason is that v4l2-ioctl.c
> +	 * copies it from userspace to kernelspace, so its definition in
> +	 * videodev2.h doesn't have a __user markup. Defining kcontrols
> +	 * with __user causes smatch warnings, so instead declare it
> +	 * without __user and cast it as a userspace pointer where needed.
> +	 */
>  	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
>  	    assign_in_user(&up->which, &kp->which) ||
>  	    get_user(count, &kp->count) ||
> @@ -883,10 +898,12 @@ static int put_v4l2_ext_controls32(struct file *file,
>  		unsigned int size = sizeof(*ucontrols);
>  		u32 id;
>  
> -		if (get_user(id, &kcontrols->id) ||
> +		if (get_user(id, (unsigned int __user *)&kcontrols->id) ||

id is u32, kcontrols->id is as well. So u32?

>  		    put_user(id, &ucontrols->id) ||
> -		    assign_in_user(&ucontrols->size, &kcontrols->size) ||
> -		    copy_in_user(&ucontrols->reserved2, &kcontrols->reserved2,
> +		    assign_in_user(&ucontrols->size,
> +				   (unsigned int __user *)&kcontrols->size) ||

The same applies to size. It actually should have been u32, locally, too.

> +		    copy_in_user(&ucontrols->reserved2,
> +				 (void __user *)&kcontrols->reserved2,
>  				 sizeof(ucontrols->reserved2)))
>  			return -EFAULT;
>  
> @@ -898,7 +915,8 @@ static int put_v4l2_ext_controls32(struct file *file,
>  		if (ctrl_is_pointer(file, id))
>  			size -= sizeof(ucontrols->value64);
>  
> -		if (copy_in_user(ucontrols, kcontrols, size))
> +		if (copy_in_user(ucontrols,
> +			         (void __user *)kcontrols, size))

Fits on previous line.

>  			return -EFAULT;
>  
>  		ucontrols++;
> @@ -952,9 +970,10 @@ static int get_v4l2_edid32(struct v4l2_edid __user *kp,
>  	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
>  	    assign_in_user(&kp->pad, &up->pad) ||
>  	    assign_in_user(&kp->start_block, &up->start_block) ||
> -	    assign_in_user(&kp->blocks, &up->blocks) ||
> +	    assign_in_user(&kp->blocks,
> +			   (u32 __user *)&up->blocks) ||

Fits on previous line.

>  	    get_user(tmp, &up->edid) ||
> -	    put_user(compat_ptr(tmp), &kp->edid) ||
> +	    put_user((void __force *)compat_ptr(tmp), &kp->edid) ||
>  	    copy_in_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
>  		return -EFAULT;
>  	return 0;
> @@ -970,7 +989,7 @@ static int put_v4l2_edid32(struct v4l2_edid __user *kp,
>  	    assign_in_user(&up->start_block, &kp->start_block) ||
>  	    assign_in_user(&up->blocks, &kp->blocks) ||
>  	    get_user(edid, &kp->edid) ||
> -	    put_user(ptr_to_compat(edid), &up->edid) ||
> +	    put_user(ptr_to_compat((void __user *)edid), &up->edid) ||
>  	    copy_in_user(up->reserved, kp->reserved, sizeof(up->reserved)))
>  		return -EFAULT;
>  	return 0;

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

[PATCH v2 2/4] media: v4l2-compat-ioctl32: better name userspace pointers

[Mauro Carvalho Chehab]

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.

Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 588 +++++++++++++-------------
 1 file changed, 294 insertions(+), 294 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 51c7c5ab15ef..680b64c1d69a 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 {
 	__u8                    global_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_ptr(p);
 	while (clipcount--) {
@@ -161,11 +161,11 @@ struct v4l2_create_buffers32 {
 	__u32			reserved[8];
 };
 
-static int __bufsize_v4l2_format(struct v4l2_format32 __user *up, u32 *size)
+static int __bufsize_v4l2_format(struct v4l2_format32 __user *p32, u32 *size)
 {
 	u32 type;
 
-	if (get_user(type, &up->type))
+	if (get_user(type, &p32->type))
 		return -EFAULT;
 
 	switch (type) {
@@ -173,7 +173,7 @@ static int __bufsize_v4l2_format(struct v4l2_format32 __user *up, u32 *size)
 	case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY: {
 		u32 clipcount;
 
-		if (get_user(clipcount, &up->fmt.win.clipcount))
+		if (get_user(clipcount, &p32->fmt.win.clipcount))
 			return -EFAULT;
 		if (clipcount > 2048)
 			return -EINVAL;
@@ -186,141 +186,141 @@ static int __bufsize_v4l2_format(struct v4l2_format32 __user *up, u32 *size)
 	}
 }
 
-static int bufsize_v4l2_format(struct v4l2_format32 __user *up, u32 *size)
+static int bufsize_v4l2_format(struct v4l2_format32 __user *p32, u32 *size)
 {
-	if (!access_ok(VERIFY_READ, up, sizeof(*up)))
+	if (!access_ok(VERIFY_READ, p32, sizeof(*p32)))
 		return -EFAULT;
-	return __bufsize_v4l2_format(up, size);
+	return __bufsize_v4l2_format(p32, size);
 }
 
-static int __get_v4l2_format32(struct v4l2_format __user *kp,
-			       struct v4l2_format32 __user *up,
+static int __get_v4l2_format32(struct v4l2_format __user *p64,
+			       struct v4l2_format32 __user *p32,
 			       void __user *aux_buf, u32 aux_space)
 {
 	u32 type;
 
-	if (get_user(type, &up->type) || put_user(type, &kp->type))
+	if (get_user(type, &p32->type) || put_user(type, &p64->type))
 		return -EFAULT;
 
 	switch (type) {
 	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
 	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
-		return copy_in_user(&kp->fmt.pix, &up->fmt.pix,
-				    sizeof(kp->fmt.pix)) ? -EFAULT : 0;
+		return copy_in_user(&p64->fmt.pix, &p32->fmt.pix,
+				    sizeof(p64->fmt.pix)) ? -EFAULT : 0;
 	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
 	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
-		return copy_in_user(&kp->fmt.pix_mp, &up->fmt.pix_mp,
-				    sizeof(kp->fmt.pix_mp)) ? -EFAULT : 0;
+		return copy_in_user(&p64->fmt.pix_mp, &p32->fmt.pix_mp,
+				    sizeof(p64->fmt.pix_mp)) ? -EFAULT : 0;
 	case V4L2_BUF_TYPE_VIDEO_OVERLAY:
 	case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY:
-		return get_v4l2_window32(&kp->fmt.win, &up->fmt.win,
+		return get_v4l2_window32(&p64->fmt.win, &p32->fmt.win,
 					 aux_buf, aux_space);
 	case V4L2_BUF_TYPE_VBI_CAPTURE:
 	case V4L2_BUF_TYPE_VBI_OUTPUT:
-		return copy_in_user(&kp->fmt.vbi, &up->fmt.vbi,
-				    sizeof(kp->fmt.vbi)) ? -EFAULT : 0;
+		return copy_in_user(&p64->fmt.vbi, &p32->fmt.vbi,
+				    sizeof(p64->fmt.vbi)) ? -EFAULT : 0;
 	case V4L2_BUF_TYPE_SLICED_VBI_CAPTURE:
 	case V4L2_BUF_TYPE_SLICED_VBI_OUTPUT:
-		return copy_in_user(&kp->fmt.sliced, &up->fmt.sliced,
-				    sizeof(kp->fmt.sliced)) ? -EFAULT : 0;
+		return copy_in_user(&p64->fmt.sliced, &p32->fmt.sliced,
+				    sizeof(p64->fmt.sliced)) ? -EFAULT : 0;
 	case V4L2_BUF_TYPE_SDR_CAPTURE:
 	case V4L2_BUF_TYPE_SDR_OUTPUT:
-		return copy_in_user(&kp->fmt.sdr, &up->fmt.sdr,
-				    sizeof(kp->fmt.sdr)) ? -EFAULT : 0;
+		return copy_in_user(&p64->fmt.sdr, &p32->fmt.sdr,
+				    sizeof(p64->fmt.sdr)) ? -EFAULT : 0;
 	case V4L2_BUF_TYPE_META_CAPTURE:
-		return copy_in_user(&kp->fmt.meta, &up->fmt.meta,
-				    sizeof(kp->fmt.meta)) ? -EFAULT : 0;
+		return copy_in_user(&p64->fmt.meta, &p32->fmt.meta,
+				    sizeof(p64->fmt.meta)) ? -EFAULT : 0;
 	default:
 		return -EINVAL;
 	}
 }
 
-static int get_v4l2_format32(struct v4l2_format __user *kp,
-			     struct v4l2_format32 __user *up,
+static int get_v4l2_format32(struct v4l2_format __user *p64,
+			     struct v4l2_format32 __user *p32,
 			     void __user *aux_buf, u32 aux_space)
 {
-	if (!access_ok(VERIFY_READ, up, sizeof(*up)))
+	if (!access_ok(VERIFY_READ, p32, sizeof(*p32)))
 		return -EFAULT;
-	return __get_v4l2_format32(kp, up, aux_buf, aux_space);
+	return __get_v4l2_format32(p64, p32, aux_buf, aux_space);
 }
 
-static int bufsize_v4l2_create(struct v4l2_create_buffers32 __user *up,
+static int bufsize_v4l2_create(struct v4l2_create_buffers32 __user *p32,
 			       u32 *size)
 {
-	if (!access_ok(VERIFY_READ, up, sizeof(*up)))
+	if (!access_ok(VERIFY_READ, p32, sizeof(*p32)))
 		return -EFAULT;
-	return __bufsize_v4l2_format(&up->format, size);
+	return __bufsize_v4l2_format(&p32->format, size);
 }
 
-static int get_v4l2_create32(struct v4l2_create_buffers __user *kp,
-			     struct v4l2_create_buffers32 __user *up,
+static int get_v4l2_create32(struct v4l2_create_buffers __user *p64,
+			     struct v4l2_create_buffers32 __user *p32,
 			     void __user *aux_buf, u32 aux_space)
 {
-	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
-	    copy_in_user(kp, up,
+	if (!access_ok(VERIFY_READ, p32, sizeof(*p32)) ||
+	    copy_in_user(p64, p32,
 			 offsetof(struct v4l2_create_buffers32, format)))
 		return -EFAULT;
-	return __get_v4l2_format32(&kp->format, &up->format,
+	return __get_v4l2_format32(&p64->format, &p32->format,
 				   aux_buf, aux_space);
 }
 
-static int __put_v4l2_format32(struct v4l2_format __user *kp,
-			       struct v4l2_format32 __user *up)
+static int __put_v4l2_format32(struct v4l2_format __user *p64,
+			       struct v4l2_format32 __user *p32)
 {
 	u32 type;
 
-	if (get_user(type, &kp->type))
+	if (get_user(type, &p64->type))
 		return -EFAULT;
 
 	switch (type) {
 	case V4L2_BUF_TYPE_VIDEO_CAPTURE:
 	case V4L2_BUF_TYPE_VIDEO_OUTPUT:
-		return copy_in_user(&up->fmt.pix, &kp->fmt.pix,
-				    sizeof(kp->fmt.pix)) ? -EFAULT : 0;
+		return copy_in_user(&p32->fmt.pix, &p64->fmt.pix,
+				    sizeof(p64->fmt.pix)) ? -EFAULT : 0;
 	case V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE:
 	case V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE:
-		return copy_in_user(&up->fmt.pix_mp, &kp->fmt.pix_mp,
-				    sizeof(kp->fmt.pix_mp)) ? -EFAULT : 0;
+		return copy_in_user(&p32->fmt.pix_mp, &p64->fmt.pix_mp,
+				    sizeof(p64->fmt.pix_mp)) ? -EFAULT : 0;
 	case V4L2_BUF_TYPE_VIDEO_OVERLAY:
 	case V4L2_BUF_TYPE_VIDEO_OUTPUT_OVERLAY:
-		return put_v4l2_window32(&kp->fmt.win, &up->fmt.win);
+		return put_v4l2_window32(&p64->fmt.win, &p32->fmt.win);
 	case V4L2_BUF_TYPE_VBI_CAPTURE:
 	case V4L2_BUF_TYPE_VBI_OUTPUT:
-		return copy_in_user(&up->fmt.vbi, &kp->fmt.vbi,
-				    sizeof(kp->fmt.vbi)) ? -EFAULT : 0;
+		return copy_in_user(&p32->fmt.vbi, &p64->fmt.vbi,
+				    sizeof(p64->fmt.vbi)) ? -EFAULT : 0;
 	case V4L2_BUF_TYPE_SLICED_VBI_CAPTURE:
 	case V4L2_BUF_TYPE_SLICED_VBI_OUTPUT:
-		return copy_in_user(&up->fmt.sliced, &kp->fmt.sliced,
-				    sizeof(kp->fmt.sliced)) ? -EFAULT : 0;
+		return copy_in_user(&p32->fmt.sliced, &p64->fmt.sliced,
+				    sizeof(p64->fmt.sliced)) ? -EFAULT : 0;
 	case V4L2_BUF_TYPE_SDR_CAPTURE:
 	case V4L2_BUF_TYPE_SDR_OUTPUT:
-		return copy_in_user(&up->fmt.sdr, &kp->fmt.sdr,
-				    sizeof(kp->fmt.sdr)) ? -EFAULT : 0;
+		return copy_in_user(&p32->fmt.sdr, &p64->fmt.sdr,
+				    sizeof(p64->fmt.sdr)) ? -EFAULT : 0;
 	case V4L2_BUF_TYPE_META_CAPTURE:
-		return copy_in_user(&up->fmt.meta, &kp->fmt.meta,
-				    sizeof(kp->fmt.meta)) ? -EFAULT : 0;
+		return copy_in_user(&p32->fmt.meta, &p64->fmt.meta,
+				    sizeof(p64->fmt.meta)) ? -EFAULT : 0;
 	default:
 		return -EINVAL;
 	}
 }
 
-static int put_v4l2_format32(struct v4l2_format __user *kp,
-			     struct v4l2_format32 __user *up)
+static int put_v4l2_format32(struct v4l2_format __user *p64,
+			     struct v4l2_format32 __user *p32)
 {
-	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)))
+	if (!access_ok(VERIFY_WRITE, p32, sizeof(*p32)))
 		return -EFAULT;
-	return __put_v4l2_format32(kp, up);
+	return __put_v4l2_format32(p64, p32);
 }
 
-static int put_v4l2_create32(struct v4l2_create_buffers __user *kp,
-			     struct v4l2_create_buffers32 __user *up)
+static int put_v4l2_create32(struct v4l2_create_buffers __user *p64,
+			     struct v4l2_create_buffers32 __user *p32)
 {
-	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
-	    copy_in_user(up, kp,
+	if (!access_ok(VERIFY_WRITE, p32, sizeof(*p32)) ||
+	    copy_in_user(p32, p64,
 			 offsetof(struct v4l2_create_buffers32, format)) ||
-	    copy_in_user(up->reserved, kp->reserved, sizeof(kp->reserved)))
+	    copy_in_user(p32->reserved, p64->reserved, sizeof(p64->reserved)))
 		return -EFAULT;
-	return __put_v4l2_format32(&kp->format, &up->format);
+	return __put_v4l2_format32(&p64->format, &p32->format);
 }
 
 struct v4l2_standard32 {
@@ -332,27 +332,27 @@ struct v4l2_standard32 {
 	__u32		     reserved[4];
 };
 
-static int get_v4l2_standard32(struct v4l2_standard __user *kp,
-			       struct v4l2_standard32 __user *up)
+static int get_v4l2_standard32(struct v4l2_standard __user *p64,
+			       struct v4l2_standard32 __user *p32)
 {
 	/* other fields are not set by the user, nor used by the driver */
-	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
-	    assign_in_user(&kp->index, &up->index))
+	if (!access_ok(VERIFY_READ, p32, sizeof(*p32)) ||
+	    assign_in_user(&p64->index, &p32->index))
 		return -EFAULT;
 	return 0;
 }
 
-static int put_v4l2_standard32(struct v4l2_standard __user *kp,
-			       struct v4l2_standard32 __user *up)
+static int put_v4l2_standard32(struct v4l2_standard __user *p64,
+			       struct v4l2_standard32 __user *p32)
 {
-	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
-	    assign_in_user(&up->index, &kp->index) ||
-	    assign_in_user(&up->id, &kp->id) ||
-	    copy_in_user(up->name, kp->name, sizeof(up->name)) ||
-	    copy_in_user(&up->frameperiod, &kp->frameperiod,
-			 sizeof(up->frameperiod)) ||
-	    assign_in_user(&up->framelines, &kp->framelines) ||
-	    copy_in_user(up->reserved, kp->reserved, sizeof(up->reserved)))
+	if (!access_ok(VERIFY_WRITE, p32, sizeof(*p32)) ||
+	    assign_in_user(&p32->index, &p64->index) ||
+	    assign_in_user(&p32->id, &p64->id) ||
+	    copy_in_user(p32->name, p64->name, sizeof(p32->name)) ||
+	    copy_in_user(&p32->frameperiod, &p64->frameperiod,
+			 sizeof(p32->frameperiod)) ||
+	    assign_in_user(&p32->framelines, &p64->framelines) ||
+	    copy_in_user(p32->reserved, p64->reserved, sizeof(p32->reserved)))
 		return -EFAULT;
 	return 0;
 }
@@ -392,31 +392,31 @@ struct v4l2_buffer32 {
 	__u32			reserved;
 };
 
-static int get_v4l2_plane32(struct v4l2_plane __user *up,
-			    struct v4l2_plane32 __user *up32,
+static int get_v4l2_plane32(struct v4l2_plane __user *p64,
+			    struct v4l2_plane32 __user *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(p64, p32, 2 * sizeof(__u32)) ||
+	    copy_in_user(&p64->data_offset, &p32->data_offset,
+			 sizeof(p64->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,
-				 sizeof(up32->m.mem_offset)))
+		if (copy_in_user(&p64->m.mem_offset, &p32->m.mem_offset,
+				 sizeof(p32->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))
+		if (get_user(p, &p32->m.userptr) ||
+		    put_user((unsigned long)compat_ptr(p), &p64->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(&p64->m.fd, &p32->m.fd, sizeof(p32->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,
-			    struct v4l2_plane32 __user *up32,
+static int put_v4l2_plane32(struct v4l2_plane __user *p64,
+			    struct v4l2_plane32 __user *p32,
 			    enum v4l2_memory memory)
 {
 	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(p32, p64, 2 * sizeof(__u32)) ||
+	    copy_in_user(&p32->data_offset, &p64->data_offset,
+			 sizeof(p64->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(&p32->m.mem_offset, &p64->m.mem_offset,
+				 sizeof(p64->m.mem_offset)))
 			return -EFAULT;
 		break;
 	case V4L2_MEMORY_USERPTR:
-		if (get_user(p, &up->m.userptr)||
+		if (get_user(p, &p64->m.userptr)||
 		    put_user((compat_ulong_t)ptr_to_compat((void __user *)p),
-			     &up32->m.userptr))
+			     &p32->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(&p32->m.fd, &p64->m.fd, sizeof(p64->m.fd)))
 			return -EFAULT;
 		break;
 	}
@@ -457,14 +457,14 @@ static int put_v4l2_plane32(struct v4l2_plane __user *up,
 	return 0;
 }
 
-static int bufsize_v4l2_buffer(struct v4l2_buffer32 __user *up, u32 *size)
+static int bufsize_v4l2_buffer(struct v4l2_buffer32 __user *p32, u32 *size)
 {
 	u32 type;
 	u32 length;
 
-	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
-	    get_user(type, &up->type) ||
-	    get_user(length, &up->length))
+	if (!access_ok(VERIFY_READ, p32, sizeof(*p32)) ||
+	    get_user(type, &p32->type) ||
+	    get_user(length, &p32->length))
 		return -EFAULT;
 
 	if (V4L2_TYPE_IS_MULTIPLANAR(type)) {
@@ -482,8 +482,8 @@ static int bufsize_v4l2_buffer(struct v4l2_buffer32 __user *up, u32 *size)
 	return 0;
 }
 
-static int get_v4l2_buffer32(struct v4l2_buffer __user *kp,
-			     struct v4l2_buffer32 __user *up,
+static int get_v4l2_buffer32(struct v4l2_buffer __user *p64,
+			     struct v4l2_buffer32 __user *p32,
 			     void __user *aux_buf, u32 aux_space)
 {
 	u32 type;
@@ -494,24 +494,24 @@ static int get_v4l2_buffer32(struct v4l2_buffer __user *kp,
 	compat_caddr_t p;
 	int ret;
 
-	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
-	    assign_in_user(&kp->index, &up->index) ||
-	    get_user(type, &up->type) ||
-	    put_user(type, &kp->type) ||
-	    assign_in_user(&kp->flags, &up->flags) ||
-	    get_user(memory, &up->memory) ||
-	    put_user(memory, &kp->memory) ||
-	    get_user(length, &up->length) ||
-	    put_user(length, &kp->length))
+	if (!access_ok(VERIFY_READ, p32, sizeof(*p32)) ||
+	    assign_in_user(&p64->index, &p32->index) ||
+	    get_user(type, &p32->type) ||
+	    put_user(type, &p64->type) ||
+	    assign_in_user(&p64->flags, &p32->flags) ||
+	    get_user(memory, &p32->memory) ||
+	    put_user(memory, &p64->memory) ||
+	    get_user(length, &p32->length) ||
+	    put_user(length, &p64->length))
 		return -EFAULT;
 
 	if (V4L2_TYPE_IS_OUTPUT(type))
-		if (assign_in_user(&kp->bytesused, &up->bytesused) ||
-		    assign_in_user(&kp->field, &up->field) ||
-		    assign_in_user(&kp->timestamp.tv_sec,
-				   &up->timestamp.tv_sec) ||
-		    assign_in_user(&kp->timestamp.tv_usec,
-				   &up->timestamp.tv_usec))
+		if (assign_in_user(&p64->bytesused, &p32->bytesused) ||
+		    assign_in_user(&p64->field, &p32->field) ||
+		    assign_in_user(&p64->timestamp.tv_sec,
+				   &p32->timestamp.tv_sec) ||
+		    assign_in_user(&p64->timestamp.tv_usec,
+				   &p32->timestamp.tv_usec))
 			return -EFAULT;
 
 	if (V4L2_TYPE_IS_MULTIPLANAR(type)) {
@@ -522,12 +522,12 @@ static int get_v4l2_buffer32(struct v4l2_buffer __user *kp,
 			 * num_planes == 0 is legal, e.g. when userspace doesn't
 			 * need planes array on DQBUF
 			 */
-			return put_user(NULL, &kp->m.planes);
+			return put_user(NULL, &p64->m.planes);
 		}
 		if (num_planes > VIDEO_MAX_PLANES)
 			return -EINVAL;
 
-		if (get_user(p, &up->m.planes))
+		if (get_user(p, &p32->m.planes))
 			return -EFAULT;
 
 		uplane32 = compat_ptr(p);
@@ -544,7 +544,7 @@ static int get_v4l2_buffer32(struct v4l2_buffer __user *kp,
 
 		uplane = aux_buf;
 		if (put_user((__force struct v4l2_plane *)uplane,
-			     &kp->m.planes))
+			     &p64->m.planes))
 			return -EFAULT;
 
 		while (num_planes--) {
@@ -558,20 +558,20 @@ static int get_v4l2_buffer32(struct v4l2_buffer __user *kp,
 		switch (memory) {
 		case V4L2_MEMORY_MMAP:
 		case V4L2_MEMORY_OVERLAY:
-			if (assign_in_user(&kp->m.offset, &up->m.offset))
+			if (assign_in_user(&p64->m.offset, &p32->m.offset))
 				return -EFAULT;
 			break;
 		case V4L2_MEMORY_USERPTR: {
 			compat_ulong_t userptr;
 
-			if (get_user(userptr, &up->m.userptr) ||
+			if (get_user(userptr, &p32->m.userptr) ||
 			    put_user((unsigned long)compat_ptr(userptr),
-				     &kp->m.userptr))
+				     &p64->m.userptr))
 				return -EFAULT;
 			break;
 		}
 		case V4L2_MEMORY_DMABUF:
-			if (assign_in_user(&kp->m.fd, &up->m.fd))
+			if (assign_in_user(&p64->m.fd, &p32->m.fd))
 				return -EFAULT;
 			break;
 		}
@@ -580,8 +580,8 @@ static int get_v4l2_buffer32(struct v4l2_buffer __user *kp,
 	return 0;
 }
 
-static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
-			     struct v4l2_buffer32 __user *up)
+static int put_v4l2_buffer32(struct v4l2_buffer __user *p64,
+			     struct v4l2_buffer32 __user *p32)
 {
 	u32 type;
 	u32 length;
@@ -591,25 +591,25 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
 	compat_caddr_t p;
 	int ret;
 
-	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
-	    assign_in_user(&up->index, &kp->index) ||
-	    get_user(type, &kp->type) ||
-	    put_user(type, &up->type) ||
-	    assign_in_user(&up->flags, &kp->flags) ||
-	    get_user(memory, &kp->memory) ||
-	    put_user(memory, &up->memory))
+	if (!access_ok(VERIFY_WRITE, p32, sizeof(*p32)) ||
+	    assign_in_user(&p32->index, &p64->index) ||
+	    get_user(type, &p64->type) ||
+	    put_user(type, &p32->type) ||
+	    assign_in_user(&p32->flags, &p64->flags) ||
+	    get_user(memory, &p64->memory) ||
+	    put_user(memory, &p32->memory))
 		return -EFAULT;
 
-	if (assign_in_user(&up->bytesused, &kp->bytesused) ||
-	    assign_in_user(&up->field, &kp->field) ||
-	    assign_in_user(&up->timestamp.tv_sec, &kp->timestamp.tv_sec) ||
-	    assign_in_user(&up->timestamp.tv_usec, &kp->timestamp.tv_usec) ||
-	    copy_in_user(&up->timecode, &kp->timecode, sizeof(kp->timecode)) ||
-	    assign_in_user(&up->sequence, &kp->sequence) ||
-	    assign_in_user(&up->reserved2, &kp->reserved2) ||
-	    assign_in_user(&up->reserved, &kp->reserved) ||
-	    get_user(length, &kp->length) ||
-	    put_user(length, &up->length))
+	if (assign_in_user(&p32->bytesused, &p64->bytesused) ||
+	    assign_in_user(&p32->field, &p64->field) ||
+	    assign_in_user(&p32->timestamp.tv_sec, &p64->timestamp.tv_sec) ||
+	    assign_in_user(&p32->timestamp.tv_usec, &p64->timestamp.tv_usec) ||
+	    copy_in_user(&p32->timecode, &p64->timecode, sizeof(p64->timecode)) ||
+	    assign_in_user(&p32->sequence, &p64->sequence) ||
+	    assign_in_user(&p32->reserved2, &p64->reserved2) ||
+	    assign_in_user(&p32->reserved, &p64->reserved) ||
+	    get_user(length, &p64->length) ||
+	    put_user(length, &p32->length))
 		return -EFAULT;
 
 	if (V4L2_TYPE_IS_MULTIPLANAR(type)) {
@@ -625,9 +625,9 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
 		 * smatch warnings, so instead declare it without __user
 		 * and cast it as a userspace pointer to put_v4l2_plane32().
 		 */
-		if (get_user(uplane, &kp->m.planes))
+		if (get_user(uplane, &p64->m.planes))
 			return -EFAULT;
-		if (get_user(p, &up->m.planes))
+		if (get_user(p, &p32->m.planes))
 			return -EFAULT;
 		uplane32 = compat_ptr(p);
 
@@ -642,15 +642,15 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
 		switch (memory) {
 		case V4L2_MEMORY_MMAP:
 		case V4L2_MEMORY_OVERLAY:
-			if (assign_in_user(&up->m.offset, &kp->m.offset))
+			if (assign_in_user(&p32->m.offset, &p64->m.offset))
 				return -EFAULT;
 			break;
 		case V4L2_MEMORY_USERPTR:
-			if (assign_in_user(&up->m.userptr, &kp->m.userptr))
+			if (assign_in_user(&p32->m.userptr, &p64->m.userptr))
 				return -EFAULT;
 			break;
 		case V4L2_MEMORY_DMABUF:
-			if (assign_in_user(&up->m.fd, &kp->m.fd))
+			if (assign_in_user(&p32->m.fd, &p64->m.fd))
 				return -EFAULT;
 			break;
 		}
@@ -675,32 +675,32 @@ struct v4l2_framebuffer32 {
 	} fmt;
 };
 
-static int get_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp,
-				  struct v4l2_framebuffer32 __user *up)
+static int get_v4l2_framebuffer32(struct v4l2_framebuffer __user *p64,
+				  struct v4l2_framebuffer32 __user *p32)
 {
 	compat_caddr_t tmp;
 
-	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
-	    get_user(tmp, &up->base) ||
-	    put_user((void __force *)compat_ptr(tmp), &kp->base) ||
-	    assign_in_user(&kp->capability, &up->capability) ||
-	    assign_in_user(&kp->flags, &up->flags) ||
-	    copy_in_user(&kp->fmt, &up->fmt, sizeof(kp->fmt)))
+	if (!access_ok(VERIFY_READ, p32, sizeof(*p32)) ||
+	    get_user(tmp, &p32->base) ||
+	    put_user((void __force *)compat_ptr(tmp), &p64->base) ||
+	    assign_in_user(&p64->capability, &p32->capability) ||
+	    assign_in_user(&p64->flags, &p32->flags) ||
+	    copy_in_user(&p64->fmt, &p32->fmt, sizeof(p64->fmt)))
 		return -EFAULT;
 	return 0;
 }
 
-static int put_v4l2_framebuffer32(struct v4l2_framebuffer __user *kp,
-				  struct v4l2_framebuffer32 __user *up)
+static int put_v4l2_framebuffer32(struct v4l2_framebuffer __user *p64,
+				  struct v4l2_framebuffer32 __user *p32)
 {
 	void *base;
 
-	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
-	    get_user(base, &kp->base) ||
-	    put_user(ptr_to_compat((void __user *)base), &up->base) ||
-	    assign_in_user(&up->capability, &kp->capability) ||
-	    assign_in_user(&up->flags, &kp->flags) ||
-	    copy_in_user(&up->fmt, &kp->fmt, sizeof(kp->fmt)))
+	if (!access_ok(VERIFY_WRITE, p32, sizeof(*p32)) ||
+	    get_user(base, &p64->base) ||
+	    put_user(ptr_to_compat((void __user *)base), &p32->base) ||
+	    assign_in_user(&p32->capability, &p64->capability) ||
+	    assign_in_user(&p32->flags, &p64->flags) ||
+	    copy_in_user(&p32->fmt, &p64->fmt, sizeof(p64->fmt)))
 		return -EFAULT;
 	return 0;
 }
@@ -721,18 +721,18 @@ struct v4l2_input32 {
  * The 64-bit v4l2_input struct has extra padding at the end of the struct.
  * Otherwise it is identical to the 32-bit version.
  */
-static inline int get_v4l2_input32(struct v4l2_input __user *kp,
-				   struct v4l2_input32 __user *up)
+static inline int get_v4l2_input32(struct v4l2_input __user *p64,
+				   struct v4l2_input32 __user *p32)
 {
-	if (copy_in_user(kp, up, sizeof(*up)))
+	if (copy_in_user(p64, p32, sizeof(*p32)))
 		return -EFAULT;
 	return 0;
 }
 
-static inline int put_v4l2_input32(struct v4l2_input __user *kp,
-				   struct v4l2_input32 __user *up)
+static inline int put_v4l2_input32(struct v4l2_input __user *p64,
+				   struct v4l2_input32 __user *p32)
 {
-	if (copy_in_user(up, kp, sizeof(*up)))
+	if (copy_in_user(p32, p64, sizeof(*p32)))
 		return -EFAULT;
 	return 0;
 }
@@ -786,13 +786,13 @@ static inline bool ctrl_is_pointer(struct file *file, u32 id)
 		(qec.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD);
 }
 
-static int bufsize_v4l2_ext_controls(struct v4l2_ext_controls32 __user *up,
+static int bufsize_v4l2_ext_controls(struct v4l2_ext_controls32 __user *p32,
 				     u32 *size)
 {
 	u32 count;
 
-	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
-	    get_user(count, &up->count))
+	if (!access_ok(VERIFY_READ, p32, sizeof(*p32)) ||
+	    get_user(count, &p32->count))
 		return -EFAULT;
 	if (count > V4L2_CID_MAX_CTRLS)
 		return -EINVAL;
@@ -801,8 +801,8 @@ static int bufsize_v4l2_ext_controls(struct v4l2_ext_controls32 __user *up,
 }
 
 static int get_v4l2_ext_controls32(struct file *file,
-				   struct v4l2_ext_controls __user *kp,
-				   struct v4l2_ext_controls32 __user *up,
+				   struct v4l2_ext_controls __user *p64,
+				   struct v4l2_ext_controls32 __user *p32,
 				   void __user *aux_buf, u32 aux_space)
 {
 	struct v4l2_ext_control32 __user *ucontrols;
@@ -811,19 +811,19 @@ static int get_v4l2_ext_controls32(struct file *file,
 	u32 n;
 	compat_caddr_t p;
 
-	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
-	    assign_in_user(&kp->which, &up->which) ||
-	    get_user(count, &up->count) ||
-	    put_user(count, &kp->count) ||
-	    assign_in_user(&kp->error_idx, &up->error_idx) ||
-	    copy_in_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
+	if (!access_ok(VERIFY_READ, p32, sizeof(*p32)) ||
+	    assign_in_user(&p64->which, &p32->which) ||
+	    get_user(count, &p32->count) ||
+	    put_user(count, &p64->count) ||
+	    assign_in_user(&p64->error_idx, &p32->error_idx) ||
+	    copy_in_user(p64->reserved, p32->reserved, sizeof(p64->reserved)))
 		return -EFAULT;
 
 	if (count == 0)
-		return put_user(NULL, &kp->controls);
+		return put_user(NULL, &p64->controls);
 	if (count > V4L2_CID_MAX_CTRLS)
 		return -EINVAL;
-	if (get_user(p, &up->controls))
+	if (get_user(p, &p32->controls))
 		return -EFAULT;
 	ucontrols = compat_ptr(p);
 	if (!access_ok(VERIFY_READ, ucontrols, count * sizeof(*ucontrols)))
@@ -832,7 +832,7 @@ static int get_v4l2_ext_controls32(struct file *file,
 		return -EFAULT;
 	kcontrols = aux_buf;
 	if (put_user((__force struct v4l2_ext_control *)kcontrols,
-		     &kp->controls))
+		     &p64->controls))
 		return -EFAULT;
 
 	for (n = 0; n < count; n++) {
@@ -860,8 +860,8 @@ static int get_v4l2_ext_controls32(struct file *file,
 }
 
 static int put_v4l2_ext_controls32(struct file *file,
-				   struct v4l2_ext_controls __user *kp,
-				   struct v4l2_ext_controls32 __user *up)
+				   struct v4l2_ext_controls __user *p64,
+				   struct v4l2_ext_controls32 __user *p32)
 {
 	struct v4l2_ext_control32 __user *ucontrols;
 	struct v4l2_ext_control *kcontrols;
@@ -877,18 +877,18 @@ static int put_v4l2_ext_controls32(struct file *file,
 	 * with __user causes smatch warnings, so instead declare it
 	 * without __user and cast it as a userspace pointer where needed.
 	 */
-	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
-	    assign_in_user(&up->which, &kp->which) ||
-	    get_user(count, &kp->count) ||
-	    put_user(count, &up->count) ||
-	    assign_in_user(&up->error_idx, &kp->error_idx) ||
-	    copy_in_user(up->reserved, kp->reserved, sizeof(up->reserved)) ||
-	    get_user(kcontrols, &kp->controls))
+	if (!access_ok(VERIFY_WRITE, p32, sizeof(*p32)) ||
+	    assign_in_user(&p32->which, &p64->which) ||
+	    get_user(count, &p64->count) ||
+	    put_user(count, &p32->count) ||
+	    assign_in_user(&p32->error_idx, &p64->error_idx) ||
+	    copy_in_user(p32->reserved, p64->reserved, sizeof(p32->reserved)) ||
+	    get_user(kcontrols, &p64->controls))
 		return -EFAULT;
 
 	if (!count || count > (U32_MAX/sizeof(*ucontrols)))
 		return 0;
-	if (get_user(p, &up->controls))
+	if (get_user(p, &p32->controls))
 		return -EFAULT;
 	ucontrols = compat_ptr(p);
 	if (!access_ok(VERIFY_WRITE, ucontrols, count * sizeof(*ucontrols)))
@@ -938,18 +938,18 @@ struct v4l2_event32 {
 	__u32				reserved[8];
 };
 
-static int put_v4l2_event32(struct v4l2_event __user *kp,
-			    struct v4l2_event32 __user *up)
+static int put_v4l2_event32(struct v4l2_event __user *p64,
+			    struct v4l2_event32 __user *p32)
 {
-	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
-	    assign_in_user(&up->type, &kp->type) ||
-	    copy_in_user(&up->u, &kp->u, sizeof(kp->u)) ||
-	    assign_in_user(&up->pending, &kp->pending) ||
-	    assign_in_user(&up->sequence, &kp->sequence) ||
-	    assign_in_user(&up->timestamp.tv_sec, &kp->timestamp.tv_sec) ||
-	    assign_in_user(&up->timestamp.tv_nsec, &kp->timestamp.tv_nsec) ||
-	    assign_in_user(&up->id, &kp->id) ||
-	    copy_in_user(up->reserved, kp->reserved, sizeof(up->reserved)))
+	if (!access_ok(VERIFY_WRITE, p32, sizeof(*p32)) ||
+	    assign_in_user(&p32->type, &p64->type) ||
+	    copy_in_user(&p32->u, &p64->u, sizeof(p64->u)) ||
+	    assign_in_user(&p32->pending, &p64->pending) ||
+	    assign_in_user(&p32->sequence, &p64->sequence) ||
+	    assign_in_user(&p32->timestamp.tv_sec, &p64->timestamp.tv_sec) ||
+	    assign_in_user(&p32->timestamp.tv_nsec, &p64->timestamp.tv_nsec) ||
+	    assign_in_user(&p32->id, &p64->id) ||
+	    copy_in_user(p32->reserved, p64->reserved, sizeof(p32->reserved)))
 		return -EFAULT;
 	return 0;
 }
@@ -962,35 +962,35 @@ struct v4l2_edid32 {
 	compat_caddr_t edid;
 };
 
-static int get_v4l2_edid32(struct v4l2_edid __user *kp,
-			   struct v4l2_edid32 __user *up)
+static int get_v4l2_edid32(struct v4l2_edid __user *p64,
+			   struct v4l2_edid32 __user *p32)
 {
 	compat_uptr_t tmp;
 
-	if (!access_ok(VERIFY_READ, up, sizeof(*up)) ||
-	    assign_in_user(&kp->pad, &up->pad) ||
-	    assign_in_user(&kp->start_block, &up->start_block) ||
-	    assign_in_user(&kp->blocks,
-			   (u32 __user *)&up->blocks) ||
-	    get_user(tmp, &up->edid) ||
-	    put_user((void __force *)compat_ptr(tmp), &kp->edid) ||
-	    copy_in_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
+	if (!access_ok(VERIFY_READ, p32, sizeof(*p32)) ||
+	    assign_in_user(&p64->pad, &p32->pad) ||
+	    assign_in_user(&p64->start_block, &p32->start_block) ||
+	    assign_in_user(&p64->blocks,
+			   (u32 __user *)&p32->blocks) ||
+	    get_user(tmp, &p32->edid) ||
+	    put_user((void __force *)compat_ptr(tmp), &p64->edid) ||
+	    copy_in_user(p64->reserved, p32->reserved, sizeof(p64->reserved)))
 		return -EFAULT;
 	return 0;
 }
 
-static int put_v4l2_edid32(struct v4l2_edid __user *kp,
-			   struct v4l2_edid32 __user *up)
+static int put_v4l2_edid32(struct v4l2_edid __user *p64,
+			   struct v4l2_edid32 __user *p32)
 {
 	void *edid;
 
-	if (!access_ok(VERIFY_WRITE, up, sizeof(*up)) ||
-	    assign_in_user(&up->pad, &kp->pad) ||
-	    assign_in_user(&up->start_block, &kp->start_block) ||
-	    assign_in_user(&up->blocks, &kp->blocks) ||
-	    get_user(edid, &kp->edid) ||
-	    put_user(ptr_to_compat((void __user *)edid), &up->edid) ||
-	    copy_in_user(up->reserved, kp->reserved, sizeof(up->reserved)))
+	if (!access_ok(VERIFY_WRITE, p32, sizeof(*p32)) ||
+	    assign_in_user(&p32->pad, &p64->pad) ||
+	    assign_in_user(&p32->start_block, &p64->start_block) ||
+	    assign_in_user(&p32->blocks, &p64->blocks) ||
+	    get_user(edid, &p64->edid) ||
+	    put_user(ptr_to_compat((void __user *)edid), &p32->edid) ||
+	    copy_in_user(p32->reserved, p64->reserved, sizeof(p32->reserved)))
 		return -EFAULT;
 	return 0;
 }
@@ -1024,20 +1024,20 @@ static int put_v4l2_edid32(struct v4l2_edid __user *kp,
 #define VIDIOC_S_OUTPUT32	_IOWR('V', 47, s32)
 
 static int alloc_userspace(unsigned int size, u32 aux_space,
-			   void __user **up_native)
+			   void __user **new_p64)
 {
-	*up_native = compat_alloc_user_space(size + aux_space);
-	if (!*up_native)
+	*new_p64 = compat_alloc_user_space(size + aux_space);
+	if (!*new_p64)
 		return -ENOMEM;
-	if (clear_user(*up_native, size))
+	if (clear_user(*new_p64, size))
 		return -EFAULT;
 	return 0;
 }
 
 static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
-	void __user *up = compat_ptr(arg);
-	void __user *up_native = NULL;
+	void __user *p32 = compat_ptr(arg);
+	void __user *new_p64 = NULL;
 	void __user *aux_buf;
 	u32 aux_space;
 	int compatible_arg = 1;
@@ -1078,50 +1078,50 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	case VIDIOC_STREAMOFF:
 	case VIDIOC_S_INPUT:
 	case VIDIOC_S_OUTPUT:
-		err = alloc_userspace(sizeof(unsigned int), 0, &up_native);
-		if (!err && assign_in_user((unsigned int __user *)up_native,
-					   (compat_uint_t __user *)up))
+		err = alloc_userspace(sizeof(unsigned int), 0, &new_p64);
+		if (!err && assign_in_user((unsigned int __user *)new_p64,
+					   (compat_uint_t __user *)p32))
 			err = -EFAULT;
 		compatible_arg = 0;
 		break;
 
 	case VIDIOC_G_INPUT:
 	case VIDIOC_G_OUTPUT:
-		err = alloc_userspace(sizeof(unsigned int), 0, &up_native);
+		err = alloc_userspace(sizeof(unsigned int), 0, &new_p64);
 		compatible_arg = 0;
 		break;
 
 	case VIDIOC_G_EDID:
 	case VIDIOC_S_EDID:
-		err = alloc_userspace(sizeof(struct v4l2_edid), 0, &up_native);
+		err = alloc_userspace(sizeof(struct v4l2_edid), 0, &new_p64);
 		if (!err)
-			err = get_v4l2_edid32(up_native, up);
+			err = get_v4l2_edid32(new_p64, p32);
 		compatible_arg = 0;
 		break;
 
 	case VIDIOC_G_FMT:
 	case VIDIOC_S_FMT:
 	case VIDIOC_TRY_FMT:
-		err = bufsize_v4l2_format(up, &aux_space);
+		err = bufsize_v4l2_format(p32, &aux_space);
 		if (!err)
 			err = alloc_userspace(sizeof(struct v4l2_format),
-					      aux_space, &up_native);
+					      aux_space, &new_p64);
 		if (!err) {
-			aux_buf = up_native + sizeof(struct v4l2_format);
-			err = get_v4l2_format32(up_native, up,
+			aux_buf = new_p64 + sizeof(struct v4l2_format);
+			err = get_v4l2_format32(new_p64, p32,
 						aux_buf, aux_space);
 		}
 		compatible_arg = 0;
 		break;
 
 	case VIDIOC_CREATE_BUFS:
-		err = bufsize_v4l2_create(up, &aux_space);
+		err = bufsize_v4l2_create(p32, &aux_space);
 		if (!err)
 			err = alloc_userspace(sizeof(struct v4l2_create_buffers),
-					      aux_space, &up_native);
+					      aux_space, &new_p64);
 		if (!err) {
-			aux_buf = up_native + sizeof(struct v4l2_create_buffers);
-			err = get_v4l2_create32(up_native, up,
+			aux_buf = new_p64 + sizeof(struct v4l2_create_buffers);
+			err = get_v4l2_create32(new_p64, p32,
 						aux_buf, aux_space);
 		}
 		compatible_arg = 0;
@@ -1131,13 +1131,13 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	case VIDIOC_QUERYBUF:
 	case VIDIOC_QBUF:
 	case VIDIOC_DQBUF:
-		err = bufsize_v4l2_buffer(up, &aux_space);
+		err = bufsize_v4l2_buffer(p32, &aux_space);
 		if (!err)
 			err = alloc_userspace(sizeof(struct v4l2_buffer),
-					      aux_space, &up_native);
+					      aux_space, &new_p64);
 		if (!err) {
-			aux_buf = up_native + sizeof(struct v4l2_buffer);
-			err = get_v4l2_buffer32(up_native, up,
+			aux_buf = new_p64 + sizeof(struct v4l2_buffer);
+			err = get_v4l2_buffer32(new_p64, p32,
 						aux_buf, aux_space);
 		}
 		compatible_arg = 0;
@@ -1145,49 +1145,49 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 
 	case VIDIOC_S_FBUF:
 		err = alloc_userspace(sizeof(struct v4l2_framebuffer), 0,
-				      &up_native);
+				      &new_p64);
 		if (!err)
-			err = get_v4l2_framebuffer32(up_native, up);
+			err = get_v4l2_framebuffer32(new_p64, p32);
 		compatible_arg = 0;
 		break;
 
 	case VIDIOC_G_FBUF:
 		err = alloc_userspace(sizeof(struct v4l2_framebuffer), 0,
-				      &up_native);
+				      &new_p64);
 		compatible_arg = 0;
 		break;
 
 	case VIDIOC_ENUMSTD:
 		err = alloc_userspace(sizeof(struct v4l2_standard), 0,
-				      &up_native);
+				      &new_p64);
 		if (!err)
-			err = get_v4l2_standard32(up_native, up);
+			err = get_v4l2_standard32(new_p64, p32);
 		compatible_arg = 0;
 		break;
 
 	case VIDIOC_ENUMINPUT:
-		err = alloc_userspace(sizeof(struct v4l2_input), 0, &up_native);
+		err = alloc_userspace(sizeof(struct v4l2_input), 0, &new_p64);
 		if (!err)
-			err = get_v4l2_input32(up_native, up);
+			err = get_v4l2_input32(new_p64, p32);
 		compatible_arg = 0;
 		break;
 
 	case VIDIOC_G_EXT_CTRLS:
 	case VIDIOC_S_EXT_CTRLS:
 	case VIDIOC_TRY_EXT_CTRLS:
-		err = bufsize_v4l2_ext_controls(up, &aux_space);
+		err = bufsize_v4l2_ext_controls(p32, &aux_space);
 		if (!err)
 			err = alloc_userspace(sizeof(struct v4l2_ext_controls),
-					      aux_space, &up_native);
+					      aux_space, &new_p64);
 		if (!err) {
-			aux_buf = up_native + sizeof(struct v4l2_ext_controls);
-			err = get_v4l2_ext_controls32(file, up_native, up,
+			aux_buf = new_p64 + sizeof(struct v4l2_ext_controls);
+			err = get_v4l2_ext_controls32(file, new_p64, p32,
 						      aux_buf, aux_space);
 		}
 		compatible_arg = 0;
 		break;
 	case VIDIOC_DQEVENT:
-		err = alloc_userspace(sizeof(struct v4l2_event), 0, &up_native);
+		err = alloc_userspace(sizeof(struct v4l2_event), 0, &new_p64);
 		compatible_arg = 0;
 		break;
 	}
@@ -1195,9 +1195,9 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 		return err;
 
 	if (compatible_arg)
-		err = native_ioctl(file, cmd, (unsigned long)up);
+		err = native_ioctl(file, cmd, (unsigned long)p32);
 	else
-		err = native_ioctl(file, cmd, (unsigned long)up_native);
+		err = native_ioctl(file, cmd, (unsigned long)new_p64);
 
 	if (err == -ENOTTY)
 		return err;
@@ -1211,11 +1211,11 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	case VIDIOC_G_EXT_CTRLS:
 	case VIDIOC_S_EXT_CTRLS:
 	case VIDIOC_TRY_EXT_CTRLS:
-		if (put_v4l2_ext_controls32(file, up_native, up))
+		if (put_v4l2_ext_controls32(file, new_p64, p32))
 			err = -EFAULT;
 		break;
 	case VIDIOC_S_EDID:
-		if (put_v4l2_edid32(up_native, up))
+		if (put_v4l2_edid32(new_p64, p32))
 			err = -EFAULT;
 		break;
 	}
@@ -1227,46 +1227,46 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	case VIDIOC_S_OUTPUT:
 	case VIDIOC_G_INPUT:
 	case VIDIOC_G_OUTPUT:
-		if (assign_in_user((compat_uint_t __user *)up,
-				   ((unsigned int __user *)up_native)))
+		if (assign_in_user((compat_uint_t __user *)p32,
+				   ((unsigned int __user *)new_p64)))
 			err = -EFAULT;
 		break;
 
 	case VIDIOC_G_FBUF:
-		err = put_v4l2_framebuffer32(up_native, up);
+		err = put_v4l2_framebuffer32(new_p64, p32);
 		break;
 
 	case VIDIOC_DQEVENT:
-		err = put_v4l2_event32(up_native, up);
+		err = put_v4l2_event32(new_p64, p32);
 		break;
 
 	case VIDIOC_G_EDID:
-		err = put_v4l2_edid32(up_native, up);
+		err = put_v4l2_edid32(new_p64, p32);
 		break;
 
 	case VIDIOC_G_FMT:
 	case VIDIOC_S_FMT:
 	case VIDIOC_TRY_FMT:
-		err = put_v4l2_format32(up_native, up);
+		err = put_v4l2_format32(new_p64, p32);
 		break;
 
 	case VIDIOC_CREATE_BUFS:
-		err = put_v4l2_create32(up_native, up);
+		err = put_v4l2_create32(new_p64, p32);
 		break;
 
 	case VIDIOC_PREPARE_BUF:
 	case VIDIOC_QUERYBUF:
 	case VIDIOC_QBUF:
 	case VIDIOC_DQBUF:
-		err = put_v4l2_buffer32(up_native, up);
+		err = put_v4l2_buffer32(new_p64, p32);
 		break;
 
 	case VIDIOC_ENUMSTD:
-		err = put_v4l2_standard32(up_native, up);
+		err = put_v4l2_standard32(new_p64, p32);
 		break;
 
 	case VIDIOC_ENUMINPUT:
-		err = put_v4l2_input32(up_native, up);
+		err = put_v4l2_input32(new_p64, p32);
 		break;
 	}
 	return err;
-- 
2.14.3

[PATCH v2 3/4] media: v4l2-compat-ioctl32: simplify casts

[Mauro Carvalho Chehab]

Making the cast right for get_user/put_user is not trivial, as
it needs to ensure that the types are the correct ones.

Improve it by using macros.

Tested with vivid with:
	$ sudo modprobe vivid no_error_inj=1
	$ v4l2-compliance-32bits -a -s10 >32bits && v4l2-compliance-64bits -a -s10 > 64bits && diff -U0 32bits 64bits
	--- 32bits	2018-04-17 11:18:29.141240772 -0300
	+++ 64bits	2018-04-17 11:18:40.635282341 -0300
	@@ -1 +1 @@
	-v4l2-compliance SHA   : bc71e4a67c6fbc5940062843bc41e7c8679634ce, 32 bits
	+v4l2-compliance SHA   : bc71e4a67c6fbc5940062843bc41e7c8679634ce, 64 bits

Using the latest version of v4l-utils with this patch applied:
	https://patchwork.linuxtv.org/patch/48746/

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 36 +++++++++++++++++++--------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 680b64c1d69a..d2f0268427c2 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -30,6 +30,24 @@
 	get_user(__assign_tmp, from) || put_user(__assign_tmp, to);	\
 })
 
+#define get_user_cast(__x, __ptr)					\
+({									\
+	get_user(__x, (typeof(*__ptr) __user *)(__ptr));		\
+})
+
+#define put_user_force(__x, __ptr)					\
+({									\
+	put_user((typeof(*__x) __force *)(__x), __ptr);			\
+})
+
+#define assign_in_user_cast(to, from)					\
+({									\
+	typeof(*from) __assign_tmp;					\
+									\
+	get_user_cast(__assign_tmp, from) || put_user(__assign_tmp, to);\
+})
+
+
 static long native_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	long ret = -ENOIOCTLCMD;
@@ -543,8 +561,7 @@ static int get_v4l2_buffer32(struct v4l2_buffer __user *p64,
 			return -EFAULT;
 
 		uplane = aux_buf;
-		if (put_user((__force struct v4l2_plane *)uplane,
-			     &p64->m.planes))
+		if (put_user_force(uplane, &p64->m.planes))
 			return -EFAULT;
 
 		while (num_planes--) {
@@ -682,7 +699,7 @@ static int get_v4l2_framebuffer32(struct v4l2_framebuffer __user *p64,
 
 	if (!access_ok(VERIFY_READ, p32, sizeof(*p32)) ||
 	    get_user(tmp, &p32->base) ||
-	    put_user((void __force *)compat_ptr(tmp), &p64->base) ||
+	    put_user_force(compat_ptr(tmp), &p64->base) ||
 	    assign_in_user(&p64->capability, &p32->capability) ||
 	    assign_in_user(&p64->flags, &p32->flags) ||
 	    copy_in_user(&p64->fmt, &p32->fmt, sizeof(p64->fmt)))
@@ -831,8 +848,7 @@ static int get_v4l2_ext_controls32(struct file *file,
 	if (aux_space < count * sizeof(*kcontrols))
 		return -EFAULT;
 	kcontrols = aux_buf;
-	if (put_user((__force struct v4l2_ext_control *)kcontrols,
-		     &p64->controls))
+	if (put_user_force(kcontrols, &p64->controls))
 		return -EFAULT;
 
 	for (n = 0; n < count; n++) {
@@ -898,10 +914,9 @@ static int put_v4l2_ext_controls32(struct file *file,
 		unsigned int size = sizeof(*ucontrols);
 		u32 id;
 
-		if (get_user(id, (unsigned int __user *)&kcontrols->id) ||
+		if (get_user_cast(id, &kcontrols->id) ||
 		    put_user(id, &ucontrols->id) ||
-		    assign_in_user(&ucontrols->size,
-				   (unsigned int __user *)&kcontrols->size) ||
+		    assign_in_user_cast(&ucontrols->size, &kcontrols->size) ||
 		    copy_in_user(&ucontrols->reserved2,
 				 (void __user *)&kcontrols->reserved2,
 				 sizeof(ucontrols->reserved2)))
@@ -970,10 +985,9 @@ static int get_v4l2_edid32(struct v4l2_edid __user *p64,
 	if (!access_ok(VERIFY_READ, p32, sizeof(*p32)) ||
 	    assign_in_user(&p64->pad, &p32->pad) ||
 	    assign_in_user(&p64->start_block, &p32->start_block) ||
-	    assign_in_user(&p64->blocks,
-			   (u32 __user *)&p32->blocks) ||
+	    assign_in_user_cast(&p64->blocks, &p32->blocks) ||
 	    get_user(tmp, &p32->edid) ||
-	    put_user((void __force *)compat_ptr(tmp), &p64->edid) ||
+	    put_user_force(compat_ptr(tmp), &p64->edid) ||
 	    copy_in_user(p64->reserved, p32->reserved, sizeof(p64->reserved)))
 		return -EFAULT;
 	return 0;
-- 
2.14.3

Re: [PATCH v2 3/4] media: v4l2-compat-ioctl32: simplify casts

[Hans Verkuil]

On 04/19/18 18:33, Mauro Carvalho Chehab wrote:
> Making the cast right for get_user/put_user is not trivial, as
> it needs to ensure that the types are the correct ones.
> 
> Improve it by using macros.
> 
> Tested with vivid with:
> 	$ sudo modprobe vivid no_error_inj=1
> 	$ v4l2-compliance-32bits -a -s10 >32bits && v4l2-compliance-64bits -a -s10 > 64bits && diff -U0 32bits 64bits
> 	--- 32bits	2018-04-17 11:18:29.141240772 -0300
> 	+++ 64bits	2018-04-17 11:18:40.635282341 -0300
> 	@@ -1 +1 @@
> 	-v4l2-compliance SHA   : bc71e4a67c6fbc5940062843bc41e7c8679634ce, 32 bits
> 	+v4l2-compliance SHA   : bc71e4a67c6fbc5940062843bc41e7c8679634ce, 64 bits
> 
> Using the latest version of v4l-utils with this patch applied:
> 	https://patchwork.linuxtv.org/patch/48746/
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

> ---
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 36 +++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index 680b64c1d69a..d2f0268427c2 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -30,6 +30,24 @@
>  	get_user(__assign_tmp, from) || put_user(__assign_tmp, to);	\
>  })
>  
> +#define get_user_cast(__x, __ptr)					\
> +({									\
> +	get_user(__x, (typeof(*__ptr) __user *)(__ptr));		\
> +})
> +
> +#define put_user_force(__x, __ptr)					\
> +({									\
> +	put_user((typeof(*__x) __force *)(__x), __ptr);			\
> +})
> +
> +#define assign_in_user_cast(to, from)					\
> +({									\
> +	typeof(*from) __assign_tmp;					\
> +									\
> +	get_user_cast(__assign_tmp, from) || put_user(__assign_tmp, to);\
> +})
> +
> +
>  static long native_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  {
>  	long ret = -ENOIOCTLCMD;
> @@ -543,8 +561,7 @@ static int get_v4l2_buffer32(struct v4l2_buffer __user *p64,
>  			return -EFAULT;
>  
>  		uplane = aux_buf;
> -		if (put_user((__force struct v4l2_plane *)uplane,
> -			     &p64->m.planes))
> +		if (put_user_force(uplane, &p64->m.planes))
>  			return -EFAULT;
>  
>  		while (num_planes--) {
> @@ -682,7 +699,7 @@ static int get_v4l2_framebuffer32(struct v4l2_framebuffer __user *p64,
>  
>  	if (!access_ok(VERIFY_READ, p32, sizeof(*p32)) ||
>  	    get_user(tmp, &p32->base) ||
> -	    put_user((void __force *)compat_ptr(tmp), &p64->base) ||
> +	    put_user_force(compat_ptr(tmp), &p64->base) ||
>  	    assign_in_user(&p64->capability, &p32->capability) ||
>  	    assign_in_user(&p64->flags, &p32->flags) ||
>  	    copy_in_user(&p64->fmt, &p32->fmt, sizeof(p64->fmt)))
> @@ -831,8 +848,7 @@ static int get_v4l2_ext_controls32(struct file *file,
>  	if (aux_space < count * sizeof(*kcontrols))
>  		return -EFAULT;
>  	kcontrols = aux_buf;
> -	if (put_user((__force struct v4l2_ext_control *)kcontrols,
> -		     &p64->controls))
> +	if (put_user_force(kcontrols, &p64->controls))
>  		return -EFAULT;
>  
>  	for (n = 0; n < count; n++) {
> @@ -898,10 +914,9 @@ static int put_v4l2_ext_controls32(struct file *file,
>  		unsigned int size = sizeof(*ucontrols);
>  		u32 id;
>  
> -		if (get_user(id, (unsigned int __user *)&kcontrols->id) ||
> +		if (get_user_cast(id, &kcontrols->id) ||
>  		    put_user(id, &ucontrols->id) ||
> -		    assign_in_user(&ucontrols->size,
> -				   (unsigned int __user *)&kcontrols->size) ||
> +		    assign_in_user_cast(&ucontrols->size, &kcontrols->size) ||
>  		    copy_in_user(&ucontrols->reserved2,
>  				 (void __user *)&kcontrols->reserved2,
>  				 sizeof(ucontrols->reserved2)))
> @@ -970,10 +985,9 @@ static int get_v4l2_edid32(struct v4l2_edid __user *p64,
>  	if (!access_ok(VERIFY_READ, p32, sizeof(*p32)) ||
>  	    assign_in_user(&p64->pad, &p32->pad) ||
>  	    assign_in_user(&p64->start_block, &p32->start_block) ||
> -	    assign_in_user(&p64->blocks,
> -			   (u32 __user *)&p32->blocks) ||
> +	    assign_in_user_cast(&p64->blocks, &p32->blocks) ||
>  	    get_user(tmp, &p32->edid) ||
> -	    put_user((void __force *)compat_ptr(tmp), &p64->edid) ||
> +	    put_user_force(compat_ptr(tmp), &p64->edid) ||
>  	    copy_in_user(p64->reserved, p32->reserved, sizeof(p64->reserved)))
>  		return -EFAULT;
>  	return 0;
> 

[PATCH v2 4/4] media: v4l2-compat-ioctl32: better document the code

[Mauro Carvalho Chehab]

This file does a lot of non-trivial struff. Document it using
kernel-doc markups where needed and improve the comments inside
do_video_ioctl().

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 166 +++++++++++++++++++++++++-
 1 file changed, 160 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index d2f0268427c2..777ed179af5f 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -22,7 +22,18 @@
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-ioctl.h>
 
-/* Use the same argument order as copy_in_user */
+/**
+ * assign_in_user() - Copy from one __user var to another one
+ *
+ * @to: __user var where data will be stored
+ * @from: __user var were data will be retrieved.
+ *
+ * As this code very often needs to allocate userspace memory, it is easier
+ * to have a macro that will do both get_user() and put_user() at once.
+ *
+ * This function complements the macros defined at asm-generic/uaccess.h.
+ * It uses the same argument order as copy_in_user()
+ */
 #define assign_in_user(to, from)					\
 ({									\
 	typeof(*from) __assign_tmp;					\
@@ -30,16 +41,57 @@
 	get_user(__assign_tmp, from) || put_user(__assign_tmp, to);	\
 })
 
+/**
+ * get_user_cast() - Stores at a kernelspace local var the contents from a
+ *		pointer with userspace data that is not tagged with __user.
+ *
+ * @__x: var where data will be stored
+ * @ptr: var were data will be retrieved.
+ *
+ * Sometimes, we need to declare a pointer without __user, because it
+ * comes from a pointer struct field that will be retrieved from userspace
+ * by the 64-bit native ioctl handler. This function ensures that the
+ * @ptr will be casted to __user before calling get_user(), in order to
+ * avoid warnings with static code analyzers like smatch.
+ */
 #define get_user_cast(__x, __ptr)					\
 ({									\
 	get_user(__x, (typeof(*__ptr) __user *)(__ptr));		\
 })
 
+/**
+ * put_user_force() - Stores at the contents of a kernelspace local var
+ *		      into an userspace pointer, removing any __user cast.
+ *
+ * @__x: var where data will be stored
+ * @ptr: var were data will be retrieved.
+ *
+ * As the compat32 code now handles with 32-bits and 64-bits __user
+ * structs, sometimes we need to remove the __user atributes from some data,
+ * by passing __force macro. This function ensures that the
+ * @ptr will be casted with __force before calling put_user(), in order to
+ * avoid warnings with static code analyzers like smatch.
+ */
 #define put_user_force(__x, __ptr)					\
 ({									\
 	put_user((typeof(*__x) __force *)(__x), __ptr);			\
 })
 
+/**
+ * assign_in_user_cast() - Copy from one __user var to another one
+ *
+ * @to: __user var where data will be stored
+ * @from: var were data will be retrieved that needs to be cast to __user.
+ *
+ * As this code very often needs to allocate userspace memory, it is easier
+ * to have a macro that will do both get_user_cast() and put_user() at once.
+ *
+ * This function should be used instead of assign_in_user() when the @from
+ * variable was not declared as __user. See get_user_cast() for more details.
+ *
+ * This function complements the macros defined at asm-generic/uaccess.h.
+ * It uses the same argument order as copy_in_user()
+ */
 #define assign_in_user_cast(to, from)					\
 ({									\
 	typeof(*from) __assign_tmp;					\
@@ -47,7 +99,16 @@
 	get_user_cast(__assign_tmp, from) || put_user(__assign_tmp, to);\
 })
 
-
+/**
+ * native_ioctl - Ancillary function that calls the native 64 bits ioctl
+ * handler.
+ *
+ * @file: pointer to &struct file with the file handler
+ * @cmd: ioctl to be called
+ * @arg: arguments passed from/to the ioctl handler
+ *
+ * This function calls the native ioctl handler at v4l2-dev, e. g. v4l2_ioctl()
+ */
 static long native_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	long ret = -ENOIOCTLCMD;
@@ -59,6 +120,21 @@ static long native_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 }
 
 
+/*
+ * Per-ioctl data copy handlers.
+ *
+ * Those come in pairs, with a get_v4l2_foo() and a put_v4l2_foo() routine,
+ * where "v4l2_foo" is the name of the V4L2 struct.
+ *
+ * They basically get two __user pointers, one with a 32-bits struct that
+ * came from the userspace call and a 64-bits struct, also allocated as
+ * userspace, but filled internally by do_video_ioctl().
+ *
+ * For ioctls that have pointers inside it, the functions will also
+ * receive an ancillary buffer with extra space, used to pass extra
+ * data to the routine.
+ */
+
 struct v4l2_clip32 {
 	struct v4l2_rect        c;
 	compat_caddr_t		next;
@@ -1009,6 +1085,13 @@ static int put_v4l2_edid32(struct v4l2_edid __user *p64,
 	return 0;
 }
 
+/*
+ * List of ioctl's that require 32-bits/64-bits conversion
+ *
+ * The V4L2 ioctls that aren't listed there don't have pointer arguments
+ * and the struct size is identical for both 32 and 64 bits versions, so
+ * don't need translations.
+ */
 
 #define VIDIOC_G_FMT32		_IOWR('V',  4, struct v4l2_format32)
 #define VIDIOC_S_FMT32		_IOWR('V',  5, struct v4l2_format32)
@@ -1037,6 +1120,21 @@ static int put_v4l2_edid32(struct v4l2_edid __user *p64,
 #define VIDIOC_G_OUTPUT32	_IOR ('V', 46, s32)
 #define VIDIOC_S_OUTPUT32	_IOWR('V', 47, s32)
 
+/**
+ * alloc_userspace() - Allocates a 64-bits userspace pointer compatible
+ *	for calling the native 64-bits version of an ioctl.
+ *
+ * @size:	size of the structure itself to be allocated.
+ * @aux_space:	extra size needed to store "extra" data, e. g. space for
+ *		other __user data that comes pointed by fields inside the
+ *		structure.
+ * @new_p64:	pointer to a pointer to be filled with the allocated struct.
+ *
+ * Return:
+ *
+ * if it can't allocate memory, either -ENOMEM or -EFAULT will be returned.
+ * Zero otherwise.
+ */
 static int alloc_userspace(unsigned int size, u32 aux_space,
 			   void __user **new_p64)
 {
@@ -1048,6 +1146,23 @@ static int alloc_userspace(unsigned int size, u32 aux_space,
 	return 0;
 }
 
+/**
+ * do_video_ioctl() - Ancillary function with handles a compat32 ioctl call
+ *
+ * @file: pointer to &struct file with the file handler
+ * @cmd: ioctl to be called
+ * @arg: arguments passed from/to the ioctl handler
+ *
+ * This function is called when a 32 bits application calls a V4L2 ioctl
+ * and the Kernel is compiled with 64 bits.
+ *
+ * This function is called by v4l2_compat_ioctl32() when the function is
+ * not private to some specific driver.
+ *
+ * It converts a 32-bits struct into a 64 bits one, calls the native 64-bits
+ * ioctl handles and fills back the 32-bits struct with the results of the
+ * native call.
+ */
 static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	void __user *p32 = compat_ptr(arg);
@@ -1057,7 +1172,9 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	int compatible_arg = 1;
 	long err = 0;
 
-	/* First, convert the command. */
+	/*
+	 * 1. When struct size is different, converts the command.
+	 */
 	switch (cmd) {
 	case VIDIOC_G_FMT32: cmd = VIDIOC_G_FMT; break;
 	case VIDIOC_S_FMT32: cmd = VIDIOC_S_FMT; break;
@@ -1086,6 +1203,11 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	case VIDIOC_S_EDID32: cmd = VIDIOC_S_EDID; break;
 	}
 
+	/*
+	 * 2. Allocates a 64-bits userspace pointer to store the
+	 * values of the ioctl and copy data from the 32-bits __user
+	 * argument into it.
+	 */
 	switch (cmd) {
 	case VIDIOC_OVERLAY:
 	case VIDIOC_STREAMON:
@@ -1208,6 +1330,15 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	if (err)
 		return err;
 
+	/*
+	 * 3. Calls the native 64-bits ioctl handler.
+	 *
+	 * For the functions where a conversion was not needed,
+	 * compatible_arg is true, and it will call it with the arguments
+	 * provided by userspace and stored at @p32 var.
+	 *
+	 * Otherwise, it will pass the newly allocated @new_p64 argument.
+	 */
 	if (compatible_arg)
 		err = native_ioctl(file, cmd, (unsigned long)p32);
 	else
@@ -1217,9 +1348,14 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 		return err;
 
 	/*
-	 * Special case: even after an error we need to put the
-	 * results back for these ioctls since the error_idx will
-	 * contain information on which control failed.
+	 * 4. Special case: even after an error we need to put the
+	 * results back for some ioctls.
+	 *
+	 * In the case of EXT_CTRLS, the error_idx will contain information
+	 * on which control failed.
+	 *
+	 * In the case of S_EDID, the driver can return E2BIG and set
+	 * the blocks to maximum allowed value.
 	 */
 	switch (cmd) {
 	case VIDIOC_G_EXT_CTRLS:
@@ -1236,6 +1372,10 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	if (err)
 		return err;
 
+	/*
+	 * 5. Copy the data returned at the 64 bits userspace pointer to
+	 * the original 32 bits structure.
+	 */
 	switch (cmd) {
 	case VIDIOC_S_INPUT:
 	case VIDIOC_S_OUTPUT:
@@ -1286,6 +1426,20 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	return err;
 }
 
+/**
+ * v4l2_compat_ioctl32() - Handles a compat32 ioctl call
+ *
+ * @file: pointer to &struct file with the file handler
+ * @cmd: ioctl to be called
+ * @arg: arguments passed from/to the ioctl handler
+ *
+ * This function is meant to be used as .compat_ioctl fops at v4l2-dev.c
+ * in order to deal with 32-bit calls on a 64-bits Kernel.
+ *
+ * This function calls do_video_ioctl() for non-private V4L2 ioctls.
+ * If the function is a private one, it calls, instead,
+ * vdev->fops->compat_ioctl32.
+ */
 long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct video_device *vdev = video_devdata(file);
-- 
2.14.3

Re: [PATCH v2 4/4] media: v4l2-compat-ioctl32: better document the code

[Mauro Carvalho Chehab]

Em Thu, 19 Apr 2018 12:33:32 -0400
Mauro Carvalho Chehab <mchehab@s-opensource.com> escreveu:

> This file does a lot of non-trivial struff. Document it using
> kernel-doc markups where needed and improve the comments inside
> do_video_ioctl().

Sent it too fast. It should be merged with this diff:

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 777ed179af5f..c460fbcbc035 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -46,12 +46,12 @@
  *		pointer with userspace data that is not tagged with __user.
  *
  * @__x: var where data will be stored
- * @ptr: var were data will be retrieved.
+ * @__ptr: var were data will be retrieved.
  *
  * Sometimes, we need to declare a pointer without __user, because it
  * comes from a pointer struct field that will be retrieved from userspace
  * by the 64-bit native ioctl handler. This function ensures that the
- * @ptr will be casted to __user before calling get_user(), in order to
+ * @__ptr will be casted to __user before calling get_user(), in order to
  * avoid warnings with static code analyzers like smatch.
  */
 #define get_user_cast(__x, __ptr)					\
@@ -64,12 +64,12 @@
  *		      into an userspace pointer, removing any __user cast.
  *
  * @__x: var where data will be stored
- * @ptr: var were data will be retrieved.
+ * @__ptr: var were data will be retrieved.
  *
  * As the compat32 code now handles with 32-bits and 64-bits __user
  * structs, sometimes we need to remove the __user atributes from some data,
  * by passing __force macro. This function ensures that the
- * @ptr will be casted with __force before calling put_user(), in order to
+ * @__ptr will be casted with __force before calling put_user(), in order to
  * avoid warnings with static code analyzers like smatch.
  */
 #define put_user_force(__x, __ptr)					\

Full patch enclosed.

Thanks,
Mauro

[PATCHv3 4/4] media: v4l2-compat-ioctl32: better document the code

This file does a lot of non-trivial struff. Document it using
kernel-doc markups where needed and improve the comments inside
do_video_ioctl().

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index d2f0268427c2..c460fbcbc035 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -22,7 +22,18 @@
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-ioctl.h>
 
-/* Use the same argument order as copy_in_user */
+/**
+ * assign_in_user() - Copy from one __user var to another one
+ *
+ * @to: __user var where data will be stored
+ * @from: __user var were data will be retrieved.
+ *
+ * As this code very often needs to allocate userspace memory, it is easier
+ * to have a macro that will do both get_user() and put_user() at once.
+ *
+ * This function complements the macros defined at asm-generic/uaccess.h.
+ * It uses the same argument order as copy_in_user()
+ */
 #define assign_in_user(to, from)					\
 ({									\
 	typeof(*from) __assign_tmp;					\
@@ -30,16 +41,57 @@
 	get_user(__assign_tmp, from) || put_user(__assign_tmp, to);	\
 })
 
+/**
+ * get_user_cast() - Stores at a kernelspace local var the contents from a
+ *		pointer with userspace data that is not tagged with __user.
+ *
+ * @__x: var where data will be stored
+ * @__ptr: var were data will be retrieved.
+ *
+ * Sometimes, we need to declare a pointer without __user, because it
+ * comes from a pointer struct field that will be retrieved from userspace
+ * by the 64-bit native ioctl handler. This function ensures that the
+ * @__ptr will be casted to __user before calling get_user(), in order to
+ * avoid warnings with static code analyzers like smatch.
+ */
 #define get_user_cast(__x, __ptr)					\
 ({									\
 	get_user(__x, (typeof(*__ptr) __user *)(__ptr));		\
 })
 
+/**
+ * put_user_force() - Stores at the contents of a kernelspace local var
+ *		      into an userspace pointer, removing any __user cast.
+ *
+ * @__x: var where data will be stored
+ * @__ptr: var were data will be retrieved.
+ *
+ * As the compat32 code now handles with 32-bits and 64-bits __user
+ * structs, sometimes we need to remove the __user atributes from some data,
+ * by passing __force macro. This function ensures that the
+ * @__ptr will be casted with __force before calling put_user(), in order to
+ * avoid warnings with static code analyzers like smatch.
+ */
 #define put_user_force(__x, __ptr)					\
 ({									\
 	put_user((typeof(*__x) __force *)(__x), __ptr);			\
 })
 
+/**
+ * assign_in_user_cast() - Copy from one __user var to another one
+ *
+ * @to: __user var where data will be stored
+ * @from: var were data will be retrieved that needs to be cast to __user.
+ *
+ * As this code very often needs to allocate userspace memory, it is easier
+ * to have a macro that will do both get_user_cast() and put_user() at once.
+ *
+ * This function should be used instead of assign_in_user() when the @from
+ * variable was not declared as __user. See get_user_cast() for more details.
+ *
+ * This function complements the macros defined at asm-generic/uaccess.h.
+ * It uses the same argument order as copy_in_user()
+ */
 #define assign_in_user_cast(to, from)					\
 ({									\
 	typeof(*from) __assign_tmp;					\
@@ -47,7 +99,16 @@
 	get_user_cast(__assign_tmp, from) || put_user(__assign_tmp, to);\
 })
 
-
+/**
+ * native_ioctl - Ancillary function that calls the native 64 bits ioctl
+ * handler.
+ *
+ * @file: pointer to &struct file with the file handler
+ * @cmd: ioctl to be called
+ * @arg: arguments passed from/to the ioctl handler
+ *
+ * This function calls the native ioctl handler at v4l2-dev, e. g. v4l2_ioctl()
+ */
 static long native_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	long ret = -ENOIOCTLCMD;
@@ -59,6 +120,21 @@ static long native_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 }
 
 
+/*
+ * Per-ioctl data copy handlers.
+ *
+ * Those come in pairs, with a get_v4l2_foo() and a put_v4l2_foo() routine,
+ * where "v4l2_foo" is the name of the V4L2 struct.
+ *
+ * They basically get two __user pointers, one with a 32-bits struct that
+ * came from the userspace call and a 64-bits struct, also allocated as
+ * userspace, but filled internally by do_video_ioctl().
+ *
+ * For ioctls that have pointers inside it, the functions will also
+ * receive an ancillary buffer with extra space, used to pass extra
+ * data to the routine.
+ */
+
 struct v4l2_clip32 {
 	struct v4l2_rect        c;
 	compat_caddr_t		next;
@@ -1009,6 +1085,13 @@ static int put_v4l2_edid32(struct v4l2_edid __user *p64,
 	return 0;
 }
 
+/*
+ * List of ioctl's that require 32-bits/64-bits conversion
+ *
+ * The V4L2 ioctls that aren't listed there don't have pointer arguments
+ * and the struct size is identical for both 32 and 64 bits versions, so
+ * don't need translations.
+ */
 
 #define VIDIOC_G_FMT32		_IOWR('V',  4, struct v4l2_format32)
 #define VIDIOC_S_FMT32		_IOWR('V',  5, struct v4l2_format32)
@@ -1037,6 +1120,21 @@ static int put_v4l2_edid32(struct v4l2_edid __user *p64,
 #define VIDIOC_G_OUTPUT32	_IOR ('V', 46, s32)
 #define VIDIOC_S_OUTPUT32	_IOWR('V', 47, s32)
 
+/**
+ * alloc_userspace() - Allocates a 64-bits userspace pointer compatible
+ *	for calling the native 64-bits version of an ioctl.
+ *
+ * @size:	size of the structure itself to be allocated.
+ * @aux_space:	extra size needed to store "extra" data, e. g. space for
+ *		other __user data that comes pointed by fields inside the
+ *		structure.
+ * @new_p64:	pointer to a pointer to be filled with the allocated struct.
+ *
+ * Return:
+ *
+ * if it can't allocate memory, either -ENOMEM or -EFAULT will be returned.
+ * Zero otherwise.
+ */
 static int alloc_userspace(unsigned int size, u32 aux_space,
 			   void __user **new_p64)
 {
@@ -1048,6 +1146,23 @@ static int alloc_userspace(unsigned int size, u32 aux_space,
 	return 0;
 }
 
+/**
+ * do_video_ioctl() - Ancillary function with handles a compat32 ioctl call
+ *
+ * @file: pointer to &struct file with the file handler
+ * @cmd: ioctl to be called
+ * @arg: arguments passed from/to the ioctl handler
+ *
+ * This function is called when a 32 bits application calls a V4L2 ioctl
+ * and the Kernel is compiled with 64 bits.
+ *
+ * This function is called by v4l2_compat_ioctl32() when the function is
+ * not private to some specific driver.
+ *
+ * It converts a 32-bits struct into a 64 bits one, calls the native 64-bits
+ * ioctl handles and fills back the 32-bits struct with the results of the
+ * native call.
+ */
 static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	void __user *p32 = compat_ptr(arg);
@@ -1057,7 +1172,9 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	int compatible_arg = 1;
 	long err = 0;
 
-	/* First, convert the command. */
+	/*
+	 * 1. When struct size is different, converts the command.
+	 */
 	switch (cmd) {
 	case VIDIOC_G_FMT32: cmd = VIDIOC_G_FMT; break;
 	case VIDIOC_S_FMT32: cmd = VIDIOC_S_FMT; break;
@@ -1086,6 +1203,11 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	case VIDIOC_S_EDID32: cmd = VIDIOC_S_EDID; break;
 	}
 
+	/*
+	 * 2. Allocates a 64-bits userspace pointer to store the
+	 * values of the ioctl and copy data from the 32-bits __user
+	 * argument into it.
+	 */
 	switch (cmd) {
 	case VIDIOC_OVERLAY:
 	case VIDIOC_STREAMON:
@@ -1208,6 +1330,15 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	if (err)
 		return err;
 
+	/*
+	 * 3. Calls the native 64-bits ioctl handler.
+	 *
+	 * For the functions where a conversion was not needed,
+	 * compatible_arg is true, and it will call it with the arguments
+	 * provided by userspace and stored at @p32 var.
+	 *
+	 * Otherwise, it will pass the newly allocated @new_p64 argument.
+	 */
 	if (compatible_arg)
 		err = native_ioctl(file, cmd, (unsigned long)p32);
 	else
@@ -1217,9 +1348,14 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 		return err;
 
 	/*
-	 * Special case: even after an error we need to put the
-	 * results back for these ioctls since the error_idx will
-	 * contain information on which control failed.
+	 * 4. Special case: even after an error we need to put the
+	 * results back for some ioctls.
+	 *
+	 * In the case of EXT_CTRLS, the error_idx will contain information
+	 * on which control failed.
+	 *
+	 * In the case of S_EDID, the driver can return E2BIG and set
+	 * the blocks to maximum allowed value.
 	 */
 	switch (cmd) {
 	case VIDIOC_G_EXT_CTRLS:
@@ -1236,6 +1372,10 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	if (err)
 		return err;
 
+	/*
+	 * 5. Copy the data returned at the 64 bits userspace pointer to
+	 * the original 32 bits structure.
+	 */
 	switch (cmd) {
 	case VIDIOC_S_INPUT:
 	case VIDIOC_S_OUTPUT:
@@ -1286,6 +1426,20 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	return err;
 }
 
+/**
+ * v4l2_compat_ioctl32() - Handles a compat32 ioctl call
+ *
+ * @file: pointer to &struct file with the file handler
+ * @cmd: ioctl to be called
+ * @arg: arguments passed from/to the ioctl handler
+ *
+ * This function is meant to be used as .compat_ioctl fops at v4l2-dev.c
+ * in order to deal with 32-bit calls on a 64-bits Kernel.
+ *
+ * This function calls do_video_ioctl() for non-private V4L2 ioctls.
+ * If the function is a private one, it calls, instead,
+ * vdev->fops->compat_ioctl32.
+ */
 long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct video_device *vdev = video_devdata(file);

Re: [PATCH v2 4/4] media: v4l2-compat-ioctl32: better document the code

[Hans Verkuil]

Hi Mauro,

A bunch of typo, grammar and style fixes below. Looks good otherwise.

On 04/19/18 18:33, Mauro Carvalho Chehab wrote:
> This file does a lot of non-trivial struff. Document it using
> kernel-doc markups where needed and improve the comments inside
> do_video_ioctl().
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> ---
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 166 +++++++++++++++++++++++++-
>  1 file changed, 160 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index d2f0268427c2..777ed179af5f 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -22,7 +22,18 @@
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-ioctl.h>
>  
> -/* Use the same argument order as copy_in_user */
> +/**
> + * assign_in_user() - Copy from one __user var to another one
> + *
> + * @to: __user var where data will be stored
> + * @from: __user var were data will be retrieved.

were -> where

> + *
> + * As this code very often needs to allocate userspace memory, it is easier
> + * to have a macro that will do both get_user() and put_user() at once.
> + *
> + * This function complements the macros defined at asm-generic/uaccess.h.
> + * It uses the same argument order as copy_in_user()
> + */
>  #define assign_in_user(to, from)					\
>  ({									\
>  	typeof(*from) __assign_tmp;					\
> @@ -30,16 +41,57 @@
>  	get_user(__assign_tmp, from) || put_user(__assign_tmp, to);	\
>  })
>  
> +/**
> + * get_user_cast() - Stores at a kernelspace local var the contents from a
> + *		pointer with userspace data that is not tagged with __user.
> + *
> + * @__x: var where data will be stored
> + * @ptr: var were data will be retrieved.

were -> where

> + *
> + * Sometimes, we need to declare a pointer without __user, because it

The two commas in this sentence can be dropped.

> + * comes from a pointer struct field that will be retrieved from userspace
> + * by the 64-bit native ioctl handler. This function ensures that the
> + * @ptr will be casted to __user before calling get_user(), in order to

casted -> cast

The comma can be dropped.

> + * avoid warnings with static code analyzers like smatch.
> + */
>  #define get_user_cast(__x, __ptr)					\
>  ({									\
>  	get_user(__x, (typeof(*__ptr) __user *)(__ptr));		\
>  })
>  
> +/**
> + * put_user_force() - Stores at the contents of a kernelspace local var

s/at//

> + *		      into an userspace pointer, removing any __user cast.
> + *
> + * @__x: var where data will be stored
> + * @ptr: var were data will be retrieved.

were -> where

> + *
> + * As the compat32 code now handles with 32-bits and 64-bits __user

I don't understand this first line. Perhaps this line up to the comma
below should just be dropped? And instead start with 'Sometimes we need to...'.

> + * structs, sometimes we need to remove the __user atributes from some data,

atributes -> attribute

> + * by passing __force macro. This function ensures that the

'by passing the __force macro'

> + * @ptr will be casted with __force before calling put_user(), in order to

casted -> cast

> + * avoid warnings with static code analyzers like smatch.
> + */
>  #define put_user_force(__x, __ptr)					\
>  ({									\
>  	put_user((typeof(*__x) __force *)(__x), __ptr);			\
>  })
>  
> +/**
> + * assign_in_user_cast() - Copy from one __user var to another one
> + *
> + * @to: __user var where data will be stored
> + * @from: var were data will be retrieved that needs to be cast to __user.

were -> where

> + *
> + * As this code very often needs to allocate userspace memory, it is easier
> + * to have a macro that will do both get_user_cast() and put_user() at once.
> + *
> + * This function should be used instead of assign_in_user() when the @from
> + * variable was not declared as __user. See get_user_cast() for more details.
> + *
> + * This function complements the macros defined at asm-generic/uaccess.h.
> + * It uses the same argument order as copy_in_user()
> + */
>  #define assign_in_user_cast(to, from)					\
>  ({									\
>  	typeof(*from) __assign_tmp;					\
> @@ -47,7 +99,16 @@
>  	get_user_cast(__assign_tmp, from) || put_user(__assign_tmp, to);\
>  })
>  
> -
> +/**
> + * native_ioctl - Ancillary function that calls the native 64 bits ioctl
> + * handler.
> + *
> + * @file: pointer to &struct file with the file handler
> + * @cmd: ioctl to be called
> + * @arg: arguments passed from/to the ioctl handler
> + *
> + * This function calls the native ioctl handler at v4l2-dev, e. g. v4l2_ioctl()
> + */
>  static long native_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  {
>  	long ret = -ENOIOCTLCMD;
> @@ -59,6 +120,21 @@ static long native_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  }
>  
>  
> +/*
> + * Per-ioctl data copy handlers.
> + *
> + * Those come in pairs, with a get_v4l2_foo() and a put_v4l2_foo() routine,
> + * where "v4l2_foo" is the name of the V4L2 struct.
> + *
> + * They basically get two __user pointers, one with a 32-bits struct that
> + * came from the userspace call and a 64-bits struct, also allocated as
> + * userspace, but filled internally by do_video_ioctl().
> + *
> + * For ioctls that have pointers inside it, the functions will also
> + * receive an ancillary buffer with extra space, used to pass extra
> + * data to the routine.
> + */
> +
>  struct v4l2_clip32 {
>  	struct v4l2_rect        c;
>  	compat_caddr_t		next;
> @@ -1009,6 +1085,13 @@ static int put_v4l2_edid32(struct v4l2_edid __user *p64,
>  	return 0;
>  }
>  
> +/*
> + * List of ioctl's that require 32-bits/64-bits conversion

ioctl's -> ioctls

> + *
> + * The V4L2 ioctls that aren't listed there don't have pointer arguments
> + * and the struct size is identical for both 32 and 64 bits versions, so
> + * don't need translations.

don't -> they don't

> + */
>  
>  #define VIDIOC_G_FMT32		_IOWR('V',  4, struct v4l2_format32)
>  #define VIDIOC_S_FMT32		_IOWR('V',  5, struct v4l2_format32)
> @@ -1037,6 +1120,21 @@ static int put_v4l2_edid32(struct v4l2_edid __user *p64,
>  #define VIDIOC_G_OUTPUT32	_IOR ('V', 46, s32)
>  #define VIDIOC_S_OUTPUT32	_IOWR('V', 47, s32)
>  
> +/**
> + * alloc_userspace() - Allocates a 64-bits userspace pointer compatible
> + *	for calling the native 64-bits version of an ioctl.
> + *
> + * @size:	size of the structure itself to be allocated.
> + * @aux_space:	extra size needed to store "extra" data, e. g. space for

e. g. -> e.g.

> + *		other __user data that comes pointed by fields inside the

comes pointer -> is pointed to

> + *		structure.
> + * @new_p64:	pointer to a pointer to be filled with the allocated struct.
> + *
> + * Return:
> + *
> + * if it can't allocate memory, either -ENOMEM or -EFAULT will be returned.
> + * Zero otherwise.
> + */
>  static int alloc_userspace(unsigned int size, u32 aux_space,
>  			   void __user **new_p64)
>  {
> @@ -1048,6 +1146,23 @@ static int alloc_userspace(unsigned int size, u32 aux_space,
>  	return 0;
>  }
>  
> +/**
> + * do_video_ioctl() - Ancillary function with handles a compat32 ioctl call
> + *
> + * @file: pointer to &struct file with the file handler
> + * @cmd: ioctl to be called
> + * @arg: arguments passed from/to the ioctl handler
> + *
> + * This function is called when a 32 bits application calls a V4L2 ioctl
> + * and the Kernel is compiled with 64 bits.

Kernel -> kernel

> + *
> + * This function is called by v4l2_compat_ioctl32() when the function is
> + * not private to some specific driver.
> + *
> + * It converts a 32-bits struct into a 64 bits one, calls the native 64-bits
> + * ioctl handles and fills back the 32-bits struct with the results of the

handles -> handler

> + * native call.
> + */
>  static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  {
>  	void __user *p32 = compat_ptr(arg);
> @@ -1057,7 +1172,9 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>  	int compatible_arg = 1;
>  	long err = 0;
>  
> -	/* First, convert the command. */
> +	/*
> +	 * 1. When struct size is different, converts the command.
> +	 */
>  	switch (cmd) {
>  	case VIDIOC_G_FMT32: cmd = VIDIOC_G_FMT; break;
>  	case VIDIOC_S_FMT32: cmd = VIDIOC_S_FMT; break;
> @@ -1086,6 +1203,11 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>  	case VIDIOC_S_EDID32: cmd = VIDIOC_S_EDID; break;
>  	}
>  
> +	/*
> +	 * 2. Allocates a 64-bits userspace pointer to store the
> +	 * values of the ioctl and copy data from the 32-bits __user
> +	 * argument into it.
> +	 */
>  	switch (cmd) {
>  	case VIDIOC_OVERLAY:
>  	case VIDIOC_STREAMON:
> @@ -1208,6 +1330,15 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>  	if (err)
>  		return err;
>  
> +	/*
> +	 * 3. Calls the native 64-bits ioctl handler.
> +	 *
> +	 * For the functions where a conversion was not needed,
> +	 * compatible_arg is true, and it will call it with the arguments
> +	 * provided by userspace and stored at @p32 var.
> +	 *
> +	 * Otherwise, it will pass the newly allocated @new_p64 argument.
> +	 */
>  	if (compatible_arg)
>  		err = native_ioctl(file, cmd, (unsigned long)p32);
>  	else
> @@ -1217,9 +1348,14 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>  		return err;
>  
>  	/*
> -	 * Special case: even after an error we need to put the
> -	 * results back for these ioctls since the error_idx will
> -	 * contain information on which control failed.
> +	 * 4. Special case: even after an error we need to put the
> +	 * results back for some ioctls.
> +	 *
> +	 * In the case of EXT_CTRLS, the error_idx will contain information
> +	 * on which control failed.
> +	 *
> +	 * In the case of S_EDID, the driver can return E2BIG and set
> +	 * the blocks to maximum allowed value.
>  	 */
>  	switch (cmd) {
>  	case VIDIOC_G_EXT_CTRLS:
> @@ -1236,6 +1372,10 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>  	if (err)
>  		return err;
>  
> +	/*
> +	 * 5. Copy the data returned at the 64 bits userspace pointer to
> +	 * the original 32 bits structure.
> +	 */
>  	switch (cmd) {
>  	case VIDIOC_S_INPUT:
>  	case VIDIOC_S_OUTPUT:
> @@ -1286,6 +1426,20 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>  	return err;
>  }
>  
> +/**
> + * v4l2_compat_ioctl32() - Handles a compat32 ioctl call
> + *
> + * @file: pointer to &struct file with the file handler
> + * @cmd: ioctl to be called
> + * @arg: arguments passed from/to the ioctl handler
> + *
> + * This function is meant to be used as .compat_ioctl fops at v4l2-dev.c
> + * in order to deal with 32-bit calls on a 64-bits Kernel.
> + *
> + * This function calls do_video_ioctl() for non-private V4L2 ioctls.
> + * If the function is a private one, it calls, instead,
> + * vdev->fops->compat_ioctl32.

If the function is a private one it calls vdev->fops->compat_ioctl32 instead.

> + */
>  long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
>  {
>  	struct video_device *vdev = video_devdata(file);
> 

Regards,

	Hans

Re: [PATCH v2 4/4] media: v4l2-compat-ioctl32: better document the code

[Mauro Carvalho Chehab]

Em Fri, 20 Apr 2018 13:16:00 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

Thanks for the review!

> > +/**
> > + * do_video_ioctl() - Ancillary function with handles a compat32 ioctl call
> > + *
> > + * @file: pointer to &struct file with the file handler
> > + * @cmd: ioctl to be called
> > + * @arg: arguments passed from/to the ioctl handler
> > + *
> > + * This function is called when a 32 bits application calls a V4L2 ioctl
> > + * and the Kernel is compiled with 64 bits.  
> 
> Kernel -> kernel

Actually, in this case, "the Kernel" is referring to the "Linux Kernel",
with is a particular, unique kernel. So, it should be on uppercase.

The remaining notes are OK. I'm enclosing the following diff and
resending this patch with it folded in a few.

Thanks,
Mauro

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index c460fbcbc035..9611c3aae8ca 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -26,7 +26,7 @@
  * assign_in_user() - Copy from one __user var to another one
  *
  * @to: __user var where data will be stored
- * @from: __user var were data will be retrieved.
+ * @from: __user var where data will be retrieved.
  *
  * As this code very often needs to allocate userspace memory, it is easier
  * to have a macro that will do both get_user() and put_user() at once.
@@ -46,12 +46,12 @@
  *		pointer with userspace data that is not tagged with __user.
  *
  * @__x: var where data will be stored
- * @__ptr: var were data will be retrieved.
+ * @__ptr: var where data will be retrieved.
  *
- * Sometimes, we need to declare a pointer without __user, because it
+ * Sometimes we need to declare a pointer without __user because it
  * comes from a pointer struct field that will be retrieved from userspace
  * by the 64-bit native ioctl handler. This function ensures that the
- * @__ptr will be casted to __user before calling get_user(), in order to
+ * @__ptr will be cast to __user before calling get_user() in order to
  * avoid warnings with static code analyzers like smatch.
  */
 #define get_user_cast(__x, __ptr)					\
@@ -60,16 +60,15 @@
 })
 
 /**
- * put_user_force() - Stores at the contents of a kernelspace local var
+ * put_user_force() - Stores the contents of a kernelspace local var
  *		      into an userspace pointer, removing any __user cast.
  *
  * @__x: var where data will be stored
- * @__ptr: var were data will be retrieved.
+ * @__ptr: var where data will be retrieved.
  *
- * As the compat32 code now handles with 32-bits and 64-bits __user
- * structs, sometimes we need to remove the __user atributes from some data,
- * by passing __force macro. This function ensures that the
- * @__ptr will be casted with __force before calling put_user(), in order to
+ * Sometimes we need to remove the __user attribute from some data,
+ * by passing the __force macro. This function ensures that the
+ * @__ptr will be cast with __force before calling put_user(), in order to
  * avoid warnings with static code analyzers like smatch.
  */
 #define put_user_force(__x, __ptr)					\
@@ -81,7 +80,7 @@
  * assign_in_user_cast() - Copy from one __user var to another one
  *
  * @to: __user var where data will be stored
- * @from: var were data will be retrieved that needs to be cast to __user.
+ * @from: var where data will be retrieved that needs to be cast to __user.
  *
  * As this code very often needs to allocate userspace memory, it is easier
  * to have a macro that will do both get_user_cast() and put_user() at once.
@@ -1086,11 +1085,11 @@ static int put_v4l2_edid32(struct v4l2_edid __user *p64,
 }
 
 /*
- * List of ioctl's that require 32-bits/64-bits conversion
+ * List of ioctls that require 32-bits/64-bits conversion
  *
  * The V4L2 ioctls that aren't listed there don't have pointer arguments
  * and the struct size is identical for both 32 and 64 bits versions, so
- * don't need translations.
+ * they don't need translations.
  */
 
 #define VIDIOC_G_FMT32		_IOWR('V',  4, struct v4l2_format32)
@@ -1125,8 +1124,8 @@ static int put_v4l2_edid32(struct v4l2_edid __user *p64,
  *	for calling the native 64-bits version of an ioctl.
  *
  * @size:	size of the structure itself to be allocated.
- * @aux_space:	extra size needed to store "extra" data, e. g. space for
- *		other __user data that comes pointed by fields inside the
+ * @aux_space:	extra size needed to store "extra" data, e.g. space for
+ *		other __user data that is pointed to fields inside the
  *		structure.
  * @new_p64:	pointer to a pointer to be filled with the allocated struct.
  *
@@ -1160,7 +1159,7 @@ static int alloc_userspace(unsigned int size, u32 aux_space,
  * not private to some specific driver.
  *
  * It converts a 32-bits struct into a 64 bits one, calls the native 64-bits
- * ioctl handles and fills back the 32-bits struct with the results of the
+ * ioctl handler and fills back the 32-bits struct with the results of the
  * native call.
  */
 static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
@@ -1437,8 +1436,8 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
  * in order to deal with 32-bit calls on a 64-bits Kernel.
  *
  * This function calls do_video_ioctl() for non-private V4L2 ioctls.
- * If the function is a private one, it calls, instead,
- * vdev->fops->compat_ioctl32.
+ * If the function is a private one it calls vdev->fops->compat_ioctl32
+ * instead.
  */
 long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
 {

Re: [PATCH v2 4/4] media: v4l2-compat-ioctl32: better document the code

[Hans Verkuil]

On 04/20/18 13:44, Mauro Carvalho Chehab wrote:
> Em Fri, 20 Apr 2018 13:16:00 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
> Thanks for the review!
> 
>>> +/**
>>> + * do_video_ioctl() - Ancillary function with handles a compat32 ioctl call
>>> + *
>>> + * @file: pointer to &struct file with the file handler
>>> + * @cmd: ioctl to be called
>>> + * @arg: arguments passed from/to the ioctl handler
>>> + *
>>> + * This function is called when a 32 bits application calls a V4L2 ioctl
>>> + * and the Kernel is compiled with 64 bits.  
>>
>> Kernel -> kernel
> 
> Actually, in this case, "the Kernel" is referring to the "Linux Kernel",
> with is a particular, unique kernel. So, it should be on uppercase.

I'm fairly certain that's not how it works, but a native speaker should
pitch in on this. Anyway, it's not important :-)

Regards,

	Hans

> 
> The remaining notes are OK. I'm enclosing the following diff and
> resending this patch with it folded in a few.
> 
> Thanks,
> Mauro
> 
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index c460fbcbc035..9611c3aae8ca 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -26,7 +26,7 @@
>   * assign_in_user() - Copy from one __user var to another one
>   *
>   * @to: __user var where data will be stored
> - * @from: __user var were data will be retrieved.
> + * @from: __user var where data will be retrieved.
>   *
>   * As this code very often needs to allocate userspace memory, it is easier
>   * to have a macro that will do both get_user() and put_user() at once.
> @@ -46,12 +46,12 @@
>   *		pointer with userspace data that is not tagged with __user.
>   *
>   * @__x: var where data will be stored
> - * @__ptr: var were data will be retrieved.
> + * @__ptr: var where data will be retrieved.
>   *
> - * Sometimes, we need to declare a pointer without __user, because it
> + * Sometimes we need to declare a pointer without __user because it
>   * comes from a pointer struct field that will be retrieved from userspace
>   * by the 64-bit native ioctl handler. This function ensures that the
> - * @__ptr will be casted to __user before calling get_user(), in order to
> + * @__ptr will be cast to __user before calling get_user() in order to
>   * avoid warnings with static code analyzers like smatch.
>   */
>  #define get_user_cast(__x, __ptr)					\
> @@ -60,16 +60,15 @@
>  })
>  
>  /**
> - * put_user_force() - Stores at the contents of a kernelspace local var
> + * put_user_force() - Stores the contents of a kernelspace local var
>   *		      into an userspace pointer, removing any __user cast.
>   *
>   * @__x: var where data will be stored
> - * @__ptr: var were data will be retrieved.
> + * @__ptr: var where data will be retrieved.
>   *
> - * As the compat32 code now handles with 32-bits and 64-bits __user
> - * structs, sometimes we need to remove the __user atributes from some data,
> - * by passing __force macro. This function ensures that the
> - * @__ptr will be casted with __force before calling put_user(), in order to
> + * Sometimes we need to remove the __user attribute from some data,
> + * by passing the __force macro. This function ensures that the
> + * @__ptr will be cast with __force before calling put_user(), in order to
>   * avoid warnings with static code analyzers like smatch.
>   */
>  #define put_user_force(__x, __ptr)					\
> @@ -81,7 +80,7 @@
>   * assign_in_user_cast() - Copy from one __user var to another one
>   *
>   * @to: __user var where data will be stored
> - * @from: var were data will be retrieved that needs to be cast to __user.
> + * @from: var where data will be retrieved that needs to be cast to __user.
>   *
>   * As this code very often needs to allocate userspace memory, it is easier
>   * to have a macro that will do both get_user_cast() and put_user() at once.
> @@ -1086,11 +1085,11 @@ static int put_v4l2_edid32(struct v4l2_edid __user *p64,
>  }
>  
>  /*
> - * List of ioctl's that require 32-bits/64-bits conversion
> + * List of ioctls that require 32-bits/64-bits conversion
>   *
>   * The V4L2 ioctls that aren't listed there don't have pointer arguments
>   * and the struct size is identical for both 32 and 64 bits versions, so
> - * don't need translations.
> + * they don't need translations.
>   */
>  
>  #define VIDIOC_G_FMT32		_IOWR('V',  4, struct v4l2_format32)
> @@ -1125,8 +1124,8 @@ static int put_v4l2_edid32(struct v4l2_edid __user *p64,
>   *	for calling the native 64-bits version of an ioctl.
>   *
>   * @size:	size of the structure itself to be allocated.
> - * @aux_space:	extra size needed to store "extra" data, e. g. space for
> - *		other __user data that comes pointed by fields inside the
> + * @aux_space:	extra size needed to store "extra" data, e.g. space for
> + *		other __user data that is pointed to fields inside the
>   *		structure.
>   * @new_p64:	pointer to a pointer to be filled with the allocated struct.
>   *
> @@ -1160,7 +1159,7 @@ static int alloc_userspace(unsigned int size, u32 aux_space,
>   * not private to some specific driver.
>   *
>   * It converts a 32-bits struct into a 64 bits one, calls the native 64-bits
> - * ioctl handles and fills back the 32-bits struct with the results of the
> + * ioctl handler and fills back the 32-bits struct with the results of the
>   * native call.
>   */
>  static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> @@ -1437,8 +1436,8 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>   * in order to deal with 32-bit calls on a 64-bits Kernel.
>   *
>   * This function calls do_video_ioctl() for non-private V4L2 ioctls.
> - * If the function is a private one, it calls, instead,
> - * vdev->fops->compat_ioctl32.
> + * If the function is a private one it calls vdev->fops->compat_ioctl32
> + * instead.
>   */
>  long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
>  {
> 

[PATCH v2] media: v4l2-compat-ioctl32: better document the code

[Mauro Carvalho Chehab]

This file does a lot of non-trivial struff. Document it using
kernel-doc markups where needed and improve the comments inside
do_video_ioctl().

Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 165 +++++++++++++++++++++++++-
 1 file changed, 159 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index d2f0268427c2..9611c3aae8ca 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -22,7 +22,18 @@
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-ioctl.h>
 
-/* Use the same argument order as copy_in_user */
+/**
+ * assign_in_user() - Copy from one __user var to another one
+ *
+ * @to: __user var where data will be stored
+ * @from: __user var where data will be retrieved.
+ *
+ * As this code very often needs to allocate userspace memory, it is easier
+ * to have a macro that will do both get_user() and put_user() at once.
+ *
+ * This function complements the macros defined at asm-generic/uaccess.h.
+ * It uses the same argument order as copy_in_user()
+ */
 #define assign_in_user(to, from)					\
 ({									\
 	typeof(*from) __assign_tmp;					\
@@ -30,16 +41,56 @@
 	get_user(__assign_tmp, from) || put_user(__assign_tmp, to);	\
 })
 
+/**
+ * get_user_cast() - Stores at a kernelspace local var the contents from a
+ *		pointer with userspace data that is not tagged with __user.
+ *
+ * @__x: var where data will be stored
+ * @__ptr: var where data will be retrieved.
+ *
+ * Sometimes we need to declare a pointer without __user because it
+ * comes from a pointer struct field that will be retrieved from userspace
+ * by the 64-bit native ioctl handler. This function ensures that the
+ * @__ptr will be cast to __user before calling get_user() in order to
+ * avoid warnings with static code analyzers like smatch.
+ */
 #define get_user_cast(__x, __ptr)					\
 ({									\
 	get_user(__x, (typeof(*__ptr) __user *)(__ptr));		\
 })
 
+/**
+ * put_user_force() - Stores the contents of a kernelspace local var
+ *		      into an userspace pointer, removing any __user cast.
+ *
+ * @__x: var where data will be stored
+ * @__ptr: var where data will be retrieved.
+ *
+ * Sometimes we need to remove the __user attribute from some data,
+ * by passing the __force macro. This function ensures that the
+ * @__ptr will be cast with __force before calling put_user(), in order to
+ * avoid warnings with static code analyzers like smatch.
+ */
 #define put_user_force(__x, __ptr)					\
 ({									\
 	put_user((typeof(*__x) __force *)(__x), __ptr);			\
 })
 
+/**
+ * assign_in_user_cast() - Copy from one __user var to another one
+ *
+ * @to: __user var where data will be stored
+ * @from: var where data will be retrieved that needs to be cast to __user.
+ *
+ * As this code very often needs to allocate userspace memory, it is easier
+ * to have a macro that will do both get_user_cast() and put_user() at once.
+ *
+ * This function should be used instead of assign_in_user() when the @from
+ * variable was not declared as __user. See get_user_cast() for more details.
+ *
+ * This function complements the macros defined at asm-generic/uaccess.h.
+ * It uses the same argument order as copy_in_user()
+ */
 #define assign_in_user_cast(to, from)					\
 ({									\
 	typeof(*from) __assign_tmp;					\
@@ -47,7 +98,16 @@
 	get_user_cast(__assign_tmp, from) || put_user(__assign_tmp, to);\
 })
 
-
+/**
+ * native_ioctl - Ancillary function that calls the native 64 bits ioctl
+ * handler.
+ *
+ * @file: pointer to &struct file with the file handler
+ * @cmd: ioctl to be called
+ * @arg: arguments passed from/to the ioctl handler
+ *
+ * This function calls the native ioctl handler at v4l2-dev, e. g. v4l2_ioctl()
+ */
 static long native_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	long ret = -ENOIOCTLCMD;
@@ -59,6 +119,21 @@ static long native_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 }
 
 
+/*
+ * Per-ioctl data copy handlers.
+ *
+ * Those come in pairs, with a get_v4l2_foo() and a put_v4l2_foo() routine,
+ * where "v4l2_foo" is the name of the V4L2 struct.
+ *
+ * They basically get two __user pointers, one with a 32-bits struct that
+ * came from the userspace call and a 64-bits struct, also allocated as
+ * userspace, but filled internally by do_video_ioctl().
+ *
+ * For ioctls that have pointers inside it, the functions will also
+ * receive an ancillary buffer with extra space, used to pass extra
+ * data to the routine.
+ */
+
 struct v4l2_clip32 {
 	struct v4l2_rect        c;
 	compat_caddr_t		next;
@@ -1009,6 +1084,13 @@ static int put_v4l2_edid32(struct v4l2_edid __user *p64,
 	return 0;
 }
 
+/*
+ * List of ioctls that require 32-bits/64-bits conversion
+ *
+ * The V4L2 ioctls that aren't listed there don't have pointer arguments
+ * and the struct size is identical for both 32 and 64 bits versions, so
+ * they don't need translations.
+ */
 
 #define VIDIOC_G_FMT32		_IOWR('V',  4, struct v4l2_format32)
 #define VIDIOC_S_FMT32		_IOWR('V',  5, struct v4l2_format32)
@@ -1037,6 +1119,21 @@ static int put_v4l2_edid32(struct v4l2_edid __user *p64,
 #define VIDIOC_G_OUTPUT32	_IOR ('V', 46, s32)
 #define VIDIOC_S_OUTPUT32	_IOWR('V', 47, s32)
 
+/**
+ * alloc_userspace() - Allocates a 64-bits userspace pointer compatible
+ *	for calling the native 64-bits version of an ioctl.
+ *
+ * @size:	size of the structure itself to be allocated.
+ * @aux_space:	extra size needed to store "extra" data, e.g. space for
+ *		other __user data that is pointed to fields inside the
+ *		structure.
+ * @new_p64:	pointer to a pointer to be filled with the allocated struct.
+ *
+ * Return:
+ *
+ * if it can't allocate memory, either -ENOMEM or -EFAULT will be returned.
+ * Zero otherwise.
+ */
 static int alloc_userspace(unsigned int size, u32 aux_space,
 			   void __user **new_p64)
 {
@@ -1048,6 +1145,23 @@ static int alloc_userspace(unsigned int size, u32 aux_space,
 	return 0;
 }
 
+/**
+ * do_video_ioctl() - Ancillary function with handles a compat32 ioctl call
+ *
+ * @file: pointer to &struct file with the file handler
+ * @cmd: ioctl to be called
+ * @arg: arguments passed from/to the ioctl handler
+ *
+ * This function is called when a 32 bits application calls a V4L2 ioctl
+ * and the Kernel is compiled with 64 bits.
+ *
+ * This function is called by v4l2_compat_ioctl32() when the function is
+ * not private to some specific driver.
+ *
+ * It converts a 32-bits struct into a 64 bits one, calls the native 64-bits
+ * ioctl handler and fills back the 32-bits struct with the results of the
+ * native call.
+ */
 static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	void __user *p32 = compat_ptr(arg);
@@ -1057,7 +1171,9 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	int compatible_arg = 1;
 	long err = 0;
 
-	/* First, convert the command. */
+	/*
+	 * 1. When struct size is different, converts the command.
+	 */
 	switch (cmd) {
 	case VIDIOC_G_FMT32: cmd = VIDIOC_G_FMT; break;
 	case VIDIOC_S_FMT32: cmd = VIDIOC_S_FMT; break;
@@ -1086,6 +1202,11 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	case VIDIOC_S_EDID32: cmd = VIDIOC_S_EDID; break;
 	}
 
+	/*
+	 * 2. Allocates a 64-bits userspace pointer to store the
+	 * values of the ioctl and copy data from the 32-bits __user
+	 * argument into it.
+	 */
 	switch (cmd) {
 	case VIDIOC_OVERLAY:
 	case VIDIOC_STREAMON:
@@ -1208,6 +1329,15 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	if (err)
 		return err;
 
+	/*
+	 * 3. Calls the native 64-bits ioctl handler.
+	 *
+	 * For the functions where a conversion was not needed,
+	 * compatible_arg is true, and it will call it with the arguments
+	 * provided by userspace and stored at @p32 var.
+	 *
+	 * Otherwise, it will pass the newly allocated @new_p64 argument.
+	 */
 	if (compatible_arg)
 		err = native_ioctl(file, cmd, (unsigned long)p32);
 	else
@@ -1217,9 +1347,14 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 		return err;
 
 	/*
-	 * Special case: even after an error we need to put the
-	 * results back for these ioctls since the error_idx will
-	 * contain information on which control failed.
+	 * 4. Special case: even after an error we need to put the
+	 * results back for some ioctls.
+	 *
+	 * In the case of EXT_CTRLS, the error_idx will contain information
+	 * on which control failed.
+	 *
+	 * In the case of S_EDID, the driver can return E2BIG and set
+	 * the blocks to maximum allowed value.
 	 */
 	switch (cmd) {
 	case VIDIOC_G_EXT_CTRLS:
@@ -1236,6 +1371,10 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	if (err)
 		return err;
 
+	/*
+	 * 5. Copy the data returned at the 64 bits userspace pointer to
+	 * the original 32 bits structure.
+	 */
 	switch (cmd) {
 	case VIDIOC_S_INPUT:
 	case VIDIOC_S_OUTPUT:
@@ -1286,6 +1425,20 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
 	return err;
 }
 
+/**
+ * v4l2_compat_ioctl32() - Handles a compat32 ioctl call
+ *
+ * @file: pointer to &struct file with the file handler
+ * @cmd: ioctl to be called
+ * @arg: arguments passed from/to the ioctl handler
+ *
+ * This function is meant to be used as .compat_ioctl fops at v4l2-dev.c
+ * in order to deal with 32-bit calls on a 64-bits Kernel.
+ *
+ * This function calls do_video_ioctl() for non-private V4L2 ioctls.
+ * If the function is a private one it calls vdev->fops->compat_ioctl32
+ * instead.
+ */
 long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct video_device *vdev = video_devdata(file);
-- 
2.14.3

Re: [PATCH v2] media: v4l2-compat-ioctl32: better document the code

[Hans Verkuil]

On 04/20/18 13:45, Mauro Carvalho Chehab wrote:
> This file does a lot of non-trivial struff. Document it using
> kernel-doc markups where needed and improve the comments inside
> do_video_ioctl().
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

> ---
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 165 +++++++++++++++++++++++++-
>  1 file changed, 159 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index d2f0268427c2..9611c3aae8ca 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -22,7 +22,18 @@
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-ioctl.h>
>  
> -/* Use the same argument order as copy_in_user */
> +/**
> + * assign_in_user() - Copy from one __user var to another one
> + *
> + * @to: __user var where data will be stored
> + * @from: __user var where data will be retrieved.
> + *
> + * As this code very often needs to allocate userspace memory, it is easier
> + * to have a macro that will do both get_user() and put_user() at once.
> + *
> + * This function complements the macros defined at asm-generic/uaccess.h.
> + * It uses the same argument order as copy_in_user()
> + */
>  #define assign_in_user(to, from)					\
>  ({									\
>  	typeof(*from) __assign_tmp;					\
> @@ -30,16 +41,56 @@
>  	get_user(__assign_tmp, from) || put_user(__assign_tmp, to);	\
>  })
>  
> +/**
> + * get_user_cast() - Stores at a kernelspace local var the contents from a
> + *		pointer with userspace data that is not tagged with __user.
> + *
> + * @__x: var where data will be stored
> + * @__ptr: var where data will be retrieved.
> + *
> + * Sometimes we need to declare a pointer without __user because it
> + * comes from a pointer struct field that will be retrieved from userspace
> + * by the 64-bit native ioctl handler. This function ensures that the
> + * @__ptr will be cast to __user before calling get_user() in order to
> + * avoid warnings with static code analyzers like smatch.
> + */
>  #define get_user_cast(__x, __ptr)					\
>  ({									\
>  	get_user(__x, (typeof(*__ptr) __user *)(__ptr));		\
>  })
>  
> +/**
> + * put_user_force() - Stores the contents of a kernelspace local var
> + *		      into an userspace pointer, removing any __user cast.
> + *
> + * @__x: var where data will be stored
> + * @__ptr: var where data will be retrieved.
> + *
> + * Sometimes we need to remove the __user attribute from some data,
> + * by passing the __force macro. This function ensures that the
> + * @__ptr will be cast with __force before calling put_user(), in order to
> + * avoid warnings with static code analyzers like smatch.
> + */
>  #define put_user_force(__x, __ptr)					\
>  ({									\
>  	put_user((typeof(*__x) __force *)(__x), __ptr);			\
>  })
>  
> +/**
> + * assign_in_user_cast() - Copy from one __user var to another one
> + *
> + * @to: __user var where data will be stored
> + * @from: var where data will be retrieved that needs to be cast to __user.
> + *
> + * As this code very often needs to allocate userspace memory, it is easier
> + * to have a macro that will do both get_user_cast() and put_user() at once.
> + *
> + * This function should be used instead of assign_in_user() when the @from
> + * variable was not declared as __user. See get_user_cast() for more details.
> + *
> + * This function complements the macros defined at asm-generic/uaccess.h.
> + * It uses the same argument order as copy_in_user()
> + */
>  #define assign_in_user_cast(to, from)					\
>  ({									\
>  	typeof(*from) __assign_tmp;					\
> @@ -47,7 +98,16 @@
>  	get_user_cast(__assign_tmp, from) || put_user(__assign_tmp, to);\
>  })
>  
> -
> +/**
> + * native_ioctl - Ancillary function that calls the native 64 bits ioctl
> + * handler.
> + *
> + * @file: pointer to &struct file with the file handler
> + * @cmd: ioctl to be called
> + * @arg: arguments passed from/to the ioctl handler
> + *
> + * This function calls the native ioctl handler at v4l2-dev, e. g. v4l2_ioctl()
> + */
>  static long native_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  {
>  	long ret = -ENOIOCTLCMD;
> @@ -59,6 +119,21 @@ static long native_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  }
>  
>  
> +/*
> + * Per-ioctl data copy handlers.
> + *
> + * Those come in pairs, with a get_v4l2_foo() and a put_v4l2_foo() routine,
> + * where "v4l2_foo" is the name of the V4L2 struct.
> + *
> + * They basically get two __user pointers, one with a 32-bits struct that
> + * came from the userspace call and a 64-bits struct, also allocated as
> + * userspace, but filled internally by do_video_ioctl().
> + *
> + * For ioctls that have pointers inside it, the functions will also
> + * receive an ancillary buffer with extra space, used to pass extra
> + * data to the routine.
> + */
> +
>  struct v4l2_clip32 {
>  	struct v4l2_rect        c;
>  	compat_caddr_t		next;
> @@ -1009,6 +1084,13 @@ static int put_v4l2_edid32(struct v4l2_edid __user *p64,
>  	return 0;
>  }
>  
> +/*
> + * List of ioctls that require 32-bits/64-bits conversion
> + *
> + * The V4L2 ioctls that aren't listed there don't have pointer arguments
> + * and the struct size is identical for both 32 and 64 bits versions, so
> + * they don't need translations.
> + */
>  
>  #define VIDIOC_G_FMT32		_IOWR('V',  4, struct v4l2_format32)
>  #define VIDIOC_S_FMT32		_IOWR('V',  5, struct v4l2_format32)
> @@ -1037,6 +1119,21 @@ static int put_v4l2_edid32(struct v4l2_edid __user *p64,
>  #define VIDIOC_G_OUTPUT32	_IOR ('V', 46, s32)
>  #define VIDIOC_S_OUTPUT32	_IOWR('V', 47, s32)
>  
> +/**
> + * alloc_userspace() - Allocates a 64-bits userspace pointer compatible
> + *	for calling the native 64-bits version of an ioctl.
> + *
> + * @size:	size of the structure itself to be allocated.
> + * @aux_space:	extra size needed to store "extra" data, e.g. space for
> + *		other __user data that is pointed to fields inside the
> + *		structure.
> + * @new_p64:	pointer to a pointer to be filled with the allocated struct.
> + *
> + * Return:
> + *
> + * if it can't allocate memory, either -ENOMEM or -EFAULT will be returned.
> + * Zero otherwise.
> + */
>  static int alloc_userspace(unsigned int size, u32 aux_space,
>  			   void __user **new_p64)
>  {
> @@ -1048,6 +1145,23 @@ static int alloc_userspace(unsigned int size, u32 aux_space,
>  	return 0;
>  }
>  
> +/**
> + * do_video_ioctl() - Ancillary function with handles a compat32 ioctl call
> + *
> + * @file: pointer to &struct file with the file handler
> + * @cmd: ioctl to be called
> + * @arg: arguments passed from/to the ioctl handler
> + *
> + * This function is called when a 32 bits application calls a V4L2 ioctl
> + * and the Kernel is compiled with 64 bits.
> + *
> + * This function is called by v4l2_compat_ioctl32() when the function is
> + * not private to some specific driver.
> + *
> + * It converts a 32-bits struct into a 64 bits one, calls the native 64-bits
> + * ioctl handler and fills back the 32-bits struct with the results of the
> + * native call.
> + */
>  static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  {
>  	void __user *p32 = compat_ptr(arg);
> @@ -1057,7 +1171,9 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>  	int compatible_arg = 1;
>  	long err = 0;
>  
> -	/* First, convert the command. */
> +	/*
> +	 * 1. When struct size is different, converts the command.
> +	 */
>  	switch (cmd) {
>  	case VIDIOC_G_FMT32: cmd = VIDIOC_G_FMT; break;
>  	case VIDIOC_S_FMT32: cmd = VIDIOC_S_FMT; break;
> @@ -1086,6 +1202,11 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>  	case VIDIOC_S_EDID32: cmd = VIDIOC_S_EDID; break;
>  	}
>  
> +	/*
> +	 * 2. Allocates a 64-bits userspace pointer to store the
> +	 * values of the ioctl and copy data from the 32-bits __user
> +	 * argument into it.
> +	 */
>  	switch (cmd) {
>  	case VIDIOC_OVERLAY:
>  	case VIDIOC_STREAMON:
> @@ -1208,6 +1329,15 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>  	if (err)
>  		return err;
>  
> +	/*
> +	 * 3. Calls the native 64-bits ioctl handler.
> +	 *
> +	 * For the functions where a conversion was not needed,
> +	 * compatible_arg is true, and it will call it with the arguments
> +	 * provided by userspace and stored at @p32 var.
> +	 *
> +	 * Otherwise, it will pass the newly allocated @new_p64 argument.
> +	 */
>  	if (compatible_arg)
>  		err = native_ioctl(file, cmd, (unsigned long)p32);
>  	else
> @@ -1217,9 +1347,14 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>  		return err;
>  
>  	/*
> -	 * Special case: even after an error we need to put the
> -	 * results back for these ioctls since the error_idx will
> -	 * contain information on which control failed.
> +	 * 4. Special case: even after an error we need to put the
> +	 * results back for some ioctls.
> +	 *
> +	 * In the case of EXT_CTRLS, the error_idx will contain information
> +	 * on which control failed.
> +	 *
> +	 * In the case of S_EDID, the driver can return E2BIG and set
> +	 * the blocks to maximum allowed value.
>  	 */
>  	switch (cmd) {
>  	case VIDIOC_G_EXT_CTRLS:
> @@ -1236,6 +1371,10 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>  	if (err)
>  		return err;
>  
> +	/*
> +	 * 5. Copy the data returned at the 64 bits userspace pointer to
> +	 * the original 32 bits structure.
> +	 */
>  	switch (cmd) {
>  	case VIDIOC_S_INPUT:
>  	case VIDIOC_S_OUTPUT:
> @@ -1286,6 +1425,20 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>  	return err;
>  }
>  
> +/**
> + * v4l2_compat_ioctl32() - Handles a compat32 ioctl call
> + *
> + * @file: pointer to &struct file with the file handler
> + * @cmd: ioctl to be called
> + * @arg: arguments passed from/to the ioctl handler
> + *
> + * This function is meant to be used as .compat_ioctl fops at v4l2-dev.c
> + * in order to deal with 32-bit calls on a 64-bits Kernel.
> + *
> + * This function calls do_video_ioctl() for non-private V4L2 ioctls.
> + * If the function is a private one it calls vdev->fops->compat_ioctl32
> + * instead.
> + */
>  long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
>  {
>  	struct video_device *vdev = video_devdata(file);
> 

Re: [PATCH v2] media: v4l2-compat-ioctl32: better document the code

[Sakari Ailus]

Hi Mauro,

Thanks. It's nice to have these things documented. A few comments below.

On Fri, Apr 20, 2018 at 07:45:46AM -0400, Mauro Carvalho Chehab wrote:
> This file does a lot of non-trivial struff. Document it using
> kernel-doc markups where needed and improve the comments inside
> do_video_ioctl().
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> ---
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 165 +++++++++++++++++++++++++-
>  1 file changed, 159 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index d2f0268427c2..9611c3aae8ca 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -22,7 +22,18 @@
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-ioctl.h>
>  
> -/* Use the same argument order as copy_in_user */
> +/**
> + * assign_in_user() - Copy from one __user var to another one

No need for empty parentheses --- I think we generally don't have them
elsewhere either, albeit I remember seeing them somewhere. But they're
still redundant. :-)

> + *
> + * @to: __user var where data will be stored
> + * @from: __user var where data will be retrieved.

Please use the full stop consistently; I guess in most of the cases we
don't have that in argument descriptions.

> + *
> + * As this code very often needs to allocate userspace memory, it is easier
> + * to have a macro that will do both get_user() and put_user() at once.

How about "Read a value from __user memory at @from using get_user() and
write it back to __user memory at @to using put_user(). A temporary
variable needed for this is allocated in the stack." ? Up to you.

> + *
> + * This function complements the macros defined at asm-generic/uaccess.h.
> + * It uses the same argument order as copy_in_user()
> + */

This starts looking so good it might be material to
include/asm-generic/uaccess.h, but let's think about that later on. :-)

>  #define assign_in_user(to, from)					\
>  ({									\
>  	typeof(*from) __assign_tmp;					\
> @@ -30,16 +41,56 @@
>  	get_user(__assign_tmp, from) || put_user(__assign_tmp, to);	\
>  })
>  
> +/**
> + * get_user_cast() - Stores at a kernelspace local var the contents from a
> + *		pointer with userspace data that is not tagged with __user.
> + *
> + * @__x: var where data will be stored
> + * @__ptr: var where data will be retrieved.
> + *
> + * Sometimes we need to declare a pointer without __user because it
> + * comes from a pointer struct field that will be retrieved from userspace
> + * by the 64-bit native ioctl handler. This function ensures that the
> + * @__ptr will be cast to __user before calling get_user() in order to
> + * avoid warnings with static code analyzers like smatch.
> + */
>  #define get_user_cast(__x, __ptr)					\
>  ({									\
>  	get_user(__x, (typeof(*__ptr) __user *)(__ptr));		\
>  })
>  
> +/**
> + * put_user_force() - Stores the contents of a kernelspace local var
> + *		      into an userspace pointer, removing any __user cast.
> + *
> + * @__x: var where data will be stored
> + * @__ptr: var where data will be retrieved.
> + *
> + * Sometimes we need to remove the __user attribute from some data,
> + * by passing the __force macro. This function ensures that the
> + * @__ptr will be cast with __force before calling put_user(), in order to
> + * avoid warnings with static code analyzers like smatch.
> + */
>  #define put_user_force(__x, __ptr)					\
>  ({									\
>  	put_user((typeof(*__x) __force *)(__x), __ptr);			\
>  })
>  
> +/**
> + * assign_in_user_cast() - Copy from one __user var to another one
> + *
> + * @to: __user var where data will be stored
> + * @from: var where data will be retrieved that needs to be cast to __user.
> + *
> + * As this code very often needs to allocate userspace memory, it is easier
> + * to have a macro that will do both get_user_cast() and put_user() at once.
> + *
> + * This function should be used instead of assign_in_user() when the @from
> + * variable was not declared as __user. See get_user_cast() for more details.
> + *
> + * This function complements the macros defined at asm-generic/uaccess.h.
> + * It uses the same argument order as copy_in_user()
> + */
>  #define assign_in_user_cast(to, from)					\
>  ({									\
>  	typeof(*from) __assign_tmp;					\
> @@ -47,7 +98,16 @@
>  	get_user_cast(__assign_tmp, from) || put_user(__assign_tmp, to);\
>  })
>  
> -
> +/**
> + * native_ioctl - Ancillary function that calls the native 64 bits ioctl
> + * handler.
> + *
> + * @file: pointer to &struct file with the file handler
> + * @cmd: ioctl to be called
> + * @arg: arguments passed from/to the ioctl handler
> + *
> + * This function calls the native ioctl handler at v4l2-dev, e. g. v4l2_ioctl()
> + */
>  static long native_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  {
>  	long ret = -ENOIOCTLCMD;
> @@ -59,6 +119,21 @@ static long native_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  }
>  
>  
> +/*
> + * Per-ioctl data copy handlers.
> + *
> + * Those come in pairs, with a get_v4l2_foo() and a put_v4l2_foo() routine,
> + * where "v4l2_foo" is the name of the V4L2 struct.
> + *
> + * They basically get two __user pointers, one with a 32-bits struct that
> + * came from the userspace call and a 64-bits struct, also allocated as
> + * userspace, but filled internally by do_video_ioctl().
> + *
> + * For ioctls that have pointers inside it, the functions will also
> + * receive an ancillary buffer with extra space, used to pass extra
> + * data to the routine.
> + */
> +
>  struct v4l2_clip32 {
>  	struct v4l2_rect        c;
>  	compat_caddr_t		next;
> @@ -1009,6 +1084,13 @@ static int put_v4l2_edid32(struct v4l2_edid __user *p64,
>  	return 0;
>  }
>  
> +/*
> + * List of ioctls that require 32-bits/64-bits conversion
> + *
> + * The V4L2 ioctls that aren't listed there don't have pointer arguments
> + * and the struct size is identical for both 32 and 64 bits versions, so
> + * they don't need translations.
> + */
>  
>  #define VIDIOC_G_FMT32		_IOWR('V',  4, struct v4l2_format32)
>  #define VIDIOC_S_FMT32		_IOWR('V',  5, struct v4l2_format32)
> @@ -1037,6 +1119,21 @@ static int put_v4l2_edid32(struct v4l2_edid __user *p64,
>  #define VIDIOC_G_OUTPUT32	_IOR ('V', 46, s32)
>  #define VIDIOC_S_OUTPUT32	_IOWR('V', 47, s32)
>  
> +/**
> + * alloc_userspace() - Allocates a 64-bits userspace pointer compatible
> + *	for calling the native 64-bits version of an ioctl.
> + *
> + * @size:	size of the structure itself to be allocated.
> + * @aux_space:	extra size needed to store "extra" data, e.g. space for
> + *		other __user data that is pointed to fields inside the
> + *		structure.
> + * @new_p64:	pointer to a pointer to be filled with the allocated struct.
> + *
> + * Return:
> + *
> + * if it can't allocate memory, either -ENOMEM or -EFAULT will be returned.
> + * Zero otherwise.
> + */
>  static int alloc_userspace(unsigned int size, u32 aux_space,
>  			   void __user **new_p64)
>  {
> @@ -1048,6 +1145,23 @@ static int alloc_userspace(unsigned int size, u32 aux_space,
>  	return 0;
>  }
>  
> +/**
> + * do_video_ioctl() - Ancillary function with handles a compat32 ioctl call
> + *
> + * @file: pointer to &struct file with the file handler
> + * @cmd: ioctl to be called
> + * @arg: arguments passed from/to the ioctl handler
> + *
> + * This function is called when a 32 bits application calls a V4L2 ioctl
> + * and the Kernel is compiled with 64 bits.
> + *
> + * This function is called by v4l2_compat_ioctl32() when the function is
> + * not private to some specific driver.
> + *
> + * It converts a 32-bits struct into a 64 bits one, calls the native 64-bits
> + * ioctl handler and fills back the 32-bits struct with the results of the
> + * native call.
> + */
>  static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  {
>  	void __user *p32 = compat_ptr(arg);
> @@ -1057,7 +1171,9 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>  	int compatible_arg = 1;
>  	long err = 0;
>  
> -	/* First, convert the command. */
> +	/*
> +	 * 1. When struct size is different, converts the command.

How about "convert" instead of "converts"? A small detail perhaps. The
custom hasn't been to use the third person in documenting functions. Same
for 2--4, as well as some of the comments above.

> +	 */
>  	switch (cmd) {
>  	case VIDIOC_G_FMT32: cmd = VIDIOC_G_FMT; break;
>  	case VIDIOC_S_FMT32: cmd = VIDIOC_S_FMT; break;
> @@ -1086,6 +1202,11 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>  	case VIDIOC_S_EDID32: cmd = VIDIOC_S_EDID; break;
>  	}
>  
> +	/*
> +	 * 2. Allocates a 64-bits userspace pointer to store the
> +	 * values of the ioctl and copy data from the 32-bits __user
> +	 * argument into it.
> +	 */
>  	switch (cmd) {
>  	case VIDIOC_OVERLAY:
>  	case VIDIOC_STREAMON:
> @@ -1208,6 +1329,15 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>  	if (err)
>  		return err;
>  
> +	/*
> +	 * 3. Calls the native 64-bits ioctl handler.
> +	 *
> +	 * For the functions where a conversion was not needed,
> +	 * compatible_arg is true, and it will call it with the arguments
> +	 * provided by userspace and stored at @p32 var.
> +	 *
> +	 * Otherwise, it will pass the newly allocated @new_p64 argument.
> +	 */
>  	if (compatible_arg)
>  		err = native_ioctl(file, cmd, (unsigned long)p32);
>  	else
> @@ -1217,9 +1347,14 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>  		return err;
>  
>  	/*
> -	 * Special case: even after an error we need to put the
> -	 * results back for these ioctls since the error_idx will
> -	 * contain information on which control failed.
> +	 * 4. Special case: even after an error we need to put the
> +	 * results back for some ioctls.
> +	 *
> +	 * In the case of EXT_CTRLS, the error_idx will contain information
> +	 * on which control failed.
> +	 *
> +	 * In the case of S_EDID, the driver can return E2BIG and set
> +	 * the blocks to maximum allowed value.
>  	 */
>  	switch (cmd) {
>  	case VIDIOC_G_EXT_CTRLS:
> @@ -1236,6 +1371,10 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>  	if (err)
>  		return err;
>  
> +	/*
> +	 * 5. Copy the data returned at the 64 bits userspace pointer to
> +	 * the original 32 bits structure.
> +	 */
>  	switch (cmd) {
>  	case VIDIOC_S_INPUT:
>  	case VIDIOC_S_OUTPUT:
> @@ -1286,6 +1425,20 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>  	return err;
>  }
>  
> +/**
> + * v4l2_compat_ioctl32() - Handles a compat32 ioctl call
> + *
> + * @file: pointer to &struct file with the file handler
> + * @cmd: ioctl to be called
> + * @arg: arguments passed from/to the ioctl handler
> + *
> + * This function is meant to be used as .compat_ioctl fops at v4l2-dev.c
> + * in order to deal with 32-bit calls on a 64-bits Kernel.
> + *
> + * This function calls do_video_ioctl() for non-private V4L2 ioctls.
> + * If the function is a private one it calls vdev->fops->compat_ioctl32
> + * instead.
> + */
>  long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
>  {
>  	struct video_device *vdev = video_devdata(file);

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com