diff mbox series

[ovs-dev,v2] datapath-windows: Do not send out nbls when cloned nbls are being accessed

Message ID 20190411161421.2132-1-kumaranand@vmware.com
State Accepted
Headers show
Series [ovs-dev,v2] datapath-windows: Do not send out nbls when cloned nbls are being accessed | expand

Commit Message

Li,Rongqing via dev April 11, 2019, 4:14 p.m. UTC
As per MSDN documentation, "As soon as a filter driver calls the
NdisFSendNetBufferLists function, it relinquishes ownership of
the NET_BUFFER_LIST structures and all associated resources.
A filter driver should never try to examine the NET_BUFFER_LIST
structures or any associated data after calling NdisFSendNetBufferLists".

https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/content/ndis/nf-ndis-ndisfsendnetbufferlists

When freeing up memory of a cloned nbl, parent's nbl and context
is being accessed, which is incorrect can cause BSOD.
With this patch, original nbl is sent out only when cloned nbl is done
with packet processing and its memory is freed.

Signed-off-by: Anand Kumar <kumaranand@vmware.com>
---
v1->v2:
- Remove the else block and by default try to send the packet out.
---
 datapath-windows/ovsext/BufferMgmt.c |  9 ++++++++-
 datapath-windows/ovsext/BufferMgmt.h |  2 ++
 datapath-windows/ovsext/PacketIO.c   | 10 ++++++++++
 3 files changed, 20 insertions(+), 1 deletion(-)

Comments

Alin-Gabriel Serdean April 16, 2019, 4:18 p.m. UTC | #1
> On 11 Apr 2019, at 19:14, Anand Kumar via dev <ovs-dev@openvswitch.org> wrote:
> 
> As per MSDN documentation, "As soon as a filter driver calls the
> NdisFSendNetBufferLists function, it relinquishes ownership of
> the NET_BUFFER_LIST structures and all associated resources.
> A filter driver should never try to examine the NET_BUFFER_LIST
> structures or any associated data after calling NdisFSendNetBufferLists".
> 
> https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/content/ndis/nf-ndis-ndisfsendnetbufferlists
> 
> When freeing up memory of a cloned nbl, parent's nbl and context
> is being accessed, which is incorrect can cause BSOD.
> With this patch, original nbl is sent out only when cloned nbl is done
> with packet processing and its memory is freed.
> 
> Signed-off-by: Anand Kumar <kumaranand@vmware.com>
> ---
> v1->v2:
> - Remove the else block and by default try to send the packet out.
> —
> 

Acked-by: Alin Gabriel Serdean <aserdean@ovn.org <mailto:aserdean@ovn.org>>
Alin-Gabriel Serdean April 24, 2019, 11:10 p.m. UTC | #2
> On 11 Apr 2019, at 19:14, Anand Kumar via dev <ovs-dev@openvswitch.org> wrote:
> 
> As per MSDN documentation, "As soon as a filter driver calls the
> NdisFSendNetBufferLists function, it relinquishes ownership of
> the NET_BUFFER_LIST structures and all associated resources.
> A filter driver should never try to examine the NET_BUFFER_LIST
> structures or any associated data after calling NdisFSendNetBufferLists".
> 
> https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/content/ndis/nf-ndis-ndisfsendnetbufferlists
> 
> When freeing up memory of a cloned nbl, parent's nbl and context
> is being accessed, which is incorrect can cause BSOD.
> With this patch, original nbl is sent out only when cloned nbl is done
> with packet processing and its memory is freed.
> 
> Signed-off-by: Anand Kumar <kumaranand@vmware.com>
> ---
> v1->v2:
> - Remove the else block and by default try to send the packet out.
> —
Applied on master.
diff mbox series

Patch

diff --git a/datapath-windows/ovsext/BufferMgmt.c b/datapath-windows/ovsext/BufferMgmt.c
index 47d872d..6627acf 100644
--- a/datapath-windows/ovsext/BufferMgmt.c
+++ b/datapath-windows/ovsext/BufferMgmt.c
@@ -81,6 +81,7 @@ 
 #include "Flow.h"
 #include "Offload.h"
 #include "NetProto.h"
+#include "PacketIO.h"
 #include "PacketParser.h"
 #include "Switch.h"
 #include "Vport.h"
@@ -267,6 +268,7 @@  OvsInitNBLContext(POVS_BUFFER_CONTEXT ctx,
     ctx->srcPortNo = srcPortNo;
     ctx->origDataLength = origDataLength;
     ctx->mru = 0;
+    ctx->pendingSend = 0;
 }
 
 
@@ -1746,8 +1748,13 @@  OvsCompleteNBL(PVOID switch_ctx,
     if (parent != NULL) {
         ctx = (POVS_BUFFER_CONTEXT)NET_BUFFER_LIST_CONTEXT_DATA_START(parent);
         ASSERT(ctx && ctx->magic == OVS_CTX_MAGIC);
+        UINT16 pendingSend = 1, exchange = 0;
         value = InterlockedDecrement((LONG volatile *)&ctx->refCount);
-        if (value == 0) {
+        InterlockedCompareExchange16((SHORT volatile *)&pendingSend, exchange, (SHORT)ctx->pendingSend);
+        if (value == 1 && pendingSend == exchange) {
+            InterlockedExchange16((SHORT volatile *)&ctx->pendingSend, 0);
+            OvsSendNBLIngress(context, parent, ctx->sendFlags);
+        } else if (value == 0){
             return OvsCompleteNBL(context, parent, FALSE);
         }
     }
diff --git a/datapath-windows/ovsext/BufferMgmt.h b/datapath-windows/ovsext/BufferMgmt.h
index 2a74988..2ae3272 100644
--- a/datapath-windows/ovsext/BufferMgmt.h
+++ b/datapath-windows/ovsext/BufferMgmt.h
@@ -55,7 +55,9 @@  typedef union _OVS_BUFFER_CONTEXT {
             UINT32 origDataLength;
             UINT32 dataOffsetDelta;
         };
+        ULONG sendFlags;
         UINT16 mru;
+        UINT16 pendingSend; /* Indicates packet can be sent or not. */
     };
 
     CHAR value[MEM_ALIGN_SIZE(sizeof(struct dummy))];
diff --git a/datapath-windows/ovsext/PacketIO.c b/datapath-windows/ovsext/PacketIO.c
index 57c583c..cc08407 100644
--- a/datapath-windows/ovsext/PacketIO.c
+++ b/datapath-windows/ovsext/PacketIO.c
@@ -161,6 +161,16 @@  OvsSendNBLIngress(POVS_SWITCH_CONTEXT switchContext,
 
     ASSERT(switchContext->dataFlowState == OvsSwitchRunning);
 
+    POVS_BUFFER_CONTEXT ctx = (POVS_BUFFER_CONTEXT)NET_BUFFER_LIST_CONTEXT_DATA_START(netBufferLists);
+    LONG refCount = 1, exchange = 0;
+    InterlockedCompareExchange((LONG volatile *)&refCount, exchange, (LONG)ctx->refCount);
+    if (refCount != exchange) {
+        InterlockedExchange((LONG volatile *)&ctx->sendFlags, sendFlags);
+        InterlockedExchange16((SHORT volatile *)&ctx->pendingSend, 1);
+        return;
+    }
+
+    InterlockedExchange16((SHORT volatile *)&ctx->pendingSend, 0);
     NdisFSendNetBufferLists(switchContext->NdisFilterHandle, netBufferLists,
                             NDIS_DEFAULT_PORT_NUMBER, sendFlags);
 }