RE: [PATCH] Fix failure cleanup in EvtchnFifoAcquire

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----Original Message-----
> From: Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx>
> Sent: Friday, July 18, 2025 9:20 AM
> To: Durrant, Paul <pdurrant@xxxxxxxxxxxx>; win-pv-
> devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Owen Smith <owen.smith@xxxxxxxxx>
> Subject: RE: [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.
> 
> 
> 
> 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
>      }
> 

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.

 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

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