diff mbox series

[ovs-dev] datapath-windows: Add guards around IpHelper adapter binding calls

Message ID 20190313223729.83632-1-vsairam@vmware.com
State Accepted
Headers show
Series [ovs-dev] datapath-windows: Add guards around IpHelper adapter binding calls | expand

Commit Message

Li,Rongqing via dev March 13, 2019, 10:37 p.m. UTC 
Protect internal adapter up/down calls with a dispatch lock. It was
observed that the InternalAdapter bind calls could happen out of order
thereby causing encap packets to not be sent properly.

Add assert around the IpHelper bind calls to ensure Up/Down gets called
only for the appropriate vports.

Signed-off-by: Sairam Venugopal <vsairam@vmware.com>
---
 datapath-windows/ovsext/Switch.h |  2 ++
 datapath-windows/ovsext/Vport.c  | 34 +++++++++++++++++++++++++++++++---
 2 files changed, 33 insertions(+), 3 deletions(-)

Comments

0-day Robot March 13, 2019, 10:59 p.m. UTC | #1 
Bleep bloop.  Greetings Sairam Venugopal 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: 105, 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
Alin Serdean April 3, 2019, 3:40 p.m. UTC | #2 
Applied on master! 

> On 25 Mar 2019, at 19:26, Alin Serdean <aserdean@cloudbasesolutions.com> wrote:
> 
> -----Original Message-----
> From: ovs-dev-bounces@openvswitch.org <ovs-dev-bounces@openvswitch.org> On Behalf Of Sairam Venugopal via dev
> Sent: Thursday, March 14, 2019 12:37 AM
> To: ovs-dev@openvswitch.org
> Subject: [ovs-dev] [PATCH] datapath-windows: Add guards around IpHelper adapter binding calls
> 
> Protect internal adapter up/down calls with a dispatch lock. It was observed that the InternalAdapter bind calls could happen out of order thereby causing encap packets to not be sent properly.
> 
> Add assert around the IpHelper bind calls to ensure Up/Down gets called only for the appropriate vports.
> 
> Signed-off-by: Sairam Venugopal <vsairam@vmware.com>
> ---
> 
> Acked-by: Alin Gabriel Serdean <aserdean@ovn.org>
diff mbox series

Patch

diff --git a/datapath-windows/ovsext/Switch.h b/datapath-windows/ovsext/Switch.h
index 18a9ec6..806ee78 100644
--- a/datapath-windows/ovsext/Switch.h
+++ b/datapath-windows/ovsext/Switch.h
@@ -131,6 +131,8 @@  typedef struct _OVS_SWITCH_CONTEXT
                                                      * vport */
     INT32                   countInternalVports;    /* the number of internal
                                                      * vports */
+    UINT32                  ipHelperBoundVportNo;   /* vportNo bound to
+                                                     * IpHelper */
 
     /*
      * 'portIdHashArray' ONLY contains ports that exist on the Hyper-V switch,
diff --git a/datapath-windows/ovsext/Vport.c b/datapath-windows/ovsext/Vport.c
index e8c4d7f..57e7510 100644
--- a/datapath-windows/ovsext/Vport.c
+++ b/datapath-windows/ovsext/Vport.c
@@ -100,6 +100,12 @@  static NTSTATUS GetNICAlias(PNDIS_SWITCH_NIC_PARAMETERS nicParam,
 static NTSTATUS OvsConvertIfCountedStrToAnsiStr(PIF_COUNTED_STRING wStr,
                                                 CHAR *str,
                                                 UINT16 maxStrLen);
+_Requires_lock_held_(switchContext->dispatchLock)
+static VOID OvsBindVportWithIpHelper(POVS_VPORT_ENTRY vport,
+                                     POVS_SWITCH_CONTEXT switchContext);
+_Requires_lock_held_(switchContext->dispatchLock)
+static VOID OvsUnBindVportWithIpHelper(POVS_VPORT_ENTRY vport,
+                                       POVS_SWITCH_CONTEXT switchContext);
 
 /*
  * --------------------------------------------------------------------------
@@ -453,7 +459,7 @@  HvConnectNic(POVS_SWITCH_CONTEXT switchContext,
     vport->nicState = NdisSwitchNicStateConnected;
 
     if (nicParam->NicType == NdisSwitchNicTypeInternal) {
-        OvsInternalAdapterUp(vport->portNo, &vport->netCfgInstanceId);
+        OvsBindVportWithIpHelper(vport, switchContext);
     }
 
     NdisReleaseRWLock(switchContext->dispatchLock, &lockState);
@@ -633,7 +639,7 @@  HvDisconnectNic(POVS_SWITCH_CONTEXT switchContext,
     }
 
     if (isInternalPort) {
-        OvsInternalAdapterDown(vport->portNo, vport->netCfgInstanceId);
+        OvsUnBindVportWithIpHelper(vport, switchContext);
         OvsRemoveAndDeleteVport(NULL, switchContext, vport, TRUE, TRUE);
         OvsPostVportEvent(&event);
     }
@@ -1308,7 +1314,7 @@  OvsRemoveAndDeleteVport(PVOID usrParamsContext,
         if (hvDelete && vport->isAbsentOnHv == FALSE) {
             switchContext->countInternalVports--;
             ASSERT(switchContext->countInternalVports >= 0);
-            OvsInternalAdapterDown(vport->portNo, vport->netCfgInstanceId);
+            OvsUnBindVportWithIpHelper(vport, switchContext);
         }
         hvSwitchPort = TRUE;
         break;
@@ -2814,3 +2820,25 @@  OvsTunnelVportPendingInit(PVOID context,
         ASSERT(*replyLen != 0);
     }
 }
+
+_Use_decl_annotations_
+static VOID
+OvsBindVportWithIpHelper(POVS_VPORT_ENTRY vport,
+                         POVS_SWITCH_CONTEXT switchContext)
+{
+    OVS_LOG_TRACE("OvsBindVportWithIpHelper: %d", vport->portNo);
+    ASSERT(switchContext->ipHelperBoundVportNo == 0);
+    switchContext->ipHelperBoundVportNo = vport->portNo;
+    OvsInternalAdapterUp(vport->portNo, &vport->netCfgInstanceId);
+}
+
+_Use_decl_annotations_
+static VOID
+OvsUnBindVportWithIpHelper(POVS_VPORT_ENTRY vport,
+                           POVS_SWITCH_CONTEXT switchContext)
+{
+    OVS_LOG_TRACE("OvsUnBindVportWithIpHelper: %d", vport->portNo);
+    ASSERT(switchContext->ipHelperBoundVportNo == vport->portNo);
+    switchContext->ipHelperBoundVportNo = 0;
+    OvsInternalAdapterDown(vport->portNo, vport->netCfgInstanceId);
+}