From b953042214201e2693a485a8ba8b19f69e5bdf34 Mon Sep 17 00:00:00 2001 From: Andy Zhou Date: Wed, 4 Mar 2015 03:26:06 -0800 Subject: [PATCH] datapath: simplify sample action implementation The current sample() function implementation is more complicated than necessary in handling single user space action optimization and skb reference counting. There is no functional changes. Msg from Chris Dunlop: The commit isn't actually designed to address the problem directly, however in simplifying sample() it removes a call to skb_get(). The skb_get() makes the skb shared, which later causes us to hit the BUG(). E.g. your v2.3.1 stack trace shows this call path: netdev_frame_hook + netdev_port_receive | skb is guaranteed not-shared, via: | skb = skb_share_check(skb, GFP_ATOMIC); + ovs_vport_receive + ovs_dp_process_received_packet + ovs_dp_process_packet_with_key + ovs_execute_actions + do_execute_actions | nla_type(a) == OVS_ACTION_ATTR_SAMPLE + sample | skb is made shared here, via: | sample_skb = skb; | skb_get(skb); + do_execute_actions | nla_type(a) == OVS_ACTION_ATTR_USERSPACE + output_userspace + ovs_dp_upcall + queue_userspace_packet + skb_checksum_help + pskb_expand_head | if (skb_shared(skb)) | BUG(); BOOM!!! Reported-by: Chris Dunlop Reported-by: "Xu (Simon) Chen" Signed-off-by: Andy Zhou Acked-by: Pravin B Shelar --- datapath/actions.c | 39 +++++++++++++++++---------------------- 1 file changed, 17 insertions(+), 22 deletions(-) diff --git a/datapath/actions.c b/datapath/actions.c index a688f1983..6c6088e8e 100644 --- a/datapath/actions.c +++ b/datapath/actions.c @@ -449,7 +449,6 @@ static int sample(struct datapath *dp, struct sk_buff *skb, { 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; @@ -469,28 +468,24 @@ static int sample(struct datapath *dp, struct sk_buff *skb, 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); - } + /* Actions list is empty, do nothing */ + if (unlikely(!rem)) + return 0; + + /* The only known usage of sample action is having a single user-space + * action. Treat this usage as a special case. + * The output_userspace() should clone the skb to be sent to the + * user space. This skb will be consumed by its caller. */ + if (likely(nla_type(a) == OVS_ACTION_ATTR_USERSPACE && + last_action(a, rem))) + return output_userspace(dp, skb, a); + + skb = skb_clone(skb, GFP_ATOMIC); + if (!skb) + /* Skip the sample action when out of memory. */ + return 0; - /* 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); + return do_execute_actions(dp, skb, a, rem); } static void execute_hash(struct sk_buff *skb, const struct nlattr *attr) -- 2.20.1