On Sat, May 10, 2014 at 7:46 PM, Simon Horman <ho...@verge.net.au> wrote: > diff --git a/datapath/actions.c b/datapath/actions.c > index ffff56d..4dc7c3e 100644 > --- a/datapath/actions.c > +++ b/datapath/actions.c > @@ -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. */
I'm generally pretty happy with the patch (and in particular the fact that we can remove keep_skb). However, I think that we have a similar issue here that I mentioned in the previous patch with a double free on the packet in the optimized case. Here, I think that we need something like skb_get() in the userspace action case. I believe that it's actually possible to always use skb_get() and then use skb_share_check() whenever we need to make a change to the skb but I'm not sure that it's really worth the extra mess (this is not useful for recirculation because we immediately write into the skb metadata). Most actions don't free the skb on error and allow a generic handler to do this in do_execute_action(). sample() has a special case but we might be able to eliminate that and the error check in the sample action at the same time. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
