Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1237734 > unrolled thread

[PATCHv2 0/7] OVS conntrack fixes for net

Back to article view | Back to linux.kernel

Contents

  [PATCHv2 0/7] OVS conntrack fixes for net Joe Stringer <joestringer@nicira.com> - 2015-10-01 23:00 +0200
    [PATCHv2 7/7] openvswitch: Change CT_ATTR_FLAGS to CT_ATTR_COMMIT Joe Stringer <joestringer@nicira.com> - 2015-10-01 23:00 +0200
      Re: [PATCHv2 7/7] openvswitch: Change CT_ATTR_FLAGS to CT_ATTR_COMMIT Pravin Shelar <pshelar@nicira.com> - 2015-10-02 00:30 +0200
    [PATCHv2 4/7] openvswitch: Ensure flow is valid before executing ct Joe Stringer <joestringer@nicira.com> - 2015-10-01 23:00 +0200

#1237734 — [PATCHv2 0/7] OVS conntrack fixes for net

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.

---
v2: Address minor style feedback, add acks.
v1: Initial post.

Joe Stringer (7):
  openvswitch: Rename LABEL->LABELS
  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 | 36 +++++++---------
 net/openvswitch/actions.c        | 19 ++++++---
 net/openvswitch/conntrack.c      | 89 ++++++++++++++++++++--------------------
 net/openvswitch/conntrack.h      | 12 ++++++
 net/openvswitch/flow.h           |  2 +-
 net/openvswitch/flow_netlink.c   | 30 ++++++++------
 6 files changed, 104 insertions(+), 84 deletions(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1237735 — [PATCHv2 7/7] openvswitch: Change CT_ATTR_FLAGS to CT_ATTR_COMMIT

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>
---
v2: Use bitmask for internal representation of COMMIT.
---
 include/uapi/linux/openvswitch.h | 14 ++++----------
 net/openvswitch/conntrack.c      | 13 ++++++-------
 2 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index c861a4cf..036f73b 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 466d557..80bf702 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -47,7 +47,7 @@ struct ovs_conntrack_info {
 	struct nf_conntrack_helper *helper;
 	struct nf_conntrack_zone zone;
 	struct nf_conn *ct;
-	u32 flags;
+	u8 commit : 1;
 	u16 family;
 	struct md_mark mark;
 	struct md_labels labels;
@@ -493,7 +493,7 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
 			return err;
 	}
 
-	if (info->flags & OVS_CT_F_COMMIT)
+	if (info->commit)
 		err = ovs_ct_commit(net, key, info, skb);
 	else
 		err = ovs_ct_lookup(net, key, info, skb);
@@ -539,8 +539,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 +575,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->commit = true;
 			break;
 #ifdef CONFIG_NF_CONNTRACK_ZONES
 		case OVS_CT_ATTR_ZONE:
@@ -701,7 +700,7 @@ 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->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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237793 — Re: [PATCHv2 7/7] openvswitch: Change CT_ATTR_FLAGS to CT_ATTR_COMMIT

On Thu, Oct 1, 2015 at 1:53 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>
> ---
> v2: Use bitmask for internal representation of COMMIT.
> ---
Acked-by: Pravin B Shelar <pshelar@nicira.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1237736 — [PATCHv2 4/7] openvswitch: Ensure flow is valid before executing ct

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>
---
v2: Acked.
---
 net/openvswitch/actions.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 4cb93f9..c6a39bf 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -1102,6 +1102,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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web