Re: [PATCH] Fix failure cleanup in EvtchnFifoAcquire

["Tu Dinh" <ngoc-tu.dinh@xxxxxxxxxx>]

Hi Paul,

On 18/07/2025 09:49, Durrant, Paul wrote:
>> -----Original Message-----
>> From: win-pv-devel <win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf
>> Of Tu Dinh
>> Sent: Friday, July 18, 2025 2:08 AM
>> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
>> Cc: Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx>; Owen Smith <owen.smith@xxxxxxxxx>
>> Subject: [EXTERNAL] [PATCH] Fix failure cleanup in EvtchnFifoAcquire
>>
>> CAUTION: This email originated from outside of the organization. Do not
>> click links or open attachments unless you can confirm the sender and know
>> the content is safe.
>>
>>
>>
>> The current index is incremented before checking for failure:
>>
> 
> I think that is by design.
> 
>>      while (Index < ProcessorCount) {
>>          Index++;
>>          [...]
>>          if (!NT_SUCCESS(status))
>>              goto fail1;
>>          Context->ControlBlockMdl[vcpu_id] = Mdl;
>>      }
>>
>> Decrement the index before going into the cleanup loop to avoid calling
>> __FreePage on invalid PMDLs.
>>
>> Signed-off-by: Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx>
>> ---
>>   src/xenbus/evtchn_fifo.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/src/xenbus/evtchn_fifo.c b/src/xenbus/evtchn_fifo.c
>> index 1137dea..ed78815 100644
>> --- a/src/xenbus/evtchn_fifo.c
>> +++ b/src/xenbus/evtchn_fifo.c
>> @@ -561,6 +561,7 @@ fail1:
>>
>>       EvtchnReset();
>>
>> +    Index--;
> 
> I think this is wrong because...
> 
>>       while (--Index >= 0) {
> 
> ... there is a pre-decrement here before the value of Index is used below.
> 
> Paul

It looks unintuitive, but Index is incremented very early into the main 
loop, before anything has a chance to fail. So Index will actually get 
double-incremented:

     Index = 0
     while (Index < ProcessorCount) {
         vcpu_id = Index = 0; // as an example
         Index++; // 1
         // success
         Context->ControlBlockMdl[vcpu_id] = Mdl;
     }
     while (Index < ProcessorCount) {
         vcpu_id = Index = 1;
         Index++; // 2
         // failure
         goto fail;
     }

     fail:
     // the second loop incremented Index again without doing any work
     Index--; // 1
     // Index is now the count of successful loop, so...
     while (--Index >= 0) {
         // need to decrement it again to get to the successful index
         // Index=0
     }

> 
>>           unsigned int    vcpu_id;
>>
>> --
>> 2.50.1.windows.1
>>
>>
>>
>> Ngoc Tu Dinh | Vates XCP-ng Developer
>>
>> XCP-ng & Xen Orchestra - Vates solutions
>>
>> web: https://vates.tech
>>
> 



Ngoc Tu Dinh | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech