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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Michal,

> On 1 Sep 2022, at 1:47 pm, Michal Orzel <michal.orzel@xxxxxxx> wrote:
> 
> Hi Rahul,
> 
> On 01/09/2022 11:13, 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 static event channel support will be added for dom0less domains
>> user can request to allocate the evtchn port numbers that are scattered
>> in nature.
>> 
>> 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>
>> ---
>> Changes in v3:
>> - fix comments in commit msg.
>> - modify code related to d->valid_evtchns and {read,write}_atomic()
>> Changes in v2:
>> - new patch in this version to avoid the security issue
>> ---
>> xen/common/event_channel.c | 55 ++++++++++++++++++++++++--------------
>> 1 file changed, 35 insertions(+), 20 deletions(-)
>> 
>> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
>> index c2c6f8c151..80b06d9743 100644
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -193,6 +193,15 @@ static struct evtchn *alloc_evtchn_bucket(struct domain 
>> *d, unsigned int port)
>>     return NULL;
>> }
>> 
>> +/*
>> + * Allocate a given port and ensure all the buckets up to that ports
>> + * have been allocated.
>> + *
>> + * The last part is important because the rest of the event channel code
>> + * relies on all the buckets up to d->valid_evtchns to be valid. However,
>> + * event channels may be sparsed when restoring a domain during Guest
>> + * Transparent Migration and Live Update.
> You got rid of mentioning these non-existing features from the commit msg,
> but you still mention them here.

I missed that I will fix that in next version.

Regards,
Rahul