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

From: Julien Grall <julien@xen.org>
To: Jan Beulich <jbeulich@suse.com>, Rahul Singh <rahul.singh@arm.com>
Cc: bertrand.marquis@arm.com, Julien Grall <jgrall@amazon.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2 1/7] xen/evtchn: Make sure all buckets below d->valid_evtchns are allocated
Date: Tue, 23 Aug 2022 09:14:34 +0100	[thread overview]
Message-ID: <9478285a-2a8a-de07-d6bc-4d9e043b7ccf@xen.org> (raw)
In-Reply-To: <90ea98d7-58f1-4808-b691-c3a773a0476d@suse.com>

Hi Jan,

On 22/08/2022 14:08, Jan Beulich wrote:
> On 19.08.2022 12:02, Rahul Singh wrote:
>> From: Julien Grall <jgrall@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 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@amazon.com>
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> 
> 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.

Correct, neither Guest Transparent Migration nor Live-Update is going to 
reach Xen in 4.17 :). Also, if we decide to continue to mention it, then
we would need to s/Guest Transparent Migration/non-cooperative 
migration/ to match the name we decided to use in upstream (see 
docs/designs/non-cooperative-migration.md).

> 
>> 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 );

I know it is my code. But I agree with this comment :).

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

-- 
Julien Grall