[PATCH] xen/grants: repurpose command line max options

[PATCH] xen/grants: repurpose command line max options

[Roger Pau Monne]

Slightly change the meaning of the command line
gnttab_max_{maptrack_,}frames: do not use them as upper bounds for the
passed values at domain creation, instead just use them as defaults
in the absence of any provided value.

It's not very useful for the options to be used both as defaults and
as capping values for domain creation inputs.  The defaults passed on
the command line are used by dom0 which has a very different grant
requirements than a regular domU.  dom0 usually needs a bigger
maptrack array, while domU usually require a bigger number of grant
frames.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 docs/misc/xen-command-line.pandoc | 12 ++++++------
 xen/common/grant_table.c          |  9 +++------
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index f68deaa6a9..f09d1b23da 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1232,9 +1232,8 @@ The usage of gnttab v2 is not security supported on ARM platforms.
 
 > Can be modified at runtime
 
-Specify the maximum number of frames which any domain may use as part
-of its grant table. This value is an upper boundary of the per-domain
-value settable via Xen tools.
+Specify the default maximum number of frames which any domain may use as part
+of its grant table unless a different value is specified at domain creation.
 
 Dom0 is using this value for sizing its grant table.
 
@@ -1245,9 +1244,10 @@ Dom0 is using this value for sizing its grant table.
 
 > Can be modified at runtime
 
-Specify the maximum number of frames to use as part of a domains
-maptrack array. This value is an upper boundary of the per-domain
-value settable via Xen tools.
+Specify the default maximum number of frames to use as part of a domains
+maptrack array unless a different value is specified at domain creation.
+
+Dom0 is using this value for sizing its maptrack array.
 
 ### global-pages
     = <boolean>
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index b896f9af0e..627bf4026c 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1956,18 +1956,15 @@ int grant_table_init(struct domain *d, int max_grant_frames,
         return -EINVAL;
     }
 
-    /* Default to maximum value if no value was specified */
+    /* Apply defaults if no value was specified */
     if ( max_grant_frames < 0 )
         max_grant_frames = opt_max_grant_frames;
     if ( max_maptrack_frames < 0 )
         max_maptrack_frames = opt_max_maptrack_frames;
 
-    if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES ||
-         max_grant_frames > opt_max_grant_frames ||
-         max_maptrack_frames > opt_max_maptrack_frames )
+    if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES )
     {
-        dprintk(XENLOG_INFO, "Bad grant table sizes: grant %u, maptrack %u\n",
-                max_grant_frames, max_maptrack_frames);
+        dprintk(XENLOG_INFO, "Bad grant table size %u\n", max_grant_frames);
         return -EINVAL;
     }
 
-- 
2.39.0

Re: [PATCH] xen/grants: repurpose command line max options

[Jan Beulich]

On 13.03.2023 13:16, Roger Pau Monne wrote:
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -1232,9 +1232,8 @@ The usage of gnttab v2 is not security supported on ARM platforms.
>  
>  > Can be modified at runtime
>  
> -Specify the maximum number of frames which any domain may use as part
> -of its grant table. This value is an upper boundary of the per-domain
> -value settable via Xen tools.
> +Specify the default maximum number of frames which any domain may use as part
> +of its grant table unless a different value is specified at domain creation.
>  
>  Dom0 is using this value for sizing its grant table.

dom0less DomU-s do as well, at the very least, also ...

> @@ -1245,9 +1244,10 @@ Dom0 is using this value for sizing its grant table.
>  
>  > Can be modified at runtime
>  
> -Specify the maximum number of frames to use as part of a domains
> -maptrack array. This value is an upper boundary of the per-domain
> -value settable via Xen tools.
> +Specify the default maximum number of frames to use as part of a domains
> +maptrack array unless a different value is specified at domain creation.
> +
> +Dom0 is using this value for sizing its maptrack array.

... here. And even ordinary DomU-s appear to default to that in the
absence of a specific value in the guest config. IOW at the very least
the info you add should not be misleading. Better would be if the pre-
existing info was adjusted at the same time.

I also wonder about the specific wording down here: While the max grant
table size can indeed be queried, this isn't the case for the maptrack
array. A domain also doesn't need to know its size, so maybe "This value
is used to size all domains' maptrack arrays, unless overridden by their
guest config"?

> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1956,18 +1956,15 @@ int grant_table_init(struct domain *d, int max_grant_frames,
>          return -EINVAL;
>      }
>  
> -    /* Default to maximum value if no value was specified */
> +    /* Apply defaults if no value was specified */
>      if ( max_grant_frames < 0 )
>          max_grant_frames = opt_max_grant_frames;
>      if ( max_maptrack_frames < 0 )
>          max_maptrack_frames = opt_max_maptrack_frames;
>  
> -    if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES ||
> -         max_grant_frames > opt_max_grant_frames ||
> -         max_maptrack_frames > opt_max_maptrack_frames )
> +    if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES )
>      {
> -        dprintk(XENLOG_INFO, "Bad grant table sizes: grant %u, maptrack %u\n",
> -                max_grant_frames, max_maptrack_frames);
> +        dprintk(XENLOG_INFO, "Bad grant table size %u\n", max_grant_frames);
>          return -EINVAL;
>      }

I think I agree with the relaxation done here, but I also think this not
introducing security concerns wants spelling out in the description: My
understanding is that even in disaggregated environments we assume only
fully privileged entities can create domains.

Jan

Re: [PATCH] xen/grants: repurpose command line max options

[Roger Pau Monné]

On Mon, Mar 13, 2023 at 05:55:09PM +0100, Jan Beulich wrote:
> On 13.03.2023 13:16, Roger Pau Monne wrote:
> > --- a/docs/misc/xen-command-line.pandoc
> > +++ b/docs/misc/xen-command-line.pandoc
> > @@ -1232,9 +1232,8 @@ The usage of gnttab v2 is not security supported on ARM platforms.
> >  
> >  > Can be modified at runtime
> >  
> > -Specify the maximum number of frames which any domain may use as part
> > -of its grant table. This value is an upper boundary of the per-domain
> > -value settable via Xen tools.
> > +Specify the default maximum number of frames which any domain may use as part
> > +of its grant table unless a different value is specified at domain creation.
> >  
> >  Dom0 is using this value for sizing its grant table.
> 
> dom0less DomU-s do as well, at the very least, also ...
> 
> > @@ -1245,9 +1244,10 @@ Dom0 is using this value for sizing its grant table.
> >  
> >  > Can be modified at runtime
> >  
> > -Specify the maximum number of frames to use as part of a domains
> > -maptrack array. This value is an upper boundary of the per-domain
> > -value settable via Xen tools.
> > +Specify the default maximum number of frames to use as part of a domains
> > +maptrack array unless a different value is specified at domain creation.
> > +
> > +Dom0 is using this value for sizing its maptrack array.
> 
> ... here. And even ordinary DomU-s appear to default to that in the
> absence of a specific value in the guest config. IOW at the very least
> the info you add should not be misleading. Better would be if the pre-
> existing info was adjusted at the same time.

Aren't domUs already clearly covered by the sentence:

"Specify the default maximum number of frames to use as part of a domains..."

IMO dom0 needs to be explicitly mentioned because in that case the
value provided is not the one used by default, but rather the one that
gets used.

> I also wonder about the specific wording down here: While the max grant
> table size can indeed be queried, this isn't the case for the maptrack
> array. A domain also doesn't need to know its size, so maybe "This value
> is used to size all domains' maptrack arrays, unless overridden by their
> guest config"?

I think the wording I've added already conveys this meaning:

"Specify the default maximum number of frames to use as part of a domains
maptrack array unless a different value is specified at domain creation."

> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -1956,18 +1956,15 @@ int grant_table_init(struct domain *d, int max_grant_frames,
> >          return -EINVAL;
> >      }
> >  
> > -    /* Default to maximum value if no value was specified */
> > +    /* Apply defaults if no value was specified */
> >      if ( max_grant_frames < 0 )
> >          max_grant_frames = opt_max_grant_frames;
> >      if ( max_maptrack_frames < 0 )
> >          max_maptrack_frames = opt_max_maptrack_frames;
> >  
> > -    if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES ||
> > -         max_grant_frames > opt_max_grant_frames ||
> > -         max_maptrack_frames > opt_max_maptrack_frames )
> > +    if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES )
> >      {
> > -        dprintk(XENLOG_INFO, "Bad grant table sizes: grant %u, maptrack %u\n",
> > -                max_grant_frames, max_maptrack_frames);
> > +        dprintk(XENLOG_INFO, "Bad grant table size %u\n", max_grant_frames);
> >          return -EINVAL;
> >      }
> 
> I think I agree with the relaxation done here, but I also think this not
> introducing security concerns wants spelling out in the description: My
> understanding is that even in disaggregated environments we assume only
> fully privileged entities can create domains.

Yes, that's my understanding, as domain creation can only be done by
privileged domains.  Of course when using a custom XSM policy
the permissions can be changed, but it's then the job of the user to
asses the security implications in that case, and there are existing
paths to cause resource exhausting when having access to the domain
create hypercall. I can add:

"The relaxation in the logic for the maximum size of the grant and
maptrack table sizes doesn't change the fact that domain creation
hypercall can cause resource exhausting, so disaggregated setups
should take it into account."

But domain creation for example also allows creating a domain that has
MSR relaxed, at which point it could also be vulnerable to other
issues.

Thanks, Roger.

Re: [PATCH] xen/grants: repurpose command line max options

[Jan Beulich]

On 14.03.2023 10:22, Roger Pau Monné wrote:
> On Mon, Mar 13, 2023 at 05:55:09PM +0100, Jan Beulich wrote:
>> On 13.03.2023 13:16, Roger Pau Monne wrote:
>>> --- a/docs/misc/xen-command-line.pandoc
>>> +++ b/docs/misc/xen-command-line.pandoc
>>> @@ -1232,9 +1232,8 @@ The usage of gnttab v2 is not security supported on ARM platforms.
>>>  
>>>  > Can be modified at runtime
>>>  
>>> -Specify the maximum number of frames which any domain may use as part
>>> -of its grant table. This value is an upper boundary of the per-domain
>>> -value settable via Xen tools.
>>> +Specify the default maximum number of frames which any domain may use as part
>>> +of its grant table unless a different value is specified at domain creation.
>>>  
>>>  Dom0 is using this value for sizing its grant table.
>>
>> dom0less DomU-s do as well, at the very least, also ...
>>
>>> @@ -1245,9 +1244,10 @@ Dom0 is using this value for sizing its grant table.
>>>  
>>>  > Can be modified at runtime
>>>  
>>> -Specify the maximum number of frames to use as part of a domains
>>> -maptrack array. This value is an upper boundary of the per-domain
>>> -value settable via Xen tools.
>>> +Specify the default maximum number of frames to use as part of a domains
>>> +maptrack array unless a different value is specified at domain creation.
>>> +
>>> +Dom0 is using this value for sizing its maptrack array.
>>
>> ... here. And even ordinary DomU-s appear to default to that in the
>> absence of a specific value in the guest config. IOW at the very least
>> the info you add should not be misleading. Better would be if the pre-
>> existing info was adjusted at the same time.
> 
> Aren't domUs already clearly covered by the sentence:
> 
> "Specify the default maximum number of frames to use as part of a domains..."

Hmm, yes, my attention was caught too much by the Dom0 statement. While ...

> IMO dom0 needs to be explicitly mentioned because in that case the
> value provided is not the one used by default, but rather the one that
> gets used.

... explicitly mentioning Dom0 is fine, I still think this needs wording
differently here, because Dom0 doesn't actively do anything with this
value (and, as said, it can't even obtain it other than by probing how
many mappings it can create).

>> I also wonder about the specific wording down here: While the max grant
>> table size can indeed be queried, this isn't the case for the maptrack
>> array. A domain also doesn't need to know its size, so maybe "This value
>> is used to size all domains' maptrack arrays, unless overridden by their
>> guest config"?
> 
> I think the wording I've added already conveys this meaning:
> 
> "Specify the default maximum number of frames to use as part of a domains
> maptrack array unless a different value is specified at domain creation."

Well, I mean specifically the Dom0 related statement.

Also to me "default maximum" reads odd (and slightly ambiguous). Would
"default upper bound on the number of ..." perhaps be a little better?

>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -1956,18 +1956,15 @@ int grant_table_init(struct domain *d, int max_grant_frames,
>>>          return -EINVAL;
>>>      }
>>>  
>>> -    /* Default to maximum value if no value was specified */
>>> +    /* Apply defaults if no value was specified */
>>>      if ( max_grant_frames < 0 )
>>>          max_grant_frames = opt_max_grant_frames;
>>>      if ( max_maptrack_frames < 0 )
>>>          max_maptrack_frames = opt_max_maptrack_frames;
>>>  
>>> -    if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES ||
>>> -         max_grant_frames > opt_max_grant_frames ||
>>> -         max_maptrack_frames > opt_max_maptrack_frames )
>>> +    if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES )
>>>      {
>>> -        dprintk(XENLOG_INFO, "Bad grant table sizes: grant %u, maptrack %u\n",
>>> -                max_grant_frames, max_maptrack_frames);
>>> +        dprintk(XENLOG_INFO, "Bad grant table size %u\n", max_grant_frames);
>>>          return -EINVAL;
>>>      }
>>
>> I think I agree with the relaxation done here, but I also think this not
>> introducing security concerns wants spelling out in the description: My
>> understanding is that even in disaggregated environments we assume only
>> fully privileged entities can create domains.
> 
> Yes, that's my understanding, as domain creation can only be done by
> privileged domains.  Of course when using a custom XSM policy
> the permissions can be changed, but it's then the job of the user to
> asses the security implications in that case, and there are existing
> paths to cause resource exhausting when having access to the domain
> create hypercall. I can add:
> 
> "The relaxation in the logic for the maximum size of the grant and
> maptrack table sizes doesn't change the fact that domain creation
> hypercall can cause resource exhausting, so disaggregated setups
> should take it into account."

SGTM, and ...

> But domain creation for example also allows creating a domain that has
> MSR relaxed, at which point it could also be vulnerable to other
> issues.

... I think while what you say is true, it still is relevant to point
out for every change which may cause yet another way of running out of
resources.

Jan

Re: [PATCH] xen/grants: repurpose command line max options

[Roger Pau Monné]

On Tue, Mar 14, 2023 at 11:04:21AM +0100, Jan Beulich wrote:
> On 14.03.2023 10:22, Roger Pau Monné wrote:
> > On Mon, Mar 13, 2023 at 05:55:09PM +0100, Jan Beulich wrote:
> >> On 13.03.2023 13:16, Roger Pau Monne wrote:
> >>> --- a/docs/misc/xen-command-line.pandoc
> >>> +++ b/docs/misc/xen-command-line.pandoc
> >>> @@ -1232,9 +1232,8 @@ The usage of gnttab v2 is not security supported on ARM platforms.
> >>>  
> >>>  > Can be modified at runtime
> >>>  
> >>> -Specify the maximum number of frames which any domain may use as part
> >>> -of its grant table. This value is an upper boundary of the per-domain
> >>> -value settable via Xen tools.
> >>> +Specify the default maximum number of frames which any domain may use as part
> >>> +of its grant table unless a different value is specified at domain creation.
> >>>  
> >>>  Dom0 is using this value for sizing its grant table.
> >>
> >> dom0less DomU-s do as well, at the very least, also ...
> >>
> >>> @@ -1245,9 +1244,10 @@ Dom0 is using this value for sizing its grant table.
> >>>  
> >>>  > Can be modified at runtime
> >>>  
> >>> -Specify the maximum number of frames to use as part of a domains
> >>> -maptrack array. This value is an upper boundary of the per-domain
> >>> -value settable via Xen tools.
> >>> +Specify the default maximum number of frames to use as part of a domains
> >>> +maptrack array unless a different value is specified at domain creation.
> >>> +
> >>> +Dom0 is using this value for sizing its maptrack array.
> >>
> >> ... here. And even ordinary DomU-s appear to default to that in the
> >> absence of a specific value in the guest config. IOW at the very least
> >> the info you add should not be misleading. Better would be if the pre-
> >> existing info was adjusted at the same time.
> > 
> > Aren't domUs already clearly covered by the sentence:
> > 
> > "Specify the default maximum number of frames to use as part of a domains..."
> 
> Hmm, yes, my attention was caught too much by the Dom0 statement. While ...
> 
> > IMO dom0 needs to be explicitly mentioned because in that case the
> > value provided is not the one used by default, but rather the one that
> > gets used.
> 
> ... explicitly mentioning Dom0 is fine, I still think this needs wording
> differently here, because Dom0 doesn't actively do anything with this
> value (and, as said, it can't even obtain it other than by probing how
> many mappings it can create).
> 
> >> I also wonder about the specific wording down here: While the max grant
> >> table size can indeed be queried, this isn't the case for the maptrack
> >> array. A domain also doesn't need to know its size, so maybe "This value
> >> is used to size all domains' maptrack arrays, unless overridden by their
> >> guest config"?
> > 
> > I think the wording I've added already conveys this meaning:
> > 
> > "Specify the default maximum number of frames to use as part of a domains
> > maptrack array unless a different value is specified at domain creation."
> 
> Well, I mean specifically the Dom0 related statement.
> 
> Also to me "default maximum" reads odd (and slightly ambiguous). Would
> "default upper bound on the number of ..." perhaps be a little better?

So what about using:

"Specify the default upper bound on the number of frames which any
domain may use as part of its grant table unless a different value is
specified at domain creation.

Note this value is the enforced upper bound for dom0."

And similar for the maptrack option.

Thanks, Roger.

Re: [PATCH] xen/grants: repurpose command line max options

[Jan Beulich]

On 14.03.2023 11:25, Roger Pau Monné wrote:
> On Tue, Mar 14, 2023 at 11:04:21AM +0100, Jan Beulich wrote:
>> On 14.03.2023 10:22, Roger Pau Monné wrote:
>>> On Mon, Mar 13, 2023 at 05:55:09PM +0100, Jan Beulich wrote:
>>>> On 13.03.2023 13:16, Roger Pau Monne wrote:
>>>>> --- a/docs/misc/xen-command-line.pandoc
>>>>> +++ b/docs/misc/xen-command-line.pandoc
>>>>> @@ -1232,9 +1232,8 @@ The usage of gnttab v2 is not security supported on ARM platforms.
>>>>>  
>>>>>  > Can be modified at runtime
>>>>>  
>>>>> -Specify the maximum number of frames which any domain may use as part
>>>>> -of its grant table. This value is an upper boundary of the per-domain
>>>>> -value settable via Xen tools.
>>>>> +Specify the default maximum number of frames which any domain may use as part
>>>>> +of its grant table unless a different value is specified at domain creation.
>>>>>  
>>>>>  Dom0 is using this value for sizing its grant table.
>>>>
>>>> dom0less DomU-s do as well, at the very least, also ...
>>>>
>>>>> @@ -1245,9 +1244,10 @@ Dom0 is using this value for sizing its grant table.
>>>>>  
>>>>>  > Can be modified at runtime
>>>>>  
>>>>> -Specify the maximum number of frames to use as part of a domains
>>>>> -maptrack array. This value is an upper boundary of the per-domain
>>>>> -value settable via Xen tools.
>>>>> +Specify the default maximum number of frames to use as part of a domains
>>>>> +maptrack array unless a different value is specified at domain creation.
>>>>> +
>>>>> +Dom0 is using this value for sizing its maptrack array.
>>>>
>>>> ... here. And even ordinary DomU-s appear to default to that in the
>>>> absence of a specific value in the guest config. IOW at the very least
>>>> the info you add should not be misleading. Better would be if the pre-
>>>> existing info was adjusted at the same time.
>>>
>>> Aren't domUs already clearly covered by the sentence:
>>>
>>> "Specify the default maximum number of frames to use as part of a domains..."
>>
>> Hmm, yes, my attention was caught too much by the Dom0 statement. While ...
>>
>>> IMO dom0 needs to be explicitly mentioned because in that case the
>>> value provided is not the one used by default, but rather the one that
>>> gets used.
>>
>> ... explicitly mentioning Dom0 is fine, I still think this needs wording
>> differently here, because Dom0 doesn't actively do anything with this
>> value (and, as said, it can't even obtain it other than by probing how
>> many mappings it can create).
>>
>>>> I also wonder about the specific wording down here: While the max grant
>>>> table size can indeed be queried, this isn't the case for the maptrack
>>>> array. A domain also doesn't need to know its size, so maybe "This value
>>>> is used to size all domains' maptrack arrays, unless overridden by their
>>>> guest config"?
>>>
>>> I think the wording I've added already conveys this meaning:
>>>
>>> "Specify the default maximum number of frames to use as part of a domains
>>> maptrack array unless a different value is specified at domain creation."
>>
>> Well, I mean specifically the Dom0 related statement.
>>
>> Also to me "default maximum" reads odd (and slightly ambiguous). Would
>> "default upper bound on the number of ..." perhaps be a little better?
> 
> So what about using:
> 
> "Specify the default upper bound on the number of frames which any
> domain may use as part of its grant table unless a different value is
> specified at domain creation.
> 
> Note this value is the enforced upper bound for dom0."
> 
> And similar for the maptrack option.

SGTM. (Maybe soften a little by using "effective" instead of "enforced",
but only if you don't mean to emphasize the "enforce" aspect.)

Jan