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

[Michal Orzel <michal.orzel@xxxxxxx>]

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.

Apart from that, all the Jan comments were addressed, so:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal