Hi Andy,
Is this the patch for the bug I found?
Thanks a lot!
[ovs-dev] [PATCH] ofproto/bond: Fix a race condition in updating post
recirculation rules
Andy Zhou Tue, 17 Feb 2015 13:27:17 -0800
When updating post recirc rules, rule management requires calls to
hmap APIs, which requires proper locking to ensure mutual exclsion in
accessing the hmap internal data structure. The locking currently is
missing from the output_normal() xlate path, thus causing
a race condition.
The race condition leads to segfault crash of ovs-vswitchd, with the
following stack trace:
The crash was found by adding and deleting bond interfaces repeatedly
with on-going traffic hitting the bond interfaces. The same test was
ran over multiple days with this patch to ensure the same crash was
not seen.
The patch added the necessary lock annotation that would have caught
the bug.
Reported-by: Salvator Cambria <salvatore.camb...@citrix.com>
Signed-off-by: Andy Zhou <az...@nicira.com>
---
ofproto/bond.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/ofproto/bond.c b/ofproto/bond.c
index c4b3a3a..89979a0 100644
--- a/ofproto/bond.c
+++ b/ofproto/bond.c
@@ -320,6 +320,7 @@ add_pr_rule(struct bond *bond, const struct match *match,
static void
update_recirc_rules(struct bond *bond)
+ OVS_REQ_WRLOCK(rwlock)
{
struct match match;
struct bond_pr_rule_op *pr_op, *next_op;
@@ -923,8 +924,9 @@ bond_may_recirc(const struct bond *bond, uint32_t
*recirc_id,
}
}
-void
-bond_update_post_recirc_rules(struct bond* bond, const bool force)
+static void
+bond_update_post_recirc_rules__(struct bond* bond, const bool force)
+ OVS_REQ_WRLOCK(rwlock)
{
struct bond_entry *e;
bool update_rules = force; /* Always update rules if caller forces it. */
@@ -945,6 +947,14 @@ bond_update_post_recirc_rules(struct bond* bond, const
bool force)
update_recirc_rules(bond);
}
}
+
+void
+bond_update_post_recirc_rules(struct bond* bond, const bool force)
+{
+ ovs_rwlock_wrlock(&rwlock);
+ bond_update_post_recirc_rules__(bond, force);
+ ovs_rwlock_unlock(&rwlock);
+}
/* Rebalancing. */
@@ -1203,7 +1213,7 @@ bond_rebalance(struct bond *bond)
}
if (use_recirc && rebalanced) {
- bond_update_post_recirc_rules(bond,true);
+ bond_update_post_recirc_rules__(bond,true);
}
done:
--
1.9.1
ZHANG Zhiming
Yunshan Networks
From: Andy Zhou
Date: 2015-05-23 04:41
To: Alex Wang
CC: zhangzhiming; discuss
Subject: Re: [ovs-discuss] one patch was omitted to be pushed to
branch-2.3---datapath: Fix recirc bug where skb is double freed
Hi, Jeremy,
Sorry for the delay. I don't think this patch is required for branch
2.3. As you may have noticed, this part of code
is different on branch 2.3. And it seems to work on my test.
Do you have the core dump from kernel crash? If yes, would you please
post the back trace?
Thanks,
Andy
On Sun, May 17, 2015 at 9:41 AM, Alex Wang <al...@nicira.com> wrote:
> Fwd to Andy,~
>
> On Sun, May 17, 2015 at 4:29 AM, zhangzhiming <zhangzhim...@yunshan.net.cn>
> wrote:
>>
>> Hi,
>>
>> I found one patch was omitted to be pushed to branch-2.3, which leads to
>> double freed skb.
>> Could someone to confirm the patch and submit it to branch-2.3?
>> Thanks!
>>
>> Here is the patch information:
>>
>>
>> commit 867e37ba00091b3e319c4c47c1598f1ae84dd32e
>> Author: Andy Zhou <az...@nicira.com>
>> Date: Mon Aug 25 15:18:19 2014 -0700
>>
>> datapath: Fix recirc bug where skb is double freed.
>>
>> If recirc action is the last action of a action list, the SKB triggers
>> the recirc will be freed twice. This patch fixes this bug.
>>
>> Reported-by: Justin Pettit <jpet...@nicira.com>
>> Signed-off-by: Andy Zhou <az...@nicira.com>
>>
>> diff --git a/datapath/actions.c b/datapath/actions.c
>> index ad22467..7f25553 100644
>> --- a/datapath/actions.c
>> +++ b/datapath/actions.c
>> @@ -809,7 +809,16 @@ static int execute_recirc(struct datapath *dp, struct
>> sk_buff *skb,
>> const struct nlattr *a, int rem)
>> {
>> struct sw_flow_key recirc_key;
>> - int err;
>> +
>> + if (!is_skb_flow_key_valid(skb)) {
>> + int err;
>> +
>> + err = ovs_flow_key_update(skb, OVS_CB(skb)->pkt_key);
>> + if (err)
>> + return err;
>> +
>> + }
>> + BUG_ON(!is_skb_flow_key_valid(skb));
>>
>> if (!last_action(a, rem)) {
>> /* Recirc action is the not the last action
>> @@ -820,19 +829,9 @@ static int execute_recirc(struct datapath *dp, struct
>> sk_buff *skb,
>> * continue on with the rest of the action list. */
>> if (!skb)
>> return 0;
>> - }
>>
>> - if (!is_skb_flow_key_valid(skb)) {
>> - err = ovs_flow_key_update(skb, OVS_CB(skb)->pkt_key);
>> - if (err) {
>> - kfree_skb(skb);
>> - return err;
>> - }
>> - }
>> - BUG_ON(!is_skb_flow_key_valid(skb));
>> -
>> - if (!last_action(a, rem))
>> flow_key_clone(skb, &recirc_key);
>> + }
>>
>> flow_key_set_recirc_id(skb, nla_get_u32(a));
>> ovs_dp_process_packet(skb, true);
>> @@ -897,6 +896,12 @@ static int do_execute_actions(struct datapath *dp,
>> struct sk_buff *skb,
>>
>> case OVS_ACTION_ATTR_RECIRC:
>> err = execute_recirc(dp, skb, a, rem);
>> + if (last_action(a, rem)) {
>> + /* If this is the last action, the skb has
>> + * been consumed or freed.
>> + * Return immediately. */
>> + return err;
>> + }
>> break;
>>
>> case OVS_ACTION_ATTR_SET:
>>
>> ________________________________
>> Jeremy Zhang
>>
>> _______________________________________________
>> discuss mailing list
>> discuss@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/discuss
>>
>_______________________________________________
discuss mailing list
discuss@openvswitch.org
http://openvswitch.org/mailman/listinfo/discuss
