diff mbox

[ovs-dev,v3,4/9] datapath-windows: Add support for Conntrack IPCTNL_MSG_CT_DELETE cmd in Datapath.c

Message ID 1466795009-5328-5-git-send-email-vsairam@vmware.com
State Superseded
Delegated to: Guru Shetty
Headers show

Commit Message

Sairam Venugopal June 24, 2016, 7:03 p.m. UTC 
Create new NETLINK_CMD and NETLINK_FAMILY to assist in flushing conntrack entries. Modify
Datapath.c to now support netfilter-netlink messages apart from the
existing netfilter-generic messages. Also hookup the command handler to
execute the OvsCtFlush in Conntrack.c

Signed-off-by: Sairam Venugopal <vsairam@vmware.com>
Acked-by: Paul-Daniel Boca <pboca@cloudbasesolutions.com>
---
 datapath-windows/include/OvsDpInterfaceExt.h | 17 ++++++
 datapath-windows/ovsext/Datapath.c           | 82 ++++++++++++++++++++++++----
 2 files changed, 89 insertions(+), 10 deletions(-)

Comments

Nithin Raju June 27, 2016, 10:18 p.m. UTC | #1 
Looks good but for a couple of suggestions.

>/* Windows kernel datapath extensions to the standard datapath interface.
>*/
> 
> /* Version number of the datapath interface extensions. */
>@@ -65,6 +68,7 @@
> #define OVS_WIN_NL_VPORT_FAMILY_ID           (NLMSG_MIN_TYPE + 4)
> #define OVS_WIN_NL_FLOW_FAMILY_ID            (NLMSG_MIN_TYPE + 5)
> #define OVS_WIN_NL_NETDEV_FAMILY_ID          (NLMSG_MIN_TYPE + 6)
>+#define OVS_WIN_NL_CT_FAMILY_ID              (NLMSG_MIN_TYPE + 7)
> 
> #define OVS_WIN_NL_INVALID_MCGRP_ID          0
> #define OVS_WIN_NL_MCGRP_START_ID            100
>@@ -156,4 +160,17 @@ enum ovs_win_netdev_attr {
> typedef struct ovs_dp_stats OVS_DP_STATS;
> typedef enum ovs_vport_type OVS_VPORT_TYPE;
> 
>+/* Conntrack Netlink */
>+#define NFNL_TYPE_CT_GET (NFNL_SUBSYS_CTNETLINK << 8 | IPCTNL_MSG_CT_GET)
>+#define NFNL_TYPE_CT_DEL (NFNL_SUBSYS_CTNETLINK << 8 |
>IPCTNL_MSG_CT_DELETE)
>+#define NFNL_SUBSYSTEM_TYPE(nlmsgType) (nlmsgType >> 8)
>+#define NFNL_CT_CMD(nlmsgType) (nlmsgType & 0xff)
>+#define IS_NFNL_CMD(nlmsgType) ((nlmsgType == NFNL_TYPE_CT_GET) ||
>(nlmsgType == NFNL_TYPE_CT_DEL))
>+#define OVS_NL_CT_ATTR_MAX (IPCTNL_MSG_MAX - 1)

This will probably change with the discussion we are having about where
the netlink defines should go to. If we decide to go with an approach of
OvsDpInterfaceCtExt.h, it would be nice to put these in enums, with
comments around them.

The only thing we need to be careful about adding code in any of the
OvsDpInterface*h. is that we¹ll be stuck with it for the foreseeable
future. We¹ll always have to make future code adhere to this interface.

