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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Jan.

> On 22 Aug 2022, at 2:08 pm, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> 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.

Ok. I will remove this and add that we need this patch to support static event 
channel.
Something like:
“ When static event channel support will be added for dom0less domains
  user can request to allocate the evtchn port numbers that are scattered
  in nature."

> 
>> Changes in v2:
>> - new patch in this version to fix the security issue
> 
> I guess you mean "avoid", not "fix".

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

Ack. I will fix this as suggested by you I next version.

Regards,
Rahul