Providing a macro for an invalid grant reference would be beneficial
for users, especially as some are using the wrong value "0" for that
purpose.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/include/public/grant_table.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/include/public/grant_table.h b/xen/include/public/grant_table.h
index 7fbd1c6d10..af00aacfd3 100644
--- a/xen/include/public/grant_table.h
+++ b/xen/include/public/grant_table.h
@@ -113,6 +113,8 @@
  */
 typedef uint32_t grant_ref_t;
 
+#define XEN_GRANT_REF_INVALID  ~0U
+
 /*
  * A grant table comprises a packed array of grant entries in one or more
  * page frames shared between Xen and a guest.
-- 
2.34.1

On 21.02.2022 13:42, Juergen Gross wrote:
> Providing a macro for an invalid grant reference would be beneficial
> for users, especially as some are using the wrong value "0" for that
> purpose.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Over the years I've been considering to add such to the public
interface, perhaps even more than once. But I'm afraid it's not that
easy. In principle 0xffffffff (which btw isn't necessarily ~0u) could
be a valid ref. It is really internal agreement by users of the
interface to set for themselves that they're not ever going to make
a valid grant behind that reference.

Jan

On 21.02.22 15:18, Jan Beulich wrote:
> On 21.02.2022 13:42, Juergen Gross wrote:
>> Providing a macro for an invalid grant reference would be beneficial
>> for users, especially as some are using the wrong value "0" for that
>> purpose.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> Over the years I've been considering to add such to the public
> interface, perhaps even more than once. But I'm afraid it's not that
> easy. In principle 0xffffffff (which btw isn't necessarily ~0u) could

I can change that to use 0xffffffff explicitly.

> be a valid ref. It is really internal agreement by users of the
> interface to set for themselves that they're not ever going to make
> a valid grant behind that reference.

As the grant reference is an index into the grant table this would
limit the size of the grant table to "only" UINT_MAX - 1. I don't
think this will be ever a concern (other than an academical one).


Juergen

On 21.02.2022 15:27, Juergen Gross wrote:
> On 21.02.22 15:18, Jan Beulich wrote:
>> On 21.02.2022 13:42, Juergen Gross wrote:
>>> Providing a macro for an invalid grant reference would be beneficial
>>> for users, especially as some are using the wrong value "0" for that
>>> purpose.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>
>> Over the years I've been considering to add such to the public
>> interface, perhaps even more than once. But I'm afraid it's not that
>> easy. In principle 0xffffffff (which btw isn't necessarily ~0u) could
> 
> I can change that to use 0xffffffff explicitly.
> 
>> be a valid ref. It is really internal agreement by users of the
>> interface to set for themselves that they're not ever going to make
>> a valid grant behind that reference.
> 
> As the grant reference is an index into the grant table this would
> limit the size of the grant table to "only" UINT_MAX - 1. I don't
> think this will be ever a concern (other than an academical one).

That wasn't my point. Limiting the table to one less entry is not a
big deal indeed. But we have no reason to mandate which gref(s) to
consider invalid. A guest could consider gref 0 the invalid one.
The hypervisor doesn't care. Imo this simply is an aspect which is
not in need of pinning down in the ABI. Yet if it was pinned down
like you do, then the hypervisor would need to make sure it refuses
to act on this mandated invalid gref.

Jan

On 21.02.22 15:31, Jan Beulich wrote:
> On 21.02.2022 15:27, Juergen Gross wrote:
>> On 21.02.22 15:18, Jan Beulich wrote:
>>> On 21.02.2022 13:42, Juergen Gross wrote:
>>>> Providing a macro for an invalid grant reference would be beneficial
>>>> for users, especially as some are using the wrong value "0" for that
>>>> purpose.
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>
>>> Over the years I've been considering to add such to the public
>>> interface, perhaps even more than once. But I'm afraid it's not that
>>> easy. In principle 0xffffffff (which btw isn't necessarily ~0u) could
>>
>> I can change that to use 0xffffffff explicitly.
>>
>>> be a valid ref. It is really internal agreement by users of the
>>> interface to set for themselves that they're not ever going to make
>>> a valid grant behind that reference.
>>
>> As the grant reference is an index into the grant table this would
>> limit the size of the grant table to "only" UINT_MAX - 1. I don't
>> think this will be ever a concern (other than an academical one).
> 
> That wasn't my point. Limiting the table to one less entry is not a
> big deal indeed. But we have no reason to mandate which gref(s) to
> consider invalid. A guest could consider gref 0 the invalid one.

With the gref being an index starting with 0 (gref 0 is valid, as it is
used for the console ring page), the natural choice for an invalid
value is the highest one being representable. A gref is of type uint32_t
resulting in this value being 0xffffffff.

While in theory a grant table could be that large, in practice this
will never happen.

> The hypervisor doesn't care. Imo this simply is an aspect which is

This isn't true. The hypervisor needs to allocate resources for being
able to handle the highest possible gref value for a guest. For a v1
grant table this would mean 32GB of grant table size. Are you really
concerned we will ever hit this limit? This isn't at the guest's
choice, after all, as the max grant table size is limited by Xen.

> not in need of pinning down in the ABI. Yet if it was pinned down
> like you do, then the hypervisor would need to make sure it refuses
> to act on this mandated invalid gref.

This is an easy one. We could just refuse to have a grant table of
that size. I can add this to the patch if you really think it is
necessary.

TBH, I think such completely theoretical concerns should not stand
in the way of additions to the ABI making life easier for consumers.


Juergen

On 21.02.2022 16:05, Juergen Gross wrote:
> On 21.02.22 15:31, Jan Beulich wrote:
>> On 21.02.2022 15:27, Juergen Gross wrote:
>>> On 21.02.22 15:18, Jan Beulich wrote:
>>>> On 21.02.2022 13:42, Juergen Gross wrote:
>>>>> Providing a macro for an invalid grant reference would be beneficial
>>>>> for users, especially as some are using the wrong value "0" for that
>>>>> purpose.
>>>>>
>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>
>>>> Over the years I've been considering to add such to the public
>>>> interface, perhaps even more than once. But I'm afraid it's not that
>>>> easy. In principle 0xffffffff (which btw isn't necessarily ~0u) could
>>>
>>> I can change that to use 0xffffffff explicitly.
>>>
>>>> be a valid ref. It is really internal agreement by users of the
>>>> interface to set for themselves that they're not ever going to make
>>>> a valid grant behind that reference.
>>>
>>> As the grant reference is an index into the grant table this would
>>> limit the size of the grant table to "only" UINT_MAX - 1. I don't
>>> think this will be ever a concern (other than an academical one).
>>
>> That wasn't my point. Limiting the table to one less entry is not a
>> big deal indeed. But we have no reason to mandate which gref(s) to
>> consider invalid. A guest could consider gref 0 the invalid one.
> 
> With the gref being an index starting with 0 (gref 0 is valid, as it is
> used for the console ring page), the natural choice for an invalid
> value is the highest one being representable. A gref is of type uint32_t
> resulting in this value being 0xffffffff.
> 
> While in theory a grant table could be that large, in practice this
> will never happen.
> 
>> The hypervisor doesn't care. Imo this simply is an aspect which is
> 
> This isn't true. The hypervisor needs to allocate resources for being
> able to handle the highest possible gref value for a guest. For a v1
> grant table this would mean 32GB of grant table size. Are you really
> concerned we will ever hit this limit? This isn't at the guest's
> choice, after all, as the max grant table size is limited by Xen.

If we're not going to hit that limit, what's wrong with declaring the
entire upper half of uint32_t space "invalid" for use a gref? If we
won't ever go up to 32Gb, we quite certainly also won't ever reach
16Gb. Yes, you probably already guessed it, we can then repeat this
process iteratively until we reach 4kb.

>> not in need of pinning down in the ABI. Yet if it was pinned down
>> like you do, then the hypervisor would need to make sure it refuses
>> to act on this mandated invalid gref.
> 
> This is an easy one. We could just refuse to have a grant table of
> that size. I can add this to the patch if you really think it is
> necessary.

Since grant table size is measured in pages, you'd then have to
refuse use of more than just that single gref. This would still not
be an immediate problem, but demonstrates again that it's unclear
where to draw such a boundary, if one is to be artificially drawn.

> TBH, I think such completely theoretical concerns should not stand
> in the way of additions to the ABI making life easier for consumers.

In case it wasn't clear - my concern isn't that sacrificing this one
entry may cause a problem, or that we'd ever see grant tables grow
this big (albeit for the latter: you never really know). Instead my
concern is that it is conceptually wrong for us to (now) introduce
such a value.

Jan

On 21.02.22 16:31, Jan Beulich wrote:
> On 21.02.2022 16:05, Juergen Gross wrote:
>> On 21.02.22 15:31, Jan Beulich wrote:
>>> On 21.02.2022 15:27, Juergen Gross wrote:
>>>> On 21.02.22 15:18, Jan Beulich wrote:
>>>>> On 21.02.2022 13:42, Juergen Gross wrote:
>>>>>> Providing a macro for an invalid grant reference would be beneficial
>>>>>> for users, especially as some are using the wrong value "0" for that
>>>>>> purpose.
>>>>>>
>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>>
>>>>> Over the years I've been considering to add such to the public
>>>>> interface, perhaps even more than once. But I'm afraid it's not that
>>>>> easy. In principle 0xffffffff (which btw isn't necessarily ~0u) could
>>>>
>>>> I can change that to use 0xffffffff explicitly.
>>>>
>>>>> be a valid ref. It is really internal agreement by users of the
>>>>> interface to set for themselves that they're not ever going to make
>>>>> a valid grant behind that reference.
>>>>
>>>> As the grant reference is an index into the grant table this would
>>>> limit the size of the grant table to "only" UINT_MAX - 1. I don't
>>>> think this will be ever a concern (other than an academical one).
>>>
>>> That wasn't my point. Limiting the table to one less entry is not a
>>> big deal indeed. But we have no reason to mandate which gref(s) to
>>> consider invalid. A guest could consider gref 0 the invalid one.
>>
>> With the gref being an index starting with 0 (gref 0 is valid, as it is
>> used for the console ring page), the natural choice for an invalid
>> value is the highest one being representable. A gref is of type uint32_t
>> resulting in this value being 0xffffffff.
>>
>> While in theory a grant table could be that large, in practice this
>> will never happen.
>>
>>> The hypervisor doesn't care. Imo this simply is an aspect which is
>>
>> This isn't true. The hypervisor needs to allocate resources for being
>> able to handle the highest possible gref value for a guest. For a v1
>> grant table this would mean 32GB of grant table size. Are you really
>> concerned we will ever hit this limit? This isn't at the guest's
>> choice, after all, as the max grant table size is limited by Xen.
> 
> If we're not going to hit that limit, what's wrong with declaring the
> entire upper half of uint32_t space "invalid" for use a gref? If we
> won't ever go up to 32Gb, we quite certainly also won't ever reach
> 16Gb. Yes, you probably already guessed it, we can then repeat this
> process iteratively until we reach 4kb.

This reasoning is nonsense, and you know it.

>>> not in need of pinning down in the ABI. Yet if it was pinned down
>>> like you do, then the hypervisor would need to make sure it refuses
>>> to act on this mandated invalid gref.
>>
>> This is an easy one. We could just refuse to have a grant table of
>> that size. I can add this to the patch if you really think it is
>> necessary.
> 
> Since grant table size is measured in pages, you'd then have to
> refuse use of more than just that single gref. This would still not
> be an immediate problem, but demonstrates again that it's unclear
> where to draw such a boundary, if one is to be artificially drawn.

It should be as high as possible. I wouldn't mind just refusing the
last possible gref, but I don't think this is necessary.

>> TBH, I think such completely theoretical concerns should not stand
>> in the way of additions to the ABI making life easier for consumers.
> 
> In case it wasn't clear - my concern isn't that sacrificing this one
> entry may cause a problem, or that we'd ever see grant tables grow
> this big (albeit for the latter: you never really know). Instead my
> concern is that it is conceptually wrong for us to (now) introduce
> such a value.

I have understood that this is your concern.

I continue to think that this concern is of purely academical nature.


Juergen

On 21.02.2022 16:57, Juergen Gross wrote:
> On 21.02.22 16:31, Jan Beulich wrote:
>> On 21.02.2022 16:05, Juergen Gross wrote:
>>> On 21.02.22 15:31, Jan Beulich wrote:
>>>> On 21.02.2022 15:27, Juergen Gross wrote:
>>>>> On 21.02.22 15:18, Jan Beulich wrote:
>>>>>> On 21.02.2022 13:42, Juergen Gross wrote:
>>>>>>> Providing a macro for an invalid grant reference would be beneficial
>>>>>>> for users, especially as some are using the wrong value "0" for that
>>>>>>> purpose.
>>>>>>>
>>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>>>
>>>>>> Over the years I've been considering to add such to the public
>>>>>> interface, perhaps even more than once. But I'm afraid it's not that
>>>>>> easy. In principle 0xffffffff (which btw isn't necessarily ~0u) could
>>>>>
>>>>> I can change that to use 0xffffffff explicitly.
>>>>>
>>>>>> be a valid ref. It is really internal agreement by users of the
>>>>>> interface to set for themselves that they're not ever going to make
>>>>>> a valid grant behind that reference.
>>>>>
>>>>> As the grant reference is an index into the grant table this would
>>>>> limit the size of the grant table to "only" UINT_MAX - 1. I don't
>>>>> think this will be ever a concern (other than an academical one).
>>>>
>>>> That wasn't my point. Limiting the table to one less entry is not a
>>>> big deal indeed. But we have no reason to mandate which gref(s) to
>>>> consider invalid. A guest could consider gref 0 the invalid one.
>>>
>>> With the gref being an index starting with 0 (gref 0 is valid, as it is
>>> used for the console ring page), the natural choice for an invalid
>>> value is the highest one being representable. A gref is of type uint32_t
>>> resulting in this value being 0xffffffff.
>>>
>>> While in theory a grant table could be that large, in practice this
>>> will never happen.
>>>
>>>> The hypervisor doesn't care. Imo this simply is an aspect which is
>>>
>>> This isn't true. The hypervisor needs to allocate resources for being
>>> able to handle the highest possible gref value for a guest. For a v1
>>> grant table this would mean 32GB of grant table size. Are you really
>>> concerned we will ever hit this limit? This isn't at the guest's
>>> choice, after all, as the max grant table size is limited by Xen.
>>
>> If we're not going to hit that limit, what's wrong with declaring the
>> entire upper half of uint32_t space "invalid" for use a gref? If we
>> won't ever go up to 32Gb, we quite certainly also won't ever reach
>> 16Gb. Yes, you probably already guessed it, we can then repeat this
>> process iteratively until we reach 4kb.
> 
> This reasoning is nonsense, and you know it.

Interesting. The underlying "where to draw a boundary" is far from
nonsense, I think. The only way to escape this question is to avoid
introduction of artificial boundaries. While it was me to submit
"SUPPORT.md: limit security support for hosts with very much memory",
I didn't do so because I thought it was a good thing to establish
such a boundary. Nor was I convinced in any way of the precise
value chosen. I merely took on carrying out what was discussed
with the security team in the context of XSA-385, when even that
discussion left it very much to me to pick an arbitrary value.

>>>> not in need of pinning down in the ABI. Yet if it was pinned down
>>>> like you do, then the hypervisor would need to make sure it refuses
>>>> to act on this mandated invalid gref.
>>>
>>> This is an easy one. We could just refuse to have a grant table of
>>> that size. I can add this to the patch if you really think it is
>>> necessary.
>>
>> Since grant table size is measured in pages, you'd then have to
>> refuse use of more than just that single gref. This would still not
>> be an immediate problem, but demonstrates again that it's unclear
>> where to draw such a boundary, if one is to be artificially drawn.
> 
> It should be as high as possible. I wouldn't mind just refusing the
> last possible gref, but I don't think this is necessary.
> 
>>> TBH, I think such completely theoretical concerns should not stand
>>> in the way of additions to the ABI making life easier for consumers.
>>
>> In case it wasn't clear - my concern isn't that sacrificing this one
>> entry may cause a problem, or that we'd ever see grant tables grow
>> this big (albeit for the latter: you never really know). Instead my
>> concern is that it is conceptually wrong for us to (now) introduce
>> such a value.
> 
> I have understood that this is your concern.
> 
> I continue to think that this concern is of purely academical nature.

Well, I'm not going to NAK the change if others agree with your view.
But I'm not going to put my name under it in any form. Academical or
not, it sets a(nother) wrong precedent.

Jan

On 22.02.22 09:55, Jan Beulich wrote:
> On 21.02.2022 16:57, Juergen Gross wrote:
>> On 21.02.22 16:31, Jan Beulich wrote:
>>> On 21.02.2022 16:05, Juergen Gross wrote:
>>>> On 21.02.22 15:31, Jan Beulich wrote:
>>>>> On 21.02.2022 15:27, Juergen Gross wrote:
>>>>>> On 21.02.22 15:18, Jan Beulich wrote:
>>>>>>> On 21.02.2022 13:42, Juergen Gross wrote:
>>>>>>>> Providing a macro for an invalid grant reference would be beneficial
>>>>>>>> for users, especially as some are using the wrong value "0" for that
>>>>>>>> purpose.
>>>>>>>>
>>>>>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>>>>>
>>>>>>> Over the years I've been considering to add such to the public
>>>>>>> interface, perhaps even more than once. But I'm afraid it's not that
>>>>>>> easy. In principle 0xffffffff (which btw isn't necessarily ~0u) could
>>>>>>
>>>>>> I can change that to use 0xffffffff explicitly.
>>>>>>
>>>>>>> be a valid ref. It is really internal agreement by users of the
>>>>>>> interface to set for themselves that they're not ever going to make
>>>>>>> a valid grant behind that reference.
>>>>>>
>>>>>> As the grant reference is an index into the grant table this would
>>>>>> limit the size of the grant table to "only" UINT_MAX - 1. I don't
>>>>>> think this will be ever a concern (other than an academical one).
>>>>>
>>>>> That wasn't my point. Limiting the table to one less entry is not a
>>>>> big deal indeed. But we have no reason to mandate which gref(s) to
>>>>> consider invalid. A guest could consider gref 0 the invalid one.
>>>>
>>>> With the gref being an index starting with 0 (gref 0 is valid, as it is
>>>> used for the console ring page), the natural choice for an invalid
>>>> value is the highest one being representable. A gref is of type uint32_t
>>>> resulting in this value being 0xffffffff.
>>>>
>>>> While in theory a grant table could be that large, in practice this
>>>> will never happen.
>>>>
>>>>> The hypervisor doesn't care. Imo this simply is an aspect which is
>>>>
>>>> This isn't true. The hypervisor needs to allocate resources for being
>>>> able to handle the highest possible gref value for a guest. For a v1
>>>> grant table this would mean 32GB of grant table size. Are you really
>>>> concerned we will ever hit this limit? This isn't at the guest's
>>>> choice, after all, as the max grant table size is limited by Xen.
>>>
>>> If we're not going to hit that limit, what's wrong with declaring the
>>> entire upper half of uint32_t space "invalid" for use a gref? If we
>>> won't ever go up to 32Gb, we quite certainly also won't ever reach
>>> 16Gb. Yes, you probably already guessed it, we can then repeat this
>>> process iteratively until we reach 4kb.
>>
>> This reasoning is nonsense, and you know it.
> 
> Interesting. The underlying "where to draw a boundary" is far from
> nonsense, I think. The only way to escape this question is to avoid
> introduction of artificial boundaries. While it was me to submit

Your reasoning that taking away the upper 0.000000025% of grant ref
space is similar to take away 50% or even 99.9999% is nonsense.

Taking away the last entry of more than 4 billion ones, while today
no usage exceeding 1 million is known (the highest value I know of
is allowing 256 grant frames for a guest, resulting in 65536 grant
refs), is hardly a relevant limitation. This is especially true, as
the grant ref space can't be extended to higher values without a
new interface.

> "SUPPORT.md: limit security support for hosts with very much memory",
> I didn't do so because I thought it was a good thing to establish
> such a boundary. Nor was I convinced in any way of the precise
> value chosen. I merely took on carrying out what was discussed
> with the security team in the context of XSA-385, when even that
> discussion left it very much to me to pick an arbitrary value.
> 
>>>>> not in need of pinning down in the ABI. Yet if it was pinned down
>>>>> like you do, then the hypervisor would need to make sure it refuses
>>>>> to act on this mandated invalid gref.
>>>>
>>>> This is an easy one. We could just refuse to have a grant table of
>>>> that size. I can add this to the patch if you really think it is
>>>> necessary.
>>>
>>> Since grant table size is measured in pages, you'd then have to
>>> refuse use of more than just that single gref. This would still not
>>> be an immediate problem, but demonstrates again that it's unclear
>>> where to draw such a boundary, if one is to be artificially drawn.
>>
>> It should be as high as possible. I wouldn't mind just refusing the
>> last possible gref, but I don't think this is necessary.
>>
>>>> TBH, I think such completely theoretical concerns should not stand
>>>> in the way of additions to the ABI making life easier for consumers.
>>>
>>> In case it wasn't clear - my concern isn't that sacrificing this one
>>> entry may cause a problem, or that we'd ever see grant tables grow
>>> this big (albeit for the latter: you never really know). Instead my
>>> concern is that it is conceptually wrong for us to (now) introduce
>>> such a value.
>>
>> I have understood that this is your concern.
>>
>> I continue to think that this concern is of purely academical nature.
> 
> Well, I'm not going to NAK the change if others agree with your view.
> But I'm not going to put my name under it in any form. Academical or
> not, it sets a(nother) wrong precedent.

Fair enough.


Juergen