[PATCH net v2 1/2] openvswitch: Set mark and labels before confirming.

[PATCH net v2 1/2] openvswitch: Set mark and labels before confirming.

[Jarno Rajahalme]

Set conntrack mark and labels right before committing so that
the initial conntrack NEW event has the mark and labels.

Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
---
v2: Separate Kernel API change to an RFC patch (2/2).

 net/openvswitch/conntrack.c | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 3d5feed..23fd4fb 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -824,23 +824,6 @@ static int ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
 	return 0;
 }
 
-/* Lookup connection and confirm if unconfirmed. */
-static int ovs_ct_commit(struct net *net, struct sw_flow_key *key,
-			 const struct ovs_conntrack_info *info,
-			 struct sk_buff *skb)
-{
-	int err;
-
-	err = __ovs_ct_lookup(net, key, info, skb);
-	if (err)
-		return err;
-	/* This is a no-op if the connection has already been confirmed. */
-	if (nf_conntrack_confirm(skb) != NF_ACCEPT)
-		return -EINVAL;
-
-	return 0;
-}
-
 static bool labels_nonzero(const struct ovs_key_ct_labels *labels)
 {
 	size_t i;
@@ -873,21 +856,33 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
 	}
 
 	if (info->commit)
-		err = ovs_ct_commit(net, key, info, skb);
+		err = __ovs_ct_lookup(net, key, info, skb);
 	else
 		err = ovs_ct_lookup(net, key, info, skb);
 	if (err)
 		goto err;
 
+	/* Apply changes before confirming the connection so that the initial
+	 * conntrack NEW netlink event carries the values given in the CT
+	 * action.
+	 */
 	if (info->mark.mask) {
 		err = ovs_ct_set_mark(skb, key, info->mark.value,
 				      info->mark.mask);
 		if (err)
 			goto err;
 	}
-	if (labels_nonzero(&info->labels.mask))
+	if (labels_nonzero(&info->labels.mask)) {
 		err = ovs_ct_set_labels(skb, key, &info->labels.value,
 					&info->labels.mask);
+		if (err)
+			goto err;
+	}
+	/* This will take care of sending queued events even if the connection
+	 * is already confirmed.
+	 */
+	if (info->commit && nf_conntrack_confirm(skb) != NF_ACCEPT)
+		err = -EINVAL;
 err:
 	skb_push(skb, nh_ofs);
 	if (err)
-- 
2.1.4

_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[RFC PATCH net v2 2/2] openvswitch: Only set mark and labels with a commit flag.

[Jarno Rajahalme]

Only set conntrack mark or labels when the commit flag is specified.
This makes sure we can not set them before the connection has been
persisted, as in that case the mark and labels would be lost in an
event of an userspace upcall.

OVS userspace already requires the commit flag to accept setting
ct_mark and/or ct_labels.  Validate for this in the kernel API.

Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
---
 net/openvswitch/conntrack.c | 76 ++++++++++++++++++++++++++++++---------------
 1 file changed, 51 insertions(+), 25 deletions(-)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 23fd4fb..52f3b9b 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -835,6 +835,42 @@ static bool labels_nonzero(const struct ovs_key_ct_labels *labels)
 	return false;
 }
 
+/* Lookup connection and confirm if unconfirmed. */
+static int ovs_ct_commit(struct net *net, struct sw_flow_key *key,
+			 const struct ovs_conntrack_info *info,
+			 struct sk_buff *skb)
+{
+	int err;
+
+	err = __ovs_ct_lookup(net, key, info, skb);
+	if (err)
+		return err;
+
+	/* Apply changes before confirming the connection so that the initial
+	 * conntrack NEW netlink event carries the values given in the CT
+	 * action.
+	 */
+	if (info->mark.mask) {
+		err = ovs_ct_set_mark(skb, key, info->mark.value,
+				      info->mark.mask);
+		if (err)
+			return err;
+	}
+	if (labels_nonzero(&info->labels.mask)) {
+		err = ovs_ct_set_labels(skb, key, &info->labels.value,
+					&info->labels.mask);
+		if (err)
+			return err;
+	}
+	/* This will take care of sending queued events even if the connection
+	 * is already confirmed.
+	 */
+	if (nf_conntrack_confirm(skb) != NF_ACCEPT)
+		return -EINVAL;
+
+	return 0;
+}
+
 /* Returns 0 on success, -EINPROGRESS if 'skb' is stolen, or other nonzero
  * value if 'skb' is freed.
  */
@@ -856,34 +892,10 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
 	}
 
 	if (info->commit)
-		err = __ovs_ct_lookup(net, key, info, skb);
+		err = ovs_ct_commit(net, key, info, skb);
 	else
 		err = ovs_ct_lookup(net, key, info, skb);
-	if (err)
-		goto err;
 
-	/* Apply changes before confirming the connection so that the initial
-	 * conntrack NEW netlink event carries the values given in the CT
-	 * action.
-	 */
-	if (info->mark.mask) {
-		err = ovs_ct_set_mark(skb, key, info->mark.value,
-				      info->mark.mask);
-		if (err)
-			goto err;
-	}
-	if (labels_nonzero(&info->labels.mask)) {
-		err = ovs_ct_set_labels(skb, key, &info->labels.value,
-					&info->labels.mask);
-		if (err)
-			goto err;
-	}
-	/* This will take care of sending queued events even if the connection
-	 * is already confirmed.
-	 */
-	if (info->commit && nf_conntrack_confirm(skb) != NF_ACCEPT)
-		err = -EINVAL;
-err:
 	skb_push(skb, nh_ofs);
 	if (err)
 		kfree_skb(skb);
@@ -1140,6 +1152,20 @@ static int parse_ct(const struct nlattr *attr, struct ovs_conntrack_info *info,
 		}
 	}
 
+#ifdef CONFIG_NF_CONNTRACK_MARK
+	if (!info->commit && info->mark.mask) {
+		OVS_NLERR(log,
+			  "Setting conntrack mark requires 'commit' flag.");
+		return -EINVAL;
+	}
+#endif
+#ifdef CONFIG_NF_CONNTRACK_LABELS
+	if (!info->commit && labels_nonzero(&info->labels.mask)) {
+		OVS_NLERR(log,
+			  "Setting conntrack labels requires 'commit' flag.");
+		return -EINVAL;
+	}
+#endif
 	if (rem > 0) {
 		OVS_NLERR(log, "Conntrack attr has %d unknown bytes", rem);
 		return -EINVAL;
-- 
2.1.4

_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [PATCH net v2 1/2] openvswitch: Set mark and labels before confirming.

[Joe Stringer]

On 21 June 2016 at 14:59, Jarno Rajahalme <jarno@ovn.org> wrote:
> Set conntrack mark and labels right before committing so that
> the initial conntrack NEW event has the mark and labels.
>
> Signed-off-by: Jarno Rajahalme <jarno@ovn.org>

Acked-by: Joe Stringer <joe@ovn.org>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [RFC PATCH net v2 2/2] openvswitch: Only set mark and labels with a commit flag.

[Joe Stringer]

On 21 June 2016 at 14:59, Jarno Rajahalme <jarno@ovn.org> wrote:
> Only set conntrack mark or labels when the commit flag is specified.
> This makes sure we can not set them before the connection has been
> persisted, as in that case the mark and labels would be lost in an
> event of an userspace upcall.
>
> OVS userspace already requires the commit flag to accept setting
> ct_mark and/or ct_labels.  Validate for this in the kernel API.
>
> Signed-off-by: Jarno Rajahalme <jarno@ovn.org>

As this is walling off an inconsistent corner of the ct action, and
OVS userspace already enforces this constraint, this looks OK to me.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev