Re: [PATCH] Fix failure cleanup in EvtchnFifoAcquire

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

Hi Paul,

On 18/07/2025 11:04, Durrant, Paul wrote:
[...]
> 
> Hi Tu,
> 
>    Are we looking at the same function? This is what I see...
> 
>    The loop starts at 
> https://xenbits.xen.org/gitweb/?p=pvdrivers/win/xenbus.git;a=blob;f=src/xenbus/evtchn_fifo.c;hb=HEAD#l505:
> 
>   505     Index = 0;
>   506     while (Index < ProcessorCount) {
>   507         unsigned int        vcpu_id;
>   508         PFN_NUMBER          Pfn;
>   509         PHYSICAL_ADDRESS    Address;
>   510
>   511         status = SystemProcessorVcpuId(Index, &vcpu_id);
>   512
>   513         Index++;
>   514
> 
> So Index is incremented before the checks:
> 
>   515         if (status == STATUS_NOT_SUPPORTED)
>   516             continue;
>   517
>   518         if (!NT_SUCCESS(status))
>   519             goto fail1;
> 
> So, say we have done one loop round then we'll have set up the control block 
> for the vcpu_id corresponding to Index 0. Index will now be 1 and let's say 
> we hit the fail.

After doing one successful loop round, Index will be 1. But as soon as 
you enter the second loop round, Index would have become 2 before you 
could hit any failure, due to the Index++ near the very top.

> 
>   559 fail1:
>   560     Error("fail1 (%08x)\n", status);
>   561
>   562     EvtchnReset();
>   563
>   564     while (--Index >= 0) {
> 
> Index will be decremented back to 0...
> 
>   565         unsigned int    vcpu_id;
>   566
>   567         status = SystemProcessorVcpuId(Index, &vcpu_id);
> 
> We look up the corresponding vcpu_id again...
> 
>   568         if (status == STATUS_NOT_SUPPORTED)
>   569             continue;
>   570
>   571         BUG_ON(!NT_SUCCESS(status));
>   572
>   573         Mdl = Context->ControlBlockMdl[vcpu_id];
>   574         Context->ControlBlockMdl[vcpu_id] = NULL;
> 
> ... and we free off the control block that was allocated before.
> 
>   575
>   576         __FreePage(Mdl);
>   577     }
> 
> I don't see any evidence of a double increment. The existing code looks 
> correct to me.
> 
>    Paul
> 
[...]



Ngoc Tu Dinh | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech