[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] RE: [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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |