Re: [PATCH v2 1/7] xen/evtchn: Make sure all buckets below d->valid_evtchns are allocated

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.08.2022 12:02, Rahul Singh wrote:
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> Since commit 01280dc19cf3 "evtchn: simplify port_is_valid()", the event
> channels code assumes that all the buckets below d->valid_evtchns are
> always allocated.
> 
> This assumption hold in most of the situation because a guest is not
> allowed to chose the port. Instead, it will be the first free from port
> 0.
> 
> When using Guest Transparent Migration and LiveUpdate, we will only
> preserve ports that are currently in use. As a guest can open/close
> event channels, this means the ports may be sparse.
> 
> The existing implementation of evtchn_allocate_port() is not able to
> deal with such situation and will end up to override bucket or/and leave
> some bucket unallocated. The latter will result to a droplet crash if
> the event channel belongs to an unallocated bucket.
> 
> This can be solved by making sure that all the buckets below
> d->valid_evtchns are allocated. There should be no impact for most of
> the situation but LM/LU as only one bucket would be allocated. For
> LM/LU, we may end up to allocate multiple buckets if ports in use are
> sparse.
> 
> A potential alternative is to check that the bucket is valid in
> is_port_valid(). This should still possible to do it without taking
> per-domain lock but will result a couple more of memory access.
> 
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>

While I'm mostly okay with the code, I think the description wants
changing / amending as long as the features talked about above aren't
anywhere near reaching upstream (afaict), to at least _also_ mention
the goal you have with this.

> Changes in v2:
>  - new patch in this version to fix the security issue

I guess you mean "avoid", not "fix".

> @@ -207,30 +216,35 @@ int evtchn_allocate_port(struct domain *d, 
> evtchn_port_t port)
>      }
>      else
>      {
> -        struct evtchn *chn;
> -        struct evtchn **grp;
> -
> -        if ( !group_from_port(d, port) )
> +        do
>          {
> -            grp = xzalloc_array(struct evtchn *, BUCKETS_PER_GROUP);
> -            if ( !grp )
> -                return -ENOMEM;
> -            group_from_port(d, port) = grp;
> -        }
> +            struct evtchn *chn;
> +            struct evtchn **grp;
> +            unsigned int alloc_port = read_atomic(&d->valid_evtchns);
>  
> -        chn = alloc_evtchn_bucket(d, port);
> -        if ( !chn )
> -            return -ENOMEM;
> -        bucket_from_port(d, port) = chn;
> +            if ( !group_from_port(d, alloc_port) )
> +            {
> +                grp = xzalloc_array(struct evtchn *, BUCKETS_PER_GROUP);
> +                if ( !grp )
> +                    return -ENOMEM;
> +                group_from_port(d, alloc_port) = grp;
> +            }
>  
> -        /*
> -         * d->valid_evtchns is used to check whether the bucket can be
> -         * accessed without the per-domain lock. Therefore,
> -         * d->valid_evtchns should be seen *after* the new bucket has
> -         * been setup.
> -         */
> -        smp_wmb();
> -        write_atomic(&d->valid_evtchns, d->valid_evtchns + 
> EVTCHNS_PER_BUCKET);
> +            chn = alloc_evtchn_bucket(d, alloc_port);
> +            if ( !chn )
> +                return -ENOMEM;
> +            bucket_from_port(d, alloc_port) = chn;
> +
> +            /*
> +             * d->valid_evtchns is used to check whether the bucket can be
> +             * accessed without the per-domain lock. Therefore,
> +             * d->valid_evtchns should be seen *after* the new bucket has
> +             * been setup.
> +             */
> +            smp_wmb();
> +            write_atomic(&d->valid_evtchns,
> +                         d->valid_evtchns + EVTCHNS_PER_BUCKET);
> +        } while ( port >= read_atomic(&d->valid_evtchns) );

This updating of d->valid_evtchns looks a little inconsistent to me,
wrt the uses of {read,write}_atomic(). To make obvious that there's
an implicit expectation that no 2nd invocation of this function
could race the updates, I'd recommend reading allocate_port ahead
of the loop and then never again. Here you'd then do

            smp_wmb();
            allocate_port += EVTCHNS_PER_BUCKET;
            write_atomic(&d->valid_evtchns, allocate_port);
        } while ( port >= allocate_port );

at the same time rendering the code (imo) a little more legible.

Jan