[ovs-dev] [ovs-dev v2] ofproto: add refcount to ofproto to fix crash at ofproto_destroy

Thu Feb 25 12:54:37 UTC 2021

On 2/25/21 4:32 AM, Guohongzhi (Russell Lab) wrote:
> Refcount and RCU are not mutually exclusive.
> IMO, the main reason for this problem is that the rule uses both refcount and rcu, while the ofproto uses only rcu, and the rule retains the pointer of the ofproto. More importantly, ofproto cannot guarantee a longer grace period than the rule.
> 

I understand that refcount and RCU are not mutually exclusive.
However, in this particular case RCU for ofproto was introduced for one
and only one reason - to avoid freeing ofproto while rules are still
alive.  This was done in commit f416c8d61601 ("ofproto: RCU postpone
rule destruction.").  The goal was to allow using rules without
refcounting them within a single grace period.  And that forced us
to postpone destruction of the ofproto for a single grace period.
Later commit 39c9459355b6 ("Use classifier versioning.") made it
possible for rules to be alive for more than one grace period, so
the commit made ofproto wait for 2 grace periods by double postponing.
As we can see now, that wasn't enough and we have to wait for more
than 2 grace periods in certain cases.

Now we're introducing refcounts to wait for all rules to be destroyed
before destroying the ofproto.  But what is the point of having
RCU if we already know that if refcount is zero than all rules are
already destroyed and we don't need to wait anything and could just
destroy the ofproto?

RCU doesn't protect ofproto itself here, it actually protects rules,
i.e. keeps ofproto alive while rules alive, but we can fully replace
this with refcounts and I see no value in having RCU additionally.

To have a full picture: right now we also have groups protected by
RCU, so we need to refcount ofproto for them too.  But that is almost
exactly same situation as we have with rules.

> 
> -----Original Message-----
> From: 贺鹏 [mailto:xnhp0320 at gmail.com] 
> Sent: 25/2/2021 11:14
> To: Ilya Maximets <i.maximets at ovn.org>
> Cc: hepeng.0320 <hepeng.0320 at bytedance.com>; <dev at openvswitch.org> <dev at openvswitch.org>; Guohongzhi (Russell Lab) <guohongzhi1 at huawei.com>
> Subject: Re: [ovs-dev] [ovs-dev v2] ofproto: add refcount to ofproto to fix crash at ofproto_destroy
> 
> Ilya Maximets <i.maximets at ovn.org> 于2021年2月25日周四 上午12:01写道：
>>
>> On 2/24/21 3:29 PM, Ilya Maximets wrote:
>>> On 7/17/20 3:50 AM, hepeng.0320 wrote:
>>>> From: Peng He <hepeng.0320 at bytedance.com>
>>>>
>>>> The call stack of rule_destroy_cb:
>>>>
>>>> remove_rules_postponed (one grace period)
>>>>     -> remove_rule_rcu
>>>>         -> remove_rule_rcu__
>>>>             -> ofproto_rule_unref -> ref count != 1
>>>>                 -> ... more grace periods.
>>>>                 -> rule_destroy_cb (> 2 grace periods)
>>>>                     -> free
>>>>
>>>> Currently ofproto_destory waits only two grace periods, this means 
>>>> when rule_destroy_cb is called, the ofproto might have been already destroyed.
>>>>
>>>> This patch adds a refcount for ofproto to ensure ofproto exists 
>>>> when it is needed to call free functions.
>>>>
>>>> This patch fixes the crashes found:
>>>> Program terminated with signal SIGSEGV, Segmentation fault.
>>>> 0  0x0000562a55169e49 in rule_destroy_cb (rule=0x562a598be2c0) at 
>>>> ofproto/ofproto.c:2956
>>>> 1  0x0000562a552623d6 in ovsrcu_call_postponed () at 
>>>> lib/ovs-rcu.c:348
>>>> 2  0x0000562a55262504 in ovsrcu_postpone_thread (arg=<optimized 
>>>> out>) at lib/ovs-rcu.c:364
>>>> 3  0x0000562a55264aef in ovsthread_wrapper (aux_=<optimized out>) 
>>>> at lib/ovs-thread.c:383
>>>> 4  0x00007febe715a4a4 in start_thread (arg=0x7febe4c1c700) at 
>>>> pthread_create.c:456
>>>> 5  0x00007febe6990d0f in clone () at 
>>>> ../sysdeps/unix/sysv/linux/x86_64/clone.S:97
>>>> (gdb) p rule->ofproto->ofproto_class
>>>> $3 = (const struct ofproto_class *) 0x0
>>>>
>>>> Also:
>>>> 0  ofproto_dpif_credit_table_stats (ofproto=0x5583583332b0, 
>>>> table_id=0 '\000', n_matches=5, n_misses=0) at 
>>>> ofproto/ofproto-dpif.c:4310
>>>> 1  0x0000558354c68514 in xlate_push_stats_entry 
>>>> (entry=entry at entry=0x7ff488031398, 
>>>> stats=stats at entry=0x7ff49af30b60) at 
>>>> ofproto/ofproto-dpif-xlate-cache.c:99
>>>> 2  0x0000558354c686f3 in xlate_push_stats (xcache=<optimized out>, 
>>>> stats=stats at entry=0x7ff49af30b60) at 
>>>> ofproto/ofproto-dpif-xlate-cache.c:181
>>>> 3  0x0000558354c5411a in revalidate_ukey (udpif=udpif at entry=0x5583583baba0, ukey=ukey at entry=0x7ff47809f770, stats=stats at entry=0x7ff49af32128, odp_actions=odp_actions at entry=0x7ff49af30c50, reval_seq=reval_seq at entry=275670456,
>>>>    recircs=recircs at entry=0x7ff49af30cd0) at 
>>>> ofproto/ofproto-dpif-upcall.c:2292
>>>> 4  0x0000558354c57cbc in revalidate (revalidator=<optimized out>) 
>>>> at ofproto/ofproto-dpif-upcall.c:2681
>>>> 5  0x0000558354c57f8e in udpif_revalidator (arg=0x5583583d2a90) at 
>>>> ofproto/ofproto-dpif-upcall.c:934
>>>> 6  0x0000558354d24aef in ovsthread_wrapper (aux_=<optimized out>) 
>>>> at lib/ovs-thread.c:383
>>>> 7  0x00007ff4a479d4a4 in start_thread (arg=0x7ff49af35700) at 
>>>> pthread_create.c:456
>>>> 8  0x00007ff4a3fd3d0f in clone () at 
>>>> ../sysdeps/unix/sysv/linux/x86_64/clone.S:97
>>>> (gdb) p ofproto->up.ofproto_class
>>>> $3 = (const struct ofproto_class *) 0x0
>>>>
>>>> Signed-off-by: Peng He <hepeng.0320 at bytedance.com>
>>>> ---
>>
>> CC: Hongzhi Guo
>>
>> And I just remembered that there is a very similar patch that was 
>> submitted to the mail-list several months betore this one.
>> Ben started review, but didn't finish:
>>
>> http://patchwork.ozlabs.org/project/openvswitch/patch/20200306130555.1
>> 9884-1-guohongzhi1 at huawei.com/
>>
>> Both patches has smilar issues, i.e. mixing RCU with refcounters and 
>> not covering all the cases.
>>
>> There are at least 2 major places where ofproto needs to be refcounted:
>> 1. rules
>>    1.a xlate caches?
>> 2. ofproto groups
>>
>> IMO, mixing RCU with refcounts basically means that we do not have a 
>> clear understanding on how it works and using RCU as a safety net.
>> We shuld stop using RCU for orproto and just have refcounts.  Once 
>> ofproto refcount reaches zero it should be immediately destroyed.
>> All the places where we need to increase refcount needs to be tracked 
>> down.
> 
> IMO, I think RCU and refcounts are not mutually exclusive. In kernel connntack system such as IPVS, sessions are used with both RCU and refcounts.
> 
> Essentially, the RCU implies a refcount if there are no places holding a pointer to the object other than hash tables (or other RCU enabled data structure), thus by removing it from hash tables, and wait a grace period, we can safely free the object because we are sure that no one holds a pointer to the object. Removing it from hash tables is like reducing the refcount by 1, and since hash tables are the only place that holds the pointer and this is the only reference that lives longer than a grace period, so by removing it, we are sure that no other ref can live longer than a grace period, and after the period, we are safe to free it.
> 
> If we use only refcounts, every time we access the object, we have to add refcounts.
> But if use RCU combined with refcount, we only need to add ref, if there are other places than the hashtables that live longer than one grace period.
> 
> So I think using RCU with refcounts actually makes code changes less, as mostly, the access to object is ephemeral, you lookup the hashtables and store the pointer using a stack variable.
> As long as you don't store the pointer in a data structure that lives longer than a grace period, then you do not have to add refcount.
> 
>>
>> It'll be great if you can cooperate to prepare a complete solution for 
>> this issue.
>>
>> Best regards, Ilya Maximets.
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 
> 
> 
> --
> hepeng
> 