Hi Rahul,
On 01/09/2022 11:13, Rahul Singh wrote:
>
> From: Julien Grall <jgr...@amazon.com>
>
> 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 <jgr...@amazon.com>
> Signed-off-by: Rahul Singh <rahul.si...@arm.com>
> ---
> 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.or...@amd.com>
~Michal
