Re: [PATCH 4/4] xen/public: Use -Wpadding for public headers

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 15 Apr 2024, Andrew Cooper wrote:
> RFC.  In theory this is a great way to avoid some of the spiketraps involved
> with C being the official representation.
> 
> However, this doesn't build.  gnttab_transfer has a layout that requires a
> CONFIG_COMPAT if we want to satisfy -Wpadding for both forms of the structure.
> 
> Thoughts on whether this cross-check is worthwhile-enough to warrant the
> ifdefary?

I like this patch and I think we have no choice but going in this
direction and adding all the padding explicitly with any related
necessary ifdefary.

The only question for me is whether to:

1) add -Wpadding
2) add __packed__
3) do both

I think it is important to add __packed__ to the headers to clear out
any misconceptions about possible hidden paddings and get a
correct-by-default behavior for anyone that would import the headers
into their own projects.

The only issue is that __packed__ makes -Wpadding not useful. We could
technically add both if we disable __packed__ for the -Wpadding build.
For instance we could use __packed which is defined by Xen, and change
the definition of __packed to nothing for the -Wpadding build.

That way we get both the nice -Wpadding checks and also the nice
obvious-by-default __packed__.




> ~Andrew
> ---
> CC: George Dunlap <George.Dunlap@xxxxxxxxxx>
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> ---
>  xen/common/Makefile              |  1 +
>  xen/common/hdr-chk.c             | 13 +++++++++++++
>  xen/include/public/grant_table.h |  7 +++++++
>  3 files changed, 21 insertions(+)
>  create mode 100644 xen/common/hdr-chk.c
> 
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index d512cad5243f..dbbda70829f1 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -15,6 +15,7 @@ obj-y += event_fifo.o
>  obj-$(CONFIG_GRANT_TABLE) += grant_table.o
>  obj-y += guestcopy.o
>  obj-y += gzip/
> +obj-y += hdr-chk.o
>  obj-$(CONFIG_HYPFS) += hypfs.o
>  obj-$(CONFIG_IOREQ_SERVER) += ioreq.o
>  obj-y += irq.o
> diff --git a/xen/common/hdr-chk.c b/xen/common/hdr-chk.c
> new file mode 100644
> index 000000000000..1c7a509dcd06
> --- /dev/null
> +++ b/xen/common/hdr-chk.c
> @@ -0,0 +1,13 @@
> +#include <xen/stdint.h>
> +
> +#include <public/xen.h>
> +
> +#pragma GCC diagnostic error "-Wpadded"
> +
> +#include <public/grant_table.h>
> +
> +#ifdef CONFIG_COMPAT
> +
> +#include <compat/grant_table.h>
> +
> +#endif /* CONFIG_COMPAT */
> diff --git a/xen/include/public/grant_table.h 
> b/xen/include/public/grant_table.h
> index 1dfa17a6d02a..a66c77d0166c 100644
> --- a/xen/include/public/grant_table.h
> +++ b/xen/include/public/grant_table.h
> @@ -355,6 +355,7 @@ struct gnttab_unmap_grant_ref {
>      grant_handle_t handle;
>      /* OUT parameters. */
>      int16_t  status;              /* => enum grant_status */
> +    uint16_t _pad0;
>  };
>  typedef struct gnttab_unmap_grant_ref gnttab_unmap_grant_ref_t;
>  DEFINE_XEN_GUEST_HANDLE(gnttab_unmap_grant_ref_t);
> @@ -371,6 +372,7 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_unmap_grant_ref_t);
>  struct gnttab_setup_table {
>      /* IN parameters. */
>      domid_t  dom;
> +    uint16_t _pad0;
>      uint32_t nr_frames;
>      /* OUT parameters. */
>      int16_t  status;              /* => enum grant_status */
> @@ -409,9 +411,12 @@ struct gnttab_transfer {
>      /* IN parameters. */
>      xen_pfn_t     mfn;
>      domid_t       domid;
> +    uint16_t      _pad0;
>      grant_ref_t   ref;
>      /* OUT parameters. */
>      int16_t       status;
> +    uint16_t      _pad1;
> +    /* XXX compat-dependent padding. */
>  };
>  typedef struct gnttab_transfer gnttab_transfer_t;
>  DEFINE_XEN_GUEST_HANDLE(gnttab_transfer_t);
> @@ -468,10 +473,12 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_copy_t);
>  struct gnttab_query_size {
>      /* IN parameters. */
>      domid_t  dom;
> +    uint16_t _ign1;
>      /* OUT parameters. */
>      uint32_t nr_frames;
>      uint32_t max_nr_frames;
>      int16_t  status;              /* => enum grant_status */
> +    uint16_t _ign2;
>  };
>  typedef struct gnttab_query_size gnttab_query_size_t;
>  DEFINE_XEN_GUEST_HANDLE(gnttab_query_size_t);
> -- 
> 2.30.2
>