Message ID | 20240619065543.31258-1-svc.ovs-community@vmware.com |
---|---|
State | Changes Requested |
Delegated to: | Alin Gabriel Serdean |
Headers | show |
Series | [ovs-dev,v3,1/1] datapath-windows : Correct the failure handling in OvsInitConntrack. | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
ovsrobot/intel-ovs-compilation | success | test: success |
Hi Wilson, Sorry for the delay and thank you for the patch. Apologies for the format but unfortunately I don't have a proper mail client on this machine. I hope this will be readable. B<-------------------------------------- diff --git a/datapath-windows/ovsext/Conntrack-nat.c b/datapath-windows/ovsext/Conntrack-nat.c index 497354ec8..a222f6aea 100644 --- a/datapath-windows/ovsext/Conntrack-nat.c +++ b/datapath-windows/ovsext/Conntrack-nat.c @@ -108,6 +108,7 @@ NTSTATUS OvsNatInit() OVS_CT_POOL_TAG); if (ovsUnNatTable == NULL) { OvsFreeMemoryWithTag(ovsNatTable, OVS_CT_POOL_TAG); + ovsNatTable = NULL; return STATUS_INSUFFICIENT_RESOURCES; } @@ -157,7 +158,6 @@ VOID OvsNatFlush(UINT16 zone) VOID OvsNatCleanup() { if (ovsNatTable == NULL) { - NdisFreeSpinLock(&ovsCtNatLock); return; } [Alin] Nice catch diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c index 39ba5cc10..156861d6c 100644 --- a/datapath-windows/ovsext/Conntrack.c +++ b/datapath-windows/ovsext/Conntrack.c @@ -94,16 +94,28 @@ OvsInitConntrack(POVS_SWITCH_CONTEXT context) if (status != STATUS_SUCCESS) { goto freeBucketLock; } - - ObReferenceObjectByHandle(threadHandle, SYNCHRONIZE, NULL, KernelMode, - &ctThreadCtx.threadObject, NULL); + ctThreadCtx.exit = 0; + status = ObReferenceObjectByHandle(threadHandle, SYNCHRONIZE, NULL, KernelMode, + &ctThreadCtx.threadObject, NULL); ZwClose(threadHandle); threadHandle = NULL; + if (!NT_SUCCESS(status)) { + ctThreadCtx.exit = 1; + KeSetEvent(&ctThreadCtx.event, 0, FALSE); + KeWaitForSingleObject(ctThreadCtx.threadObject, Executive, + KernelMode, FALSE, NULL); + goto freeBucketLock; + } [Alin] This is already done in OvsCleanupConntrack lets just readjust the code and call it in case of failure. zoneInfo = OvsAllocateMemoryWithTag(sizeof(OVS_CT_ZONE_INFO) * CT_MAX_ZONE, OVS_CT_POOL_TAG); if (zoneInfo == NULL) { status = STATUS_INSUFFICIENT_RESOURCES; + ctThreadCtx.exit = 1; + KeSetEvent(&ctThreadCtx.event, 0, FALSE); + KeWaitForSingleObject(ctThreadCtx.threadObject, Executive, + KernelMode, FALSE, NULL); + ObDereferenceObject(ctThreadCtx.threadObject); goto freeBucketLock; } [Alin] Same as above lets call OvsCleanupConntrack. @@ -119,7 +131,7 @@ OvsInitConntrack(POVS_SWITCH_CONTEXT context) if (status != STATUS_SUCCESS) { OvsCleanupConntrack(); } - return STATUS_SUCCESS; + return status; freeBucketLock: for (UINT32 i = 0; i < numBucketLocks; i++) { @@ -172,6 +184,7 @@ OvsCleanupConntrack(VOID) NdisFreeSpinLock(&ovsCtZoneLock); if (zoneInfo) { OvsFreeMemoryWithTag(zoneInfo, OVS_CT_POOL_TAG); + zoneInfo = NULL; [Alin] Why is this needed? } } @@ -1520,6 +1533,8 @@ OvsConntrackEntryCleaner(PVOID data) LOCK_STATE_EX lockState; BOOLEAN success = TRUE; + OVS_LOG_INFO("Start the OVS ConntrackEntry Cleaner system thread," + " context: %p", context); while (success) { if (context->exit) { break; @@ -1541,6 +1556,7 @@ OvsConntrackEntryCleaner(PVOID data) KeWaitForSingleObject(&context->event, Executive, KernelMode, FALSE, (LARGE_INTEGER *)&threadSleepTimeout); } + OVS_LOG_INFO("Terminate the OVS ConntrackEntry Cleaner system thread"); PsTerminateSystemThread(STATUS_SUCCESS); } -- 2.39.2 (Apple Git-143)
diff --git a/datapath-windows/ovsext/Conntrack-nat.c b/datapath-windows/ovsext/Conntrack-nat.c index 497354ec8..a222f6aea 100644 --- a/datapath-windows/ovsext/Conntrack-nat.c +++ b/datapath-windows/ovsext/Conntrack-nat.c @@ -108,6 +108,7 @@ NTSTATUS OvsNatInit() OVS_CT_POOL_TAG); if (ovsUnNatTable == NULL) { OvsFreeMemoryWithTag(ovsNatTable, OVS_CT_POOL_TAG); + ovsNatTable = NULL; return STATUS_INSUFFICIENT_RESOURCES; } @@ -157,7 +158,6 @@ VOID OvsNatFlush(UINT16 zone) VOID OvsNatCleanup() { if (ovsNatTable == NULL) { - NdisFreeSpinLock(&ovsCtNatLock); return; } diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c index 39ba5cc10..156861d6c 100644 --- a/datapath-windows/ovsext/Conntrack.c +++ b/datapath-windows/ovsext/Conntrack.c @@ -94,16 +94,28 @@ OvsInitConntrack(POVS_SWITCH_CONTEXT context) if (status != STATUS_SUCCESS) { goto freeBucketLock; } - - ObReferenceObjectByHandle(threadHandle, SYNCHRONIZE, NULL, KernelMode, - &ctThreadCtx.threadObject, NULL); + ctThreadCtx.exit = 0; + status = ObReferenceObjectByHandle(threadHandle, SYNCHRONIZE, NULL, KernelMode, + &ctThreadCtx.threadObject, NULL); ZwClose(threadHandle); threadHandle = NULL; + if (!NT_SUCCESS(status)) { + ctThreadCtx.exit = 1; + KeSetEvent(&ctThreadCtx.event, 0, FALSE); + KeWaitForSingleObject(ctThreadCtx.threadObject, Executive, + KernelMode, FALSE, NULL); + goto freeBucketLock; + } zoneInfo = OvsAllocateMemoryWithTag(sizeof(OVS_CT_ZONE_INFO) * CT_MAX_ZONE, OVS_CT_POOL_TAG); if (zoneInfo == NULL) { status = STATUS_INSUFFICIENT_RESOURCES; + ctThreadCtx.exit = 1; + KeSetEvent(&ctThreadCtx.event, 0, FALSE); + KeWaitForSingleObject(ctThreadCtx.threadObject, Executive, + KernelMode, FALSE, NULL); + ObDereferenceObject(ctThreadCtx.threadObject); goto freeBucketLock; } @@ -119,7 +131,7 @@ OvsInitConntrack(POVS_SWITCH_CONTEXT context) if (status != STATUS_SUCCESS) { OvsCleanupConntrack(); } - return STATUS_SUCCESS; + return status; freeBucketLock: for (UINT32 i = 0; i < numBucketLocks; i++) { @@ -172,6 +184,7 @@ OvsCleanupConntrack(VOID) NdisFreeSpinLock(&ovsCtZoneLock); if (zoneInfo) { OvsFreeMemoryWithTag(zoneInfo, OVS_CT_POOL_TAG); + zoneInfo = NULL; } } @@ -1520,6 +1533,8 @@ OvsConntrackEntryCleaner(PVOID data) LOCK_STATE_EX lockState; BOOLEAN success = TRUE; + OVS_LOG_INFO("Start the OVS ConntrackEntry Cleaner system thread," + " context: %p", context); while (success) { if (context->exit) { break; @@ -1541,6 +1556,7 @@ OvsConntrackEntryCleaner(PVOID data) KeWaitForSingleObject(&context->event, Executive, KernelMode, FALSE, (LARGE_INTEGER *)&threadSleepTimeout); } + OVS_LOG_INFO("Terminate the OVS ConntrackEntry Cleaner system thread"); PsTerminateSystemThread(STATUS_SUCCESS); }