| Message ID | 20210305124949.6719-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tools/libs: Multiple fixes to header handling | expand |
Andrew Cooper writes ("[PATCH 2/3] xen/dmop: Strip __XEN_TOOLS__ header guard from public API"):
> Exactly as with c/s f40e1c52e4, this is inappropriate for a stable library.
>
> That change actually broke the build with:
>
> include/xendevicemodel.h:52:5: error: unknown type name 'ioservid_t'
> ioservid_t *id);
> ^
>
> as libxendevicemodel.h now uses a type it can't see a typedef for. However,
> nothing noticed because the header.chk logic is also broken (fixed
> subsequently).
>
> Strip the guard from the public header, and remove compensation from
> devicemodel's private.h
Release-Acked-by: Ian Jackson <iwj@xenproject.org>
On 05.03.2021 13:49, Andrew Cooper wrote: > Exactly as with c/s f40e1c52e4, this is inappropriate for a stable library. > > That change actually broke the build with: > > include/xendevicemodel.h:52:5: error: unknown type name 'ioservid_t' > ioservid_t *id); > ^ > > as libxendevicemodel.h now uses a type it can't see a typedef for. However, > nothing noticed because the header.chk logic is also broken (fixed > subsequently). While I agree up to here, ... > Strip the guard from the public header, and remove compensation from > devicemodel's private.h ... I'm unconvinced that entirely dropping the guard from the public header is wanted (or needed): We use these to make clear that in particular kernels aren't supposed to make use of the enclosed entities. If a type needs exposing, it (and only it) wants moving ou of the guarded region imo. Jan
On 05/03/2021 13:53, Jan Beulich wrote: > On 05.03.2021 13:49, Andrew Cooper wrote: >> Exactly as with c/s f40e1c52e4, this is inappropriate for a stable library. >> >> That change actually broke the build with: >> >> include/xendevicemodel.h:52:5: error: unknown type name 'ioservid_t' >> ioservid_t *id); >> ^ >> >> as libxendevicemodel.h now uses a type it can't see a typedef for. However, >> nothing noticed because the header.chk logic is also broken (fixed >> subsequently). > While I agree up to here, ... > >> Strip the guard from the public header, and remove compensation from >> devicemodel's private.h > ... I'm unconvinced that entirely dropping the guard from the > public header is wanted (or needed): We use these to make clear > that in particular kernels aren't supposed to make use of the > enclosed entities. If a type needs exposing, it (and only it) > wants moving ou of the guarded region imo. DMOP was invented specifically so a kernel module (i915, for Intel gVT-g) was independent of the domctl ABI version. Improving the life of dom0 userspace was an intended consequence, but not the driving force behind the change. Exactly the same is true for stubdoms currently, and I am very serious about purging unstable interfaces eventually. ~Andrew
On 05.03.2021 15:12, Andrew Cooper wrote: > On 05/03/2021 13:53, Jan Beulich wrote: >> On 05.03.2021 13:49, Andrew Cooper wrote: >>> Exactly as with c/s f40e1c52e4, this is inappropriate for a stable library. >>> >>> That change actually broke the build with: >>> >>> include/xendevicemodel.h:52:5: error: unknown type name 'ioservid_t' >>> ioservid_t *id); >>> ^ >>> >>> as libxendevicemodel.h now uses a type it can't see a typedef for. However, >>> nothing noticed because the header.chk logic is also broken (fixed >>> subsequently). >> While I agree up to here, ... >> >>> Strip the guard from the public header, and remove compensation from >>> devicemodel's private.h >> ... I'm unconvinced that entirely dropping the guard from the >> public header is wanted (or needed): We use these to make clear >> that in particular kernels aren't supposed to make use of the >> enclosed entities. If a type needs exposing, it (and only it) >> wants moving ou of the guarded region imo. > > DMOP was invented specifically so a kernel module (i915, for Intel > gVT-g) was independent of the domctl ABI version. > > Improving the life of dom0 userspace was an intended consequence, but > not the driving force behind the change. This is news to me - so far it had been my understanding that it was introduced to have a way for the kernel to audit and hand on requests to the hypervisor without needing to know all the inner details. I wasn't even aware a kernel module was using any of these. Jan
On 05.03.2021 15:18, Jan Beulich wrote: > On 05.03.2021 15:12, Andrew Cooper wrote: >> On 05/03/2021 13:53, Jan Beulich wrote: >>> On 05.03.2021 13:49, Andrew Cooper wrote: >>>> Exactly as with c/s f40e1c52e4, this is inappropriate for a stable library. >>>> >>>> That change actually broke the build with: >>>> >>>> include/xendevicemodel.h:52:5: error: unknown type name 'ioservid_t' >>>> ioservid_t *id); >>>> ^ >>>> >>>> as libxendevicemodel.h now uses a type it can't see a typedef for. However, >>>> nothing noticed because the header.chk logic is also broken (fixed >>>> subsequently). >>> While I agree up to here, ... >>> >>>> Strip the guard from the public header, and remove compensation from >>>> devicemodel's private.h >>> ... I'm unconvinced that entirely dropping the guard from the >>> public header is wanted (or needed): We use these to make clear >>> that in particular kernels aren't supposed to make use of the >>> enclosed entities. If a type needs exposing, it (and only it) >>> wants moving ou of the guarded region imo. >> >> DMOP was invented specifically so a kernel module (i915, for Intel >> gVT-g) was independent of the domctl ABI version. >> >> Improving the life of dom0 userspace was an intended consequence, but >> not the driving force behind the change. > > This is news to me - so far it had been my understanding that it > was introduced to have a way for the kernel to audit and hand on > requests to the hypervisor without needing to know all the inner > details. I wasn't even aware a kernel module was using any of > these. And indeed, quote from docs/designs/dmop.markdown: "The aim of DMOP is to prevent a compromised device model from compromising domains other than the one it is providing emulation for (which is therefore likely already compromised)." And it goes on discussing only the purpose that I've been aware of. Jan
Jan Beulich writes ("Re: [PATCH 2/3] xen/dmop: Strip __XEN_TOOLS__ header guard from public API"):
> This is news to me - so far it had been my understanding that it
> was introduced to have a way for the kernel to audit and hand on
> requests to the hypervisor without needing to know all the inner
> details. I wasn't even aware a kernel module was using any of
> these.
Quite so.
Ian.
On 05/03/2021 14:21, Jan Beulich wrote: > On 05.03.2021 15:18, Jan Beulich wrote: >> On 05.03.2021 15:12, Andrew Cooper wrote: >>> On 05/03/2021 13:53, Jan Beulich wrote: >>>> On 05.03.2021 13:49, Andrew Cooper wrote: >>>>> Exactly as with c/s f40e1c52e4, this is inappropriate for a stable library. >>>>> >>>>> That change actually broke the build with: >>>>> >>>>> include/xendevicemodel.h:52:5: error: unknown type name 'ioservid_t' >>>>> ioservid_t *id); >>>>> ^ >>>>> >>>>> as libxendevicemodel.h now uses a type it can't see a typedef for. However, >>>>> nothing noticed because the header.chk logic is also broken (fixed >>>>> subsequently). >>>> While I agree up to here, ... >>>> >>>>> Strip the guard from the public header, and remove compensation from >>>>> devicemodel's private.h >>>> ... I'm unconvinced that entirely dropping the guard from the >>>> public header is wanted (or needed): We use these to make clear >>>> that in particular kernels aren't supposed to make use of the >>>> enclosed entities. If a type needs exposing, it (and only it) >>>> wants moving ou of the guarded region imo. >>> DMOP was invented specifically so a kernel module (i915, for Intel >>> gVT-g) was independent of the domctl ABI version. >>> >>> Improving the life of dom0 userspace was an intended consequence, but >>> not the driving force behind the change. >> This is news to me - so far it had been my understanding that it >> was introduced to have a way for the kernel to audit and hand on >> requests to the hypervisor without needing to know all the inner >> details. I wasn't even aware a kernel module was using any of >> these. > And indeed, quote from docs/designs/dmop.markdown: > > "The aim of DMOP is to prevent a compromised device model from > compromising domains other than the one it is providing emulation > for (which is therefore likely already compromised)." > > And it goes on discussing only the purpose that I've been aware > of. The use in the dom0 kernel wasn't kept secret in the slightest. It was discussed on at the time, and at dev summits. But upstream tends to only remember/care about the bits which pertain directly to upstream, and the design particulars of the DMOP ABI were specifically for userspace. ~Andrew
Andrew Cooper writes ("Re: [PATCH 2/3] xen/dmop: Strip __XEN_TOOLS__ header guard from public API"):
> The use in the dom0 kernel wasn't kept secret in the slightest. It was
> discussed on at the time, and at dev summits.
No-one is accusing anyone of keeping anything secret.
> But upstream tends to only remember/care about the bits which pertain
> directly to upstream,
I would prefer to say that upstream only tends to remember things
which are WRITTEN DOWN.
Ian.
diff --git a/tools/libs/devicemodel/private.h b/tools/libs/devicemodel/private.h index c4a225f8af..c24f3396bb 100644 --- a/tools/libs/devicemodel/private.h +++ b/tools/libs/devicemodel/private.h @@ -1,8 +1,6 @@ #ifndef XENDEVICEMODEL_PRIVATE_H #define XENDEVICEMODEL_PRIVATE_H -#define __XEN_TOOLS__ 1 - #include <xentoollog.h> #include <xendevicemodel.h> #include <xencall.h> diff --git a/xen/include/public/hvm/dm_op.h b/xen/include/public/hvm/dm_op.h index ef7fbc0d3d..fa3f083fed 100644 --- a/xen/include/public/hvm/dm_op.h +++ b/xen/include/public/hvm/dm_op.h @@ -25,9 +25,6 @@ #define __XEN_PUBLIC_HVM_DM_OP_H__ #include "../xen.h" - -#if defined(__XEN__) || defined(__XEN_TOOLS__) - #include "../event_channel.h" #ifndef uint64_aligned_t @@ -491,8 +488,6 @@ struct xen_dm_op { } u; }; -#endif /* __XEN__ || __XEN_TOOLS__ */ - struct xen_dm_op_buf { XEN_GUEST_HANDLE(void) h; xen_ulong_t size;
Exactly as with c/s f40e1c52e4, this is inappropriate for a stable library. That change actually broke the build with: include/xendevicemodel.h:52:5: error: unknown type name 'ioservid_t' ioservid_t *id); ^ as libxendevicemodel.h now uses a type it can't see a typedef for. However, nothing noticed because the header.chk logic is also broken (fixed subsequently). Strip the guard from the public header, and remove compensation from devicemodel's private.h Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Paul Durrant <paul@xen.org> CC: Ian Jackson <iwj@xenproject.org> For 4.15. This is a build fix, even if current staging can't spot the breakage. These two issues highlight that libxendevcemodel.h has never been checked since its introduction, because the checking logic only saw an empty file. --- tools/libs/devicemodel/private.h | 2 -- xen/include/public/hvm/dm_op.h | 5 ----- 2 files changed, 7 deletions(-)