[PATCH net-next] openvswitch: Only set mark and labels when commiting a connection.

[PATCH net-next] openvswitch: Only set mark and labels when commiting a connection.

[Jarno Rajahalme]

Only allow setting 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 on the kernel API.

Finally, 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>
---
 net/openvswitch/conntrack.c | 72 ++++++++++++++++++++++++++++++---------------
 1 file changed, 48 insertions(+), 24 deletions(-)

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index 3d5feed..f1612f5 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -824,6 +824,17 @@ static int ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
 	return 0;
 }
 
+static bool labels_nonzero(const struct ovs_key_ct_labels *labels)
+{
+	size_t i;
+
+	for (i = 0; i < sizeof(*labels); i++)
+		if (labels->ct_labels[i])
+			return true;
+
+	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,
@@ -834,24 +845,35 @@ static int ovs_ct_commit(struct net *net, struct sw_flow_key *key,
 	err = __ovs_ct_lookup(net, key, info, skb);
 	if (err)
 		return err;
-	/* This is a no-op if the connection has already been confirmed. */
+
+	/* As any changes to an unconfirmed connection may be lost due
+	 * to an upcall, we require the presence of the 'commit' flag
+	 * for setting mask and/or labels.  Perform the changes before
+	 * confirming the connection so that the initial mark and label
+	 * values are present in the initial CT NEW netlink event.
+	 */
+	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;
+	}
+
+	/* 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;
 }
 
-static bool labels_nonzero(const struct ovs_key_ct_labels *labels)
-{
-	size_t i;
-
-	for (i = 0; i < sizeof(*labels); i++)
-		if (labels->ct_labels[i])
-			return true;
-
-	return false;
-}
-
 /* Returns 0 on success, -EINPROGRESS if 'skb' is stolen, or other nonzero
  * value if 'skb' is freed.
  */
@@ -876,19 +898,7 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
 		err = ovs_ct_commit(net, key, info, skb);
 	else
 		err = ovs_ct_lookup(net, key, info, skb);
-	if (err)
-		goto err;
 
-	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);
-err:
 	skb_push(skb, nh_ofs);
 	if (err)
 		kfree_skb(skb);
@@ -1145,6 +1155,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-next] openvswitch: Only set mark and labels when commiting a connection.

[Jarno Rajahalme]

The title should have been:

openvswitch: Only set mark and labels with a commit flag.

This reflects the fact that modifying the mark and/or labels of an existing connection is allowed. The commit flag is still required to do that, though.

  Jarno

> On Jun 20, 2016, at 5:19 PM, Jarno Rajahalme <jarno@ovn.org> wrote:
> 
> Only allow setting 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 on the kernel API.
> 
> Finally, 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>
> ---
> net/openvswitch/conntrack.c | 72 ++++++++++++++++++++++++++++++---------------
> 1 file changed, 48 insertions(+), 24 deletions(-)
> 
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index 3d5feed..f1612f5 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -824,6 +824,17 @@ static int ovs_ct_lookup(struct net *net, struct sw_flow_key *key,
> 	return 0;
> }
> 
> +static bool labels_nonzero(const struct ovs_key_ct_labels *labels)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < sizeof(*labels); i++)
> +		if (labels->ct_labels[i])
> +			return true;
> +
> +	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,
> @@ -834,24 +845,35 @@ static int ovs_ct_commit(struct net *net, struct sw_flow_key *key,
> 	err = __ovs_ct_lookup(net, key, info, skb);
> 	if (err)
> 		return err;
> -	/* This is a no-op if the connection has already been confirmed. */
> +
> +	/* As any changes to an unconfirmed connection may be lost due
> +	 * to an upcall, we require the presence of the 'commit' flag
> +	 * for setting mask and/or labels.  Perform the changes before
> +	 * confirming the connection so that the initial mark and label
> +	 * values are present in the initial CT NEW netlink event.
> +	 */
> +	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;
> +	}
> +
> +	/* 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;
> }
> 
> -static bool labels_nonzero(const struct ovs_key_ct_labels *labels)
> -{
> -	size_t i;
> -
> -	for (i = 0; i < sizeof(*labels); i++)
> -		if (labels->ct_labels[i])
> -			return true;
> -
> -	return false;
> -}
> -
> /* Returns 0 on success, -EINPROGRESS if 'skb' is stolen, or other nonzero
>  * value if 'skb' is freed.
>  */
> @@ -876,19 +898,7 @@ int ovs_ct_execute(struct net *net, struct sk_buff *skb,
> 		err = ovs_ct_commit(net, key, info, skb);
> 	else
> 		err = ovs_ct_lookup(net, key, info, skb);
> -	if (err)
> -		goto err;
> 
> -	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);
> -err:
> 	skb_push(skb, nh_ofs);
> 	if (err)
> 		kfree_skb(skb);
> @@ -1145,6 +1155,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-next] openvswitch: Only set mark and labels when commiting a connection.

[Joe Stringer]

On 20 June 2016 at 17:19, Jarno Rajahalme <jarno@ovn.org> wrote:
> Only allow setting 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 on the kernel API.
>
> Finally, 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>

The structure of this commit message suggests there are multiple
changes trying to be addressed in one patch. I suggest splitting them
out.

In terms of applying the mark and labels before committing the
connection, that's actually the behaviour I would expect if you were
to execute ct(mark=foo,commit). The NEW event should include these
pieces, and should have all along.

> @@ -1145,6 +1155,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

I'm of mixed minds about this, but I lean towards agreeing with it. On
one hand, it's applying more restrictions on an otherwise fairly loose
interface and if anyone is relying on this behaviour then it would be
surprising to have this restriction introduced. On the other hand, it
doesn't make a lot of sense to set a label/mark but not to commit the
connection. As you say, the behaviour isn't exactly consistent in that
case today anyway: If there was a flow with
actions=ct(mark=foo),recirc() followed by a userspace upcall, then the
mark would be reflected in the flow key but not saved to any persisted
connection. A subsequent ct(commit) after upcall wouldn't persist it,
either. However if there were two flows already in the datapath to do
this, then it /would/ be persisted. Restricting the mark/labels
modification to only if you have the "commit" flag would address that
consistency issue. The OVS userspace enforcing this constraint also
hints that this was an unintentional omission from kernel validation.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [PATCH net-next] openvswitch: Only set mark and labels when commiting a connection.

[Jarno Rajahalme]

Thanks for the review!

> On Jun 21, 2016, at 1:57 PM, Joe Stringer <joe@ovn.org> wrote:
> 
> On 20 June 2016 at 17:19, Jarno Rajahalme <jarno@ovn.org> wrote:
>> Only allow setting 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 on the kernel API.
>> 
>> Finally, 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>
> 
> The structure of this commit message suggests there are multiple
> changes trying to be addressed in one patch. I suggest splitting them
> out.
> 

Done for v2 I just sent for net.

> In terms of applying the mark and labels before committing the
> connection, that's actually the behaviour I would expect if you were
> to execute ct(mark=foo,commit). The NEW event should include these
> pieces, and should have all along.

Right, the v2 patch 1/2 does this.

> 
>> @@ -1145,6 +1155,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
> 
> I'm of mixed minds about this, but I lean towards agreeing with it. On
> one hand, it's applying more restrictions on an otherwise fairly loose
> interface and if anyone is relying on this behaviour then it would be
> surprising to have this restriction introduced. On the other hand, it
> doesn't make a lot of sense to set a label/mark but not to commit the
> connection. As you say, the behaviour isn't exactly consistent in that
> case today anyway: If there was a flow with
> actions=ct(mark=foo),recirc() followed by a userspace upcall, then the
> mark would be reflected in the flow key but not saved to any persisted
> connection. A subsequent ct(commit) after upcall wouldn't persist it,
> either. However if there were two flows already in the datapath to do
> this, then it /would/ be persisted. Restricting the mark/labels
> modification to only if you have the "commit" flag would address that
> consistency issue. The OVS userspace enforcing this constraint also
> hints that this was an unintentional omission from kernel validation.

I separated this out to the v2 patch 2/2.

  Jarno

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