> /* Netlink netdev family. */
> NETLINK_CMD nlNetdevFamilyCmdOps[] = {
>     { .cmd = OVS_WIN_NETDEV_CMD_GET,
>@@ -885,6 +904,9 @@ OvsDeviceControl(PDEVICE_OBJECT deviceObject,
> 
>     ASSERT(ovsMsg);
>     switch (ovsMsg->nlMsg.nlmsgType) {
>+    case NFNL_TYPE_CT_DEL:
>+        nlFamilyOps = &nlCtFamilyOps;
>+        break;
>     case OVS_WIN_NL_CTRL_FAMILY_ID:
>         nlFamilyOps = &nlControlFamilyOps;
>         break;
>@@ -961,6 +983,30 @@ ValidateNetlinkCmd(UINT32 devOp,
>         goto done;
>     }
> 
>+    /*
>+        Verify if the Netlink message is part of Netfilter Netlink
>+        This is currently used by Conntrack
>+    */

minor: comment styling should be:
/*
 *
 *
 */
>@@ -1022,14 +1068,29 @@ InvokeNetlinkCmdHandler(POVS_USER_PARAMS_CONTEXT
>usrParamsCtx,
>     NTSTATUS status = STATUS_INVALID_PARAMETER;
>     UINT16 i;
> 
>-    for (i = 0; i < nlFamilyOps->opsCount; i++) {
>-        if (nlFamilyOps->cmds[i].cmd ==
>usrParamsCtx->ovsMsg->genlMsg.cmd) {
>-            NetlinkCmdHandler *handler = nlFamilyOps->cmds[i].handler;
>-            ASSERT(handler);
>-            if (handler) {
>-                status = handler(usrParamsCtx, replyLen);
>+    if (IS_NFNL_CMD(usrParamsCtx->ovsMsg->nlMsg.nlmsgType)) {
>+        /* If nlMsg is of type Netfilter-Netlink parse the Cmd
>accordingly */
>+        UINT8 cmd = NFNL_CT_CMD(usrParamsCtx->ovsMsg->nlMsg.nlmsgType);
>+        for (i = 0; i < nlFamilyOps->opsCount; i++) {
>+            if (nlFamilyOps->cmds[i].cmd == cmd) {
>+                NetlinkCmdHandler *handler =
>nlFamilyOps->cmds[i].handler;
>+                ASSERT(handler);
>+                if (handler) {
>+                    status = handler(usrParamsCtx, replyLen);
>+                }
>+                break;
>+            }
>+        }
>+    } else {
>+        for (i = 0; i < nlFamilyOps->opsCount; i++) {
>+            if (nlFamilyOps->cmds[i].cmd ==
>usrParamsCtx->ovsMsg->genlMsg.cmd) {
>+                NetlinkCmdHandler *handler =
>nlFamilyOps->cmds[i].handler;
>+                ASSERT(handler);
>+                if (handler) {
>+                    status = handler(usrParamsCtx, replyLen);
>+                }
>+                break;
>             }
>-            break;
>         }

Suggestion:
This code could be refactored:
if (NF) {
  cmd = xx;
} else {
  cmd = usrParamsCtx->ovsMsg->genlMsg.cmd;
}

for (i = 0; i < nlFamilyOps->opsCount; i++) {
    if (nlFamilyOps->cmds[i].cmd == usrParamsCtx->ovsMsg->genlMsg.cmd) {
[Š]
diff mbox

Patch

diff --git a/datapath-windows/include/OvsDpInterfaceExt.h b/datapath-windows/include/OvsDpInterfaceExt.h
index e235376..1044de7 100644
--- a/datapath-windows/include/OvsDpInterfaceExt.h
+++ b/datapath-windows/include/OvsDpInterfaceExt.h
@@ -17,6 +17,9 @@ 
 #ifndef __OVS_DP_INTERFACE_EXT_H_
 #define __OVS_DP_INTERFACE_EXT_H_ 1
 
+#include "include/windows/linux/netfilter/nfnetlink.h"
+#include "include/windows/linux/netfilter/nfnetlink_conntrack.h"
+
 /* Windows kernel datapath extensions to the standard datapath interface. */
 
 /* Version number of the datapath interface extensions. */
@@ -65,6 +68,7 @@ 
 #define OVS_WIN_NL_VPORT_FAMILY_ID           (NLMSG_MIN_TYPE + 4)
 #define OVS_WIN_NL_FLOW_FAMILY_ID            (NLMSG_MIN_TYPE + 5)
 #define OVS_WIN_NL_NETDEV_FAMILY_ID          (NLMSG_MIN_TYPE + 6)
+#define OVS_WIN_NL_CT_FAMILY_ID              (NLMSG_MIN_TYPE + 7)
 
 #define OVS_WIN_NL_INVALID_MCGRP_ID          0
 #define OVS_WIN_NL_MCGRP_START_ID            100
@@ -156,4 +160,17 @@  enum ovs_win_netdev_attr {
 typedef struct ovs_dp_stats OVS_DP_STATS;
 typedef enum ovs_vport_type OVS_VPORT_TYPE;
 
+/* Conntrack Netlink */
+#define NFNL_TYPE_CT_GET (NFNL_SUBSYS_CTNETLINK << 8 | IPCTNL_MSG_CT_GET)
+#define NFNL_TYPE_CT_DEL (NFNL_SUBSYS_CTNETLINK << 8 | IPCTNL_MSG_CT_DELETE)
+#define NFNL_SUBSYSTEM_TYPE(nlmsgType) (nlmsgType >> 8)
+#define NFNL_CT_CMD(nlmsgType) (nlmsgType & 0xff)
+#define IS_NFNL_CMD(nlmsgType) ((nlmsgType == NFNL_TYPE_CT_GET) || (nlmsgType == NFNL_TYPE_CT_DEL))
+#define OVS_NL_CT_ATTR_MAX (IPCTNL_MSG_MAX - 1)
+
+#define OVS_CT_FAMILY  "ovs_ct"
+#define OVS_CT_MCGROUP "ovs_ct"
+#define OVS_CT_VERSION 1
+
+
 #endif /* __OVS_DP_INTERFACE_EXT_H_ */
diff --git a/datapath-windows/ovsext/Datapath.c b/datapath-windows/ovsext/Datapath.c
index b2c7020..7cc8390 100644
--- a/datapath-windows/ovsext/Datapath.c
+++ b/datapath-windows/ovsext/Datapath.c
@@ -103,7 +103,8 @@  NetlinkCmdHandler        OvsGetNetdevCmdHandler,
                          OvsDeleteVportCmdHandler,
                          OvsPendPacketCmdHandler,
                          OvsSubscribePacketCmdHandler,
-                         OvsReadPacketCmdHandler;
+                         OvsReadPacketCmdHandler,
+                         OvsCtDeleteCmdHandler;
 
 static NTSTATUS HandleGetDpTransaction(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
                                        UINT32 *replyLen);
@@ -282,6 +283,24 @@  NETLINK_FAMILY nlFLowFamilyOps = {
     .opsCount = ARRAY_SIZE(nlFlowFamilyCmdOps)
 };
 
+/* Netlink Ct family. */
+NETLINK_CMD nlCtFamilyCmdOps[] = {
+    { .cmd              = IPCTNL_MSG_CT_DELETE,
+      .handler          = OvsCtDeleteCmdHandler,
+      .supportedDevOp   = OVS_TRANSACTION_DEV_OP,
+      .validateDpIndex  = TRUE
+    }
+};
+
+NETLINK_FAMILY nlCtFamilyOps = {
+    .name     = OVS_CT_FAMILY, /* Keep this for consistency*/
+    .id       = OVS_WIN_NL_CT_FAMILY_ID, /* Keep this for consistency*/
+    .version  = OVS_CT_VERSION, /* Keep this for consistency*/
+    .maxAttr  = OVS_NL_CT_ATTR_MAX,
+    .cmds     = nlCtFamilyCmdOps,
+    .opsCount = ARRAY_SIZE(nlCtFamilyCmdOps)
+};
+
 /* Netlink netdev family. */
 NETLINK_CMD nlNetdevFamilyCmdOps[] = {
     { .cmd = OVS_WIN_NETDEV_CMD_GET,
@@ -885,6 +904,9 @@  OvsDeviceControl(PDEVICE_OBJECT deviceObject,
 
     ASSERT(ovsMsg);
     switch (ovsMsg->nlMsg.nlmsgType) {
+    case NFNL_TYPE_CT_DEL:
+        nlFamilyOps = &nlCtFamilyOps;
+        break;
     case OVS_WIN_NL_CTRL_FAMILY_ID:
         nlFamilyOps = &nlControlFamilyOps;
         break;
@@ -961,6 +983,30 @@  ValidateNetlinkCmd(UINT32 devOp,
         goto done;
     }
 
+    /*
+        Verify if the Netlink message is part of Netfilter Netlink
+        This is currently used by Conntrack
+    */
+    if (IS_NFNL_CMD(ovsMsg->nlMsg.nlmsgType)) {
+
+        /* Validate Netfilter Netlink version is 0 */
+        if (ovsMsg->nfGenMsg.version != NFNETLINK_V0) {
+            status = STATUS_INVALID_PARAMETER;
+            goto done;
+        }
+
+        /* Validate Netfilter Netlink Subsystem */
+        if (NFNL_SUBSYSTEM_TYPE(ovsMsg->nlMsg.nlmsgType)
+            != NFNL_SUBSYS_CTNETLINK) {
+            status = STATUS_INVALID_PARAMETER;
+            goto done;
+        }
+
+        /* Exit the function because there aren't any other validations */
+        status = STATUS_SUCCESS;
+        goto done;
+    }
+
     for (i = 0; i < nlFamilyOps->opsCount; i++) {
         if (nlFamilyOps->cmds[i].cmd == ovsMsg->genlMsg.cmd) {
             /* Validate if the command is valid for the device operation. */
@@ -1022,14 +1068,29 @@  InvokeNetlinkCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
     NTSTATUS status = STATUS_INVALID_PARAMETER;
     UINT16 i;
 
-    for (i = 0; i < nlFamilyOps->opsCount; i++) {
-        if (nlFamilyOps->cmds[i].cmd == usrParamsCtx->ovsMsg->genlMsg.cmd) {
-            NetlinkCmdHandler *handler = nlFamilyOps->cmds[i].handler;
-            ASSERT(handler);
-            if (handler) {
-                status = handler(usrParamsCtx, replyLen);
+    if (IS_NFNL_CMD(usrParamsCtx->ovsMsg->nlMsg.nlmsgType)) {
+        /* If nlMsg is of type Netfilter-Netlink parse the Cmd accordingly */
+        UINT8 cmd = NFNL_CT_CMD(usrParamsCtx->ovsMsg->nlMsg.nlmsgType);
+        for (i = 0; i < nlFamilyOps->opsCount; i++) {
+            if (nlFamilyOps->cmds[i].cmd == cmd) {
+                NetlinkCmdHandler *handler = nlFamilyOps->cmds[i].handler;
+                ASSERT(handler);
+                if (handler) {
+                    status = handler(usrParamsCtx, replyLen);
+                }
+                break;
+            }
+        }
+    } else {
+        for (i = 0; i < nlFamilyOps->opsCount; i++) {
+            if (nlFamilyOps->cmds[i].cmd == usrParamsCtx->ovsMsg->genlMsg.cmd) {
+                NetlinkCmdHandler *handler = nlFamilyOps->cmds[i].handler;
+                ASSERT(handler);
+                if (handler) {
+                    status = handler(usrParamsCtx, replyLen);
+                }
+                break;
             }
-            break;
         }
     }
 
@@ -1055,8 +1116,9 @@  InvokeNetlinkCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
             POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)
                 usrParamsCtx->outputBuffer;
 
-            if (usrParamsCtx->ovsMsg->genlMsg.cmd == OVS_CTRL_CMD_EVENT_NOTIFY ||
-                usrParamsCtx->ovsMsg->genlMsg.cmd == OVS_CTRL_CMD_READ_NOTIFY) {
+            if (!IS_NFNL_CMD(usrParamsCtx->ovsMsg->nlMsg.nlmsgType) &&
+                (usrParamsCtx->ovsMsg->genlMsg.cmd == OVS_CTRL_CMD_EVENT_NOTIFY ||
+                 usrParamsCtx->ovsMsg->genlMsg.cmd == OVS_CTRL_CMD_READ_NOTIFY)) {
                 /* There's no input buffer associated with such requests. */
                 NL_BUFFER nlBuffer;
                 msgIn = &msgInTmp;