datapath: sample action without side effects

[cascardo/ovs.git] / datapath / actions.c

diff --git a/datapath/actions.c b/datapath/actions.c
index 3c1615b..603c7cb 100644 (file)
--- 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,11 +435,17 @@ 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;
+       struct sk_buff *sample_skb;
        int rem;
 
        for (a = nla_data(attr), rem = nla_len(attr); rem > 0;
@@ -456,8 +462,31 @@ 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;
+               skb_get(skb);
+       } else {
+               sample_skb = skb_clone(skb, GFP_ATOMIC);
+       }
+
+       /* Note that do_execute_actions() never consumes skb.
+        * In the case where skb has been cloned above it is the clone that
+        * is consumed.  Otherwise the skb_get(skb) call prevents
+        * consumption by do_execute_actions(). Thus, it is safe to simply
+        * return the error code and let the caller (also
+        * do_execute_actions()) free skb on error. */
+       return do_execute_actions(dp, sample_skb, a, rem);
 }
 
 static void execute_hash(struct sk_buff *skb, const struct nlattr *attr)
@@ -545,7 +574,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
@@ -589,9 +618,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;
@@ -610,8 +638,6 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
 
                case OVS_ACTION_ATTR_SAMPLE:
                        err = sample(dp, skb, a);
-                       if (unlikely(err)) /* skb already freed. */
-                               return err;
                        break;
                }
 
@@ -621,12 +647,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;
@@ -679,8 +702,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))