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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

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

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

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

> +                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()).

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

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

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

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

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

Jan

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