[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

RE: [PATCH] Fix failure cleanup in EvtchnFifoAcquire


  • To: Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx>, "win-pv-devel@xxxxxxxxxxxxxxxxxxxx" <win-pv-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: "Durrant, Paul" <pdurrant@xxxxxxxxxxxx>
  • Date: Fri, 18 Jul 2025 09:03:38 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amazon.co.uk; dmarc=pass action=none header.from=amazon.co.uk; dkim=pass header.d=amazon.co.uk; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector10001; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=lvA4GoITi+fVbIGVH7lnNXTq46x4ZGwCroFZRnf12hE=; b=f740ij8TID+6VvIqrqhdB3Hv3VQ1ypWysOVoR9G3FeqVLzUOXNpYliO+bFsXPwNjn2pEKXnzx2iSnWRzUKTClN9/d1x0uf+T1PruUlCGudxdaGMCa4+Uvb4K4jZ3XlenC7bsmaf727x4pDr+Zzr2dNygpO1Y3oD6k0S6+4eARr8/9T1jLjYnCZnXqbPc/S1HDZ4N9VoUzSqirUzseVluVGzVf170Cj1DTkvwXDRgCEIXDpBaY6oAsmexBn8QbNdYGRKWl2jyHwZJEhvifiyZ3bYNkd3rqHZ5ftskhKLnu9VNI26fVM0YpRgvnZMTWQ6d3YjtdEm61uKK6LJlOuN+4g==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=atLeF3jbV0NOi5DOEK4Cirj/LcoTvtoXiTwnKW3ZSOPVEP4BQJVzNAs1DWezM5P7yf3nw/4taghWSEKEcRxK1+NFIT+M8Krz/bO2eK7XX3K+rkK6yWqhwH1xj2WA3YOl0ovRMGRxLs/yLj9hYsZ8Lw3vJyHtlPMPZ/DTjGXMBA4HIULzSDTZam3o+mMpnErujF4YUx8AqUZ4tA4uLvxIAvzUGHUPBX7fjd1XDvd1tJjCOqeujG4pu9t8+mNx/CgKCoT9baRQjLCOTwOEqEghWgFS7IBwvmj7gnGU87PE+wZTdvSP0X4sug1fDwjptsemeTTEmH7JFShMPotvyzAK0g==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=amazon.co.uk;
  • Cc: Owen Smith <owen.smith@xxxxxxxxx>
  • Delivery-date: Fri, 18 Jul 2025 09:04:01 +0000
  • List-id: Developer list for the Windows PV Drivers subproject <win-pv-devel.lists.xenproject.org>
  • Thread-index: AQHb94C0z/qpxQRpNEOFwkCMZij8nrQ3gWNwgAAJCYCAAAWvkA==
  • Thread-topic: [PATCH] Fix failure cleanup in EvtchnFifoAcquire

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.