diff mbox

[ovs-dev,RFC,4/5] openvswitch: conntrack netlink API updates.

Message ID 1445379629-112880-4-git-send-email-jrajahalme@nicira.com
State Not Applicable
Headers show

Commit Message

Jarno Rajahalme Oct. 20, 2015, 10:20 p.m. UTC 
This patch makes changes already done in the net repo, so that the
rest of the code compiles on top of the current net-next.  Eventually,
this patch will not be needed and should be removed before merging.

Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com>
---
 include/uapi/linux/openvswitch.h | 18 +++++-------------
 net/openvswitch/conntrack.c      | 15 +++++++++------
 net/openvswitch/flow_netlink.c   |  6 +++---
 3 files changed, 17 insertions(+), 22 deletions(-)

Comments

Pablo Neira Ayuso Oct. 21, 2015, 10:41 a.m. UTC | #1 
On Tue, Oct 20, 2015 at 03:20:28PM -0700, Jarno Rajahalme wrote:
> diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> index 77850f1..1e6c2ac 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_LABEL */

This is very strange.

You're changing the size of the netlink attribute, this will break
existing userspace applications.
Thomas Graf Oct. 21, 2015, 11:18 a.m. UTC | #2 
On 10/21/15 at 12:41pm, Pablo Neira Ayuso wrote:
> On Tue, Oct 20, 2015 at 03:20:28PM -0700, Jarno Rajahalme wrote:
> > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
> > index 77850f1..1e6c2ac 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_LABEL */
> 
> This is very strange.
> 
> You're changing the size of the netlink attribute, this will break
> existing userspace applications.

This seems to be a temporary forward port of the following commit. The
change is backwards compatible.

commit fbccce5965a58d56aaed9e9acd1bec75d8a66e87
Author: Joe Stringer <joestringer@nicira.com>
Date:   Tue Oct 6 11:00:00 2015 -0700

    openvswitch: Extend ct_state match field to 32 bits

    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>
    Acked-by: Pravin B Shelar <pshelar@nicira.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>
diff mbox

Patch

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index 4036e1b..098d8b5 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_LABEL,	/* 16-octet connection tracking label */
@@ -451,9 +451,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.
@@ -632,7 +632,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_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_LABEL,      /* label to associate with this connection. */
@@ -643,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 097ace4..6997107 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -42,6 +42,8 @@  struct md_label {
 	struct ovs_key_ct_label mask;
 };
 
+#define OVS_CT_F_COMMIT		0x01
+
 /* Conntrack action context for execution. */
 struct ovs_conntrack_info {
 	struct nf_conntrack_helper *helper;
@@ -165,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) &&
@@ -537,8 +539,8 @@  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),
@@ -574,8 +576,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 |= OVS_CT_F_COMMIT;
 			break;
 #ifdef CONFIG_NF_CONNTRACK_ZONES
 		case OVS_CT_ATTR_ZONE:
@@ -699,7 +701,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 & OVS_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))
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index 77850f1..1e6c2ac 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_LABEL */
@@ -351,7 +351,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_LABEL]	 = { .len = sizeof(struct ovs_key_ct_label) },
@@ -864,7 +864,7 @@  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]);
 
 		SW_FLOW_KEY_PUT(match, ct.state, ct_state, is_mask);
 		*attrs &= ~(1ULL << OVS_KEY_ATTR_CT_STATE);