| 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 | expand |
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
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 <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
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
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
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
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(-)