[ovs-dev] [PATCH v2 2/2] datapath: sample action without side effects

[Simon Horman]

The sample action is rather generic, allowing arbitrary actions to be
executed based on a probability. However its use, within the Open vSwitch
code-base is limited: only a single user-space action is ever nested.

A consequence of the current implementation of sample actions is that
depending on weather the sample action executed (due to its probability)
any side-effects of nested actions may or may not be present before
executing subsequent actions.  This has the potential to complicate
verification of valid actions by the (kernel) datapath. And indeed adding
support for push and pop MPLS actions inside sample actions is one case
where such case.

In order to allow all supported actions to be continue to be nested inside
sample actions without the potential need for complex verification code
this patch changes the implementation of the sample action in the kernel
datapath so that sample actions are more like a function call and any side
effects of nested actions are not present when executing subsequent
actions.

With the above in mind the motivation for this change is twofold:

* To contain side-effects the sample action in the hope of making it
  easier to deal with in the future and;
* To avoid some rather complex verification code introduced in the MPLS
  datapath patch.

Some notes about the implementation:

* This patch silently changes the behaviour of sample actions whose nested
  actions have side-effects. There are no known users of such sample
  actions.

* sample() does not clone the skb for the only known use-case of the sample
  action: a single nested userspace action. In such a case a clone is not
  needed as the userspace action has no side effects.

  Given that there are no known users of other nested actions and in order
  to avoid the complexity of predicting if other sequences of actions have
  side-effects in such cases the skb is cloned.

* As sample() provides a cloned skb in the unlikely case where there are
  nested actions other than a single userspace action it is no longer
  necessary to clone the skb in do_execute_actions() when executing a
  recirculation action just because the keep_skb parameter is set: this
  parameter was only set when processing the nested actions of a sample
  action.  Moreover it is possible to remove the keep_skb parameter of
  do_execute_actions entirely.

---
v2
* As suggested by Jesse Gross
  - Do not add attributes to noisily break compatibility with user-space
  - (Try to) streamline cloning of skbs by sample and recirculate actions
---
 datapath/actions.c | 48 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 34 insertions(+), 14 deletions(-)

diff --git a/datapath/actions.c b/datapath/actions.c
index ffff56d..4dc7c3e 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -39,7 +39,7 @@
 #include "vport.h"
 
 static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
-			      const struct nlattr *attr, int len, bool keep_skb);
+			      const struct nlattr *attr, int len);
 
 static int make_writable(struct sk_buff *skb, int write_len)
 {
@@ -435,12 +435,18 @@ static int output_userspace(struct datapath *dp, struct sk_buff *skb,
 	return ovs_dp_upcall(dp, skb, &upcall);
 }
 
+static bool last_action(const struct nlattr *a, int rem)
+{
+	return a->nla_len == rem;
+}
+
 static int sample(struct datapath *dp, struct sk_buff *skb,
 		  const struct nlattr *attr)
 {
 	const struct nlattr *acts_list = NULL;
 	const struct nlattr *a;
-	int rem;
+	struct sk_buff *sample_skb;
+	int rem, err;
 
 	for (a = nla_data(attr), rem = nla_len(attr); rem > 0;
 		 a = nla_next(a, &rem)) {
@@ -456,8 +462,27 @@ static int sample(struct datapath *dp, struct sk_buff *skb,
 		}
 	}
 
-	return do_execute_actions(dp, skb, nla_data(acts_list),
-				  nla_len(acts_list), true);
+	rem = nla_len(acts_list);
+	a = nla_data(acts_list);
+
+	/* Actions list is either empty or only contains a single user-space
+	 * action, the latter being a special case as it is the only known
+	 * usage of the sample action.
+	 * In these special cases don't clone the skb as there are no
+	 * side-effects in the nested actions.
+	 * Otherwise, clone in case the nested actions have side effects. */
+	if (likely(rem == 0 ||
+		   (nla_type(a) == OVS_ACTION_ATTR_USERSPACE &&
+		    last_action(a, rem))))
+		sample_skb = skb;
+	else
+		sample_skb = skb_clone(skb, GFP_ATOMIC);
+
+	err =  do_execute_actions(dp, sample_skb, a, rem);
+	if (unlikely(err && sample_skb != skb))
+		kfree_skb(skb); /* sample_skb but not skb already freed. */
+	return err;
+
 }
 
 static void execute_hash(struct sk_buff *skb, const struct nlattr *attr)
@@ -543,7 +568,7 @@ static int execute_recirc(struct datapath *dp, struct sk_buff *skb,
 
 /* Execute a list of actions against 'skb'. */
 static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
-			const struct nlattr *attr, int len, bool keep_skb)
+			const struct nlattr *attr, int len)
 {
 	/* Every output action needs a separate clone of 'skb', but the common
 	 * case is just a single output action, so that doing a clone and
@@ -587,9 +612,8 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
 
 		case OVS_ACTION_ATTR_RECIRC: {
 			struct sk_buff *recirc_skb;
-			const bool last_action = (a->nla_len == rem);
 
-			if (!last_action || keep_skb)
+			if (!last_action(a, rem))
 				recirc_skb = skb_clone(skb, GFP_ATOMIC);
 			else
 				recirc_skb = skb;
@@ -619,12 +643,9 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
 		}
 	}
 
-	if (prev_port != -1) {
-		if (keep_skb)
-			skb = skb_clone(skb, GFP_ATOMIC);
-
+	if (prev_port != -1)
 		do_output(dp, skb, prev_port);
-	} else if (!keep_skb)
+	else
 		consume_skb(skb);
 
 	return 0;
@@ -677,8 +698,7 @@ int ovs_execute_actions(struct datapath *dp, struct sk_buff *skb, bool recirc)
 	}
 
 	OVS_CB(skb)->tun_key = NULL;
-	error = do_execute_actions(dp, skb, acts->actions,
-					 acts->actions_len, false);
+	error = do_execute_actions(dp, skb, acts->actions, acts->actions_len);
 
 	/* Check whether sub-actions looped too much. */
 	if (unlikely(loop->looping))
-- 
1.8.4