[ovs-dev,v1] datapath-windows: Do not send out nbls when cloned nbls are being accessed
diff mbox series

Message ID 20190409160357.5096-1-kumaranand@vmware.com
State Superseded
Headers show
Series
  • [ovs-dev,v1] datapath-windows: Do not send out nbls when cloned nbls are being accessed
Related show

Commit Message

Anand Kumar via dev April 9, 2019, 4:03 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>

Change-Id: Ie662133a6fcd5a26ca3c87d31c9cee1fc56c2d27
---
 datapath-windows/ovsext/BufferMgmt.c |  9 ++++++++-
 datapath-windows/ovsext/BufferMgmt.h |  2 ++
 datapath-windows/ovsext/PacketIO.c   | 13 +++++++++++--
 3 files changed, 21 insertions(+), 3 deletions(-)

Comments

0-day Robot April 9, 2019, 5:51 p.m. UTC | #1
Bleep bloop.  Greetings Anand Kumar via dev, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
ERROR: Author should not be mailing list.
Lines checked: 31, Warnings: 0, Errors: 1


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot
Aaron Conole April 10, 2019, 5:49 p.m. UTC | #2
0-day Robot <robot@bytheb.org> writes:

> Bleep bloop.  Greetings Anand Kumar via dev, I am a robot and I have tried out your patch.
> Thanks for your contribution.
>
> I encountered some error that I wasn't expecting.  See the details below.
>
>
> checkpatch:
> ERROR: Author should not be mailing list.
> Lines checked: 31, Warnings: 0, Errors: 1

I looked into this - it seems that your patch doesn't have a Reply-To
field.

I plan to catch cases like this and duplicate the 'Signed-off-by:' as a
'From:'.

Sorry for the noise, again.

>
> Please check this out.  If you feel there has been an error, please email aconole@bytheb.org
>
> Thanks,
> 0-day Robot
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Aaron Conole April 10, 2019, 5:53 p.m. UTC | #3
Aaron Conole <aconole@redhat.com> writes:

> 0-day Robot <robot@bytheb.org> writes:
>
>> Bleep bloop.  Greetings Anand Kumar via dev, I am a robot and I have tried out your patch.
>> Thanks for your contribution.
>>
>> I encountered some error that I wasn't expecting.  See the details below.
>>
>>
>> checkpatch:
>> ERROR: Author should not be mailing list.
>> Lines checked: 31, Warnings: 0, Errors: 1
>
> I looked into this - it seems that your patch doesn't have a Reply-To
> field.

Clarification, when fetched from Patchwork it doesn't seem that the
Reply-To field is included.  Perhaps this is something useful to address
in Patchwork as well?

> I plan to catch cases like this and duplicate the 'Signed-off-by:' as a
> 'From:'.
>
> Sorry for the noise, again.

>>
>> Please check this out.  If you feel there has been an error, please email aconole@bytheb.org
>>
>> Thanks,
>> 0-day Robot
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Stephen Finucane April 11, 2019, 1:36 p.m. UTC | #4
On Wed, 2019-04-10 at 13:53 -0400, Aaron Conole wrote:
> Aaron Conole <aconole@redhat.com> writes:
> 
> > 0-day Robot <robot@bytheb.org> writes:
> > 
> > > Bleep bloop.  Greetings Anand Kumar via dev, I am a robot and I have tried out your patch.
> > > Thanks for your contribution.
> > > 
> > > I encountered some error that I wasn't expecting.  See the details below.
> > > 
> > > 
> > > checkpatch:
> > > ERROR: Author should not be mailing list.
> > > Lines checked: 31, Warnings: 0, Errors: 1
> > 
> > I looked into this - it seems that your patch doesn't have a Reply-To
> > field.
> 
> Clarification, when fetched from Patchwork it doesn't seem that the
> Reply-To field is included.  Perhaps this is something useful to address
> in Patchwork as well?

This was resolved in Patchwork 2.1 [1]. Unfortunately the ozlabs.org
instance hasn't been updated yet and I've no influence over that.
Hopefully they'll bump to 2.2 when that's released in the coming
months.

Stephen

[1] https://github.com/getpatchwork/patchwork/commit/01b9cbb9ce9f44387be22b5d036dde0fff4bb14b

> > I plan to catch cases like this and duplicate the 'Signed-off-by:' as a
> > 'From:'.
> > 
> > Sorry for the noise, again.
> > > Please check this out.  If you feel there has been an error, please email aconole@bytheb.org
> > > 
> > > Thanks,
> > > 0-day Robot
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Aaron Conole April 11, 2019, 1:58 p.m. UTC | #5
Stephen Finucane <sfinucan@redhat.com> writes:

> On Wed, 2019-04-10 at 13:53 -0400, Aaron Conole wrote:
>> Aaron Conole <aconole@redhat.com> writes:
>> 
>> > 0-day Robot <robot@bytheb.org> writes:
>> > 
>> > > Bleep bloop.  Greetings Anand Kumar via dev, I am a robot and I have tried out your patch.
>> > > Thanks for your contribution.
>> > > 
>> > > I encountered some error that I wasn't expecting.  See the details below.
>> > > 
>> > > 
>> > > checkpatch:
>> > > ERROR: Author should not be mailing list.
>> > > Lines checked: 31, Warnings: 0, Errors: 1
>> > 
>> > I looked into this - it seems that your patch doesn't have a Reply-To
>> > field.
>> 
>> Clarification, when fetched from Patchwork it doesn't seem that the
>> Reply-To field is included.  Perhaps this is something useful to address
>> in Patchwork as well?
>
> This was resolved in Patchwork 2.1 [1]. Unfortunately the ozlabs.org
> instance hasn't been updated yet and I've no influence over that.
> Hopefully they'll bump to 2.2 when that's released in the coming
> months.
>
> Stephen
>
> [1] https://github.com/getpatchwork/patchwork/commit/01b9cbb9ce9f44387be22b5d036dde0fff4bb14b

Thanks for the information, Stephen!

>> > I plan to catch cases like this and duplicate the 'Signed-off-by:' as a
>> > 'From:'.
>> > 
>> > Sorry for the noise, again.
>> > > Please check this out.  If you feel there has been an error,
>> > > please email aconole@bytheb.org
>> > > 
>> > > Thanks,
>> > > 0-day Robot
>> > > _______________________________________________
>> > > dev mailing list
>> > > dev@openvswitch.org
>> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> > _______________________________________________
>> > dev mailing list
>> > dev@openvswitch.org
>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Patch
diff mbox series

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..56876f2 100644
--- a/datapath-windows/ovsext/PacketIO.c
+++ b/datapath-windows/ovsext/PacketIO.c
@@ -161,8 +161,17 @@  OvsSendNBLIngress(POVS_SWITCH_CONTEXT switchContext,
 
     ASSERT(switchContext->dataFlowState == OvsSwitchRunning);
 
-    NdisFSendNetBufferLists(switchContext->NdisFilterHandle, netBufferLists,
-                            NDIS_DEFAULT_PORT_NUMBER, sendFlags);
+    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);
+    } else {
+        InterlockedExchange16((SHORT volatile *)&ctx->pendingSend, 0);
+        NdisFSendNetBufferLists(switchContext->NdisFilterHandle, netBufferLists,
+            NDIS_DEFAULT_PORT_NUMBER, sendFlags);
+    }
 }
 
 static __inline VOID