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.

Fixes: 7c7f7e8fba01 ("include/public/memory.h: remove the XENMEM_rsrc_acq_caller_owned flag")
Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/memory.c         |  2 +-
 xen/include/public/memory.h | 12 +++++++++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/xen/common/memory.c b/xen/common/memory.c
index 0d7c413df8..9b5214a8a9 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1234,7 +1234,7 @@ static int acquire_resource(
     if ( copy_from_guest(&xmar, arg, 1) )
         return -EFAULT;
 
-    if ( xmar.pad != 0 )
+    if ( xmar.flags != 0 )
         return -EINVAL;
 
     /*
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index 383a9468c3..fd768e0b7b 100644
--- 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)
+
     /*
      * IN - the index of the initial frame to be mapped. This parameter
      *      is ignored if nr_frames is 0.  This value may be updated
-- 
2.34.1

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.

> --- 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.

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.

Jan

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

On 07.02.2022 12:00, Juergen Gross wrote:
> On 07.02.22 11:46, Jan Beulich wrote:
>> On 07.02.2022 11:36, Juergen Gross wrote:
>>> --- 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?

Pretty sure. And I think in this case it's better to break the build
of consumers (so we're sure they'd notice, assuming they import the
header directly in the first place). It's rather an exceptional case
after all.

Jan

On 07.02.22 13:46, Jan Beulich wrote:
> On 07.02.2022 12:00, Juergen Gross wrote:
>> On 07.02.22 11:46, Jan Beulich wrote:
>>> On 07.02.2022 11:36, Juergen Gross wrote:
>>>> --- 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?
> 
> Pretty sure. And I think in this case it's better to break the build
> of consumers (so we're sure they'd notice, assuming they import the
> header directly in the first place). It's rather an exceptional case
> after all.

Okay, I'll just add a comment regarding the reserved bit then, without
reverting any part of commit 7c7f7e8fba01.


Juergen