Re: [PATCH v2] Fix failure cleanup in EvtchnFifoAcquire

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

On 18/07/2025 12:16, Durrant, Paul wrote:
>> -----Original Message-----
>> From: win-pv-devel <win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf
>> Of Tu Dinh
>> Sent: Friday, July 18, 2025 10:59 AM
>> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
>> Cc: Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx>; Owen Smith <owen.smith@xxxxxxxxx>;
>> Durrant, Paul <pdurrant@xxxxxxxxxxxx>
>> Subject: [EXTERNAL] [PATCH v2] 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:
>>
>>      while (Index < ProcessorCount) {
>>          Index++;
>>          [...]
>>          if (!NT_SUCCESS(status))
>>              goto fail1;
>>          Context->ControlBlockMdl[vcpu_id] = Mdl;
>>      }
>>
>> Make the main loop a normal for loop to avoid calling __FreePage on
>> invalid PMDLs.
>>
>> Signed-off-by: Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx>
>> ---
>>   src/xenbus/evtchn_fifo.c | 5 +----
>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/src/xenbus/evtchn_fifo.c b/src/xenbus/evtchn_fifo.c
>> index 1137dea..fd06ef4 100644
>> --- a/src/xenbus/evtchn_fifo.c
>> +++ b/src/xenbus/evtchn_fifo.c
>> @@ -502,16 +502,13 @@ EvtchnFifoAcquire(
>>
>>       ProcessorCount =
>> KeQueryMaximumProcessorCountEx(ALL_PROCESSOR_GROUPS);
>>
>> -    Index = 0;
>> -    while (Index < ProcessorCount) {
>> +    for (Index = 0; Index < ProcessorCount; Index++) {
> 
> Prevailing style uses while loops I think so stick with that for consistency, 
> just move the increment down.

I don't see how the increment could be moved down without breaking the 
index skip when SystemProcessorVcpuId() == STATUS_NOT_SUPPORTED.

I also find the while loop usage in EvtchnFifoAcquire very confusing, 
and it was the cause of the cleanup bug in the first place. So I'd 
prefer to make it more idiomatic this way.

> 
>>           unsigned int        vcpu_id;
>>           PFN_NUMBER          Pfn;
>>           PHYSICAL_ADDRESS    Address;
>>
>>           status = SystemProcessorVcpuId(Index, &vcpu_id);
>>
>> -        Index++;
>> -
>>           if (status == STATUS_NOT_SUPPORTED)
>>               continue;
>>
>> --
>> 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