Re: [PATCH v2] xen/vcpu: Rework sanity checks in vcpu_create()

["Jan Beulich" <JBeulich@suse.com>]

>>> On 11.09.18 at 18:46, <andrew.cooper3@citrix.com> wrote:
> Poisoning idle_vcpu[0] with the sanity debug value isn't actually a clever
> idea, because it passes a NULL pointer check but isn't a usable vcpu.  It is
> also the reason for the (!is_idle_domain(d) || vcpu_id) part of the existing
> sanity BUG_ON().

But you completely discount the intended effect of this poisoning:
During early boot, a NULL deref is liable to not fault, while a deref
of INVALID_VCPU is always going to (on x86 at least).

> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -138,7 +138,21 @@ struct vcpu *vcpu_create(

This patch does not look to be based on current staging.

>  {
>      struct vcpu *v;
>  
> -    BUG_ON((!is_idle_domain(d) || vcpu_id) && d->vcpu[vcpu_id]);
> +    /*
> +     * Sanity check some input expectations:
> +     *  - d->max_vcpus and d->vcpu[] should be set up
> +     *  - vcpu_id should be bounded by d->max_vcpus
> +     *  - Vcpus should be tightly packed and allocated in ascending order
> +     *    (except for the idle domain).
> +     *  - No previous vcpu with this id should be allocated
> +     */
> +    if ( !d->max_vcpus || !d->vcpu || vcpu_id >= d->max_vcpus ||
> +         (!is_idle_domain(d) && vcpu_id && !d->vcpu[vcpu_id - 1]) ||

Note how you still require an is_idle_domain() special case here
anyway.

> +         d->vcpu[vcpu_id] )
> +    {
> +        ASSERT_UNREACHABLE();
> +        return NULL;

I assume you consider it acceptable for release builds to report
back -ENOMEM in the (hopefully indeed impossible) case of
execution going this path?

> @@ -178,16 +192,10 @@ struct vcpu *vcpu_create(
>      if ( arch_vcpu_create(v) != 0 )
>          goto fail_sched;
>  
> +    /* Insert the vcpu into the domain's vcpu list. */
>      d->vcpu[vcpu_id] = v;
> -    if ( vcpu_id != 0 )
> -    {
> -        int prev_id = v->vcpu_id - 1;
> -        while ( (prev_id >= 0) && (d->vcpu[prev_id] == NULL) )
> -            prev_id--;
> -        BUG_ON(prev_id < 0);
> -        v->next_in_list = d->vcpu[prev_id]->next_in_list;
> -        d->vcpu[prev_id]->next_in_list = v;
> -    }
> +    if ( !is_idle_domain(d) && vcpu_id > 0 )
> +        d->vcpu[vcpu_id - 1]->next_in_list = v;

While before this change for_each_vcpu(idle_domain) was
broken only for the (currently impossible on x86 at least) case
of CPU0 not being online (nor parked), afaict it will now be
broken altogether, leading to NULL deref-s when used. Is that
really what you want (if so, the description should say so, and
why that's okay)?

Jan



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