[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 4/4] xen/public: arm: rework the macro set_xen_guest_handle_raw
On Wed, 4 Nov 2015, Julien Grall wrote: > On 04/11/15 11:27, Ian Campbell wrote: > > On Wed, 2015-11-04 at 11:17 +0000, Julien Grall wrote: > >>> > >>> we could: > >>> #ifdef(__XEN__) > >>> #define XEN_BUILD_BUG_ON(x) BUILD_BUG_ON(x) > >>> #elif !defined(XEN_BUILD_BUG_ON) > >>> #define XEN_BUILD_BUG_ON(x) > >>> #endif > >>> and using XEN_BUILD_BUG_ON in the macro > >>> > >>> So, __XEN__ builds get the check and users can opt in by providing > >>> XEN_BUILD_BUG_ON if they want. If they don't then they don't get the > >>> check. > >> > >> I wouldn't let the user the choice to disable build-time check. > > > > Up to you. Note that "the user" here would potentially include > > xen.git/tools, and I would expect them to want to define it. > > > > Also... > > > >> There is > >> no harm to open-code them as I did today and avoid possible issue in the > >> code later. > > > > ... there is always a downside to open coding. If you don't want to expose > > the ability to define a BUG_ON to the application then just drop that #elif > > from the chain. > > Good point. I will give a look. > > > > >>> Or maybe we could just omit this from the public API by one or both of > >>> a) > >>> adding an explicit 8 byte type to the union purely to force the size > >>> and/or > >> > >> This is already done in the current version: > >> > >> typedef union { type *p; uint64_aligned_t q; } \ > >> __guest_handle_64_ ## name; > >> > >> However I don't see how this ensure that the caller of > >> set_xen_raw_guest_handle will effectively hnd as a pointer to an 8-byte > >> placeholder. > > > > Not sure I follow. If hnd isn't a suitable struct xen guest handle then > > other things will fail. If a user is passing a non struct xen_guest_handle > > to this which happens to contain the same fields then more fool them, and > > if it happens to be 8 bytes anyway your check won't catch that. > > With the 2 checks in set_xen_raw_guest_handle we catch most of the > problem. They both ensure that the handle is 8-byte and the pointer is > valid. However we don't check that the padding is at the beginning of > the structure. > > It's better than what we have today as we don't even check that the > handle is 8-byte. > > [...] > > >>> This looses out on the arm32 hypervisor sanity checking that the padding > >>> bytes are 0 (as required by the ABI) but TBH I haven't checked that the > >>> current version has that property either. > >> > >> It's done during the assignation by the compiler: > >> > >> (hnd).q = (uint64_t)(uintptr_t)(val); > > > > I meant on the reading side. > > It's the responsibility of the caller to zero the padding. There is > nothing to do on the reading side, the hypervisor will use "p" which > will be the size of the natural pointer. Sorry to be pedantic, but why is this safe given that previously you wrote: Finally, based on the defect report #283 [2], the behavior of writing from one member and reading from another is not clear. Looks like it might be better to remove the union completely? _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |