Re: [PATCH v8 13/15] xen: make grant resource limits per domain

[Juergen Gross <jgross@suse.com>]

On 20/09/17 13:48, Jan Beulich wrote:
>>>> On 20.09.17 at 13:10, <jgross@suse.com> wrote:
>> On 20/09/17 12:34, Jan Beulich wrote:
>>>>>> On 20.09.17 at 08:34, <jgross@suse.com> wrote:
>>>> --- a/xen/common/compat/grant_table.c
>>>> +++ b/xen/common/compat/grant_table.c
>>>> @@ -1777,13 +1784,15 @@ active_alloc_failed:
>>>>  
>>>>  static long
>>>>  gnttab_setup_table(
>>>> -    XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int count)
>>>> +    XEN_GUEST_HANDLE_PARAM(gnttab_setup_table_t) uop, unsigned int count,
>>>> +    unsigned int limit_max)
>>>>  {
>>>>      struct vcpu *curr = current;
>>>>      struct gnttab_setup_table op;
>>>>      struct domain *d = NULL;
>>>>      struct grant_table *gt;
>>>>      unsigned int i;
>>>> +    long ret = 0;
>>>
>>> Wouldn't int suffice here?
>>
>> I just followed the return type of the function. I think this way is
>> cleaner, but in case you like int better I can change it.
> 
> I sort of expected this reply, but that's not in line with what you
> did in gnttab_get_status_frames() then. I think we will want to
> eventually change all function return types to int where the wider
> type isn't needed.

Okay. Should I include a patch doing that in this series or would you
prefer this cleanup being delayed to 4.11?

>>>> @@ -3442,6 +3469,8 @@ grant_table_create(
>>>>      /* Simple stuff. */
>>>>      percpu_rwlock_resource_init(&t->lock, grant_rwlock);
>>>>      spin_lock_init(&t->maptrack_lock);
>>>> +    t->max_grant_frames = max_grant_frames;
>>>> +    t->max_maptrack_frames = max_maptrack_frames;
>>>
>>> This together with ...
>>>
>>>> @@ -3655,7 +3686,11 @@ int grant_table_set_limits(struct domain *d, unsigned int grant_frames,
>>>>  
>>>>      /* Set limits. */
>>>>      if ( !gt->active )
>>>> +    {
>>>> +        gt->max_grant_frames = grant_frames;
>>>> +        gt->max_maptrack_frames = maptrack_frames;
>>>>          ret = grant_table_init(gt);
>>>> +    }
>>>
>>> .. this raises the question of whether it is legal to decrease the
>>> limits. There may be code depending on it only ever growing.
>>
>> Before grant_table_init() has been called there is no problem
>> decreasing the limits, as nothing depending on them has been setup
>> yet.
> 
> Oh, right, I didn't pay attention to this being a one-time action.
> 
>>> Additionally to take the input values without applying some
>>> upper cap - while we have XSA-77, we still shouldn't introduce
>>> new issues making disaggregation more unsafe. Perhaps the
>>> global limits could serve as a cap here?

Thinking more about it: what can happen in worst case when no cap
is being enforced?

A domain with the privilege to create another domain with arbitrary
amounts of memory (we have no cap here) might go crazy and give the
created domain an arbitrary amount of grant frames or maptrack
frames. So what is the difference whether the memory is spent directly
for the domain or via grant frames? In both cases there will be no
memory left for other purposes. I can't see how this would be worse
than allocating the same amount of memory directly for the new domain.

>> I thought about a cap and TBH I'm not sure which would be sane to
>> apply. The global limits seem wrong, especially looking at patch 14:
>> those limits will be for dom0 only then. And dom0 won't need many
>> grant frames in the normal case...
> 
> I've been thinking about this Dom0 aspect too over lunch. What
> about allowing the hardware domain to set its limit (only upwards
> of course) in setup_table(), without any upper bound enforced?
> This would free up the globals to be used as system wide limits
> again.

This would be possible, of course.

The question is whether the need to re-allocate the frame pointer arrays
is it worth.

>> So I could make up a cap in form of either a configurable constant
>> (CONFIG_* or boot parameter?) or as a fraction of domain memory. Any
>> preferences here?
> 
> A config constant as well as a fraction of domain memory might lock
> out special purpose guests. Which would leave command line options
> - as per above perhaps the ones we already have.

In case we really need the cap parameters I'd prefer distinct ones from
the dom0 initial values.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel