Hi Nithin, I saw that Ben already applied this patch yesterday. However, I just have one comment below.
> -----Original Message----- > From: Nithin Raju [mailto:nit...@vmware.com] > Sent: Tuesday, August 12, 2014 10:59 PM > To: dev@openvswitch.org; Saurabh Shah > Cc: Nithin Raju > Subject: [PATCH] datapath-windows: check source port during tunnel Tx > > In the Windows datapath, Tx tunneling functionality is implemented by > checking if the outport in the action is a tunnel port. If so, the packet is > encapsulated and sent out on the PIF bridge for as second flow lookup. > Basically, we don't use the hypervisor's IP stack to send out a packet, and > short circuit the path ourselves. On the PIF bridge, the source port of the > encapsulated packet is the VTEP port ie. the internal port. > > If a Tunneling port is added to the PIF bridge (a possible misconfiguration), > where the VTEP(internal) port and the external port (physical NIC) also > reside, a flooding action can cause a loop, by re-injecting the packet on the > same PIF bridge which again floods to the tunnel port. > > In this change, we break the loop by encapsulating packets only if they are > sent out by a VIF or if they originate from userspace ie. userspace generated. > We make use of the input port attribute in the packet execute ioctl. > > This change is based off of the legacy datapath interface published in > OvsPub.h. This interface has a input port field in the packet execute ioctl. > I looked in dpif-linux.c that uses the netlink based datapath interface and > even in that case, we do add the the source port in: > dpif_linux_encode_execute() -> odp_key_from_pkt_metadata(). > So, this fix is applicable when we adopt the netlink based datapath interface > as well. > > The Rx side of OvsDetectTunnelPkt() has only documentation updates. The > fix is on the Tx side. > > Validation (using dpif-windows.c): > - Was able to perform VTEP <-> VTEP ping with the configuration posted in > the issue. > - Was able to perform VIF <-> VIF ping when the setup was configured > correctly. > > Signed-off-by: Nithin Raju <nit...@vmware.com> > Reported-by: Alin Serdean <aserd...@cloudbasesolutions.com> > Reported-at: https://github.com/openvswitch/ovs-issues/issues/20 > --- > datapath-windows/ovsext/OvsActions.c | 58 > +++++++++++++++++++++++++----------- > datapath-windows/ovsext/OvsUser.c | 19 ++++++++---- > datapath-windows/ovsext/OvsVport.h | 7 +++++ > 3 files changed, 60 insertions(+), 24 deletions(-) > > diff --git a/datapath-windows/ovsext/OvsActions.c b/datapath- > windows/ovsext/OvsActions.c > index 4a2c117..635becc 100644 > --- a/datapath-windows/ovsext/OvsActions.c > +++ b/datapath-windows/ovsext/OvsActions.c > @@ -224,14 +224,18 @@ OvsDetectTunnelRxPkt(OvsForwardingContext > *ovsFwdCtx, > /* > * -------------------------------------------------------------------------- > * OvsDetectTunnelPkt -- > - * Utility function to detect the tunnel type of a TX/RX packet. > + * Utility function to detect if a packet is to be subjected to > + * tunneling (Tx) or de-tunneling (Rx). Various factors such as source > + * port, destination port, packet contents, and previously setup tunnel > + * context are used. > * > * Result: > - * True - if the tunnel type was detected. > - * False - if not a tunnel packet or tunnel type not supported. > - * > - * if result==True, the forwarding context gets initialized with the > - * right tunnel vport. > + * True - If the packet is to be subjected to tunneling. > + * In case of invalid tunnel context, the tunneling functionality is > + * a no-op and is completed within this function itself by consuming > + * all of the tunneling context. > + * False - If not a tunnel packet or tunnel type not supported. Caller > should > + * process the packet as a non-tunnel packet. > * -------------------------------------------------------------------------- > */ > static __inline BOOLEAN > @@ -239,16 +243,17 @@ OvsDetectTunnelPkt(OvsForwardingContext > *ovsFwdCtx, > const POVS_VPORT_ENTRY dstVport, > const OvsFlowKey *flowKey) { > - /* > - * The source of NBL during tunneling Rx could be the external port or if > - * it being executed from userspace, the source port is default port. > - */ > - > if (OvsIsInternalVportType(dstVport->ovsType)) { > + /* > + * Rx: > + * The source of NBL during tunneling Rx could be the external > + * port or if it is being executed from userspace, the source port is > + * default port. > + */ > BOOLEAN validSrcPort = (ovsFwdCtx->fwdDetail->SourcePortId == > - ovsFwdCtx->switchContext->externalPortId) > - || (ovsFwdCtx->fwdDetail->SourcePortId == > - NDIS_SWITCH_DEFAULT_PORT_ID); > + ovsFwdCtx->switchContext->externalPortId) || > + (ovsFwdCtx->fwdDetail->SourcePortId == > + NDIS_SWITCH_DEFAULT_PORT_ID); > > if (validSrcPort && OvsDetectTunnelRxPkt(ovsFwdCtx, flowKey)) { > ASSERT(ovsFwdCtx->tunnelTxNic == NULL); @@ -258,9 +263,27 @@ > OvsDetectTunnelPkt(OvsForwardingContext *ovsFwdCtx, > } else if (OvsIsTunnelVportType(dstVport->ovsType)) { > ASSERT(ovsFwdCtx->tunnelTxNic == NULL); > ASSERT(ovsFwdCtx->tunnelRxNic == NULL); > - ASSERT(ovsFwdCtx->tunKey.dst != 0); > - ovsActionStats.txVxlan++; > - ovsFwdCtx->tunnelTxNic = dstVport; > + > + /* > + * Tx: > + * The destination port is a tunnel port. Encapsulation must be > + * performed only on packets that originate from a VIF port or from > + * userspace (default port) > + * > + * If the packet will not be encapsulated, consume the tunnel context > + * by clearing it. > + */ > + if (ovsFwdCtx->srcVportNo != 0 && > + !OvsIsVifVportNo(ovsFwdCtx->srcVportNo)) { > + ovsFwdCtx->tunKey.dst = 0; > + } > + > + /* Tunnel the packet only if tunnel context is set. */ > + if (ovsFwdCtx->tunKey.dst != 0) { > + ovsActionStats.txVxlan++; > + ovsFwdCtx->tunnelTxNic = dstVport; > + } > + It looks like the two if's can be combined. I don't see that variable being of any relevance if we are not adding the output port. Also, we should use NDIS_DEFAULT_SWITCH_PORT_ID instead of 0. > return TRUE; > } > > @@ -318,7 +341,6 @@ OvsAddPorts(OvsForwardingContext *ovsFwdCtx, > NET_BUFFER_DATA_LENGTH(NET_BUFFER_LIST_FIRST_NB(ovsFwdCtx- > >curNbl)); > > if (OvsDetectTunnelPkt(ovsFwdCtx, vport, flowKey)) { > - ASSERT(ovsFwdCtx->tunnelTxNic || ovsFwdCtx->tunnelRxNic); > return NDIS_STATUS_SUCCESS; > } > > diff --git a/datapath-windows/ovsext/OvsUser.c b/datapath- > windows/ovsext/OvsUser.c > index 8271d52..5093f20 100644 > --- a/datapath-windows/ovsext/OvsUser.c > +++ b/datapath-windows/ovsext/OvsUser.c > @@ -314,6 +314,7 @@ OvsExecuteDpIoctl(PVOID inputBuffer, > PNDIS_SWITCH_FORWARDING_DETAIL_NET_BUFFER_LIST_INFO > fwdDetail; > OvsFlowKey key; > OVS_PACKET_HDR_INFO layers; > + POVS_VPORT_ENTRY vport; > > if (inputLength < sizeof(*execute) || outputLength != 0) { > return STATUS_INFO_LENGTH_MISMATCH; @@ -351,8 +352,14 @@ > OvsExecuteDpIoctl(PVOID inputBuffer, > } > > fwdDetail = NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(pNbl); > - fwdDetail->SourcePortId = NDIS_SWITCH_DEFAULT_PORT_ID; > - fwdDetail->SourceNicIndex = 0; > + vport = OvsFindVportByPortNo(gOvsSwitchContext, execute->inPort); > + if (vport) { > + fwdDetail->SourcePortId = vport->portId; > + fwdDetail->SourceNicIndex = vport->nicIndex; > + } else { > + fwdDetail->SourcePortId = NDIS_SWITCH_DEFAULT_PORT_ID; > + fwdDetail->SourceNicIndex = 0; > + } > // XXX: Figure out if any of the other members of fwdDetail need to be > set. > > ndisStatus = OvsExtractFlow(pNbl, fwdDetail->SourcePortId, &key, > &layers, @@ -362,10 +369,10 @@ OvsExecuteDpIoctl(PVOID inputBuffer, > NdisAcquireRWLockRead(gOvsSwitchContext->dispatchLock, > &lockState, > NDIS_RWL_AT_DISPATCH_LEVEL); > ndisStatus = OvsActionsExecute(gOvsSwitchContext, NULL, pNbl, > - 0, // XXX: we are passing 0 for > srcVportNo > - > NDIS_SEND_FLAGS_SWITCH_DESTINATION_GROUP, > - &key, NULL, &layers, actions, > - execute->actionsLen); > + vport ? vport->portNo : 0, > + > NDIS_SEND_FLAGS_SWITCH_DESTINATION_GROUP, > + &key, NULL, &layers, actions, > + execute->actionsLen); > pNbl = NULL; > NdisReleaseRWLock(gOvsSwitchContext->dispatchLock, &lockState); > } > diff --git a/datapath-windows/ovsext/OvsVport.h b/datapath- > windows/ovsext/OvsVport.h > index 8fe23f1..4ab0019 100644 > --- a/datapath-windows/ovsext/OvsVport.h > +++ b/datapath-windows/ovsext/OvsVport.h > @@ -161,6 +161,13 @@ OvsIsTunnelVportNo(UINT32 portNo) > return (idx >= OVS_TUNNEL_INDEX_START && idx <= > OVS_TUNNEL_INDEX_END); } > > +static __inline BOOLEAN > +OvsIsVifVportNo(UINT32 portNo) > +{ > + UINT32 idx = OVS_VPORT_INDEX(portNo); > + return (idx >= OVS_VM_VPORT_START && idx <= > OVS_VM_VPORT_MAX); } > + > static __inline POVS_VPORT_ENTRY > OvsGetTunnelVport(OVS_VPORT_TYPE type) > { > -- > 1.9.1 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
