Thanks for doing this. Looks good but for a few cosmetic comments.
Also, remember to flip the protocol for nf sockets in userspace to
NETLINK_NETFILTER. IIRC, we use NETLINK_GENERIC.
Acked-by: Nithin Raju <nit...@vmware.com>
-----Original Message-----
From: dev <dev-boun...@openvswitch.org> on behalf of Sairam Venugopal
<vsai...@vmware.com>
Date: Thursday, July 7, 2016 at 6:14 PM
To: "dev@openvswitch.org" <dev@openvswitch.org>
Subject: [ovs-dev] [PATCH] Windows: Add support for handling
protocol (netlink family)
>Windows datapath currently has no notion of netlink family.
>It assumes all netlink messages to belong to NETLINK_GENERIC family.
>This patch adds support for handling other protocols if the userspace
>sends it down to kernel.
>
>This patch introduces a new NETLINK_CMD - OVS_CTRL_CMD_SOCK_PROP to manage
>all properties associated with a socket. The properties are passed down as
>netlink message attributes. This makes it easier to introduce other
>properties in the future.
>
>Signed-off-by: Sairam Venugopal <vsai...@vmware.com>
>---
> datapath-windows/include/OvsDpInterfaceExt.h | 15 +++-
> datapath-windows/ovsext/Datapath.c | 102
>++++++++++++++++++++++++++-
> datapath-windows/ovsext/Datapath.h | 1 +
> lib/netlink-socket.c | 75 ++++++++++++++++++++
> 4 files changed, 191 insertions(+), 2 deletions(-)
>
>diff --git a/datapath-windows/include/OvsDpInterfaceExt.h
>b/datapath-windows/include/OvsDpInterfaceExt.h
>index 8850535..e75ce28 100644
>--- a/datapath-windows/include/OvsDpInterfaceExt.h
>+++ b/datapath-windows/include/OvsDpInterfaceExt.h
>@@ -94,7 +94,10 @@ enum ovs_win_control_cmd {
>
> /* This command is logically belong to the Vport family */
minor: ³is logically belong² => ³logically belongs²
(not your code)
> OVS_CTRL_CMD_EVENT_NOTIFY,
>- OVS_CTRL_CMD_READ_NOTIFY
>+ OVS_CTRL_CMD_READ_NOTIFY,
>+
>+ /* Used for Socket property */
>+ OVS_CTRL_CMD_SOCK_PROP
> };
>
> /* NL Attributes for joining/unjoining an MC group */
>@@ -163,4 +166,14 @@ enum ovs_win_netdev_attr {
> typedef struct ovs_dp_stats OVS_DP_STATS;
> typedef enum ovs_vport_type OVS_VPORT_TYPE;
>
>+/* NL Attributes for setting socket attributes */
>+enum ovs_nl_sock_attr {
>+ /* (UINT32) Netlink Protocol set in Userspace and read in Kernel */
>+ OVS_NL_ATTR_SOCK_PROTO,
>+ /* (UINT32) Instance PID set in Kernel and read in Userspace */
>+ OVS_NL_ATTR_SOCK_PID,
>+ __OVS_NL_ATTR_SOCK_MAX
>+};
>+#define OVS_WIN_SOCK_ATTR_MAX (__OVS_NL_ATTR_SOCK_MAX - 1)
>+
> #endif /* __OVS_DP_INTERFACE_EXT_H_ */
>diff --git a/datapath-windows/ovsext/Datapath.c
>b/datapath-windows/ovsext/Datapath.c
>index 16d58ef..210184d 100644
>--- a/datapath-windows/ovsext/Datapath.c
>+++ b/datapath-windows/ovsext/Datapath.c
>@@ -87,7 +87,8 @@ static NetlinkCmdHandler OvsPendEventCmdHandler,
> OvsReadEventCmdHandler,
> OvsNewDpCmdHandler,
> OvsGetDpCmdHandler,
>- OvsSetDpCmdHandler;
>+ OvsSetDpCmdHandler,
>+ OvsManageSocketProperty;
Minor: All NL handlers are called: *CmdHandler. So, maybe
OvsSockPropCmdHandler().
>
> NetlinkCmdHandler OvsGetNetdevCmdHandler,
> OvsGetVportCmdHandler,
>@@ -147,6 +148,11 @@ NETLINK_CMD nlControlFamilyCmdOps[] = {
> .handler = OvsReadPacketCmdHandler,
> .supportedDevOp = OVS_READ_DEV_OP,
> .validateDpIndex = FALSE,
>+ },
>+ { .cmd = OVS_CTRL_CMD_SOCK_PROP,
>+ .handler = OvsManageSocketProperty,
>+ .supportedDevOp = OVS_TRANSACTION_DEV_OP,
>+ .validateDpIndex = FALSE,
> }
> };
>
>@@ -1685,3 +1691,97 @@ OvsReadEventCmdHandler(POVS_USER_PARAMS_CONTEXT
>usrParamsCtx,
> cleanup:
> return status;
> }
>+
>+/*
>+ *
>--------------------------------------------------------------------------
>+ * Command Handler for 'OVS_CTRL_CMD_SOCK_PROP'.
>+ *
>+ * Handler to set and verify socket properties between userspace and
>kernel.
>+ *
>+ * Protocol is passed down by the userspace. It refers to the NETLINK
>family
>+ * and could be of different types (NETLINK_GENERIC/NETLINK_NETFILTER
>etc.,)
>+ * This function parses out the protocol and adds it to the open
>instance.
>+ *
>+ * PID is generated by the kernel and is set in userspace after
>querying the
>+ * kernel for it. This function does not modify PID set in the kernel,
>+ * instead it verifies if it was sent down correctly.
>+ *
>+ * XXX -This method can be modified to handle all Socket properties
>thereby
>+ * eliminating the use of OVS_IOCTL_GET_PID
>+ *
>+ *
>--------------------------------------------------------------------------
>+ */
>+static NTSTATUS
>+OvsManageSocketProperty(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
>+ UINT32 *replyLen)
>+{
>+ static const NL_POLICY ovsSocketPolicy[] = {
>+ [OVS_NL_ATTR_SOCK_PROTO] = { .type = NL_A_U32, .optional = TRUE
>},
>+ [OVS_NL_ATTR_SOCK_PID] = { .type = NL_A_U32, .optional = TRUE }
>+ };
>+ PNL_ATTR attrs[ARRAY_SIZE(ovsSocketPolicy)];
>+
>+ if (usrParamsCtx->outputLength < sizeof(OVS_MESSAGE)) {
>+ return STATUS_NDIS_INVALID_LENGTH;
>+ }
>+
>+ NL_BUFFER nlBuf;
>+ POVS_MESSAGE msgIn = (POVS_MESSAGE)usrParamsCtx->inputBuffer;
>+ POVS_MESSAGE msgOut = (POVS_MESSAGE)usrParamsCtx->outputBuffer;
>+ POVS_OPEN_INSTANCE instance =
>(POVS_OPEN_INSTANCE)usrParamsCtx->ovsInstance;
>+ UINT32 protocol;
>+ PNL_MSG_HDR nlMsg;
>+ UINT32 pid;
>+
>+ /* Parse the input */
>+ if (!NlAttrParse((PNL_MSG_HDR)msgIn,
>+ NLMSG_HDRLEN + GENL_HDRLEN + OVS_HDRLEN,
>+ NlMsgAttrsLen((PNL_MSG_HDR)msgIn),
>+ ovsSocketPolicy, ARRAY_SIZE(ovsSocketPolicy),
>+ attrs, ARRAY_SIZE(attrs))) {
>+ return STATUS_INVALID_PARAMETER;
>+ }
minor: align arg #2 onwards either 4 spaces to the right or along the same
level as arg #1.
>+
>+ /* Set the Protocol if it was passed down */
>+ if (attrs[OVS_NL_ATTR_SOCK_PROTO]) {
>+ protocol = NlAttrGetU32(attrs[OVS_NL_ATTR_SOCK_PROTO]);
>+ if (protocol) {
>+ instance->protocol = protocol;
>+ }
>+ }
>+
>+ /* Verify if the PID sent down matches the kernel */
>+ if (attrs[OVS_NL_ATTR_SOCK_PID]) {
>+ pid = NlAttrGetU32(attrs[OVS_NL_ATTR_SOCK_PID]);
>+ if (pid != instance->pid) {
>+ OVS_LOG_ERROR("Received invalid pid:%d expected:%d",
>+ pid, instance->pid);
>+ return STATUS_INVALID_PARAMETER;
>+ }
>+ }
>+
>+ /* Prepare the output */
>+ NlBufInit(&nlBuf, usrParamsCtx->outputBuffer,
>usrParamsCtx->outputLength);
>+ if(!NlFillOvsMsg(&nlBuf, msgIn->nlMsg.nlmsgType, NLM_F_MULTI,
>+ msgIn->nlMsg.nlmsgSeq, msgIn->nlMsg.nlmsgPid,
>+ msgIn->genlMsg.cmd, msgIn->genlMsg.version,
>+ gOvsSwitchContext->dpNo)){
>+ return STATUS_INVALID_BUFFER_SIZE;
>+ }
>+
>+ if (!NlMsgPutTailU32(&nlBuf, OVS_NL_ATTR_SOCK_PID,
>+ instance->pid)) {
>+ return STATUS_INVALID_BUFFER_SIZE;
>+ }
>+
>+ if (!NlMsgPutTailU32(&nlBuf, OVS_NL_ATTR_SOCK_PROTO,
>+ instance->protocol)) {
>+ return STATUS_INVALID_BUFFER_SIZE;
>+ }
>+
>+ nlMsg = (PNL_MSG_HDR)NlBufAt(&nlBuf, 0, 0);
>+ nlMsg->nlmsgLen = NlBufSize(&nlBuf);
>+ *replyLen = msgOut->nlMsg.nlmsgLen;
>+
>+ return STATUS_SUCCESS;
>+}
>diff --git a/datapath-windows/ovsext/Datapath.h
>b/datapath-windows/ovsext/Datapath.h
>index 09e233f..2b41d82 100644
>--- a/datapath-windows/ovsext/Datapath.h
>+++ b/datapath-windows/ovsext/Datapath.h
>@@ -51,6 +51,7 @@ typedef struct _OVS_OPEN_INSTANCE {
> PVOID eventQueue;
> POVS_USER_PACKET_QUEUE packetQueue;
> UINT32 pid;
>+ UINT32 protocol; /* Refers to NETLINK Family (eg. NETLINK_GENERIC)*/
>
> struct {
> POVS_MESSAGE ovsMsg; /* OVS message passed during dump start.
>*/
>diff --git a/lib/netlink-socket.c b/lib/netlink-socket.c
>index 32b0cc3..51375d4 100644
>--- a/lib/netlink-socket.c
>+++ b/lib/netlink-socket.c
>@@ -59,6 +59,7 @@ static void log_nlmsg(const char *function, int error,
> const void *message, size_t size, int protocol);
> #ifdef _WIN32
> static int get_sock_pid_from_kernel(struct nl_sock *sock);
>+static int set_sock_property(struct nl_sock *sock);
> #endif
> ?
> /* Netlink sockets. */
>@@ -165,6 +166,10 @@ nl_sock_create(int protocol, struct nl_sock **sockp)
> if (retval != 0) {
> goto error;
> }
>+ retval = set_sock_property(sock);
>+ if (retval != 0) {
>+ goto error;
>+ }
> #else
> if (setsockopt(sock->fd, SOL_SOCKET, SO_RCVBUFFORCE,
> &rcvbuf, sizeof rcvbuf)) {
>@@ -284,6 +289,76 @@ get_sock_pid_from_kernel(struct nl_sock *sock)
>
> return retval;
> }
>+
>+/* Used for setting and managing socket properties in userspace and
>kernel.
>+ * Currently two attributes are tracked - pid and protocol
>+ * protocol - supplied by userspace based on the netlink family. Windows
>uses
>+ * this property to set the value in kernel datapath.
>+ * eg: (NETLINK_GENERIC/ NETLINK_NETFILTER)
>+ * pid - generated by windows kernel and set in userspace. The
>property
>+ * is not modified.
>+ * Also verify if Protocol and PID in Kernel reflects the values in
>userspace
>+ * */
>+static int
>+set_sock_property(struct nl_sock *sock)
>+{
>+ static const struct nl_policy ovs_socket_policy[] = {
>+ [OVS_NL_ATTR_SOCK_PROTO] = { .type = NL_A_BE32, .optional = true
>},
>+ [OVS_NL_ATTR_SOCK_PID] = { .type = NL_A_BE32, .optional = true }
>+ };
>+
>+ struct ofpbuf request, *reply;
>+ struct ovs_header *ovs_header;
>+ struct nlattr *attrs[ARRAY_SIZE(ovs_socket_policy)];
>+ int retval = 0;
>+ int error;
>+
>+ ofpbuf_init(&request, 0);
>+ nl_msg_put_genlmsghdr(&request, 0, OVS_WIN_NL_CTRL_FAMILY_ID, 0,
>+ OVS_CTRL_CMD_SOCK_PROP,
>OVS_WIN_CONTROL_VERSION);
>+ ovs_header = ofpbuf_put_uninit(&request, sizeof *ovs_header);
>+ ovs_header->dp_ifindex = 0;
>+
>+ nl_msg_put_be32(&request, OVS_NL_ATTR_SOCK_PROTO, sock->protocol);
>+ /* pid is already set as part of get_sock_pid_from_kernel()
>+ * This is added to maintain consistency
>+ */
>+ nl_msg_put_be32(&request, OVS_NL_ATTR_SOCK_PID, sock->pid);
>+
>+ error = nl_sock_transact(sock, &request, &reply);
>+ ofpbuf_uninit(&request);
>+ if (error) {
>+ retval = EINVAL;
>+ }
>+
>+ if (!nl_policy_parse(reply,
>+ NLMSG_HDRLEN + GENL_HDRLEN + sizeof *ovs_header,
>+ ovs_socket_policy, attrs,
>+ ARRAY_SIZE(ovs_socket_policy))) {
>+ ofpbuf_delete(reply);
>+ retval = EINVAL;
>+ }
>+ /* Verify if the properties are setup properly */
>+ if(attrs[OVS_NL_ATTR_SOCK_PROTO]) {
minor: whitespace needed after Œif'
>+ int protocol = nl_attr_get_be32(attrs[OVS_NL_ATTR_SOCK_PROTO]);
>+ if(protocol != sock->protocol) {
minor: whitespace needed after Œif'
>+ VLOG_ERR("Invalid protocol returned:%d expected:%d",
>+ protocol, sock->protocol);
>+ retval = EINVAL;
>+ }
>+ }
>+
>+ if(attrs[OVS_NL_ATTR_SOCK_PID]) {
minor: whitespace needed after Œif'
>+ int pid = nl_attr_get_be32(attrs[OVS_NL_ATTR_SOCK_PID]);
>+ if(pid != sock->pid) {
minor: whitespace needed after Œif'
>+ VLOG_ERR("Invalid pid returned:%d expected:%d",
>+ pid, sock->pid);
>+ retval = EINVAL;
>+ }
>+ }
>+
>+ return retval;
>+}
> #endif /* _WIN32 */
>
> #ifdef _WIN32
>--
>2.9.0.windows.1
>
>_______________________________________________
>dev mailing list
>dev@openvswitch.org
>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailma
>n_listinfo_dev&d=CwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=pN
>HQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=5RKcCre7K3WdKv0m8O_a7zgr-B9prf
>gWhRJcc2afLps&s=hSVROJkjb846dj_5qSkSX-nXy_aghVtbqHu1cF40KjM&e=
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
