| Message ID | 20240415154155.2718064-5-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/xlat: Improvements to compat hypercall checking | expand |
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@citrix.com> > CC: Jan Beulich <JBeulich@suse.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Julien Grall <julien@xen.org> > --- > 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 >
On 16.04.2024 00:15, Stefano Stabellini wrote: > 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. The other issue is that __packed__ isn't standard C, yet we intend the public header to be plain C89 compatible. Jan
On 15.04.2024 17:41, 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? #ifdef-ary in general would be okay. But any #ifdef CONFIG_* would look pretty odd to me in a public header. Perhaps as #if defined(__XEN__) && defined(CONFIG_COMPAT) it might be tolerable. > --- /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" Everywhere up to here you say -Wpadding. > +#include <public/grant_table.h> > + > +#ifdef CONFIG_COMPAT > + > +#include <compat/grant_table.h> > + > +#endif /* CONFIG_COMPAT */ I'm not overly happy to see a 2nd header checking "pass" added. We already have the headers.chk goal in xen/include/Makefile, after all. For the non- generated headers adding -Wpadded there would seem more natural to me, first and foremost because then it is less likely that one of the two places would be missed if a new header is added. Something long those lines may then need adding for the generated compat headers, but again preferably without enumerating them all in yet another place. > --- 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; While you may view it as nitpicking, in the public headers I'm pretty firm on not wanting to see new name space violations, i.e. new names with leading underscores which aren't file-scope identifiers. Furthermore what's the deal with using "pad0" here and in one more place, but "ign1" / "ign2" in other cases? > @@ -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 */ I'm surprised no padding field would be needed right below here, seeing that what follows is a handle: #if __XEN_INTERFACE_VERSION__ < 0x00040300 XEN_GUEST_HANDLE(ulong) frame_list; #else XEN_GUEST_HANDLE(xen_pfn_t) frame_list; #endif The size of this padding field would then also be compat-dependent, I suppose. Jan
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);