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

[Juergen Gross <jgross@xxxxxxxx>]

On 20/09/17 12:34, Jan Beulich wrote:
>>>> On 20.09.17 at 08:34, <jgross@xxxxxxxx> wrote:
>> --- a/xen/common/compat/grant_table.c
>> +++ b/xen/common/compat/grant_table.c
>> @@ -157,21 +157,14 @@ int compat_grant_table_op(unsigned int cmd,
>>                  unsigned int max_frame_list_size_in_page =
>>                      (COMPAT_ARG_XLAT_SIZE - sizeof(*nat.setup)) /
>>                      sizeof(*nat.setup->frame_list.p);
>> -                if ( max_frame_list_size_in_page < max_grant_frames )
> 
> The latest here, but perhaps even earlier I think max_grant_frames
> should become static, so one can be reasonably certain that all other
> references are gone.

Patch 14 removes the last references, so I can't do it earlier.

>> @@ -290,8 +293,8 @@ num_act_frames_from_sha_frames(const unsigned int num)
>>      return DIV_ROUND_UP(num * sha_per_page, ACGNT_PER_PAGE);
>>  }
>>  
>> -#define max_nr_active_grant_frames \
>> -    num_act_frames_from_sha_frames(max_grant_frames)
>> +#define max_nr_active_grant_frames(gt) \
>> +    num_act_frames_from_sha_frames(gt->max_grant_frames)
> 
> Parentheses around gt please.

Okay.

>> @@ -1718,8 +1724,9 @@ gnttab_grow_table(struct domain *d, unsigned int 
>> req_nr_frames)
>>      ASSERT(gt->active);
>>  
>>      if ( req_nr_frames < INITIAL_NR_GRANT_FRAMES )
>> -        req_nr_frames = INITIAL_NR_GRANT_FRAMES;
>> -    ASSERT(req_nr_frames <= max_grant_frames);
>> +        req_nr_frames = min_t(unsigned int, INITIAL_NR_GRANT_FRAMES,
>> +                                            gt->max_grant_frames);
> 
> Perhaps give the INITIAL_NR_GRANT_FRAMES value a U suffix
> instead of using min_t() here?

Okay.

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

>> @@ -1819,6 +1819,21 @@ gnttab_setup_table(
>>      gt = d->grant_table;
>>      grant_write_lock(gt);
>>  
>> +    if ( unlikely(op.nr_frames > gt->max_grant_frames) )
>> +    {
>> +        gdprintk(XENLOG_INFO, "Domain is limited to %d grant-table 
>> frames.\n",
> 
> %u and you also want to log the subject domain ID (which may not
> be current's; same for the other log message below as well as the
> similar one in gnttab_get_status_frames()).

Okay.

>> +                gt->max_grant_frames);
>> +        op.status = GNTST_general_error;
>> +        goto unlock;
>> +    }
>> +    if ( unlikely(limit_max < gt->max_grant_frames) )
> 
> With the check moved here it can be relaxed afaict: Code below
> only writes op.nr_frames entries (same then again for
> gnttab_get_status_frames()).

Okay.

>> @@ -1852,10 +1867,10 @@ gnttab_setup_table(
>>      if ( d )
>>          rcu_unlock_domain(d);
>>  
>> -    if ( unlikely(__copy_field_to_guest(uop, &op, status)) )
>> +    if ( !ret && unlikely(__copy_field_to_guest(uop, &op, status)) )
> 
> I wonder whether it wouldn't be better to switch that check
> producing -EINVAL to also report the failure in op.status, now
> that it lives here (same then for gnttab_get_status_frames()
> once again).

Okay.

>>  static long
>>  gnttab_get_status_frames(XEN_GUEST_HANDLE_PARAM(gnttab_get_status_frames_t) 
>> uop,
>> -                         int count)
>> +                         int count, unsigned int limit_max)
> 
> Take the opportunity and switch count to unsigned int?

Okay.

>> @@ -3320,7 +3347,7 @@ do_grant_table_op(
>>  
>>      case GNTTABOP_setup_table:
>>          rc = gnttab_setup_table(
>> -            guest_handle_cast(uop, gnttab_setup_table_t), count);
>> +            guest_handle_cast(uop, gnttab_setup_table_t), count, ~0);
> 
> UINT_MAX?

Yes.

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

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

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

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/xen/include/asm-arm/grant_table.h
>> +++ b/xen/include/asm-arm/grant_table.h
>> @@ -26,8 +26,8 @@ static inline int replace_grant_supported(void)
>>      return 1;
>>  }
>>  
>> -#define gnttab_init_arch(gt)                                             \
>> -    ( ((gt)->arch.gfn = xzalloc_array(gfn_t, max_grant_frames)) == 0     \
>> +#define gnttab_init_arch(gt)                                               \
>> +    ( ((gt)->arch.gfn = xzalloc_array(gfn_t, (gt)->max_grant_frames)) == 0 \
> 
> Mind switching to use NULL at this occasion?

I'll change it in patch 12 then.


Juergen

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