[PATCH net 0/7] OVS conntrack fixes for net

[PATCH net 0/7] OVS conntrack fixes for net

[Joe Stringer]

The userspace side of the Open vSwitch conntrack changes is currently
undergoing review, which has highlighted some minor bugs in the existing
conntrack implementation in the kernel, as well as pointing out some
future-proofing that can be done on the interface to reduce the need for
additional compatibility code in future.

The biggest changes here are to the userspace API for the ct_state match
field and the CT action. This series proposes to firstly extend the ct_state
match field to 32 bits, ensuring to reject any currently unsupported bits.
Secondly, rather than representing CT action flags within a 32-bit field,
simply use a netlink attribute as presence of the single flag that is
defined today. This also serves to reject unsupported ct action flag bits.

Joe Stringer (7):
  openvswitch: Make LABELS name more consistent
  openvswitch: Fix typos in CT headers
  openvswitch: Fix skb leak in ovs_fragment()
  openvswitch: Ensure flow is valid before executing ct
  openvswitch: Reject ct_state unsupported bits
  openvswitch: Extend ct_state match field to 32 bits
  openvswitch: Change CT_ATTR_FLAGS to CT_ATTR_COMMIT

 include/uapi/linux/openvswitch.h | 28 +++++++++++-----------------
 net/openvswitch/actions.c        | 21 ++++++++++++++++-----
 net/openvswitch/conntrack.c      | 32 +++++++++++++++++++-------------
 net/openvswitch/conntrack.h      | 12 ++++++++++++
 net/openvswitch/flow_netlink.c   | 26 ++++++++++++++++----------
 5 files changed, 74 insertions(+), 45 deletions(-)

-- 
2.1.4

[PATCH net 1/7] openvswitch: Make LABELS name more consistent

[Joe Stringer]

Conntrack LABELS (plural) are exposed by conntrack; rename the OVS name
for these to be consistent with conntrack.

Fixes: c2ac667 "openvswitch: Allow matching on conntrack label"
Signed-off-by: Joe Stringer <joestringer@nicira.com>
---
 include/uapi/linux/openvswitch.h |  4 ++--
 net/openvswitch/actions.c        |  2 +-
 net/openvswitch/conntrack.c      | 10 +++++-----
 net/openvswitch/flow_netlink.c   | 14 +++++++-------
 4 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index 32e07d8..9afcd60 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -326,7 +326,7 @@ enum ovs_key_attr {
 	OVS_KEY_ATTR_CT_STATE,	/* u8 bitmask of OVS_CS_F_* */
 	OVS_KEY_ATTR_CT_ZONE,	/* u16 connection tracking zone. */
 	OVS_KEY_ATTR_CT_MARK,	/* u32 connection tracking mark */
-	OVS_KEY_ATTR_CT_LABEL,	/* 16-octet connection tracking label */
+	OVS_KEY_ATTR_CT_LABELS,	/* 16-octet connection tracking label */
 
 #ifdef __KERNEL__
 	OVS_KEY_ATTR_TUNNEL_INFO,  /* struct ip_tunnel_info */
@@ -633,7 +633,7 @@ enum ovs_ct_attr {
 	OVS_CT_ATTR_FLAGS,      /* u8 bitmask of OVS_CT_F_*. */
 	OVS_CT_ATTR_ZONE,       /* u16 zone id. */
 	OVS_CT_ATTR_MARK,       /* mark to associate with this connection. */
-	OVS_CT_ATTR_LABEL,      /* label to associate with this connection. */
+	OVS_CT_ATTR_LABELS,     /* labels to associate with this connection. */
 	OVS_CT_ATTR_HELPER,     /* netlink helper to assist detection of
 				   related connections. */
 	__OVS_CT_ATTR_MAX
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 315f533..e23a61c 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -968,7 +968,7 @@ static int execute_masked_set_action(struct sk_buff *skb,
 	case OVS_KEY_ATTR_CT_STATE:
 	case OVS_KEY_ATTR_CT_ZONE:
 	case OVS_KEY_ATTR_CT_MARK:
-	case OVS_KEY_ATTR_CT_LABEL:
+	case OVS_KEY_ATTR_CT_LABELS:
 		err = -EINVAL;
 		break;
 	}
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 002a755..8c5d482c 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -179,7 +179,7 @@ int ovs_ct_put_key(const struct sw_flow_key *key, struct sk_buff *skb)
 		return -EMSGSIZE;
 
 	if (IS_ENABLED(CONFIG_NF_CONNTRACK_LABELS) &&
-	    nla_put(skb, OVS_KEY_ATTR_CT_LABEL, sizeof(key->ct.label),
+	    nla_put(skb, OVS_KEY_ATTR_CT_LABELS, sizeof(key->ct.label),
 		    &key->ct.label))
 		return -EMSGSIZE;
 
@@ -545,7 +545,7 @@ static const struct ovs_ct_len_tbl ovs_ct_attr_lens[OVS_CT_ATTR_MAX + 1] = {
 				    .maxlen = sizeof(u16) },
 	[OVS_CT_ATTR_MARK]	= { .minlen = sizeof(struct md_mark),
 				    .maxlen = sizeof(struct md_mark) },
-	[OVS_CT_ATTR_LABEL]	= { .minlen = sizeof(struct md_label),
+	[OVS_CT_ATTR_LABELS]	= { .minlen = sizeof(struct md_label),
 				    .maxlen = sizeof(struct md_label) },
 	[OVS_CT_ATTR_HELPER]	= { .minlen = 1,
 				    .maxlen = NF_CT_HELPER_NAME_LEN }
@@ -593,7 +593,7 @@ static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info,
 		}
 #endif
 #ifdef CONFIG_NF_CONNTRACK_LABELS
-		case OVS_CT_ATTR_LABEL: {
+		case OVS_CT_ATTR_LABELS: {
 			struct md_label *label = nla_data(a);
 
 			info->label = *label;
@@ -633,7 +633,7 @@ bool ovs_ct_verify(struct net *net, enum ovs_key_attr attr)
 	    attr == OVS_KEY_ATTR_CT_MARK)
 		return true;
 	if (IS_ENABLED(CONFIG_NF_CONNTRACK_LABELS) &&
-	    attr == OVS_KEY_ATTR_CT_LABEL) {
+	    attr == OVS_KEY_ATTR_CT_LABELS) {
 		struct ovs_net *ovs_net = net_generic(net, ovs_net_id);
 
 		return ovs_net->xt_label;
@@ -711,7 +711,7 @@ int ovs_ct_action_to_attr(const struct ovs_conntrack_info *ct_info,
 		    &ct_info->mark))
 		return -EMSGSIZE;
 	if (IS_ENABLED(CONFIG_NF_CONNTRACK_LABELS) &&
-	    nla_put(skb, OVS_CT_ATTR_LABEL, sizeof(ct_info->label),
+	    nla_put(skb, OVS_CT_ATTR_LABELS, sizeof(ct_info->label),
 		    &ct_info->label))
 		return -EMSGSIZE;
 	if (ct_info->helper) {
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index 5c030a4..ea82cd5 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -294,7 +294,7 @@ size_t ovs_key_attr_size(void)
 		+ nla_total_size(1)   /* OVS_KEY_ATTR_CT_STATE */
 		+ nla_total_size(2)   /* OVS_KEY_ATTR_CT_ZONE */
 		+ nla_total_size(4)   /* OVS_KEY_ATTR_CT_MARK */
-		+ nla_total_size(16)  /* OVS_KEY_ATTR_CT_LABEL */
+		+ nla_total_size(16)  /* OVS_KEY_ATTR_CT_LABELS */
 		+ nla_total_size(12)  /* OVS_KEY_ATTR_ETHERNET */
 		+ nla_total_size(2)   /* OVS_KEY_ATTR_ETHERTYPE */
 		+ nla_total_size(4)   /* OVS_KEY_ATTR_VLAN */
@@ -352,7 +352,7 @@ static const struct ovs_len_tbl ovs_key_lens[OVS_KEY_ATTR_MAX + 1] = {
 	[OVS_KEY_ATTR_CT_STATE]	 = { .len = sizeof(u8) },
 	[OVS_KEY_ATTR_CT_ZONE]	 = { .len = sizeof(u16) },
 	[OVS_KEY_ATTR_CT_MARK]	 = { .len = sizeof(u32) },
-	[OVS_KEY_ATTR_CT_LABEL]	 = { .len = sizeof(struct ovs_key_ct_label) },
+	[OVS_KEY_ATTR_CT_LABELS] = { .len = sizeof(struct ovs_key_ct_label) },
 };
 
 static bool check_attr_len(unsigned int attr_len, unsigned int expected_len)
@@ -833,14 +833,14 @@ static int metadata_from_nlattrs(struct net *net, struct sw_flow_match *match,
 		SW_FLOW_KEY_PUT(match, ct.mark, mark, is_mask);
 		*attrs &= ~(1ULL << OVS_KEY_ATTR_CT_MARK);
 	}
-	if (*attrs & (1 << OVS_KEY_ATTR_CT_LABEL) &&
-	    ovs_ct_verify(net, OVS_KEY_ATTR_CT_LABEL)) {
+	if (*attrs & (1 << OVS_KEY_ATTR_CT_LABELS) &&
+	    ovs_ct_verify(net, OVS_KEY_ATTR_CT_LABELS)) {
 		const struct ovs_key_ct_label *cl;
 
-		cl = nla_data(a[OVS_KEY_ATTR_CT_LABEL]);
+		cl = nla_data(a[OVS_KEY_ATTR_CT_LABELS]);
 		SW_FLOW_KEY_MEMCPY(match, ct.label, cl->ct_label,
 				   sizeof(*cl), is_mask);
-		*attrs &= ~(1ULL << OVS_KEY_ATTR_CT_LABEL);
+		*attrs &= ~(1ULL << OVS_KEY_ATTR_CT_LABELS);
 	}
 	return 0;
 }
@@ -1973,7 +1973,7 @@ static int validate_set(const struct nlattr *a,
 	case OVS_KEY_ATTR_PRIORITY:
 	case OVS_KEY_ATTR_SKB_MARK:
 	case OVS_KEY_ATTR_CT_MARK:
-	case OVS_KEY_ATTR_CT_LABEL:
+	case OVS_KEY_ATTR_CT_LABELS:
 	case OVS_KEY_ATTR_ETHERNET:
 		break;
 
-- 
2.1.4

[PATCH net 2/7] openvswitch: Fix typos in CT headers

[Joe Stringer]

These comments hadn't caught up to their implementations, fix them.

Fixes: 7f8a436 "openvswitch: Add conntrack action"
Signed-off-by: Joe Stringer <joestringer@nicira.com>
---
 include/uapi/linux/openvswitch.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index 9afcd60..7cbb9d5 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -630,7 +630,7 @@ struct ovs_action_hash {
  */
 enum ovs_ct_attr {
 	OVS_CT_ATTR_UNSPEC,
-	OVS_CT_ATTR_FLAGS,      /* u8 bitmask of OVS_CT_F_*. */
+	OVS_CT_ATTR_FLAGS,      /* u32 bitmask of OVS_CT_F_*. */
 	OVS_CT_ATTR_ZONE,       /* u16 zone id. */
 	OVS_CT_ATTR_MARK,       /* mark to associate with this connection. */
 	OVS_CT_ATTR_LABELS,     /* labels to associate with this connection. */
@@ -705,7 +705,7 @@ enum ovs_action_attr {
 				       * data immediately followed by a mask.
 				       * The data must be zero for the unmasked
 				       * bits. */
-	OVS_ACTION_ATTR_CT,           /* One nested OVS_CT_ATTR_* . */
+	OVS_ACTION_ATTR_CT,           /* Nested OVS_CT_ATTR_* . */
 
 	__OVS_ACTION_ATTR_MAX,	      /* Nothing past this will be accepted
 				       * from userspace. */
-- 
2.1.4

[PATCH net 3/7] openvswitch: Fix skb leak in ovs_fragment()

[Joe Stringer]

If ovs_fragment() was unable to fragment the skb due to an L2 header
that exceeds the supported length, skbs would be leaked. Fix the bug.

Fixes: 7f8a436 "openvswitch: Add conntrack action"
Signed-off-by: Joe Stringer <joestringer@nicira.com>
---
 net/openvswitch/actions.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index e23a61c..e1afbd1 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -684,7 +684,7 @@ static void ovs_fragment(struct vport *vport, struct sk_buff *skb, u16 mru,
 {
 	if (skb_network_offset(skb) > MAX_L2_LEN) {
 		OVS_NLERR(1, "L2 header too long to fragment");
-		return;
+		goto out;
 	}
 
 	if (ethertype == htons(ETH_P_IP)) {
@@ -708,8 +708,7 @@ static void ovs_fragment(struct vport *vport, struct sk_buff *skb, u16 mru,
 		struct rt6_info ovs_rt;
 
 		if (!v6ops) {
-			kfree_skb(skb);
-			return;
+			goto out;
 		}
 
 		prepare_frag(vport, skb);
@@ -728,8 +727,14 @@ static void ovs_fragment(struct vport *vport, struct sk_buff *skb, u16 mru,
 		WARN_ONCE(1, "Failed fragment ->%s: eth=%04x, MRU=%d, MTU=%d.",
 			  ovs_vport_name(vport), ntohs(ethertype), mru,
 			  vport->dev->mtu);
-		kfree_skb(skb);
+		goto out;
 	}
+
+	skb = NULL;
+
+out:
+	if (skb)
+		kfree_skb(skb);
 }
 
 static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port,
-- 
2.1.4

[PATCH net 4/7] openvswitch: Ensure flow is valid before executing ct

[Joe Stringer]

The ct action uses parts of the flow key, so we need to ensure that it
is valid before executing that action.

Fixes: 7f8a436 "openvswitch: Add conntrack action"
Signed-off-by: Joe Stringer <joestringer@nicira.com>
---
 net/openvswitch/actions.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index e1afbd1..9a88f15 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -1104,6 +1104,12 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
 			break;
 
 		case OVS_ACTION_ATTR_CT:
+			if (!is_flow_key_valid(key)) {
+				err = ovs_flow_key_update(skb, key);
+				if (err)
+					return err;
+			}
+
 			err = ovs_ct_execute(ovs_dp_get_net(dp), skb, key,
 					     nla_data(a));
 
-- 
2.1.4

[PATCH net 5/7] openvswitch: Reject ct_state unsupported bits

[Joe Stringer]

Previously, if userspace specified ct_state bits in the flow key which
are currently undefined (and therefore unsupported), then they would be
ignored. This could cause unexpected behaviour in future if userspace is
extended to support additional bits but attempts to communicate with the
current version of the kernel. This patch rectifies the situation by
rejecting such ct_state bits.

Fixes: 7f8a436 "openvswitch: Add conntrack action"
Signed-off-by: Joe Stringer <joestringer@nicira.com>
---
 net/openvswitch/conntrack.h    | 12 ++++++++++++
 net/openvswitch/flow_netlink.c |  6 ++++++
 2 files changed, 18 insertions(+)

diff --git a/net/openvswitch/conntrack.h b/net/openvswitch/conntrack.h
index 43f5dd7..c658d95 100644
--- a/net/openvswitch/conntrack.h
+++ b/net/openvswitch/conntrack.h
@@ -34,6 +34,13 @@ int ovs_ct_execute(struct net *, struct sk_buff *, struct sw_flow_key *,
 void ovs_ct_fill_key(const struct sk_buff *skb, struct sw_flow_key *key);
 int ovs_ct_put_key(const struct sw_flow_key *key, struct sk_buff *skb);
 void ovs_ct_free_action(const struct nlattr *a);
+
+static inline bool ovs_ct_state_supported(u8 state)
+{
+	return !(state & ~(OVS_CS_F_NEW | OVS_CS_F_ESTABLISHED |
+			 OVS_CS_F_RELATED | OVS_CS_F_REPLY_DIR |
+			 OVS_CS_F_INVALID | OVS_CS_F_TRACKED));
+}
 #else
 #include <linux/errno.h>
 
@@ -46,6 +53,11 @@ static inline bool ovs_ct_verify(struct net *net, int attr)
 	return false;
 }
 
+static inline bool ovs_ct_state_supported(u8 state)
+{
+	return false;
+}
+
 static inline int ovs_ct_copy_action(struct net *net, const struct nlattr *nla,
 				     const struct sw_flow_key *key,
 				     struct sw_flow_actions **acts, bool log)
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index ea82cd5..c4917c9 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -816,6 +816,12 @@ static int metadata_from_nlattrs(struct net *net, struct sw_flow_match *match,
 	    ovs_ct_verify(net, OVS_KEY_ATTR_CT_STATE)) {
 		u8 ct_state = nla_get_u8(a[OVS_KEY_ATTR_CT_STATE]);
 
+		if (!is_mask && !ovs_ct_state_supported(ct_state)) {
+			OVS_NLERR(log, "ct_state flags %02x unsupported",
+				  ct_state);
+			return -EINVAL;
+		}
+
 		SW_FLOW_KEY_PUT(match, ct.state, ct_state, is_mask);
 		*attrs &= ~(1ULL << OVS_KEY_ATTR_CT_STATE);
 	}
-- 
2.1.4

[PATCH net 6/7] openvswitch: Extend ct_state match field to 32 bits

[Joe Stringer]

The ct_state field was initially added as an 8-bit field, however six of
the bits are already being used and use cases are already starting to
appear that may push the limits of this field. This patch extends the
field to 32 bits while retaining the internal representation of 8 bits.
This should cover forward compatibility of the ABI for the foreseeable
future.

This patch also reorders the OVS_CS_F_* bits to be sequential.

Suggested-by: Jarno Rajahalme <jrajahalme@nicira.com>
Signed-off-by: Joe Stringer <joestringer@nicira.com>
---
 include/uapi/linux/openvswitch.h | 8 ++++----
 net/openvswitch/conntrack.c      | 2 +-
 net/openvswitch/conntrack.h      | 4 ++--
 net/openvswitch/flow_netlink.c   | 8 ++++----
 4 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index 7cbb9d5..f121af5 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -323,7 +323,7 @@ enum ovs_key_attr {
 	OVS_KEY_ATTR_MPLS,      /* array of struct ovs_key_mpls.
 				 * The implementation may restrict
 				 * the accepted length of the array. */
-	OVS_KEY_ATTR_CT_STATE,	/* u8 bitmask of OVS_CS_F_* */
+	OVS_KEY_ATTR_CT_STATE,	/* u32 bitmask of OVS_CS_F_* */
 	OVS_KEY_ATTR_CT_ZONE,	/* u16 connection tracking zone. */
 	OVS_KEY_ATTR_CT_MARK,	/* u32 connection tracking mark */
 	OVS_KEY_ATTR_CT_LABELS,	/* 16-octet connection tracking label */
@@ -449,9 +449,9 @@ struct ovs_key_ct_label {
 #define OVS_CS_F_ESTABLISHED       0x02 /* Part of an existing connection. */
 #define OVS_CS_F_RELATED           0x04 /* Related to an established
 					 * connection. */
-#define OVS_CS_F_INVALID           0x20 /* Could not track connection. */
-#define OVS_CS_F_REPLY_DIR         0x40 /* Flow is in the reply direction. */
-#define OVS_CS_F_TRACKED           0x80 /* Conntrack has occurred. */
+#define OVS_CS_F_REPLY_DIR         0x08 /* Flow is in the reply direction. */
+#define OVS_CS_F_INVALID           0x10 /* Could not track connection. */
+#define OVS_CS_F_TRACKED           0x20 /* Conntrack has occurred. */
 
 /**
  * enum ovs_flow_attr - attributes for %OVS_FLOW_* commands.
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 8c5d482c..167cf43 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -167,7 +167,7 @@ void ovs_ct_fill_key(const struct sk_buff *skb, struct sw_flow_key *key)
 
 int ovs_ct_put_key(const struct sw_flow_key *key, struct sk_buff *skb)
 {
-	if (nla_put_u8(skb, OVS_KEY_ATTR_CT_STATE, key->ct.state))
+	if (nla_put_u32(skb, OVS_KEY_ATTR_CT_STATE, key->ct.state))
 		return -EMSGSIZE;
 
 	if (IS_ENABLED(CONFIG_NF_CONNTRACK_ZONES) &&
diff --git a/net/openvswitch/conntrack.h b/net/openvswitch/conntrack.h
index c658d95..7a125422 100644
--- a/net/openvswitch/conntrack.h
+++ b/net/openvswitch/conntrack.h
@@ -35,7 +35,7 @@ void ovs_ct_fill_key(const struct sk_buff *skb, struct sw_flow_key *key);
 int ovs_ct_put_key(const struct sw_flow_key *key, struct sk_buff *skb);
 void ovs_ct_free_action(const struct nlattr *a);
 
-static inline bool ovs_ct_state_supported(u8 state)
+static inline bool ovs_ct_state_supported(u32 state)
 {
 	return !(state & ~(OVS_CS_F_NEW | OVS_CS_F_ESTABLISHED |
 			 OVS_CS_F_RELATED | OVS_CS_F_REPLY_DIR |
@@ -53,7 +53,7 @@ static inline bool ovs_ct_verify(struct net *net, int attr)
 	return false;
 }
 
-static inline bool ovs_ct_state_supported(u8 state)
+static inline bool ovs_ct_state_supported(u32 state)
 {
 	return false;
 }
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index c4917c9..292eb13 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -291,7 +291,7 @@ size_t ovs_key_attr_size(void)
 		+ nla_total_size(4)   /* OVS_KEY_ATTR_SKB_MARK */
 		+ nla_total_size(4)   /* OVS_KEY_ATTR_DP_HASH */
 		+ nla_total_size(4)   /* OVS_KEY_ATTR_RECIRC_ID */
-		+ nla_total_size(1)   /* OVS_KEY_ATTR_CT_STATE */
+		+ nla_total_size(4)   /* OVS_KEY_ATTR_CT_STATE */
 		+ nla_total_size(2)   /* OVS_KEY_ATTR_CT_ZONE */
 		+ nla_total_size(4)   /* OVS_KEY_ATTR_CT_MARK */
 		+ nla_total_size(16)  /* OVS_KEY_ATTR_CT_LABELS */
@@ -349,7 +349,7 @@ static const struct ovs_len_tbl ovs_key_lens[OVS_KEY_ATTR_MAX + 1] = {
 	[OVS_KEY_ATTR_TUNNEL]	 = { .len = OVS_ATTR_NESTED,
 				     .next = ovs_tunnel_key_lens, },
 	[OVS_KEY_ATTR_MPLS]	 = { .len = sizeof(struct ovs_key_mpls) },
-	[OVS_KEY_ATTR_CT_STATE]	 = { .len = sizeof(u8) },
+	[OVS_KEY_ATTR_CT_STATE]	 = { .len = sizeof(u32) },
 	[OVS_KEY_ATTR_CT_ZONE]	 = { .len = sizeof(u16) },
 	[OVS_KEY_ATTR_CT_MARK]	 = { .len = sizeof(u32) },
 	[OVS_KEY_ATTR_CT_LABELS] = { .len = sizeof(struct ovs_key_ct_label) },
@@ -814,10 +814,10 @@ static int metadata_from_nlattrs(struct net *net, struct sw_flow_match *match,
 
 	if (*attrs & (1 << OVS_KEY_ATTR_CT_STATE) &&
 	    ovs_ct_verify(net, OVS_KEY_ATTR_CT_STATE)) {
-		u8 ct_state = nla_get_u8(a[OVS_KEY_ATTR_CT_STATE]);
+		u32 ct_state = nla_get_u32(a[OVS_KEY_ATTR_CT_STATE]);
 
 		if (!is_mask && !ovs_ct_state_supported(ct_state)) {
-			OVS_NLERR(log, "ct_state flags %02x unsupported",
+			OVS_NLERR(log, "ct_state flags %08x unsupported",
 				  ct_state);
 			return -EINVAL;
 		}
-- 
2.1.4

[PATCH net 7/7] openvswitch: Change CT_ATTR_FLAGS to CT_ATTR_COMMIT

[Joe Stringer]

Previously, the CT_ATTR_FLAGS attribute, when nested under the
OVS_ACTION_ATTR_CT, encoded a 32-bit bitmask of flags that modify the
semantics of the ct action. It's more extensible to just represent each
flag as a nested attribute, and this requires no additional error
checking to reject flags that aren't currently supported.

Suggested-by: Ben Pfaff <blp@nicira.com>
Signed-off-by: Joe Stringer <joestringer@nicira.com>
---
 include/uapi/linux/openvswitch.h | 14 ++++----------
 net/openvswitch/conntrack.c      | 20 +++++++++++++-------
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index f121af5..e14563e 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -618,7 +618,9 @@ struct ovs_action_hash {
 
 /**
  * enum ovs_ct_attr - Attributes for %OVS_ACTION_ATTR_CT action.
- * @OVS_CT_ATTR_FLAGS: u32 connection tracking flags.
+ * @OVS_CT_ATTR_COMMIT: If present, commits the connection to the conntrack
+ * table. This allows future packets for the same connection to be identified
+ * as 'established' or 'related'.
  * @OVS_CT_ATTR_ZONE: u16 connection tracking zone.
  * @OVS_CT_ATTR_MARK: u32 value followed by u32 mask. For each bit set in the
  * mask, the corresponding bit in the value is copied to the connection
@@ -630,7 +632,7 @@ struct ovs_action_hash {
  */
 enum ovs_ct_attr {
 	OVS_CT_ATTR_UNSPEC,
-	OVS_CT_ATTR_FLAGS,      /* u32 bitmask of OVS_CT_F_*. */
+	OVS_CT_ATTR_COMMIT,     /* No argument, commits connection. */
 	OVS_CT_ATTR_ZONE,       /* u16 zone id. */
 	OVS_CT_ATTR_MARK,       /* mark to associate with this connection. */
 	OVS_CT_ATTR_LABELS,     /* labels to associate with this connection. */
@@ -641,14 +643,6 @@ enum ovs_ct_attr {
 
 #define OVS_CT_ATTR_MAX (__OVS_CT_ATTR_MAX - 1)
 
-/*
- * OVS_CT_ATTR_FLAGS flags - bitmask of %OVS_CT_F_*
- * @OVS_CT_F_COMMIT: Commits the flow to the conntrack table. This allows
- * future packets for the same connection to be identified as 'established'
- * or 'related'.
- */
-#define OVS_CT_F_COMMIT		0x01
-
 /**
  * enum ovs_action_attr - Action types.
  *
diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 167cf43..effa78c 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -42,12 +42,18 @@ struct md_label {
 	struct ovs_key_ct_label mask;
 };
 
+/* Flags for performing connection tracking.
+ *
+ * CT_F_COMMIT: Commits the flow to the conntrack table.
+ */
+#define CT_F_COMMIT	BIT(0)
+
 /* Conntrack action context for execution. */
 struct ovs_conntrack_info {
 	struct nf_conntrack_helper *helper;
 	struct nf_conntrack_zone zone;
 	struct nf_conn *ct;
-	u32 flags;
+	u8 flags;		/* bitmask of CT_F_*. */
 	u16 family;
 	struct md_mark mark;
 	struct md_label label;
@@ -493,7 +499,7 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
 			return err;
 	}
 
-	if (info->flags & OVS_CT_F_COMMIT)
+	if (info->flags & CT_F_COMMIT)
 		err = ovs_ct_commit(net, key, info, skb);
 	else
 		err = ovs_ct_lookup(net, key, info, skb);
@@ -539,8 +545,7 @@ static int ovs_ct_add_helper(struct ovs_conntrack_info *info, const char *name,
 }
 
 static const struct ovs_ct_len_tbl ovs_ct_attr_lens[OVS_CT_ATTR_MAX + 1] = {
-	[OVS_CT_ATTR_FLAGS]	= { .minlen = sizeof(u32),
-				    .maxlen = sizeof(u32) },
+	[OVS_CT_ATTR_COMMIT]	= { .minlen = 0, .maxlen = 0 },
 	[OVS_CT_ATTR_ZONE]	= { .minlen = sizeof(u16),
 				    .maxlen = sizeof(u16) },
 	[OVS_CT_ATTR_MARK]	= { .minlen = sizeof(struct md_mark),
@@ -576,8 +581,8 @@ static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info,
 		}
 
 		switch (type) {
-		case OVS_CT_ATTR_FLAGS:
-			info->flags = nla_get_u32(a);
+		case OVS_CT_ATTR_COMMIT:
+			info->flags |= CT_F_COMMIT;
 			break;
 #ifdef CONFIG_NF_CONNTRACK_ZONES
 		case OVS_CT_ATTR_ZONE:
@@ -701,7 +706,8 @@ int ovs_ct_action_to_attr(const struct ovs_conntrack_info *ct_info,
 	if (!start)
 		return -EMSGSIZE;
 
-	if (nla_put_u32(skb, OVS_CT_ATTR_FLAGS, ct_info->flags))
+	if (ct_info->flags & CT_F_COMMIT &&
+	    nla_put_flag(skb, OVS_CT_ATTR_COMMIT))
 		return -EMSGSIZE;
 	if (IS_ENABLED(CONFIG_NF_CONNTRACK_ZONES) &&
 	    nla_put_u16(skb, OVS_CT_ATTR_ZONE, ct_info->zone.id))
-- 
2.1.4

Re: [PATCH net 3/7] openvswitch: Fix skb leak in ovs_fragment()

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 1041 bytes --]

> On Sep 29, 2015, at 3:39 PM, Joe Stringer <joestringer@nicira.com> wrote:
> 
> @@ -728,8 +727,14 @@ static void ovs_fragment(struct vport *vport, struct sk_buff *skb, u16 mru,
> 		WARN_ONCE(1, "Failed fragment ->%s: eth=%04x, MRU=%d, MTU=%d.",
> 			  ovs_vport_name(vport), ntohs(ethertype), mru,
> 			  vport->dev->mtu);
> -		kfree_skb(skb);
> +		goto out;
> 	}
> +
> +	skb = NULL;
> +
> +out:
> +	if (skb)
> +		kfree_skb(skb);
> }
> 
> static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port,

Wouldn't that hunk be better as:

@@ -728,8 +727,13 @@ static void ovs_fragment(struct vport *vport, struct sk_buff *skb, u16 mru,
		WARN_ONCE(1, "Failed fragment ->%s: eth=%04x, MRU=%d, MTU=%d.",
			  ovs_vport_name(vport), ntohs(ethertype), mru,
			  vport->dev->mtu);
-		kfree_skb(skb);
+		goto out;
	}
+
+	return;
+
+out:
+	kfree_skb(skb);
}

static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port,

--
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH net 3/7] openvswitch: Fix skb leak in ovs_fragment()

[Joe Stringer]

On 29 September 2015 at 15:48, Rustad, Mark D <mark.d.rustad@intel.com> wrote:
>> On Sep 29, 2015, at 3:39 PM, Joe Stringer <joestringer@nicira.com> wrote:
>>
>> @@ -728,8 +727,14 @@ static void ovs_fragment(struct vport *vport, struct sk_buff *skb, u16 mru,
>>               WARN_ONCE(1, "Failed fragment ->%s: eth=%04x, MRU=%d, MTU=%d.",
>>                         ovs_vport_name(vport), ntohs(ethertype), mru,
>>                         vport->dev->mtu);
>> -             kfree_skb(skb);
>> +             goto out;
>>       }
>> +
>> +     skb = NULL;
>> +
>> +out:
>> +     if (skb)
>> +             kfree_skb(skb);
>> }
>>
>> static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port,
>
> Wouldn't that hunk be better as:
>
> @@ -728,8 +727,13 @@ static void ovs_fragment(struct vport *vport, struct sk_buff *skb, u16 mru,
>                 WARN_ONCE(1, "Failed fragment ->%s: eth=%04x, MRU=%d, MTU=%d.",
>                           ovs_vport_name(vport), ntohs(ethertype), mru,
>                           vport->dev->mtu);
> -               kfree_skb(skb);
> +               goto out;
>         }
> +
> +       return;
> +
> +out:
> +       kfree_skb(skb);
> }
>
> static void do_output(struct datapath *dp, struct sk_buff *skb, int out_port,
>
> --
> Mark Rustad, Networking Division, Intel Corporation

Sure thing, I'll roll this change in to a v2 when the rest of the
series is reviewed.

Re: [PATCH net 3/7] openvswitch: Fix skb leak in ovs_fragment()

[Sergei Shtylyov]

Hello.

On 09/30/2015 01:39 AM, Joe Stringer wrote:

> If ovs_fragment() was unable to fragment the skb due to an L2 header
> that exceeds the supported length, skbs would be leaked. Fix the bug.
>
> Fixes: 7f8a436 "openvswitch: Add conntrack action"
> Signed-off-by: Joe Stringer <joestringer@nicira.com>
> ---
>   net/openvswitch/actions.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index e23a61c..e1afbd1 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
[...]
> @@ -728,8 +727,14 @@ static void ovs_fragment(struct vport *vport, struct sk_buff *skb, u16 mru,
>   		WARN_ONCE(1, "Failed fragment ->%s: eth=%04x, MRU=%d, MTU=%d.",
>   			  ovs_vport_name(vport), ntohs(ethertype), mru,
>   			  vport->dev->mtu);
> -		kfree_skb(skb);
> +		goto out;
>   	}
> +
> +	skb = NULL;

    I'd just return here.

> +
> +out:
> +	if (skb)
> +		kfree_skb(skb);

    kfree_skb() checks for NULL.

[...]

MBR, Sergei

Re: [PATCH net 1/7] openvswitch: Make LABELS name more consistent

[Pravin Shelar]

On Tue, Sep 29, 2015 at 3:39 PM, Joe Stringer <joestringer@nicira.com> wrote:
> Conntrack LABELS (plural) are exposed by conntrack; rename the OVS name
> for these to be consistent with conntrack.
>
> Fixes: c2ac667 "openvswitch: Allow matching on conntrack label"
> Signed-off-by: Joe Stringer <joestringer@nicira.com>
> ---
>  include/uapi/linux/openvswitch.h |  4 ++--
>  net/openvswitch/actions.c        |  2 +-
>  net/openvswitch/conntrack.c      | 10 +++++-----
>  net/openvswitch/flow_netlink.c   | 14 +++++++-------
>  4 files changed, 15 insertions(+), 15 deletions(-)
>
...

> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index 002a755..8c5d482c 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -179,7 +179,7 @@ int ovs_ct_put_key(const struct sw_flow_key *key, struct sk_buff *skb)
>                 return -EMSGSIZE;
>
>         if (IS_ENABLED(CONFIG_NF_CONNTRACK_LABELS) &&
> -           nla_put(skb, OVS_KEY_ATTR_CT_LABEL, sizeof(key->ct.label),
> +           nla_put(skb, OVS_KEY_ATTR_CT_LABELS, sizeof(key->ct.label),
>                     &key->ct.label))
>                 return -EMSGSIZE;
>
> @@ -545,7 +545,7 @@ static const struct ovs_ct_len_tbl ovs_ct_attr_lens[OVS_CT_ATTR_MAX + 1] = {
>                                     .maxlen = sizeof(u16) },
>         [OVS_CT_ATTR_MARK]      = { .minlen = sizeof(struct md_mark),
>                                     .maxlen = sizeof(struct md_mark) },
> -       [OVS_CT_ATTR_LABEL]     = { .minlen = sizeof(struct md_label),
> +       [OVS_CT_ATTR_LABELS]    = { .minlen = sizeof(struct md_label),
>                                     .maxlen = sizeof(struct md_label) },
>         [OVS_CT_ATTR_HELPER]    = { .minlen = 1,
>                                     .maxlen = NF_CT_HELPER_NAME_LEN }
> @@ -593,7 +593,7 @@ static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info,
>                 }
>  #endif
>  #ifdef CONFIG_NF_CONNTRACK_LABELS
> -               case OVS_CT_ATTR_LABEL: {
> +               case OVS_CT_ATTR_LABELS: {
>                         struct md_label *label = nla_data(a);
>
>                         info->label = *label;
> @@ -633,7 +633,7 @@ bool ovs_ct_verify(struct net *net, enum ovs_key_attr attr)
>             attr == OVS_KEY_ATTR_CT_MARK)
>                 return true;
>         if (IS_ENABLED(CONFIG_NF_CONNTRACK_LABELS) &&
> -           attr == OVS_KEY_ATTR_CT_LABEL) {
> +           attr == OVS_KEY_ATTR_CT_LABELS) {
>                 struct ovs_net *ovs_net = net_generic(net, ovs_net_id);
>
>                 return ovs_net->xt_label;
> @@ -711,7 +711,7 @@ int ovs_ct_action_to_attr(const struct ovs_conntrack_info *ct_info,
>                     &ct_info->mark))
>                 return -EMSGSIZE;
>         if (IS_ENABLED(CONFIG_NF_CONNTRACK_LABELS) &&
> -           nla_put(skb, OVS_CT_ATTR_LABEL, sizeof(ct_info->label),
> +           nla_put(skb, OVS_CT_ATTR_LABELS, sizeof(ct_info->label),
>                     &ct_info->label))
>                 return -EMSGSIZE;
>         if (ct_info->helper) {
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index 5c030a4..ea82cd5 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -294,7 +294,7 @@ size_t ovs_key_attr_size(void)
>                 + nla_total_size(1)   /* OVS_KEY_ATTR_CT_STATE */
>                 + nla_total_size(2)   /* OVS_KEY_ATTR_CT_ZONE */
>                 + nla_total_size(4)   /* OVS_KEY_ATTR_CT_MARK */
> -               + nla_total_size(16)  /* OVS_KEY_ATTR_CT_LABEL */
> +               + nla_total_size(16)  /* OVS_KEY_ATTR_CT_LABELS */
>                 + nla_total_size(12)  /* OVS_KEY_ATTR_ETHERNET */
>                 + nla_total_size(2)   /* OVS_KEY_ATTR_ETHERTYPE */
>                 + nla_total_size(4)   /* OVS_KEY_ATTR_VLAN */
> @@ -352,7 +352,7 @@ static const struct ovs_len_tbl ovs_key_lens[OVS_KEY_ATTR_MAX + 1] = {
>         [OVS_KEY_ATTR_CT_STATE]  = { .len = sizeof(u8) },
>         [OVS_KEY_ATTR_CT_ZONE]   = { .len = sizeof(u16) },
>         [OVS_KEY_ATTR_CT_MARK]   = { .len = sizeof(u32) },
> -       [OVS_KEY_ATTR_CT_LABEL]  = { .len = sizeof(struct ovs_key_ct_label) },
> +       [OVS_KEY_ATTR_CT_LABELS] = { .len = sizeof(struct ovs_key_ct_label) },
>  };
After this change, struct ovs_key_ct_label and other reference to
label also should be renamed to labels. is there reason you did only
this one?

Re: [PATCH net 2/7] openvswitch: Fix typos in CT headers

[Pravin Shelar]

On Tue, Sep 29, 2015 at 3:39 PM, Joe Stringer <joestringer@nicira.com> wrote:
> These comments hadn't caught up to their implementations, fix them.
>
> Fixes: 7f8a436 "openvswitch: Add conntrack action"
> Signed-off-by: Joe Stringer <joestringer@nicira.com>

Acked-by: Pravin B Shelar <pshelar@nicira.com>

Re: [PATCH net 4/7] openvswitch: Ensure flow is valid before executing ct

[Pravin Shelar]

On Tue, Sep 29, 2015 at 3:39 PM, Joe Stringer <joestringer@nicira.com> wrote:
> The ct action uses parts of the flow key, so we need to ensure that it
> is valid before executing that action.
>
> Fixes: 7f8a436 "openvswitch: Add conntrack action"
> Signed-off-by: Joe Stringer <joestringer@nicira.com>

Acked-by: Pravin B Shelar <pshelar@nicira.com>

Re: [PATCH net 5/7] openvswitch: Reject ct_state unsupported bits

[Pravin Shelar]

On Tue, Sep 29, 2015 at 3:39 PM, Joe Stringer <joestringer@nicira.com> wrote:
> Previously, if userspace specified ct_state bits in the flow key which
> are currently undefined (and therefore unsupported), then they would be
> ignored. This could cause unexpected behaviour in future if userspace is
> extended to support additional bits but attempts to communicate with the
> current version of the kernel. This patch rectifies the situation by
> rejecting such ct_state bits.
>
> Fixes: 7f8a436 "openvswitch: Add conntrack action"
> Signed-off-by: Joe Stringer <joestringer@nicira.com>
> ---
>  net/openvswitch/conntrack.h    | 12 ++++++++++++
>  net/openvswitch/flow_netlink.c |  6 ++++++
>  2 files changed, 18 insertions(+)
>
> diff --git a/net/openvswitch/conntrack.h b/net/openvswitch/conntrack.h
> index 43f5dd7..c658d95 100644
> --- a/net/openvswitch/conntrack.h
> +++ b/net/openvswitch/conntrack.h
> @@ -34,6 +34,13 @@ int ovs_ct_execute(struct net *, struct sk_buff *, struct sw_flow_key *,
>  void ovs_ct_fill_key(const struct sk_buff *skb, struct sw_flow_key *key);
>  int ovs_ct_put_key(const struct sw_flow_key *key, struct sk_buff *skb);
>  void ovs_ct_free_action(const struct nlattr *a);
> +
> +static inline bool ovs_ct_state_supported(u8 state)
> +{
> +       return !(state & ~(OVS_CS_F_NEW | OVS_CS_F_ESTABLISHED |
> +                        OVS_CS_F_RELATED | OVS_CS_F_REPLY_DIR |
> +                        OVS_CS_F_INVALID | OVS_CS_F_TRACKED));
> +}
>  #else
>  #include <linux/errno.h>
>
> @@ -46,6 +53,11 @@ static inline bool ovs_ct_verify(struct net *net, int attr)
>         return false;
>  }
>
> +static inline bool ovs_ct_state_supported(u8 state)
> +{
> +       return false;
> +}
> +
>  static inline int ovs_ct_copy_action(struct net *net, const struct nlattr *nla,
>                                      const struct sw_flow_key *key,
>                                      struct sw_flow_actions **acts, bool log)
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index ea82cd5..c4917c9 100644
> --- a/net/openvswitch/flow_netlink.c
> +++ b/net/openvswitch/flow_netlink.c
> @@ -816,6 +816,12 @@ static int metadata_from_nlattrs(struct net *net, struct sw_flow_match *match,
>             ovs_ct_verify(net, OVS_KEY_ATTR_CT_STATE)) {
>                 u8 ct_state = nla_get_u8(a[OVS_KEY_ATTR_CT_STATE]);
>
We also need to return error if kernel does not support the feature.

> +               if (!is_mask && !ovs_ct_state_supported(ct_state)) {
> +                       OVS_NLERR(log, "ct_state flags %02x unsupported",
> +                                 ct_state);
> +                       return -EINVAL;
> +               }
> +
>                 SW_FLOW_KEY_PUT(match, ct.state, ct_state, is_mask);
>                 *attrs &= ~(1ULL << OVS_KEY_ATTR_CT_STATE);
>         }
> --
> 2.1.4
>

Re: [PATCH net 6/7] openvswitch: Extend ct_state match field to 32 bits

[Pravin Shelar]

On Tue, Sep 29, 2015 at 3:39 PM, Joe Stringer <joestringer@nicira.com> wrote:
> The ct_state field was initially added as an 8-bit field, however six of
> the bits are already being used and use cases are already starting to
> appear that may push the limits of this field. This patch extends the
> field to 32 bits while retaining the internal representation of 8 bits.
> This should cover forward compatibility of the ABI for the foreseeable
> future.
>
> This patch also reorders the OVS_CS_F_* bits to be sequential.
>
> Suggested-by: Jarno Rajahalme <jrajahalme@nicira.com>
> Signed-off-by: Joe Stringer <joestringer@nicira.com>

Looks good.
Acked-by: Pravin B Shelar <pshelar@nicira.com>

Re: [PATCH net 7/7] openvswitch: Change CT_ATTR_FLAGS to CT_ATTR_COMMIT

[Pravin Shelar]

On Tue, Sep 29, 2015 at 3:39 PM, Joe Stringer <joestringer@nicira.com> wrote:
> Previously, the CT_ATTR_FLAGS attribute, when nested under the
> OVS_ACTION_ATTR_CT, encoded a 32-bit bitmask of flags that modify the
> semantics of the ct action. It's more extensible to just represent each
> flag as a nested attribute, and this requires no additional error
> checking to reject flags that aren't currently supported.
>
> Suggested-by: Ben Pfaff <blp@nicira.com>
> Signed-off-by: Joe Stringer <joestringer@nicira.com>
> ---
...
...

> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index 167cf43..effa78c 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -42,12 +42,18 @@ struct md_label {
>         struct ovs_key_ct_label mask;
>  };
>
> +/* Flags for performing connection tracking.
> + *
> + * CT_F_COMMIT: Commits the flow to the conntrack table.
> + */
> +#define CT_F_COMMIT    BIT(0)
> +
>  /* Conntrack action context for execution. */
>  struct ovs_conntrack_info {
>         struct nf_conntrack_helper *helper;
>         struct nf_conntrack_zone zone;
>         struct nf_conn *ct;
> -       u32 flags;
> +       u8 flags;               /* bitmask of CT_F_*. */

I think bit field has better readability in such use case.
Otherwise looks good.

Re: [PATCH net 5/7] openvswitch: Reject ct_state unsupported bits

[Joe Stringer]

On 30 September 2015 at 17:31, Pravin Shelar <pshelar@nicira.com> wrote:
> On Tue, Sep 29, 2015 at 3:39 PM, Joe Stringer <joestringer@nicira.com> wrote:
>> Previously, if userspace specified ct_state bits in the flow key which
>> are currently undefined (and therefore unsupported), then they would be
>> ignored. This could cause unexpected behaviour in future if userspace is
>> extended to support additional bits but attempts to communicate with the
>> current version of the kernel. This patch rectifies the situation by
>> rejecting such ct_state bits.
>>
>> Fixes: 7f8a436 "openvswitch: Add conntrack action"
>> Signed-off-by: Joe Stringer <joestringer@nicira.com>
>> ---
>>  net/openvswitch/conntrack.h    | 12 ++++++++++++
>>  net/openvswitch/flow_netlink.c |  6 ++++++
>>  2 files changed, 18 insertions(+)
>>
>> diff --git a/net/openvswitch/conntrack.h b/net/openvswitch/conntrack.h
>> index 43f5dd7..c658d95 100644
>> --- a/net/openvswitch/conntrack.h
>> +++ b/net/openvswitch/conntrack.h
>> @@ -34,6 +34,13 @@ int ovs_ct_execute(struct net *, struct sk_buff *, struct sw_flow_key *,
>>  void ovs_ct_fill_key(const struct sk_buff *skb, struct sw_flow_key *key);
>>  int ovs_ct_put_key(const struct sw_flow_key *key, struct sk_buff *skb);
>>  void ovs_ct_free_action(const struct nlattr *a);
>> +
>> +static inline bool ovs_ct_state_supported(u8 state)
>> +{
>> +       return !(state & ~(OVS_CS_F_NEW | OVS_CS_F_ESTABLISHED |
>> +                        OVS_CS_F_RELATED | OVS_CS_F_REPLY_DIR |
>> +                        OVS_CS_F_INVALID | OVS_CS_F_TRACKED));
>> +}
>>  #else
>>  #include <linux/errno.h>
>>
>> @@ -46,6 +53,11 @@ static inline bool ovs_ct_verify(struct net *net, int attr)
>>         return false;
>>  }
>>
>> +static inline bool ovs_ct_state_supported(u8 state)
>> +{
>> +       return false;
>> +}
>> +
>>  static inline int ovs_ct_copy_action(struct net *net, const struct nlattr *nla,
>>                                      const struct sw_flow_key *key,
>>                                      struct sw_flow_actions **acts, bool log)
>> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
>> index ea82cd5..c4917c9 100644
>> --- a/net/openvswitch/flow_netlink.c
>> +++ b/net/openvswitch/flow_netlink.c
>> @@ -816,6 +816,12 @@ static int metadata_from_nlattrs(struct net *net, struct sw_flow_match *match,
>>             ovs_ct_verify(net, OVS_KEY_ATTR_CT_STATE)) {
>>                 u8 ct_state = nla_get_u8(a[OVS_KEY_ATTR_CT_STATE]);
>>
> We also need to return error if kernel does not support the feature.

Already handled by ovs_ct_verify() in conntrack.h.

Re: [PATCH net 1/7] openvswitch: Make LABELS name more consistent

[Joe Stringer]

On 30 September 2015 at 17:31, Pravin Shelar <pshelar@nicira.com> wrote:
> On Tue, Sep 29, 2015 at 3:39 PM, Joe Stringer <joestringer@nicira.com> wrote:
>> Conntrack LABELS (plural) are exposed by conntrack; rename the OVS name
>> for these to be consistent with conntrack.
>>
>> Fixes: c2ac667 "openvswitch: Allow matching on conntrack label"
>> Signed-off-by: Joe Stringer <joestringer@nicira.com>
>> ---
>>  include/uapi/linux/openvswitch.h |  4 ++--
>>  net/openvswitch/actions.c        |  2 +-
>>  net/openvswitch/conntrack.c      | 10 +++++-----
>>  net/openvswitch/flow_netlink.c   | 14 +++++++-------
>>  4 files changed, 15 insertions(+), 15 deletions(-)
>>
> ...
>
>> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
>> index 002a755..8c5d482c 100644
>> --- a/net/openvswitch/conntrack.c
>> +++ b/net/openvswitch/conntrack.c
>> @@ -179,7 +179,7 @@ int ovs_ct_put_key(const struct sw_flow_key *key, struct sk_buff *skb)
>>                 return -EMSGSIZE;
>>
>>         if (IS_ENABLED(CONFIG_NF_CONNTRACK_LABELS) &&
>> -           nla_put(skb, OVS_KEY_ATTR_CT_LABEL, sizeof(key->ct.label),
>> +           nla_put(skb, OVS_KEY_ATTR_CT_LABELS, sizeof(key->ct.label),
>>                     &key->ct.label))
>>                 return -EMSGSIZE;
>>
>> @@ -545,7 +545,7 @@ static const struct ovs_ct_len_tbl ovs_ct_attr_lens[OVS_CT_ATTR_MAX + 1] = {
>>                                     .maxlen = sizeof(u16) },
>>         [OVS_CT_ATTR_MARK]      = { .minlen = sizeof(struct md_mark),
>>                                     .maxlen = sizeof(struct md_mark) },
>> -       [OVS_CT_ATTR_LABEL]     = { .minlen = sizeof(struct md_label),
>> +       [OVS_CT_ATTR_LABELS]    = { .minlen = sizeof(struct md_label),
>>                                     .maxlen = sizeof(struct md_label) },
>>         [OVS_CT_ATTR_HELPER]    = { .minlen = 1,
>>                                     .maxlen = NF_CT_HELPER_NAME_LEN }
>> @@ -593,7 +593,7 @@ static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info,
>>                 }
>>  #endif
>>  #ifdef CONFIG_NF_CONNTRACK_LABELS
>> -               case OVS_CT_ATTR_LABEL: {
>> +               case OVS_CT_ATTR_LABELS: {
>>                         struct md_label *label = nla_data(a);
>>
>>                         info->label = *label;
>> @@ -633,7 +633,7 @@ bool ovs_ct_verify(struct net *net, enum ovs_key_attr attr)
>>             attr == OVS_KEY_ATTR_CT_MARK)
>>                 return true;
>>         if (IS_ENABLED(CONFIG_NF_CONNTRACK_LABELS) &&
>> -           attr == OVS_KEY_ATTR_CT_LABEL) {
>> +           attr == OVS_KEY_ATTR_CT_LABELS) {
>>                 struct ovs_net *ovs_net = net_generic(net, ovs_net_id);
>>
>>                 return ovs_net->xt_label;
>> @@ -711,7 +711,7 @@ int ovs_ct_action_to_attr(const struct ovs_conntrack_info *ct_info,
>>                     &ct_info->mark))
>>                 return -EMSGSIZE;
>>         if (IS_ENABLED(CONFIG_NF_CONNTRACK_LABELS) &&
>> -           nla_put(skb, OVS_CT_ATTR_LABEL, sizeof(ct_info->label),
>> +           nla_put(skb, OVS_CT_ATTR_LABELS, sizeof(ct_info->label),
>>                     &ct_info->label))
>>                 return -EMSGSIZE;
>>         if (ct_info->helper) {
>> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
>> index 5c030a4..ea82cd5 100644
>> --- a/net/openvswitch/flow_netlink.c
>> +++ b/net/openvswitch/flow_netlink.c
>> @@ -294,7 +294,7 @@ size_t ovs_key_attr_size(void)
>>                 + nla_total_size(1)   /* OVS_KEY_ATTR_CT_STATE */
>>                 + nla_total_size(2)   /* OVS_KEY_ATTR_CT_ZONE */
>>                 + nla_total_size(4)   /* OVS_KEY_ATTR_CT_MARK */
>> -               + nla_total_size(16)  /* OVS_KEY_ATTR_CT_LABEL */
>> +               + nla_total_size(16)  /* OVS_KEY_ATTR_CT_LABELS */
>>                 + nla_total_size(12)  /* OVS_KEY_ATTR_ETHERNET */
>>                 + nla_total_size(2)   /* OVS_KEY_ATTR_ETHERTYPE */
>>                 + nla_total_size(4)   /* OVS_KEY_ATTR_VLAN */
>> @@ -352,7 +352,7 @@ static const struct ovs_len_tbl ovs_key_lens[OVS_KEY_ATTR_MAX + 1] = {
>>         [OVS_KEY_ATTR_CT_STATE]  = { .len = sizeof(u8) },
>>         [OVS_KEY_ATTR_CT_ZONE]   = { .len = sizeof(u16) },
>>         [OVS_KEY_ATTR_CT_MARK]   = { .len = sizeof(u32) },
>> -       [OVS_KEY_ATTR_CT_LABEL]  = { .len = sizeof(struct ovs_key_ct_label) },
>> +       [OVS_KEY_ATTR_CT_LABELS] = { .len = sizeof(struct ovs_key_ct_label) },
>>  };
> After this change, struct ovs_key_ct_label and other reference to
> label also should be renamed to labels. is there reason you did only
> this one?

Nope, fair point. I'll fix them.

Re: [PATCH net 5/7] openvswitch: Reject ct_state unsupported bits

[Pravin Shelar]

On Wed, Sep 30, 2015 at 6:20 PM, Joe Stringer <joestringer@nicira.com> wrote:
> On 30 September 2015 at 17:31, Pravin Shelar <pshelar@nicira.com> wrote:
>> On Tue, Sep 29, 2015 at 3:39 PM, Joe Stringer <joestringer@nicira.com> wrote:
>>> Previously, if userspace specified ct_state bits in the flow key which
>>> are currently undefined (and therefore unsupported), then they would be
>>> ignored. This could cause unexpected behaviour in future if userspace is
>>> extended to support additional bits but attempts to communicate with the
>>> current version of the kernel. This patch rectifies the situation by
>>> rejecting such ct_state bits.
>>>
>>> Fixes: 7f8a436 "openvswitch: Add conntrack action"
>>> Signed-off-by: Joe Stringer <joestringer@nicira.com>
>>> ---
>>>  net/openvswitch/conntrack.h    | 12 ++++++++++++
>>>  net/openvswitch/flow_netlink.c |  6 ++++++
>>>  2 files changed, 18 insertions(+)
>>>
>>> diff --git a/net/openvswitch/conntrack.h b/net/openvswitch/conntrack.h
>>> index 43f5dd7..c658d95 100644
>>> --- a/net/openvswitch/conntrack.h
>>> +++ b/net/openvswitch/conntrack.h
>>> @@ -34,6 +34,13 @@ int ovs_ct_execute(struct net *, struct sk_buff *, struct sw_flow_key *,
>>>  void ovs_ct_fill_key(const struct sk_buff *skb, struct sw_flow_key *key);
>>>  int ovs_ct_put_key(const struct sw_flow_key *key, struct sk_buff *skb);
>>>  void ovs_ct_free_action(const struct nlattr *a);
>>> +
>>> +static inline bool ovs_ct_state_supported(u8 state)
>>> +{
>>> +       return !(state & ~(OVS_CS_F_NEW | OVS_CS_F_ESTABLISHED |
>>> +                        OVS_CS_F_RELATED | OVS_CS_F_REPLY_DIR |
>>> +                        OVS_CS_F_INVALID | OVS_CS_F_TRACKED));
>>> +}
>>>  #else
>>>  #include <linux/errno.h>
>>>
>>> @@ -46,6 +53,11 @@ static inline bool ovs_ct_verify(struct net *net, int attr)
>>>         return false;
>>>  }
>>>
>>> +static inline bool ovs_ct_state_supported(u8 state)
>>> +{
>>> +       return false;
>>> +}
>>> +
>>>  static inline int ovs_ct_copy_action(struct net *net, const struct nlattr *nla,
>>>                                      const struct sw_flow_key *key,
>>>                                      struct sw_flow_actions **acts, bool log)
>>> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
>>> index ea82cd5..c4917c9 100644
>>> --- a/net/openvswitch/flow_netlink.c
>>> +++ b/net/openvswitch/flow_netlink.c
>>> @@ -816,6 +816,12 @@ static int metadata_from_nlattrs(struct net *net, struct sw_flow_match *match,
>>>             ovs_ct_verify(net, OVS_KEY_ATTR_CT_STATE)) {
>>>                 u8 ct_state = nla_get_u8(a[OVS_KEY_ATTR_CT_STATE]);
>>>
>> We also need to return error if kernel does not support the feature.
>
> Already handled by ovs_ct_verify() in conntrack.h.

Right. Patch looks good then.

Acked-by: Pravin B Shelar <pshelar@nicira.com>