Re: [PATCH] xen/public: partially revert commit 7c7f7e8fba01

[Juergen Gross <jgross@xxxxxxxx>]

On 07.02.22 11:46, Jan Beulich wrote:
> On 07.02.2022 11:36, Juergen Gross wrote:
> > Commit 7c7f7e8fba01 changed xen/include/public/memory.h in an incompatible
> > way. Unfortunately the changed parts were already in use in the Linux
> > kernel, so an update of the header in the kernel would result in a build
> > breakage.
> >
> > Even when removing its usage from the kernel the used flag bit should be
> > marked as reserved in order to avoid to give it a different semantic in
> > the future.
> >
> > Do a partial revert of said commit in order to enable the kernel to take
> > an updated version of memory.h.
> I don't think it should be a partial revert, and as said on irc I'm of
> the opinion that ...
>
> > Fixes: 7c7f7e8fba01 ("include/public/memory.h: remove the 
> > XENMEM_rsrc_acq_caller_owned flag")
> ... it's 0e2e54966af5 which should have taken measures to protect
> against re-use of the flag as an output.
The design of that feature was flawed from the beginning, as it was used
in the kernel right away. So I think the initial revert was the
effective start of the problem.

> > --- a/xen/include/public/memory.h
> > +++ b/xen/include/public/memory.h
> > @@ -662,7 +662,17 @@ struct xen_mem_acquire_resource {
> >        * two calls.
> >        */
> >       uint32_t nr_frames;
> > -    uint32_t pad;
> > +
> > +    /*
> > +     * OUT - Must be zero on entry. On return this may contain a bitwise
> > +     *       OR of the following values.
> > +     */
> > +    uint32_t flags;
> > +
> > +    /* No longer supported - will be never set */
> > +#define _XENMEM_rsrc_acq_caller_owned 0
> > +#define XENMEM_rsrc_acq_caller_owned (1u << _XENMEM_rsrc_acq_caller_owned)
> I think this goes too far: Neither do we want to re-introduce the
> #define-s, nor should we re-fix the purpose of the padding field
> to be OUT (only). All we need to make sure is that the field
> coming in as zero won't get responded to by setting bit 0 of it.
> Imo this can only reasonably be done by way of adding a comment.
> This comment may, in turn, mention XENMEM_rsrc_acq_caller_owned
> of course.
The kernel could be changed to no longer use that #define before
updating the header from Xen, but are we really sure there are no
other users, too?

I'm fine doing it that way, but I think I should spell out the
consequences of that decision.

> Btw., if the field was to become OUT-only again, I think you'd
> also need to revert the change to xen/common/compat/memory.c. At
> least to not leave a trap for someone to later fall into.
Okay, if you like that better.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature