diff --git a/datapath/actions.c b/datapath/actions.c index 3c1615b20d3..603c7cb5f5f 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,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))