diff mbox series

[v2] xen/public: fix flexible array definitions

Message ID 20231130092112.18118-1-jgross@suse.com (mailing list archive)
State Superseded
Headers show
Series [v2] xen/public: fix flexible array definitions | expand

Commit Message

Jürgen Groß Nov. 30, 2023, 9:21 a.m. UTC
Flexible arrays in public headers can be problematic with some
compilers.

With XEN_FLEX_ARRAY_DIM there is a mechanism available to deal with
this issue, but care must be taken to not change the affected structs
in an incompatible way.

So bump __XEN_LATEST_INTERFACE_VERSION__ and introduce a new macro
XENPV_FLEX_ARRAY_DIM which will be XENPV_FLEX_ARRAY_DIM with the
interface version being new enough and "1" (the value used today in
the affected headers) when the interface version is an old one.

Replace the arr[1] instances (this includes the ones seen to be
problematic in recent Linux kernels [1]) with arr[XENPV_FLEX_ARRAY_DIM]
in order to avoid compilation errors.

[1]: https://bugzilla.kernel.org/show_bug.cgi?id=217693

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- bump interface version and make change only for new version
  (Jan Beulich)
---
 xen/include/public/io/cameraif.h | 2 +-
 xen/include/public/io/displif.h  | 2 +-
 xen/include/public/io/fsif.h     | 4 ++--
 xen/include/public/io/pvcalls.h  | 2 +-
 xen/include/public/io/ring.h     | 5 +++--
 xen/include/public/io/sndif.h    | 2 +-
 xen/include/public/xen-compat.h  | 2 +-
 xen/include/public/xen.h         | 6 ++++++
 8 files changed, 16 insertions(+), 9 deletions(-)

Comments

Julien Grall Nov. 30, 2023, 11:33 a.m. UTC | #1
Hi Juergen,

