All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC] [PATCH] v4l2: use unsigned rather than enums in ioctl() structs
@ 2012-04-05 17:52 Rémi Denis-Courmont
  2012-04-11 17:02 ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 13+ messages in thread
From: Rémi Denis-Courmont @ 2012-04-05 17:52 UTC (permalink / raw)
  To: mchehab, linux-media, linux-kernel

With an enumeration, the compiler assumes that the integer value is one
allowed by the underlying enumeration type. With optimization enabled
this can result in byte code that is unable to cope with other values.
For instance, GCC can compile a switch() block using a "jump table" to
avoid repetitive conditional branching.

This can be a problem both from user to kernel and kernel to user.

* Evil user space can pass error values to the kernel via system
calls. There are no sane ways to protect the kernel: the compiler may
optimize away range checks if it deems that all legitimate values are
within the range.

* The kernel can break the user space binary interface whenever a new
value is added to an existing enumeration. A newer kernel can then
return an enumerated value that was not allowed by older kernel headers
against which the user program was compiled. In principles, V4L2 user
space needs to be recompiled whenever videodev2.h has updated enums...
(This an obvious problem with V4L2_QUERYCTRL.)

Fortunately, the Linux ABI disables short-enums on all platforms. Even
the Linux variant of the ARM AAPCS contradicts the standard ARM AAPCS
in doing so.

Signed-off-by: Rémi Denis-Courmont <remi@remlab.net>
---
 include/linux/videodev2.h |   42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index c9c9a46..df3b8f0 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -292,10 +292,10 @@ struct v4l2_pix_format {
 	__u32         		width;
 	__u32			height;
 	__u32			pixelformat;
-	enum v4l2_field  	field;
+	unsigned		field;
 	__u32            	bytesperline;	/* for padding, zero if unused */
 	__u32          		sizeimage;
-	enum v4l2_colorspace	colorspace;
+	unsigned		colorspace;
 	__u32			priv;		/* private data, depends on pixelformat */
 };
 
@@ -432,7 +432,7 @@ struct v4l2_pix_format {
  */
 struct v4l2_fmtdesc {
 	__u32		    index;             /* Format number      */
-	enum v4l2_buf_type  type;              /* buffer type        */
+	unsigned	    type;              /* buffer type        */
 	__u32               flags;
 	__u8		    description[32];   /* Description string */
 	__u32		    pixelformat;       /* Format fourcc      */
@@ -573,8 +573,8 @@ struct v4l2_jpegcompression {
  */
 struct v4l2_requestbuffers {
 	__u32			count;
-	enum v4l2_buf_type      type;
-	enum v4l2_memory        memory;
+	unsigned		type;
+	unsigned		memory;
 	__u32			reserved[2];
 };
 
@@ -636,16 +636,16 @@ struct v4l2_plane {
  */
 struct v4l2_buffer {
 	__u32			index;
-	enum v4l2_buf_type      type;
+	unsigned		type;
 	__u32			bytesused;
 	__u32			flags;
-	enum v4l2_field		field;
+	unsigned		field;
 	struct timeval		timestamp;
 	struct v4l2_timecode	timecode;
 	__u32			sequence;
 
 	/* memory location */
-	enum v4l2_memory        memory;
+	unsigned		memory;
 	union {
 		__u32           offset;
 		unsigned long   userptr;
@@ -708,7 +708,7 @@ struct v4l2_clip {
 
 struct v4l2_window {
 	struct v4l2_rect        w;
-	enum v4l2_field  	field;
+	unsigned		field;
 	__u32			chromakey;
 	struct v4l2_clip	__user *clips;
 	__u32			clipcount;
@@ -745,14 +745,14 @@ struct v4l2_outputparm {
  *	I N P U T   I M A G E   C R O P P I N G
  */
 struct v4l2_cropcap {
-	enum v4l2_buf_type      type;
+	unsigned		type;
 	struct v4l2_rect        bounds;
 	struct v4l2_rect        defrect;
 	struct v4l2_fract       pixelaspect;
 };
 
 struct v4l2_crop {
-	enum v4l2_buf_type      type;
+	unsigned		type;
 	struct v4l2_rect        c;
 };
 
@@ -1156,7 +1156,7 @@ enum v4l2_ctrl_type {
 /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
 struct v4l2_queryctrl {
 	__u32		     id;
-	enum v4l2_ctrl_type  type;
+	unsigned	     type;
 	__u8		     name[32];	/* Whatever */
 	__s32		     minimum;	/* Note signedness */
 	__s32		     maximum;
@@ -1788,7 +1788,7 @@ enum v4l2_jpeg_chroma_subsampling {
 struct v4l2_tuner {
 	__u32                   index;
 	__u8			name[32];
-	enum v4l2_tuner_type    type;
+	unsigned		type;
 	__u32			capability;
 	__u32			rangelow;
 	__u32			rangehigh;
@@ -1838,14 +1838,14 @@ struct v4l2_modulator {
 
 struct v4l2_frequency {
 	__u32		      tuner;
-	enum v4l2_tuner_type  type;
+	unsigned	      type;
 	__u32		      frequency;
 	__u32		      reserved[8];
 };
 
 struct v4l2_hw_freq_seek {
 	__u32		      tuner;
-	enum v4l2_tuner_type  type;
+	unsigned	      type;
 	__u32		      seek_upward;
 	__u32		      wrap_around;
 	__u32		      spacing;
@@ -2056,7 +2056,7 @@ struct v4l2_sliced_vbi_cap {
 				 (equals frame lines 313-336 for 625 line video
 				  standards, 263-286 for 525 line standards) */
 	__u16   service_lines[2][24];
-	enum v4l2_buf_type type;
+	unsigned type;
 	__u32   reserved[3];    /* must be 0 */
 };
 
@@ -2146,8 +2146,8 @@ struct v4l2_pix_format_mplane {
 	__u32				width;
 	__u32				height;
 	__u32				pixelformat;
-	enum v4l2_field			field;
-	enum v4l2_colorspace		colorspace;
+	unsigned			field;
+	unsigned			colorspace;
 
 	struct v4l2_plane_pix_format	plane_fmt[VIDEO_MAX_PLANES];
 	__u8				num_planes;
@@ -2165,7 +2165,7 @@ struct v4l2_pix_format_mplane {
  * @raw_data:	placeholder for future extensions and custom formats
  */
 struct v4l2_format {
-	enum v4l2_buf_type type;
+	unsigned type;
 	union {
 		struct v4l2_pix_format		pix;     /* V4L2_BUF_TYPE_VIDEO_CAPTURE */
 		struct v4l2_pix_format_mplane	pix_mp;  /* V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE */
@@ -2179,7 +2179,7 @@ struct v4l2_format {
 /*	Stream type-dependent parameters
  */
 struct v4l2_streamparm {
-	enum v4l2_buf_type type;
+	unsigned type;
 	union {
 		struct v4l2_captureparm	capture;
 		struct v4l2_outputparm	output;
@@ -2299,7 +2299,7 @@ struct v4l2_dbg_chip_ident {
 struct v4l2_create_buffers {
 	__u32			index;
 	__u32			count;
-	enum v4l2_memory        memory;
+	unsigned		memory;
 	struct v4l2_format	format;
 	__u32			reserved[8];
 };
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [RFC] [PATCH] v4l2: use unsigned rather than enums in ioctl() structs
  2012-04-05 17:52 [RFC] [PATCH] v4l2: use unsigned rather than enums in ioctl() structs Rémi Denis-Courmont
@ 2012-04-11 17:02 ` Mauro Carvalho Chehab
  2012-04-11 18:47   ` Rémi Denis-Courmont
  0 siblings, 1 reply; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2012-04-11 17:02 UTC (permalink / raw)
  To: Rémi Denis-Courmont; +Cc: mchehab, linux-media, linux-kernel, Alan Cox

Em 05-04-2012 14:52, Rémi Denis-Courmont escreveu:
> With an enumeration, the compiler assumes that the integer value is one
> allowed by the underlying enumeration type. With optimization enabled
> this can result in byte code that is unable to cope with other values.
> For instance, GCC can compile a switch() block using a "jump table" to
> avoid repetitive conditional branching.
> 
> This can be a problem both from user to kernel and kernel to user.
> 
> * Evil user space can pass error values to the kernel via system
> calls. There are no sane ways to protect the kernel: the compiler may
> optimize away range checks if it deems that all legitimate values are
> within the range.
> 
> * The kernel can break the user space binary interface whenever a new
> value is added to an existing enumeration. A newer kernel can then
> return an enumerated value that was not allowed by older kernel headers
> against which the user program was compiled. In principles, V4L2 user
> space needs to be recompiled whenever videodev2.h has updated enums...
> (This an obvious problem with V4L2_QUERYCTRL.)
> 
> Fortunately, the Linux ABI disables short-enums on all platforms. Even
> the Linux variant of the ARM AAPCS contradicts the standard ARM AAPCS
> in doing so.


Using unsigned instead of enum is not a good idea, from API POV, as unsigned
has different sizes on 32 bits and 64 bits. Yet, using enum was really a very
bad idea, and, on all new stuff, we're not accepting any new enum field.

That's said, a patch like that were proposed in the past. See:
	http://www.spinics.net/lists/vfl/msg25686.html

Alan said there [1] that:
	"Its a fundamental change to a public structure from enum to u32. I think you
	 are right but this change seems too late by several years."

I also didn't like it, because of the same reasons.

[1] http://www.spinics.net/lists/vfl/msg25687.html

I don't think anything has changed since then that would make it easier
to apply a change like that.

Well, eventually, a compat code at the v4l2-ioctl.c might be written to translate
a call if the enum size is different than the integer size.

Regards,
Mauro

> 
> Signed-off-by: Rémi Denis-Courmont <remi@remlab.net>
> ---
>  include/linux/videodev2.h |   42 +++++++++++++++++++++---------------------
>  1 file changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index c9c9a46..df3b8f0 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -292,10 +292,10 @@ struct v4l2_pix_format {
>  	__u32         		width;
>  	__u32			height;
>  	__u32			pixelformat;
> -	enum v4l2_field  	field;
> +	unsigned		field;
>  	__u32            	bytesperline;	/* for padding, zero if unused */
>  	__u32          		sizeimage;
> -	enum v4l2_colorspace	colorspace;
> +	unsigned		colorspace;
>  	__u32			priv;		/* private data, depends on pixelformat */
>  };
>  
> @@ -432,7 +432,7 @@ struct v4l2_pix_format {
>   */
>  struct v4l2_fmtdesc {
>  	__u32		    index;             /* Format number      */
> -	enum v4l2_buf_type  type;              /* buffer type        */
> +	unsigned	    type;              /* buffer type        */
>  	__u32               flags;
>  	__u8		    description[32];   /* Description string */
>  	__u32		    pixelformat;       /* Format fourcc      */
> @@ -573,8 +573,8 @@ struct v4l2_jpegcompression {
>   */
>  struct v4l2_requestbuffers {
>  	__u32			count;
> -	enum v4l2_buf_type      type;
> -	enum v4l2_memory        memory;
> +	unsigned		type;
> +	unsigned		memory;
>  	__u32			reserved[2];
>  };
>  
> @@ -636,16 +636,16 @@ struct v4l2_plane {
>   */
>  struct v4l2_buffer {
>  	__u32			index;
> -	enum v4l2_buf_type      type;
> +	unsigned		type;
>  	__u32			bytesused;
>  	__u32			flags;
> -	enum v4l2_field		field;
> +	unsigned		field;
>  	struct timeval		timestamp;
>  	struct v4l2_timecode	timecode;
>  	__u32			sequence;
>  
>  	/* memory location */
> -	enum v4l2_memory        memory;
> +	unsigned		memory;
>  	union {
>  		__u32           offset;
>  		unsigned long   userptr;
> @@ -708,7 +708,7 @@ struct v4l2_clip {
>  
>  struct v4l2_window {
>  	struct v4l2_rect        w;
> -	enum v4l2_field  	field;
> +	unsigned		field;
>  	__u32			chromakey;
>  	struct v4l2_clip	__user *clips;
>  	__u32			clipcount;
> @@ -745,14 +745,14 @@ struct v4l2_outputparm {
>   *	I N P U T   I M A G E   C R O P P I N G
>   */
>  struct v4l2_cropcap {
> -	enum v4l2_buf_type      type;
> +	unsigned		type;
>  	struct v4l2_rect        bounds;
>  	struct v4l2_rect        defrect;
>  	struct v4l2_fract       pixelaspect;
>  };
>  
>  struct v4l2_crop {
> -	enum v4l2_buf_type      type;
> +	unsigned		type;
>  	struct v4l2_rect        c;
>  };
>  
> @@ -1156,7 +1156,7 @@ enum v4l2_ctrl_type {
>  /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
>  struct v4l2_queryctrl {
>  	__u32		     id;
> -	enum v4l2_ctrl_type  type;
> +	unsigned	     type;
>  	__u8		     name[32];	/* Whatever */
>  	__s32		     minimum;	/* Note signedness */
>  	__s32		     maximum;
> @@ -1788,7 +1788,7 @@ enum v4l2_jpeg_chroma_subsampling {
>  struct v4l2_tuner {
>  	__u32                   index;
>  	__u8			name[32];
> -	enum v4l2_tuner_type    type;
> +	unsigned		type;
>  	__u32			capability;
>  	__u32			rangelow;
>  	__u32			rangehigh;
> @@ -1838,14 +1838,14 @@ struct v4l2_modulator {
>  
>  struct v4l2_frequency {
>  	__u32		      tuner;
> -	enum v4l2_tuner_type  type;
> +	unsigned	      type;
>  	__u32		      frequency;
>  	__u32		      reserved[8];
>  };
>  
>  struct v4l2_hw_freq_seek {
>  	__u32		      tuner;
> -	enum v4l2_tuner_type  type;
> +	unsigned	      type;
>  	__u32		      seek_upward;
>  	__u32		      wrap_around;
>  	__u32		      spacing;
> @@ -2056,7 +2056,7 @@ struct v4l2_sliced_vbi_cap {
>  				 (equals frame lines 313-336 for 625 line video
>  				  standards, 263-286 for 525 line standards) */
>  	__u16   service_lines[2][24];
> -	enum v4l2_buf_type type;
> +	unsigned type;
>  	__u32   reserved[3];    /* must be 0 */
>  };
>  
> @@ -2146,8 +2146,8 @@ struct v4l2_pix_format_mplane {
>  	__u32				width;
>  	__u32				height;
>  	__u32				pixelformat;
> -	enum v4l2_field			field;
> -	enum v4l2_colorspace		colorspace;
> +	unsigned			field;
> +	unsigned			colorspace;
>  
>  	struct v4l2_plane_pix_format	plane_fmt[VIDEO_MAX_PLANES];
>  	__u8				num_planes;
> @@ -2165,7 +2165,7 @@ struct v4l2_pix_format_mplane {
>   * @raw_data:	placeholder for future extensions and custom formats
>   */
>  struct v4l2_format {
> -	enum v4l2_buf_type type;
> +	unsigned type;
>  	union {
>  		struct v4l2_pix_format		pix;     /* V4L2_BUF_TYPE_VIDEO_CAPTURE */
>  		struct v4l2_pix_format_mplane	pix_mp;  /* V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE */
> @@ -2179,7 +2179,7 @@ struct v4l2_format {
>  /*	Stream type-dependent parameters
>   */
>  struct v4l2_streamparm {
> -	enum v4l2_buf_type type;
> +	unsigned type;
>  	union {
>  		struct v4l2_captureparm	capture;
>  		struct v4l2_outputparm	output;
> @@ -2299,7 +2299,7 @@ struct v4l2_dbg_chip_ident {
>  struct v4l2_create_buffers {
>  	__u32			index;
>  	__u32			count;
> -	enum v4l2_memory        memory;
> +	unsigned		memory;
>  	struct v4l2_format	format;
>  	__u32			reserved[8];
>  };


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] [PATCH] v4l2: use unsigned rather than enums in ioctl() structs
  2012-04-11 17:02 ` Mauro Carvalho Chehab
@ 2012-04-11 18:47   ` Rémi Denis-Courmont
  2012-04-11 19:53     ` Mauro Carvalho Chehab
                       ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Rémi Denis-Courmont @ 2012-04-11 18:47 UTC (permalink / raw)
  To: Mauro Carvalho Chehab; +Cc: mchehab, linux-media, linux-kernel, Alan Cox

	Hello,

Le mercredi 11 avril 2012 20:02:00 Mauro Carvalho Chehab, vous avez écrit :
> Using unsigned instead of enum is not a good idea, from API POV, as
> unsigned has different sizes on 32 bits and 64 bits.

Fair enough. But then we can do that instead:
typedef XXX __enum_t;
where XXX is the unsigned integer with the right number of bits. Since Linux 
does not use short enums, this ought to work fine.

> Yet, using enum was really a very bad idea, and, on all new stuff,
> we're not accepting any new enum field.

That is unfortunately not true. You do follow that rule for new fields to 
existing V4L2 structure. But you have been royally ignoring that rule when it 
comes to extending existing enumerations:

linux-media does regularly add new enum values to existing enums. That is new 
stuff too, and every single time you do that, you do BREAK THE USERSPACE ABI. 
This is entirely unacceptable and against established kernel development 
policies.

For instance, in Linux 3.1, V4L2_CTRL_TYPE_BITMASK was added. This broke 
userspace. And there are some pending patches adding more of the same thing... 
And V4L2_MEMORY_DMABUF will similarly break the user-to-kernel interface, 
which is yet worse.

> That's said, a patch like that were proposed in the past. See:
> 	http://www.spinics.net/lists/vfl/msg25686.html
> 
> Alan said there [1] that:
> 	"Its a fundamental change to a public structure from enum to u32. I think
> you are right but this change seems too late by several years."

I cannot see how the kernel is protected against userspace injecting error 
values into enums. For all I know, depending how the kernel is compiled, 
userspace might be able to trigger "undefined" behavior in kernel space.

Is it be several years too late to fix a bug?

(...)
> I don't think anything has changed since then that would make it easier
> to apply a change like that.

OK. But then you cannot update extend existing enums... No DMA buffers, no 
integer menu controls...

-- 
Rémi Denis-Courmont
http://www.remlab.net/
http://fi.linkedin.com/in/remidenis

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] [PATCH] v4l2: use unsigned rather than enums in ioctl() structs
  2012-04-11 18:47   ` Rémi Denis-Courmont
@ 2012-04-11 19:53     ` Mauro Carvalho Chehab
  2012-04-11 20:32       ` Rémi Denis-Courmont
  2012-04-11 20:08     ` Mauro Carvalho Chehab
  2012-04-12  8:04     ` James Courtier-Dutton
  2 siblings, 1 reply; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2012-04-11 19:53 UTC (permalink / raw)
  To: Rémi Denis-Courmont
  Cc: mchehab, linux-media, linux-kernel, Alan Cox, Hans Verkuil

Em 11-04-2012 15:47, Rémi Denis-Courmont escreveu:
> 	Hello,
> 
> Le mercredi 11 avril 2012 20:02:00 Mauro Carvalho Chehab, vous avez écrit :
>> Using unsigned instead of enum is not a good idea, from API POV, as
>> unsigned has different sizes on 32 bits and 64 bits.
> 
> Fair enough. But then we can do that instead:
> typedef XXX __enum_t;
> where XXX is the unsigned integer with the right number of bits. Since Linux 
> does not use short enums, this ought to work fine.
> 
>> Yet, using enum was really a very bad idea, and, on all new stuff,
>> we're not accepting any new enum field.
> 
> That is unfortunately not true. You do follow that rule for new fields to 
> existing V4L2 structure.

Yes, that's what I meant.

> But you have been royally ignoring that rule when it 
> comes to extending existing enumerations:

The existing enumerations can be extended, by adding new values for unused
values, otherwise API functionality can't be extended. Yet, except
for a gcc bug (or weird optimize option), I fail to see why this would break 
the ABI. 

If this is all about some gcc optimization with newer gcc versions, then maybe
the solution may be to add some pragmas at the code to disable such optimization
when compiling the structs with enum's at videodev2.h.

> linux-media does regularly add new enum values to existing enums. That is new 
> stuff too, and every single time you do that, you do BREAK THE USERSPACE ABI. 
> This is entirely unacceptable and against established kernel development 
> policies.
> 
> For instance, in Linux 3.1, V4L2_CTRL_TYPE_BITMASK was added. This broke 
> userspace.

The patch is:

commit fa4d7096d1fb7c012ebaacefee132007a21e0965
Author: Hans Verkuil <hans.verkuil@cisco.com>
Date:   Mon May 23 04:07:05 2011 -0300

    [media] v4l2-ctrls: add new bitmask control type
    
    Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
    Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
    Acked-by: Sakari Ailus <sakari.ailus@iki.fi>
    Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
...
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index f002006..148d1a5 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1040,6 +1040,7 @@ enum v4l2_ctrl_type {
        V4L2_CTRL_TYPE_INTEGER64     = 5,
        V4L2_CTRL_TYPE_CTRL_CLASS    = 6,
        V4L2_CTRL_TYPE_STRING        = 7,
+ V4L2_CTRL_TYPE_BITMASK       = 8,
 };
 
 /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */

This doesn't change the existing v4l2_ctrl_type codes. So, it shouldn't be
producing any harm at the existing code.

> And there are some pending patches adding more of the same thing... 
> And V4L2_MEMORY_DMABUF will similarly break the user-to-kernel interface, 
> which is yet worse.
> 
>> That's said, a patch like that were proposed in the past. See:
>> 	http://www.spinics.net/lists/vfl/msg25686.html
>>
>> Alan said there [1] that:
>> 	"Its a fundamental change to a public structure from enum to u32. I think
>> you are right but this change seems too late by several years."
> 
> I cannot see how the kernel is protected against userspace injecting error 
> values into enums. For all I know, depending how the kernel is compiled, 
> userspace might be able to trigger "undefined" behavior in kernel space.
> 
> Is it be several years too late to fix a bug?

No, but causing an ABI breakage like what it would happen by replacing
"enums" by u32 would break all binaries on x86_64 kernels (and vice-versa
if using u64, breaking all i386 binaries). 

>From what I remember from the 2006 discussions, even using "unsigned" may 
lead into breakages on some non-x86 architectures that use u16 for enums,
as, on those archs, unsigned is 32 bits.

> 
> (...)
>> I don't think anything has changed since then that would make it easier
>> to apply a change like that.
> 
> OK. But then you cannot update extend existing enums... No DMA buffers, no 
> integer menu controls...
> 

Regards,
Mauro

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [RFC] [PATCH] v4l2: use unsigned rather than enums in ioctl() structs
  2012-04-11 18:47   ` Rémi Denis-Courmont
  2012-04-11 19:53     ` Mauro Carvalho Chehab
@ 2012-04-11 20:08     ` Mauro Carvalho Chehab
  2012-04-12  8:04     ` James Courtier-Dutton
  2 siblings, 0 replies; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2012-04-11 20:08 UTC (permalink / raw)
  To: Rémi Denis-Courmont; +Cc: mchehab, linux-media, linux-kernel, Alan Cox

Em 11-04-2012 15:47, Rémi Denis-Courmont escreveu:
> 	Hello,
> 
> Le mercredi 11 avril 2012 20:02:00 Mauro Carvalho Chehab, vous avez écrit :
>> Using unsigned instead of enum is not a good idea, from API POV, as
>> unsigned has different sizes on 32 bits and 64 bits.
> 
> Fair enough. But then we can do that instead:
> typedef XXX __enum_t;
> where XXX is the unsigned integer with the right number of bits. Since Linux 
> does not use short enums, this ought to work fine.

I forgot to comment about that on the last e-mail. 

A solution close to the above one were already proposed:
	http://www.spinics.net/lists/vfl/msg25707.html

There were also another proposal there that might solve:
	http://www.spinics.net/lists/vfl/msg25702.html


Something like:

#if sizeof(enum) == 1
	typedef u8	__enum_t;
#elif sizeof(enum) == 2
	typedef u16	__enum_t;
#elif sizeof(enum) == 4
	typedef u32	__enum_t;
#elif sizeof(enum) == 8
	typedef u64	__enum_t;
#else
	typedef enum __enum_t;
#endif

Can actually work. Not sure if I really like adding a typedef, but maybe
this is the less dirty way to fix it.

We'll need to properly test the v4l2-compat32 code, as it will need 
to handle a different enum size on userspace. So, there, we'll likely
need to replace every enum with just "u32". Hmm... arm with 64 bits
(if/when added) may be an additional issue for the compat stuff.

Regards,
Mauro

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] [PATCH] v4l2: use unsigned rather than enums in ioctl() structs
  2012-04-11 19:53     ` Mauro Carvalho Chehab
@ 2012-04-11 20:32       ` Rémi Denis-Courmont
  2012-04-12 17:22         ` Nick Bowler
  0 siblings, 1 reply; 13+ messages in thread
From: Rémi Denis-Courmont @ 2012-04-11 20:32 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: mchehab, linux-media, linux-kernel, Alan Cox, Hans Verkuil

Le mercredi 11 avril 2012 22:53:23 Mauro Carvalho Chehab, vous avez écrit :
> > But you have been royally ignoring that rule when it
> > comes to extending existing enumerations:
> The existing enumerations can be extended, by adding new values for unused
> values, otherwise API functionality can't be extended.

Yes. That's why they should be some unsigned type.

> Yet, except
> for a gcc bug (or weird optimize option), I fail to see why this would
> break the ABI.

>From the perspective of the compiler, this is a feature not a bug. In C and 
C++, loading or storing a value in an enumerated variable whereby the value is 
not a member of the enumeration is undefined. In other words, the compiler can 
assume that this does not happen, and optimize it away.

> If this is all about some gcc optimization with newer gcc versions, then
> maybe the solution may be to add some pragmas at the code to disable such
> optimization when compiling the structs with enum's at videodev2.h.

Maybe the Linux kernel could be specifically compiled to prevent GCC from 
range-optimizing enumerations. But as -fno-jump-table only disables one of 
several potential range optimizations, I doubt this is even possible.

Regardless, you cannot require all of Linux userspace to rely on an hypothetic 
non-standard GNU C extension. Thus extending enums remains a silent userspace 
ABI break in any case.

-- 
Rémi Denis-Courmont
http://www.remlab.net/
http://fi.linkedin.com/in/remidenis

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] [PATCH] v4l2: use unsigned rather than enums in ioctl() structs
  2012-04-11 18:47   ` Rémi Denis-Courmont
  2012-04-11 19:53     ` Mauro Carvalho Chehab
  2012-04-11 20:08     ` Mauro Carvalho Chehab
@ 2012-04-12  8:04     ` James Courtier-Dutton
  2012-04-12 14:55       ` Mauro Carvalho Chehab
  2 siblings, 1 reply; 13+ messages in thread
From: James Courtier-Dutton @ 2012-04-12  8:04 UTC (permalink / raw)
  To: Rémi Denis-Courmont
  Cc: Mauro Carvalho Chehab, mchehab, linux-media, linux-kernel, Alan Cox

2012/4/11 Rémi Denis-Courmont <remi@remlab.net>:
>        Hello,
>
> Le mercredi 11 avril 2012 20:02:00 Mauro Carvalho Chehab, vous avez écrit :
>> Using unsigned instead of enum is not a good idea, from API POV, as
>> unsigned has different sizes on 32 bits and 64 bits.
>
> Fair enough. But then we can do that instead:
> typedef XXX __enum_t;
> where XXX is the unsigned integer with the right number of bits. Since Linux
> does not use short enums, this ought to work fine.
>
>> Yet, using enum was really a very bad idea, and, on all new stuff,
>> we're not accepting any new enum field.
>
> That is unfortunately not true. You do follow that rule for new fields to
> existing V4L2 structure. But you have been royally ignoring that rule when it
> comes to extending existing enumerations:
>
> linux-media does regularly add new enum values to existing enums. That is new
> stuff too, and every single time you do that, you do BREAK THE USERSPACE ABI.
> This is entirely unacceptable and against established kernel development
> policies.
>
> For instance, in Linux 3.1, V4L2_CTRL_TYPE_BITMASK was added. This broke
> userspace. And there are some pending patches adding more of the same thing...
> And V4L2_MEMORY_DMABUF will similarly break the user-to-kernel interface,
> which is yet worse.
>

I agree that breaking user-to-kernel interface is not advised.
We came across a similar problem some years ago with the ALSA sound
kernel drivers.
The solution we used was:
1) If a change is likely to change the user-to-kernel API, add a new
IOCTL for it.
Then old userland software can use the old IOCTL, and new userland
software can use the new IOCTL.
2) Add an version IOCTL that returns the current API level, so that
the app can be written to support more than one API interface,
depending on which kernel it is running on. The version IOCTL simply
returns an u32 value. This is a consistent part of the user-kernel API
that will never change.
3) Add "depreciated" compiler warnings to all the old API IOCTL calls,
so app developers know they should be working to update their apps.
4) After a few years, remove the old IOCTLs.
5) Use "uint32_t" and "uint64_t" types for all IOCTL calls, and not
"unsigned int" or "unsigned long int".
I.e. All structures passed in IOCTLs use fixed bit sized parameters
for everything except of course pointers. Pointers depend on
architecture.
6) Add a #if #endif around the old API, so a user compiling their own
kernel can decide if the old API exists or not. User might want to do
this for security reasons.

Kind Regards

James

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] [PATCH] v4l2: use unsigned rather than enums in ioctl() structs
  2012-04-12  8:04     ` James Courtier-Dutton
@ 2012-04-12 14:55       ` Mauro Carvalho Chehab
  2012-04-12 15:41         ` Rémi Denis-Courmont
  2012-04-13  8:25         ` [RFC] [PATCH] v4l2: use unsigned rather than enums in ioctl() structs James Courtier-Dutton
  0 siblings, 2 replies; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2012-04-12 14:55 UTC (permalink / raw)
  To: James Courtier-Dutton
  Cc: Rémi Denis-Courmont, mchehab, linux-media, linux-kernel, Alan Cox

Em 12-04-2012 05:04, James Courtier-Dutton escreveu:
> 2012/4/11 Rémi Denis-Courmont <remi@remlab.net>:
>>        Hello,
>>
>> Le mercredi 11 avril 2012 20:02:00 Mauro Carvalho Chehab, vous avez écrit :
>>> Using unsigned instead of enum is not a good idea, from API POV, as
>>> unsigned has different sizes on 32 bits and 64 bits.
>>
>> Fair enough. But then we can do that instead:
>> typedef XXX __enum_t;
>> where XXX is the unsigned integer with the right number of bits. Since Linux
>> does not use short enums, this ought to work fine.
>>
>>> Yet, using enum was really a very bad idea, and, on all new stuff,
>>> we're not accepting any new enum field.
>>
>> That is unfortunately not true. You do follow that rule for new fields to
>> existing V4L2 structure. But you have been royally ignoring that rule when it
>> comes to extending existing enumerations:
>>
>> linux-media does regularly add new enum values to existing enums. That is new
>> stuff too, and every single time you do that, you do BREAK THE USERSPACE ABI.
>> This is entirely unacceptable and against established kernel development
>> policies.
>>
>> For instance, in Linux 3.1, V4L2_CTRL_TYPE_BITMASK was added. This broke
>> userspace. And there are some pending patches adding more of the same thing...
>> And V4L2_MEMORY_DMABUF will similarly break the user-to-kernel interface,
>> which is yet worse.
>>
> 
> I agree that breaking user-to-kernel interface is not advised.
> We came across a similar problem some years ago with the ALSA sound
> kernel drivers.
> The solution we used was:
> 1) If a change is likely to change the user-to-kernel API, add a new
> IOCTL for it.
> Then old userland software can use the old IOCTL, and new userland
> software can use the new IOCTL.

V4L2 API has about 80 ioctl's. Add compat code for most of them (as most have
enum's) is not fun.

Also, the issue is not that trivial. Just to give you one example:

struct v4l2_pix_format {
        __u32                   width;
       	__u32                   height;
        __u32                   pixelformat;
        enum v4l2_field         field;
        __u32                   bytesperline;   /* for padding, zero if unused */
        __u32                   sizeimage;
        enum v4l2_colorspace    colorspace;
       	__u32                   priv;           /* private data, depends on pixelformat */
};


This struct has 2 enums, and it is used by a couple structs, like this one:

struct v4l2_framebuffer {
        __u32                   capability;
       	__u32                   flags;
        void                    *base;
       	struct v4l2_pix_format  fmt;
};

This struct is used by a couple ioctls:

#define VIDIOC_G_FBUF            _IOR('V', 10, struct v4l2_framebuffer)
#define VIDIOC_S_FBUF            _IOW('V', 11, struct v4l2_framebuffer)

The better is to really replace "enum" by an integer (__u32?) at the structs,
but this will break existing apps.

One alternative would be to fork this header and add a compat layer
that would print a WARN_ONCE message, if ever reached, asking the user
to re-compile the application against the new header. 

We did that strategy in the past, appending _OLD to the legacy api ioctl's.

> 2) Add an version IOCTL that returns the current API level, so that
> the app can be written to support more than one API interface,
> depending on which kernel it is running on. The version IOCTL simply
> returns an u32 value. This is a consistent part of the user-kernel API
> that will never change.

There's one ioctl that already provides the API level, plus other info.
This ioctl doesn't contain any enum, so it is backward compatible.

> 3) Add "depreciated" compiler warnings to all the old API IOCTL calls,
> so app developers know they should be working to update their apps.

The issue here is with binaries compiled against the old headers. So, this
won't work.

> 4) After a few years, remove the old IOCTLs.
> 5) Use "uint32_t" and "uint64_t" types for all IOCTL calls, and not
> "unsigned int" or "unsigned long int".

No ioctls (well, except for 2 deprecated ones VIDIOC_G_JPEGCOMP/VIDIOC_S_JPEGCOMP)
are using __u8/__u32/__u64 for integers. The only issue there is with enum's.

> I.e. All structures passed in IOCTLs use fixed bit sized parameters
> for everything except of course pointers. Pointers depend on
> architecture.
> 6) Add a #if #endif around the old API, so a user compiling their own
> kernel can decide if the old API exists or not. User might want to do
> this for security reasons.

Add an #if block there will make the header very hard to deal with, as this
is already complex enough without it. The V4L2 API header has 2420 lines.
Such #if blocks will almost duplicate the header size.

I can see only two viable fixes for it:

1) add a typedef for the enum, using the sizeof(enum) in order to select the
size of the used integer.

Pros:
	- Patch is easy to write/easy to review;
	- Won't change the struct size, so applications compiled without
	  strong gcc optimization won't break;
Cons:
	- It will add a typedef, with is ugly;
	- struct size on 32 bits will be different thant he size on 64 bits
	  (not really an issue, as v4l2-compat32 will handle that;
	- v4l2-compat32 code may require changes.

2) just replace it by a 32 bits integer.

Pros:
	- no typedefs;
	- struct size won't change between 32/64 bits (except when they also
	  have pointers);
Cons:
	- will break ABI. So, a compat code is required;
	- will require a "videodev2.h" fork for the legacy API with the enum's;
	- will require a compat code to convert from enum into integer and
	  vice-versa.

Comments/Votes?
Mauro

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] [PATCH] v4l2: use unsigned rather than enums in ioctl() structs
  2012-04-12 14:55       ` Mauro Carvalho Chehab
@ 2012-04-12 15:41         ` Rémi Denis-Courmont
  2012-04-17 17:50           ` Mauro Carvalho Chehab
  2012-04-13  8:25         ` [RFC] [PATCH] v4l2: use unsigned rather than enums in ioctl() structs James Courtier-Dutton
  1 sibling, 1 reply; 13+ messages in thread
From: Rémi Denis-Courmont @ 2012-04-12 15:41 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: James Courtier-Dutton, mchehab, linux-media, linux-kernel, Alan Cox

On Thu, 12 Apr 2012 11:55:12 -0300, Mauro Carvalho Chehab

<mchehab@redhat.com> wrote:

> I can see only two viable fixes for it:

> 

> 1) add a typedef for the enum, using the sizeof(enum) in order to select

> the

> size of the used integer.

> 

> Pros:

> 	- Patch is easy to write/easy to review;

> 	- Won't change the struct size, so applications compiled without

> 	  strong gcc optimization won't break;

> Cons:

> 	- It will add a typedef, with is ugly;

> 	- struct size on 32 bits will be different thant he size on 64 bits

> 	  (not really an issue, as v4l2-compat32 will handle that;



On which platforms do enums occupy 64-bits? Alpha? More to the point, on

which platform is enum not the same size as unsigned?



At least on x86-64, enum is 32-bits and so is unsigned.



> 	- v4l2-compat32 code may require changes.

> 

> 2) just replace it by a 32 bits integer.

> 

> Pros:

> 	- no typedefs;

> 	- struct size won't change between 32/64 bits (except when they also

> 	  have pointers);

> Cons:

> 	- will break ABI. So, a compat code is required;

> 	- will require a "videodev2.h" fork for the legacy API with the enum's;

> 	- will require a compat code to convert from enum into integer and

> 	  vice-versa.

> 

> Comments/Votes?



-- 

Rémi Denis-Courmont

Sent from my collocated server

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] [PATCH] v4l2: use unsigned rather than enums in ioctl() structs
  2012-04-11 20:32       ` Rémi Denis-Courmont
@ 2012-04-12 17:22         ` Nick Bowler
  0 siblings, 0 replies; 13+ messages in thread
From: Nick Bowler @ 2012-04-12 17:22 UTC (permalink / raw)
  To: Rémi Denis-Courmont
  Cc: Mauro Carvalho Chehab, mchehab, linux-media, linux-kernel,
	Alan Cox, Hans Verkuil

On 2012-04-11 23:32 +0300, Rémi Denis-Courmont wrote:
> From the perspective of the compiler, this is a feature not a bug. In
> C and C++, loading or storing a value in an enumerated variable
> whereby the value is not a member of the enumeration is undefined.

I'm afraid that this is not the case in C, although it may be in C++
(enums are very different in C++ than they are in C).  In C, enum types
are required to be compatible with some integer type capable of storing
the values of all the enum members (see C11§6.7.2.2#4).  Compatibility
is a very strong condition, and implies that the two types are
interchangable without affecting the meaning of the program (see
C11§6.2.7).  Integer types have a number of specific requirements, one
thing that's relevant here is that they do not have "holes" in their
representable values: there is a minimum and maximum representable
value, and all integers between them are representable (C11§6.2.6.2#1).

Thus, while the choice of integer type used may depend on the values of
the corresponding enum constants, storing any value (regardless of
whether or not its a member of the enumeration) is subject to the same
rules as the implementation-defined compatbile integer type.  This is
always well-defined for values within the range of the type.
(C11§6.3.1.3#1 and C11§6.3.1.4#1).

> In other words, the compiler can assume that this does not happen, and
> optimize it away.

No, a conforming C compiler cannot assume such assignments do not
happen, for the reasons outlined above.

Cheers,
-- 
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] [PATCH] v4l2: use unsigned rather than enums in ioctl() structs
  2012-04-12 14:55       ` Mauro Carvalho Chehab
  2012-04-12 15:41         ` Rémi Denis-Courmont
@ 2012-04-13  8:25         ` James Courtier-Dutton
  1 sibling, 0 replies; 13+ messages in thread
From: James Courtier-Dutton @ 2012-04-13  8:25 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Rémi Denis-Courmont, mchehab, linux-media, linux-kernel, Alan Cox

On 12 April 2012 15:55, Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
> Em 12-04-2012 05:04, James Courtier-Dutton escreveu:
>> 6) Add a #if #endif around the old API, so a user compiling their own
>> kernel can decide if the old API exists or not. User might want to do
>> this for security reasons.
>
> Add an #if block there will make the header very hard to deal with, as this
> is already complex enough without it. The V4L2 API header has 2420 lines.
> Such #if blocks will almost duplicate the header size.
>

But it will work.
If you change the kernel-user API, this is a necessary evil you really
just have to do.

For ALSA, we had a #define ALSA_API_5 and #define ALSA_API_9.
The application writer defined one of these before including the
header file, and this specified which API version the application
writer used. This handles the use from user space.

After about 2 years, you can remove the old API version, and the
header file is then cleaned up.

You need to think about it as an API change.
So, you are really going from V4L2 to V4L3.

The kernel side of things is a bit messier.
You have to use different IOCTLs for the old API than the new API for
every IOCTL that has a changed parameter passed to it.
We managed to avoid this particular nasty, because in ALSA we had
libasound. So, we implemented all the nasty stuff in libasound,
letting the kernel only have to implement one API, either the new or
the old. So long as you installed a new libasound, the old application
would stay working.

I don't think you have something like libasound for v4l2 that every
application is using, so I would probably go with implementing V4L3.
I.e. A brand new API for Video in Linux.
You could say the driver for moving from V4L2 to V4L3 would be
security due to problems with emums.
Note: You can still use enums in the header file, but just pass their
value over the api as an int and not an emun type.
See the /linux/include/sound/asound.h for an example.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC] [PATCH] v4l2: use unsigned rather than enums in ioctl() structs
  2012-04-12 15:41         ` Rémi Denis-Courmont
@ 2012-04-17 17:50           ` Mauro Carvalho Chehab
  2012-04-27  8:24             ` [RFC 1/1] v4l: Implement compat handlers for ioctls containing enums Sakari Ailus
  0 siblings, 1 reply; 13+ messages in thread
From: Mauro Carvalho Chehab @ 2012-04-17 17:50 UTC (permalink / raw)
  To: Rémi Denis-Courmont
  Cc: James Courtier-Dutton, mchehab, linux-media, linux-kernel, Alan Cox

On Thu, 12 Apr 2012 11:55:12 -0300, Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
> I can see only two viable fixes for it:
>
> 1) add a typedef for the enum, using the sizeof(enum) in order to select
> the
> size of the used integer.
>
> Pros:
> 	- Patch is easy to write/easy to review;
> 	- Won't change the struct size, so applications compiled without
> 	  strong gcc optimization won't break;
> Cons:
> 	- It will add a typedef, with is ugly;
> 	- struct size on 32 bits will be different thant he size on 64 bits
> 	  (not really an issue, as v4l2-compat32 will handle that;
> 	- v4l2-compat32 code may require changes.
>
> 2) just replace it by a 32 bits integer.
>
> Pros:
> 	- no typedefs;
> 	- struct size won't change between 32/64 bits (except when they also
> 	  have pointers);
> Cons:
> 	- will break ABI. So, a compat code is required;
> 	- will require a "videodev2.h" fork for the legacy API with the enum's;
> 	- will require a compat code to convert from enum into integer and
> 	  vice-versa.


After reflecting about that, I think that (2) is better at long term. Doing it
won't be easy, as there are 24 ioctl's affected by enum's (if I didn't forget
anything - please double check!).

We'll need to patch for videodev2.h, v4l2-ioctl.h and v4l2-compat-ioctl32.c,
in order to add the compatibility code for handling the old API calls.

If done well, this could be an opportunity to cleanup the code at v4l2-compat-ioctl32.c,
as, i thesis, only structures with pointers would need compat bits, but the
current implementation forces a compatibility code for everything.

Anyway, the first step is something like the patch bellow.

Comments are welcome.

Regards,
Mauro

[RFC] [media] videodev2: don't use enum's at the public API.

Enum size is not portable and g++ might try to do wrong optimizations
with it.

PS.: Patch is incomplete, as the compatibility bits aren't there. 

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

diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index e69cacc..37443a9 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -292,10 +292,10 @@ struct v4l2_pix_format {
 	__u32         		width;
 	__u32			height;
 	__u32			pixelformat;
-	enum v4l2_field  	field;
+	__u32		  	field;		/* enum v4l2_field */
 	__u32            	bytesperline;	/* for padding, zero if unused */
 	__u32          		sizeimage;
-	enum v4l2_colorspace	colorspace;
+	__u32			colorspace;	/* enum v4l2_colorspace */
 	__u32			priv;		/* private data, depends on pixelformat */
 };
 
@@ -432,7 +432,7 @@ struct v4l2_pix_format {
  */
 struct v4l2_fmtdesc {
 	__u32		    index;             /* Format number      */
-	enum v4l2_buf_type  type;              /* buffer type        */
+	__u32		  type;                /* buffer type (enum v4l2_buf_type) */
 	__u32               flags;
 	__u8		    description[32];   /* Description string */
 	__u32		    pixelformat;       /* Format fourcc      */
@@ -573,8 +573,8 @@ struct v4l2_jpegcompression {
  */
 struct v4l2_requestbuffers {
 	__u32			count;
-	enum v4l2_buf_type      type;
-	enum v4l2_memory        memory;
+	__u32		      type;		/* enum v4l2_buf_type */
+	__u32		        memory;		/* enum v4l2_memory */
 	__u32			reserved[2];
 };
 
@@ -636,16 +636,16 @@ struct v4l2_plane {
  */
 struct v4l2_buffer {
 	__u32			index;
-	enum v4l2_buf_type      type;
+	__u32			type;		/* enum v4l2_buf_type */
 	__u32			bytesused;
 	__u32			flags;
-	enum v4l2_field		field;
+	__u32			field;		/* enum v4l2_field */
 	struct timeval		timestamp;
 	struct v4l2_timecode	timecode;
 	__u32			sequence;
 
 	/* memory location */
-	enum v4l2_memory        memory;
+	__u32		        memory;		/* enum v4l2_memory */
 	union {
 		__u32           offset;
 		unsigned long   userptr;
@@ -708,7 +708,7 @@ struct v4l2_clip {
 
 struct v4l2_window {
 	struct v4l2_rect        w;
-	enum v4l2_field  	field;
+	__u32			field;		/* enum v4l2_field */
 	__u32			chromakey;
 	struct v4l2_clip	__user *clips;
 	__u32			clipcount;
@@ -745,14 +745,14 @@ struct v4l2_outputparm {
  *	I N P U T   I M A G E   C R O P P I N G
  */
 struct v4l2_cropcap {
-	enum v4l2_buf_type      type;
+	__u32			type;		/* enum v4l2_buf_type */
 	struct v4l2_rect        bounds;
 	struct v4l2_rect        defrect;
 	struct v4l2_fract       pixelaspect;
 };
 
 struct v4l2_crop {
-	enum v4l2_buf_type      type;
+	__u32			type;		/* enum v4l2_buf_type */
 	struct v4l2_rect        c;
 };
 
@@ -1157,7 +1157,7 @@ enum v4l2_ctrl_type {
 /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
 struct v4l2_queryctrl {
 	__u32		     id;
-	enum v4l2_ctrl_type  type;
+	__u32		     type;	/* enum v4l2_ctrl_type */
 	__u8		     name[32];	/* Whatever */
 	__s32		     minimum;	/* Note signedness */
 	__s32		     maximum;
@@ -1792,7 +1792,7 @@ enum v4l2_jpeg_chroma_subsampling {
 struct v4l2_tuner {
 	__u32                   index;
 	__u8			name[32];
-	enum v4l2_tuner_type    type;
+	__u32			type;		/* enum v4l2_tuner_type */
 	__u32			capability;
 	__u32			rangelow;
 	__u32			rangehigh;
@@ -1842,14 +1842,14 @@ struct v4l2_modulator {
 
 struct v4l2_frequency {
 	__u32		      tuner;
-	enum v4l2_tuner_type  type;
+	__u32		      type;		/* enum v4l2_tuner_type */
 	__u32		      frequency;
 	__u32		      reserved[8];
 };
 
 struct v4l2_hw_freq_seek {
 	__u32		      tuner;
-	enum v4l2_tuner_type  type;
+	__u32		      type;		/* enum v4l2_tuner_type */
 	__u32		      seek_upward;
 	__u32		      wrap_around;
 	__u32		      spacing;
@@ -2060,7 +2060,7 @@ struct v4l2_sliced_vbi_cap {
 				 (equals frame lines 313-336 for 625 line video
 				  standards, 263-286 for 525 line standards) */
 	__u16   service_lines[2][24];
-	enum v4l2_buf_type type;
+	__u32	 type;		/* enum v4l2_buf_type */
 	__u32   reserved[3];    /* must be 0 */
 };
 
@@ -2150,8 +2150,8 @@ struct v4l2_pix_format_mplane {
 	__u32				width;
 	__u32				height;
 	__u32				pixelformat;
-	enum v4l2_field			field;
-	enum v4l2_colorspace		colorspace;
+	__u32				field;		/* enum v4l2_field */
+	__u32				colorspace;	/* enum v4l2_colorspace */
 
 	struct v4l2_plane_pix_format	plane_fmt[VIDEO_MAX_PLANES];
 	__u8				num_planes;
@@ -2169,7 +2169,7 @@ struct v4l2_pix_format_mplane {
  * @raw_data:	placeholder for future extensions and custom formats
  */
 struct v4l2_format {
-	enum v4l2_buf_type type;
+	__u32	type;		/* enum v4l2_buf_type */
 	union {
 		struct v4l2_pix_format		pix;     /* V4L2_BUF_TYPE_VIDEO_CAPTURE */
 		struct v4l2_pix_format_mplane	pix_mp;  /* V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE */
@@ -2183,7 +2183,7 @@ struct v4l2_format {
 /*	Stream type-dependent parameters
  */
 struct v4l2_streamparm {
-	enum v4l2_buf_type type;
+	__u32	type;		/* enum v4l2_buf_type */
 	union {
 		struct v4l2_captureparm	capture;
 		struct v4l2_outputparm	output;
@@ -2303,7 +2303,7 @@ struct v4l2_dbg_chip_ident {
 struct v4l2_create_buffers {
 	__u32			index;
 	__u32			count;
-	enum v4l2_memory        memory;
+	__u32		        memory;		/* enum v4l2_memory */
 	struct v4l2_format	format;
 	__u32			reserved[8];
 };
@@ -2417,4 +2417,213 @@ struct v4l2_create_buffers {
 
 #define BASE_VIDIOC_PRIVATE	192		/* 192-255 are private */
 
+/*
+ * Backward-compatible IOCTL's to be used by V4L2 core to work with the
+ * old ioctl's defined with "enum's" inside the structures
+ */
+
+#ifdef CONFIG_V4L2_COMPAT
+
+struct v4l2_pix_format_enum {
+	__u32         		width;
+	__u32			height;
+	__u32			pixelformat;
+	enum v4l2_field  	field;
+	__u32            	bytesperline;	/* for padding, zero if unused */
+	__u32          		sizeimage;
+	enum v4l2_colorspace	colorspace;
+	__u32			priv;		/* private data, depends on pixelformat */
+};
+
+struct v4l2_fmtdesc_enum {
+	__u32		    index;             /* Format number      */
+	enum v4l2_buf_type  type;              /* buffer type        */
+	__u32               flags;
+	__u8		    description[32];   /* Description string */
+	__u32		    pixelformat;       /* Format fourcc      */
+	__u32		    reserved[4];
+};
+
+struct v4l2_requestbuffers_enum {
+	__u32			count;
+	enum v4l2_buf_type      type;
+	enum v4l2_memory        memory;
+	__u32			reserved[2];
+};
+
+struct v4l2_buffer_enum {
+	__u32			index;
+	enum v4l2_buf_type      type;
+	__u32			bytesused;
+	__u32			flags;
+	enum v4l2_field		field;
+	struct timeval		timestamp;
+	struct v4l2_timecode	timecode;
+	__u32			sequence;
+
+	/* memory location */
+	enum v4l2_memory        memory;
+	union {
+		__u32           offset;
+		unsigned long   userptr;
+		struct v4l2_plane *planes;
+	} m;
+	__u32			length;
+	__u32			input;
+	__u32			reserved;
+};
+
+struct v4l2_framebuffer_enum {
+	__u32			capability;
+	__u32			flags;
+/* FIXME: in theory we should pass something like PCI device + memory
+ * region + offset instead of some physical address */
+	void                    *base;
+	struct v4l2_pix_format_enum	fmt;
+};
+
+struct v4l2_window_enum {
+	struct v4l2_rect        w;
+	enum v4l2_field  	field;
+	__u32			chromakey;
+	struct v4l2_clip	__user *clips;
+	__u32			clipcount;
+	void			__user *bitmap;
+	__u8                    global_alpha;
+};
+
+struct v4l2_crop_enumcap_enum {
+	enum v4l2_buf_type      type;
+	struct v4l2_rect        bounds;
+	struct v4l2_rect        defrect;
+	struct v4l2_fract       pixelaspect;
+};
+
+struct v4l2_crop_enum {
+	enum v4l2_buf_type      type;
+	struct v4l2_rect        c;
+};
+
+struct v4l2_queryctrl_enum {
+	__u32		     id;
+	enum v4l2_ctrl_type  type;
+	__u8		     name[32];	/* Whatever */
+	__s32		     minimum;	/* Note signedness */
+	__s32		     maximum;
+	__s32		     step;
+	__s32		     default_value;
+	__u32                flags;
+	__u32		     reserved[2];
+};
+
+struct v4l2_tuner_enum {
+	__u32                   index;
+	__u8			name[32];
+	enum v4l2_tuner_type    type;
+	__u32			capability;
+	__u32			rangelow;
+	__u32			rangehigh;
+	__u32			rxsubchans;
+	__u32			audmode;
+	__s32			signal;
+	__s32			afc;
+	__u32			reserved[4];
+};
+
+struct v4l2_frequency_enum {
+	__u32		      tuner;
+	enum v4l2_tuner_type  type;
+	__u32		      frequency;
+	__u32		      reserved[8];
+};
+
+struct v4l2_hw_freq_seek_enum {
+	__u32		      tuner;
+	enum v4l2_tuner_type  type;
+	__u32		      seek_upward;
+	__u32		      wrap_around;
+	__u32		      spacing;
+	__u32		      reserved[7];
+};
+
+struct v4l2_sliced_vbi_cap_enum {
+	__u16   service_set;
+	/* service_lines[0][...] specifies lines 0-23 (1-23 used) of the first field
+	   service_lines[1][...] specifies lines 0-23 (1-23 used) of the second field
+				 (equals frame lines 313-336 for 625 line video
+				  standards, 263-286 for 525 line standards) */
+	__u16   service_lines[2][24];
+	enum v4l2_buf_type type;
+	__u32   reserved[3];    /* must be 0 */
+};
+
+struct v4l2_pix_format_mplane_enum {
+	__u32				width;
+	__u32				height;
+	__u32				pixelformat;
+	enum v4l2_field			field;
+	enum v4l2_colorspace		colorspace;
+
+	struct v4l2_plane_pix_format	plane_fmt[VIDEO_MAX_PLANES];
+	__u8				num_planes;
+	__u8				reserved[11];
+} __attribute__ ((packed));
+
+struct v4l2_format_enum {
+	enum v4l2_buf_type type;
+	union {
+		struct v4l2_pix_format_enum		pix;     /* V4L2_BUF_TYPE_VIDEO_CAPTURE */
+		struct v4l2_pix_format_mplane_enum	pix_mp;  /* V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE */
+		struct v4l2_window_enum		win;     /* V4L2_BUF_TYPE_VIDEO_OVERLAY */
+		struct v4l2_vbi_format		vbi;     /* V4L2_BUF_TYPE_VBI_CAPTURE */
+		struct v4l2_sliced_vbi_format	sliced;  /* V4L2_BUF_TYPE_SLICED_VBI_CAPTURE */
+		__u8	raw_data[200];                   /* user-defined */
+	} fmt;
+};
+
+/*	Stream type-dependent parameters
+ */
+struct v4l2_streamparm_enum {
+	enum v4l2_buf_type type;
+	union {
+		struct v4l2_captureparm	capture;
+		struct v4l2_outputparm	output;
+		__u8	raw_data[200];  /* user-defined */
+	} parm;
+};
+
+struct v4l2_create_buffers_enum {
+	__u32			index;
+	__u32			count;
+	enum v4l2_memory        memory;
+	struct v4l2_format_enum	format;
+	__u32			reserved[8];
+};
+
+#define VIDIOC_ENUM_FMT_ENUM     	_IOWR('V',  2, struct v4l2_fmtdesc_enum)
+#define VIDIOC_G_FMT_ENUM		_IOWR('V',  4, struct v4l2_format_enum)
+#define VIDIOC_S_FMT_ENUM		_IOWR('V',  5, struct v4l2_format_enum)
+#define VIDIOC_REQBUFS_ENUM		_IOWR('V',  8, struct v4l2_requestbuffers_enum)
+#define VIDIOC_QUERYBUF_ENUM		_IOWR('V',  9, struct v4l2_buffer_enum)
+#define VIDIOC_G_FBUF_ENUM		_IOR('V', 10, struct v4l2_framebuffer_enum)
+#define VIDIOC_S_FBUF_ENUM		_IOW('V', 11, struct v4l2_framebuffer_enum)
+#define VIDIOC_QBUF_ENUM		_IOWR('V', 15, struct v4l2_buffer_enum)
+#define VIDIOC_DQBUF_ENUM		_IOWR('V', 17, struct v4l2_buffer_enum)
+#define VIDIOC_G_PARM_ENUM		_IOWR('V', 21, struct v4l2_streamparm_enum)
+#define VIDIOC_S_PARM_ENUM		_IOWR('V', 22, struct v4l2_streamparm_enum)
+#define VIDIOC_G_TUNER_ENUM		_IOWR('V', 29, struct v4l2_tuner_enum)
+#define VIDIOC_S_TUNER_ENUM		_IOW('V', 30, struct v4l2_tuner_enum)
+#define VIDIOC_QUERYCTRL_ENUM		_IOWR('V', 36, struct v4l2_queryctrl_enum)
+#define VIDIOC_G_FREQUENCY_ENUM		_IOWR('V', 56, struct v4l2_frequency_enum)
+#define VIDIOC_S_FREQUENCY_ENUM		_IOW('V', 57, struct v4l2_frequency_enum)
+#define VIDIOC_CROPCAP_ENUM		_IOWR('V', 58, struct v4l2_crop_enumcap_enum)
+#define VIDIOC_G_CROP_ENUM		_IOWR('V', 59, struct v4l2_crop_enum)
+#define VIDIOC_S_CROP_ENUM		_IOW('V', 60, struct v4l2_crop_enum)
+#define VIDIOC_TRY_FMT_ENUM      	_IOWR('V', 64, struct v4l2_format_enum)
+#define VIDIOC_G_SLICED_VBI_CAP_ENUM	_IOWR('V', 69, struct v4l2_sliced_vbi_cap_enum)
+#define VIDIOC_S_HW_FREQ_SEEK_ENUM	_IOW('V', 82, struct v4l2_hw_freq_seek_enum)
+#define VIDIOC_CREATE_BUFS_ENUM		_IOWR('V', 92, struct v4l2_create_buffers_enum)
+#define VIDIOC_PREPARE_BUF_ENUM		_IOWR('V', 93, struct v4l2_buffer_enum)
+#endif /* V4L2_COMPAT */
+
 #endif /* __LINUX_VIDEODEV2_H */



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [RFC 1/1] v4l: Implement compat handlers for ioctls containing enums
  2012-04-17 17:50           ` Mauro Carvalho Chehab
@ 2012-04-27  8:24             ` Sakari Ailus
  0 siblings, 0 replies; 13+ messages in thread
From: Sakari Ailus @ 2012-04-27  8:24 UTC (permalink / raw)
  To: mchehab
  Cc: linux-media, laurent.pinchart, remi, james.dutton, alan, linux-kernel

Quite a few V4L2 IOCTLs contained enum types in IOCTL definitions which are
considered bad. To get rid of these types, the enum types are replaced with
integer types with fixed size. This causes the actual IOCTL commands to
change, which requires special handling during the transition period to new
IOCTL commands.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
Hi all,

I'm sending this as RFC and this what it really means: I haven't tested the
patch, not even compiled it. What I'd like to ask is how do you like the
approach. Handling for only one IOCTL is implemented.

The compat IOCTLs are recognised and special handling for them is performed
in place of copy_from_user() and copy_to_user(). I do not handle array
IOCTLs yet.

I thought of the option of copying everything to kernel space first and then
performing the conversion there, but doing it at the same time does not seem
to cause much additional complications. The expense is likely more CPU time
usage but less stack usage / memory allocation which also can consume
noteworthy amounts of CPU time.

This patch goes on top of Mauro's earlier patch.

 drivers/media/video/v4l2-ioctl.c |  207 ++++++++++++++++++++++++++++++++++++--
 1 files changed, 198 insertions(+), 9 deletions(-)

diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index 5b2ec1f..cb2ed57 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -2303,6 +2303,177 @@ static long __video_do_ioctl(struct file *file,
 	return ret;
 }
 
+static long copy_compat_from_user(unsigned int cmd, void *parg,
+				  void __user *arg)
+{
+	union {
+		struct v4l2_fmtdesc_enum *fmtdesc;
+		struct v4l2_format_enum *fmt;
+		struct v4l2_requestbuffers_enum *reqbufs;
+		struct v4l2_framebuffer_enum *fb;
+		struct v4l2_buffer_enum *buf;
+		struct v4l2_streamparm_enum *sparm;
+		struct v4l2_tuner_enum *tuner;
+		struct v4l2_queryctrl_enum *qc;
+		struct v4l2_frequency_enum *freq;
+		struct v4l2_crop_enumcap_enum *cropcap;
+		struct v4l2_crop_enum *crop;
+		struct v4l2_sliced_vbi_cap_enum *vbi_cap;
+		struct v4l2_hw_freq_seek_enum *hw_freq_seek;
+		struct v4l2_create_buffers_enum *create_bufs;
+	} __user cu = arg;
+	union {
+		struct v4l2_fmtdesc fmtdesc;
+		struct v4l2_format fmt;
+		struct v4l2_requestbuffers reqbufs;
+		struct v4l2_framebuffer fb;
+		struct v4l2_buffer buf;
+		struct v4l2_streamparm sparm;
+		struct v4l2_tuner tuner;
+		struct v4l2_queryctrl qc;
+		struct v4l2_frequency freq;
+		struct v4l2_crop_enumcap cropcap;
+		struct v4l2_crop crop;
+		struct v4l2_sliced_vbi_cap vbi_cap;
+		struct v4l2_hw_freq_seek hw_freq_seek;
+		struct v4l2_create_buffers create_bufs;
+	} k = parg;
+
+	switch (cmd) {
+	case VIDIOC_ENUM_FMT_ENUM:
+		if (!access_ok(VERIFY_READ, cu, sizeof(*cu->fmtdesc))
+		    || get_user(k->fmtdesc.index, cu->fmtdesc->index)
+		    || get_user(k->fmtdesc.type, cu->fmtdesc->type)
+		    || get_user(k->fmtdesc.flags, cu->fmtdesc->flags)
+		    || copy_from_user(k->fmtdesc.description,
+				      cu->fmtdesc->description,
+				      sizeof(k->fmtdesc.description))
+		    || get_user(k->fmtdesc.pixelformat,
+				u->fmtdesc->pixelformat)
+		    || copy_from_user(k->fmtdesc.reserved,
+				      cu->fmtdesc->reserved,
+				      sizeof(k->fmtdesc.reserved)))
+			return -EFAULT;
+		return 0;
+	default:
+		WARN();
+		return -EINVAL;
+	}
+}
+
+static long copy_compat_to_user(unsigned int cmd, void __user *arg,
+				void *parg)
+{
+	union {
+		struct v4l2_fmtdesc_enum *fmtdesc;
+		struct v4l2_format_enum *fmt;
+		struct v4l2_requestbuffers_enum *reqbufs;
+		struct v4l2_framebuffer_enum *fb;
+		struct v4l2_buffer_enum *buf;
+		struct v4l2_streamparm_enum *sparm;
+		struct v4l2_tuner_enum *tuner;
+		struct v4l2_queryctrl_enum *qc;
+		struct v4l2_frequency_enum *freq;
+		struct v4l2_crop_enumcap_enum *cropcap;
+		struct v4l2_crop_enum *crop;
+		struct v4l2_sliced_vbi_cap_enum *vbi_cap;
+		struct v4l2_hw_freq_seek_enum *hw_freq_seek;
+		struct v4l2_create_buffers_enum *create_bufs;
+	} __user cu = arg;
+	union {
+		struct v4l2_fmtdesc fmtdesc;
+		struct v4l2_format fmt;
+		struct v4l2_requestbuffers reqbufs;
+		struct v4l2_framebuffer fb;
+		struct v4l2_buffer buf;
+		struct v4l2_streamparm sparm;
+		struct v4l2_tuner tuner;
+		struct v4l2_queryctrl qc;
+		struct v4l2_frequency freq;
+		struct v4l2_crop_enumcap cropcap;
+		struct v4l2_crop crop;
+		struct v4l2_sliced_vbi_cap vbi_cap;
+		struct v4l2_hw_freq_seek hw_freq_seek;
+		struct v4l2_create_buffers create_bufs;
+	} k = parg;
+
+	switch (cmd) {
+	case VIDIOC_ENUM_FMT_ENUM:
+		if (!access_ok(VERIFY_WRITE, cu, sizeof(*cu->fmtdesc))
+		    || put_user(cu->fmtdesc->index, k->fmtdesc.index)
+		    || put_user(cu->fmtdesc->type, k->fmtdesc.type)
+		    || put_user(cu->fmtdesc->flags, k->fmtdesc.flags)
+		    || copy_to_user(cu->fmtdesc->description,
+				    k->fmtdesc.description,
+				    sizeof(k->fmtdesc.description))
+		    || put_user(cu->fmtdesc->pixelformat,
+				k->fmtdesc.pixelformat)
+		    || copy_to_user(cu->fmtdesc->reserved, k->fmtdesc.reserved,
+				    sizeof(k->fmtdesc.reserved)))
+			return -EFAULT;
+		return 0;
+	default:
+		WARN();
+		return -EINVAL;
+	}
+}
+
+static unsigned int get_non_compat_cmd(unsigned int cmd)
+{
+	switch (cmd) {
+	case VIDIOC_ENUM_FMT_ENUM:
+		return VIDIOC_ENUM_FMT;
+	case VIDIOC_G_FMT_ENUM:
+		return VIDIOC_G_FMT;
+	case VIDIOC_S_FMT_ENUM:
+		return VIDIOC_S_FMT;
+	case VIDIOC_REQBUFS_ENUM:
+		return VIDIOC_REQBUFS;
+	case VIDIOC_QUERYBUF_ENUM:
+		return VIDIOC_QUERYBUF;
+	case VIDIOC_G_FBUF_ENUM:
+		return VIDIOC_G_FBUF;
+	case VIDIOC_S_FBUF_ENUM:
+		return VIDIOC_S_FBUF;
+	case VIDIOC_QBUF_ENUM:
+		return VIDIOC_QBUF;
+	case VIDIOC_DQBUF_ENUM:
+		return VIDIOC_DQBUF;
+	case VIDIOC_G_PARM_ENUM:
+		return VIDIOC_G_PARM;
+	case VIDIOC_S_PARM_ENUM:
+		return VIDIOC_S_PARM;
+	case VIDIOC_G_TUNER_ENUM:
+		return VIDIOC_G_TUNER;
+	case VIDIOC_S_TUNER_ENUM:
+		return VIDIOC_S_TUNER;
+	case VIDIOC_QUERYCTRL_ENUM:
+		return VIDIOC_QUERYCTRL;
+	case VIDIOC_G_FREQUENCY_ENUM:
+		return VIDIOC_G_FREQUENCY;
+	case VIDIOC_S_FREQUENCY_ENUM:
+		return VIDIOC_S_FREQUENCY;
+	case VIDIOC_CROPCAP_ENUM:
+		return VIDIOC_CROPCAP;
+	case VIDIOC_G_CROP_ENUM:
+		return VIDIOC_G_CROP;
+	case VIDIOC_S_CROP_ENUM:
+		return VIDIOC_S_CROP;
+	case VIDIOC_TRY_FMT_ENUM:
+		return VIDIOC_TRY_FMT;
+	case VIDIOC_G_SLICED_VBI_CAP_ENUM:
+		return VIDIOC_G_SLICED_VBI_CAP;
+	case VIDIOC_S_HW_FREQ_SEEK_ENUM:
+		return VIDIOC_S_HW_FREQ_SEEK;
+	case VIDIOC_CREATE_BUFS_ENUM:
+		return VIDIOC_CREATE_BUFS;
+	case VIDIOC_PREPARE_BUF_ENUM:
+		return VIDIOC_PREPARE_BUF;
+	default:
+		return cmd;
+	}
+}
+
 /* In some cases, only a few fields are used as input, i.e. when the app sets
  * "index" and then the driver fills in the rest of the structure for the thing
  * with that index.  We only need to copy up the first non-input field.  */
@@ -2390,7 +2561,7 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size,
 }
 
 long
-video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
+video_usercopy(struct file *file, unsigned int compat_cmd, unsigned long arg,
 	       v4l2_kioctl func)
 {
 	char	sbuf[128];
@@ -2401,6 +2572,7 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
 	size_t  array_size = 0;
 	void __user *user_ptr = NULL;
 	void	**kernel_ptr = NULL;
+	unsigned int cmd = get_non_compat_cmd(cmd);
 
 	/*  Copy arguments into temp kernel buffer  */
 	if (_IOC_DIR(cmd) != _IOC_NONE) {
@@ -2418,12 +2590,23 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg,
 		if (_IOC_DIR(cmd) & _IOC_WRITE) {
 			unsigned long n = cmd_input_size(cmd);
 
-			if (copy_from_user(parg, (void __user *)arg, n))
-				goto out;
-
-			/* zero out anything we don't copy from userspace */
-			if (n < _IOC_SIZE(cmd))
-				memset((u8 *)parg + n, 0, _IOC_SIZE(cmd) - n);
+			if (cmd == compat_cmd) {
+				if (copy_from_user(
+					    parg, (void __user *)arg, n))
+					goto out;
+
+				/*
+				 * zero out anything we don't copy
+				 * from userspace
+				 */
+				if (n < _IOC_SIZE(cmd))
+					memset((u8 *)parg + n, 0,
+					       _IOC_SIZE(cmd) - n);
+			} else {
+				if (copy_compat_from_user(cmd, parg,
+							  (void __user *)arg))
+					goto out;
+			}
 		} else {
 			/* read-only ioctl */
 			memset(parg, 0, _IOC_SIZE(cmd));
@@ -2471,8 +2654,14 @@ out_array_args:
 	switch (_IOC_DIR(cmd)) {
 	case _IOC_READ:
 	case (_IOC_WRITE | _IOC_READ):
-		if (copy_to_user((void __user *)arg, parg, _IOC_SIZE(cmd)))
-			err = -EFAULT;
+		if (cmd == compat_cmd) {
+			if (copy_to_user((void __user *)arg, parg,
+					 _IOC_SIZE(cmd)))
+				err = -EFAULT;
+		} else {
+			if (copy_compat_to_user(cmd, arg, parg))
+				err = -EFAULT;
+		}
 		break;
 	}
 
-- 
1.7.2.5


^ permalink raw reply related	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2012-04-27  8:34 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-05 17:52 [RFC] [PATCH] v4l2: use unsigned rather than enums in ioctl() structs Rémi Denis-Courmont
2012-04-11 17:02 ` Mauro Carvalho Chehab
2012-04-11 18:47   ` Rémi Denis-Courmont
2012-04-11 19:53     ` Mauro Carvalho Chehab
2012-04-11 20:32       ` Rémi Denis-Courmont
2012-04-12 17:22         ` Nick Bowler
2012-04-11 20:08     ` Mauro Carvalho Chehab
2012-04-12  8:04     ` James Courtier-Dutton
2012-04-12 14:55       ` Mauro Carvalho Chehab
2012-04-12 15:41         ` Rémi Denis-Courmont
2012-04-17 17:50           ` Mauro Carvalho Chehab
2012-04-27  8:24             ` [RFC 1/1] v4l: Implement compat handlers for ioctls containing enums Sakari Ailus
2012-04-13  8:25         ` [RFC] [PATCH] v4l2: use unsigned rather than enums in ioctl() structs James Courtier-Dutton

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.