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 <chris@onthe.net.au>
Reported-by: "Xu (Simon) Chen" <xchenum@gmail.com>
Signed-off-by: Andy Zhou <azhou@nicira.com>
Acked-by: Pravin B Shelar <pshelar@nicira.com>
{
const struct nlattr *acts_list = NULL;
const struct nlattr *a;
{
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;
int rem;
for (a = nla_data(attr), rem = nla_len(attr); rem > 0;
rem = nla_len(acts_list);
a = nla_data(acts_list);
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)
}
static void execute_hash(struct sk_buff *skb, const struct nlattr *attr)