On 30/11/2023 09:21, Juergen Gross wrote:
> ---
> V2:
> - bump interface version and make change only for new version
>    (Jan Beulich)
> ---
>   xen/include/public/io/cameraif.h | 2 +-
>   xen/include/public/io/displif.h  | 2 +-
>   xen/include/public/io/fsif.h     | 4 ++--
>   xen/include/public/io/pvcalls.h  | 2 +-
>   xen/include/public/io/ring.h     | 5 +++--
>   xen/include/public/io/sndif.h    | 2 +-
>   xen/include/public/xen-compat.h  | 2 +-
>   xen/include/public/xen.h         | 6 ++++++
>   8 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/include/public/io/cameraif.h b/xen/include/public/io/cameraif.h
> index 13763abef9..a389443769 100644
> --- a/xen/include/public/io/cameraif.h
> +++ b/xen/include/public/io/cameraif.h
> @@ -763,7 +763,7 @@ struct xencamera_buf_create_req {
>    */
>   struct xencamera_page_directory {
>       grant_ref_t gref_dir_next_page;
> -    grant_ref_t gref[1]; /* Variable length */
> +    grant_ref_t gref[XENPV_FLEX_ARRAY_DIM];
>   };
>   
>   /*
> diff --git a/xen/include/public/io/displif.h b/xen/include/public/io/displif.h
> index 73d0cbdf15..132c96fa5c 100644
> --- a/xen/include/public/io/displif.h
> +++ b/xen/include/public/io/displif.h
> @@ -537,7 +537,7 @@ struct xendispl_dbuf_create_req {
>   
>   struct xendispl_page_directory {
>       grant_ref_t gref_dir_next_page;
> -    grant_ref_t gref[1]; /* Variable length */
> +    grant_ref_t gref[XENPV_FLEX_ARRAY_DIM];
>   };
>   
>   /*
> diff --git a/xen/include/public/io/fsif.h b/xen/include/public/io/fsif.h
> index ec57850233..dcade1c698 100644
> --- a/xen/include/public/io/fsif.h
> +++ b/xen/include/public/io/fsif.h
> @@ -40,7 +40,7 @@ struct fsif_read_request {
>       int32_t pad;
>       uint64_t len;
>       uint64_t offset;
> -    grant_ref_t grefs[1];  /* Variable length */
> +    grant_ref_t grefs[XENPV_FLEX_ARRAY_DIM];
>   };
>   
>   struct fsif_write_request {
> @@ -48,7 +48,7 @@ struct fsif_write_request {
>       int32_t pad;
>       uint64_t len;
>       uint64_t offset;
> -    grant_ref_t grefs[1];  /* Variable length */
> +    grant_ref_t grefs[XENPV_FLEX_ARRAY_DIM];
>   };
>   
>   struct fsif_stat_request {
> diff --git a/xen/include/public/io/pvcalls.h b/xen/include/public/io/pvcalls.h
> index 230b0719e3..af0e9abd13 100644
> --- a/xen/include/public/io/pvcalls.h
> +++ b/xen/include/public/io/pvcalls.h
> @@ -30,7 +30,7 @@ struct pvcalls_data_intf {
>       uint8_t pad2[52];
>   
>       RING_IDX ring_order;
> -    grant_ref_t ref[];
> +    grant_ref_t ref[XENPV_FLEX_ARRAY_DIM];

I am probably missing something. In the commit message, you suggested 
that XENPV_FLEX_ARRAY_DIM will use 1 for older interface version for 
compatibility reason.

Yet, if I am not mistaken, [] is not equivalent to [1]. So aren't you 
effectively breaking the compatibility?

Cheers,
Andrew Cooper Nov. 30, 2023, 11:34 a.m. UTC | #2
On 30/11/2023 9:21 am, Juergen Gross wrote:
> Flexible arrays in public headers can be problematic with some
> compilers.
>
> With XEN_FLEX_ARRAY_DIM there is a mechanism available to deal with
> this issue, but care must be taken to not change the affected structs
> in an incompatible way.
>
> So bump __XEN_LATEST_INTERFACE_VERSION__ and introduce a new macro
> XENPV_FLEX_ARRAY_DIM which will be XENPV_FLEX_ARRAY_DIM with the
> interface version being new enough and "1" (the value used today in
> the affected headers) when the interface version is an old one.
>
> Replace the arr[1] instances (this includes the ones seen to be
> problematic in recent Linux kernels [1]) with arr[XENPV_FLEX_ARRAY_DIM]
> in order to avoid compilation errors.
>
> [1]: https://bugzilla.kernel.org/show_bug.cgi?id=217693
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

CHANGELOG please.  This change, however it ends up looking, absolutely
needs covering.

~Andrew
Jürgen Groß Nov. 30, 2023, 11:39 a.m. UTC | #3
On 30.11.23 12:33, Julien Grall wrote:
> Hi Juergen,
> 
> On 30/11/2023 09:21, Juergen Gross wrote:
>> ---
>> V2:
>> - bump interface version and make change only for new version
>>    (Jan Beulich)
>> ---
>>   xen/include/public/io/cameraif.h | 2 +-
>>   xen/include/public/io/displif.h  | 2 +-
>>   xen/include/public/io/fsif.h     | 4 ++--
>>   xen/include/public/io/pvcalls.h  | 2 +-
>>   xen/include/public/io/ring.h     | 5 +++--
>>   xen/include/public/io/sndif.h    | 2 +-
>>   xen/include/public/xen-compat.h  | 2 +-
>>   xen/include/public/xen.h         | 6 ++++++
>>   8 files changed, 16 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/include/public/io/cameraif.h b/xen/include/public/io/cameraif.h
>> index 13763abef9..a389443769 100644
>> --- a/xen/include/public/io/cameraif.h
>> +++ b/xen/include/public/io/cameraif.h
>> @@ -763,7 +763,7 @@ struct xencamera_buf_create_req {
>>    */
>>   struct xencamera_page_directory {
>>       grant_ref_t gref_dir_next_page;
>> -    grant_ref_t gref[1]; /* Variable length */
>> +    grant_ref_t gref[XENPV_FLEX_ARRAY_DIM];
>>   };
>>   /*
>> diff --git a/xen/include/public/io/displif.h b/xen/include/public/io/displif.h
>> index 73d0cbdf15..132c96fa5c 100644
>> --- a/xen/include/public/io/displif.h
>> +++ b/xen/include/public/io/displif.h
>> @@ -537,7 +537,7 @@ struct xendispl_dbuf_create_req {
>>   struct xendispl_page_directory {
>>       grant_ref_t gref_dir_next_page;
>> -    grant_ref_t gref[1]; /* Variable length */
>> +    grant_ref_t gref[XENPV_FLEX_ARRAY_DIM];
>>   };
>>   /*
>> diff --git a/xen/include/public/io/fsif.h b/xen/include/public/io/fsif.h
>> index ec57850233..dcade1c698 100644
>> --- a/xen/include/public/io/fsif.h
>> +++ b/xen/include/public/io/fsif.h
>> @@ -40,7 +40,7 @@ struct fsif_read_request {
>>       int32_t pad;
>>       uint64_t len;
>>       uint64_t offset;
>> -    grant_ref_t grefs[1];  /* Variable length */
>> +    grant_ref_t grefs[XENPV_FLEX_ARRAY_DIM];
>>   };
>>   struct fsif_write_request {
>> @@ -48,7 +48,7 @@ struct fsif_write_request {
>>       int32_t pad;
>>       uint64_t len;
>>       uint64_t offset;
>> -    grant_ref_t grefs[1];  /* Variable length */
>> +    grant_ref_t grefs[XENPV_FLEX_ARRAY_DIM];
>>   };
>>   struct fsif_stat_request {
>> diff --git a/xen/include/public/io/pvcalls.h b/xen/include/public/io/pvcalls.h
>> index 230b0719e3..af0e9abd13 100644
>> --- a/xen/include/public/io/pvcalls.h
>> +++ b/xen/include/public/io/pvcalls.h
>> @@ -30,7 +30,7 @@ struct pvcalls_data_intf {
>>       uint8_t pad2[52];
>>       RING_IDX ring_order;
>> -    grant_ref_t ref[];
>> +    grant_ref_t ref[XENPV_FLEX_ARRAY_DIM];
> 
> I am probably missing something. In the commit message, you suggested that 
> XENPV_FLEX_ARRAY_DIM will use 1 for older interface version for compatibility 
> reason.
> 
> Yet, if I am not mistaken, [] is not equivalent to [1]. So aren't you 
> effectively breaking the compatibility?

Oh, thanks for catching this one!

I agree, this should be XEN_FLEX_ARRAY_DIM instead.


Juergen
Jan Beulich Nov. 30, 2023, 1:14 p.m. UTC | #4
On 30.11.2023 10:21, Juergen Gross wrote:
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -46,6 +46,12 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
>  #else
>  #define XEN_FLEX_ARRAY_DIM  1 /* variable size */
>  #endif
> +/* Some PV I/O interfaces need a compatibility variant. */
> +#if __XEN_INTERFACE_VERSION__ < 0x00041300
> +#define XENPV_FLEX_ARRAY_DIM  1 /* variable size */
> +#else
> +#define XENPV_FLEX_ARRAY_DIM  XEN_FLEX_ARRAY_DIM
> +#endif

May I suggest that this live in io/ring.h, for the lack of a better place
under io/?

Jan
Jürgen Groß Nov. 30, 2023, 1:27 p.m. UTC | #5
On 30.11.23 14:14, Jan Beulich wrote:
> On 30.11.2023 10:21, Juergen Gross wrote:
>> --- a/xen/include/public/xen.h
>> +++ b/xen/include/public/xen.h
>> @@ -46,6 +46,12 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
>>   #else
>>   #define XEN_FLEX_ARRAY_DIM  1 /* variable size */
>>   #endif
>> +/* Some PV I/O interfaces need a compatibility variant. */
>> +#if __XEN_INTERFACE_VERSION__ < 0x00041300
>> +#define XENPV_FLEX_ARRAY_DIM  1 /* variable size */
>> +#else
>> +#define XENPV_FLEX_ARRAY_DIM  XEN_FLEX_ARRAY_DIM
>> +#endif
> 
> May I suggest that this live in io/ring.h, for the lack of a better place
> under io/?

Yes, fine with me.


Juergen
Jürgen Groß Nov. 30, 2023, 1:28 p.m. UTC | #6
On 30.11.23 12:34, Andrew Cooper wrote:
> On 30/11/2023 9:21 am, Juergen Gross wrote:
>> Flexible arrays in public headers can be problematic with some
>> compilers.
>>
>> With XEN_FLEX_ARRAY_DIM there is a mechanism available to deal with
>> this issue, but care must be taken to not change the affected structs
>> in an incompatible way.
>>
>> So bump __XEN_LATEST_INTERFACE_VERSION__ and introduce a new macro
>> XENPV_FLEX_ARRAY_DIM which will be XENPV_FLEX_ARRAY_DIM with the
>> interface version being new enough and "1" (the value used today in
>> the affected headers) when the interface version is an old one.
>>
>> Replace the arr[1] instances (this includes the ones seen to be
>> problematic in recent Linux kernels [1]) with arr[XENPV_FLEX_ARRAY_DIM]
>> in order to avoid compilation errors.
>>
>> [1]: https://bugzilla.kernel.org/show_bug.cgi?id=217693
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> CHANGELOG please.  This change, however it ends up looking, absolutely
> needs covering.

Would this be fine:

  ### Changed
+ - Changed flexible array definitions in public I/O interface headers to not
+   use "1" as the number of array elements.


Juergen
diff mbox series

Patch

diff --git a/xen/include/public/io/cameraif.h b/xen/include/public/io/cameraif.h
index 13763abef9..a389443769 100644
--- a/xen/include/public/io/cameraif.h
+++ b/xen/include/public/io/cameraif.h
@@ -763,7 +763,7 @@  struct xencamera_buf_create_req {
  */
 struct xencamera_page_directory {
     grant_ref_t gref_dir_next_page;
-    grant_ref_t gref[1]; /* Variable length */
+    grant_ref_t gref[XENPV_FLEX_ARRAY_DIM];
 };
 
 /*
diff --git a/xen/include/public/io/displif.h b/xen/include/public/io/displif.h
index 73d0cbdf15..132c96fa5c 100644
--- a/xen/include/public/io/displif.h
+++ b/xen/include/public/io/displif.h
@@ -537,7 +537,7 @@  struct xendispl_dbuf_create_req {
 
 struct xendispl_page_directory {
     grant_ref_t gref_dir_next_page;
-    grant_ref_t gref[1]; /* Variable length */
+    grant_ref_t gref[XENPV_FLEX_ARRAY_DIM];
 };
 
 /*
diff --git a/xen/include/public/io/fsif.h b/xen/include/public/io/fsif.h
index ec57850233..dcade1c698 100644
--- a/xen/include/public/io/fsif.h
+++ b/xen/include/public/io/fsif.h
@@ -40,7 +40,7 @@  struct fsif_read_request {
     int32_t pad;
     uint64_t len;
     uint64_t offset;
-    grant_ref_t grefs[1];  /* Variable length */
+    grant_ref_t grefs[XENPV_FLEX_ARRAY_DIM];
 };
 
 struct fsif_write_request {
@@ -48,7 +48,7 @@  struct fsif_write_request {
     int32_t pad;
     uint64_t len;
     uint64_t offset;
-    grant_ref_t grefs[1];  /* Variable length */
+    grant_ref_t grefs[XENPV_FLEX_ARRAY_DIM];
 };
 
 struct fsif_stat_request {
diff --git a/xen/include/public/io/pvcalls.h b/xen/include/public/io/pvcalls.h
index 230b0719e3..af0e9abd13 100644
--- a/xen/include/public/io/pvcalls.h
+++ b/xen/include/public/io/pvcalls.h
@@ -30,7 +30,7 @@  struct pvcalls_data_intf {
     uint8_t pad2[52];
 
     RING_IDX ring_order;
-    grant_ref_t ref[];
+    grant_ref_t ref[XENPV_FLEX_ARRAY_DIM];
 };
 DEFINE_XEN_FLEX_RING(pvcalls);
 
diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h
index 0cae4367be..955fe5a1fc 100644
--- a/xen/include/public/io/ring.h
+++ b/xen/include/public/io/ring.h
@@ -25,6 +25,7 @@ 
  * and grant_table.h from the Xen public headers.
  */
 
+#include "../xen.h"
 #include "../xen-compat.h"
 
 #if __XEN_INTERFACE_VERSION__ < 0x00030208
@@ -110,7 +111,7 @@  struct __name##_sring {                                                 \
         uint8_t pvt_pad[4];                                             \
     } pvt;                                                              \
     uint8_t __pad[44];                                                  \
-    union __name##_sring_entry ring[1]; /* variable-length */           \
+    union __name##_sring_entry ring[XENPV_FLEX_ARRAY_DIM];              \
 };                                                                      \
                                                                         \
 /* "Front" end's private variables */                                   \
@@ -479,7 +480,7 @@  struct name##_data_intf {                                                     \
     uint8_t pad2[56];                                                         \
                                                                               \
     RING_IDX ring_order;                                                      \
-    grant_ref_t ref[];                                                        \
+    grant_ref_t ref[XENPV_FLEX_ARRAY_DIM];                                    \
 };                                                                            \
 DEFINE_XEN_FLEX_RING(name)
 
diff --git a/xen/include/public/io/sndif.h b/xen/include/public/io/sndif.h
index 4234a47c87..cce1459b7b 100644
--- a/xen/include/public/io/sndif.h
+++ b/xen/include/public/io/sndif.h
@@ -659,7 +659,7 @@  struct xensnd_open_req {
 
 struct xensnd_page_directory {
     grant_ref_t gref_dir_next_page;
-    grant_ref_t gref[1]; /* Variable length */
+    grant_ref_t gref[XENPV_FLEX_ARRAY_DIM];
 };
 
 /*
diff --git a/xen/include/public/xen-compat.h b/xen/include/public/xen-compat.h
index 97fe698498..078b796664 100644
--- a/xen/include/public/xen-compat.h
+++ b/xen/include/public/xen-compat.h
@@ -10,7 +10,7 @@ 
 #ifndef __XEN_PUBLIC_XEN_COMPAT_H__
 #define __XEN_PUBLIC_XEN_COMPAT_H__
 
-#define __XEN_LATEST_INTERFACE_VERSION__ 0x00040e00
+#define __XEN_LATEST_INTERFACE_VERSION__ 0x00041300
 
 #if defined(__XEN__) || defined(__XEN_TOOLS__)
 /* Xen is built with matching headers and implements the latest interface. */
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index b812a0a324..62c4b04fa2 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -46,6 +46,12 @@  DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
 #else
 #define XEN_FLEX_ARRAY_DIM  1 /* variable size */
 #endif
+/* Some PV I/O interfaces need a compatibility variant. */
+#if __XEN_INTERFACE_VERSION__ < 0x00041300
+#define XENPV_FLEX_ARRAY_DIM  1 /* variable size */
+#else
+#define XENPV_FLEX_ARRAY_DIM  XEN_FLEX_ARRAY_DIM
+#endif
 
 /* Turn a plain number into a C unsigned (long (long)) constant. */
 #define __xen_mk_uint(x)  x ## U