* [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 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 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-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 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-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
* 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
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.