[ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first

[Ilya Maximets]

On 6/3/20 7:26 AM, Ben Pfaff wrote:
> On Wed, Jun 03, 2020 at 01:22:52AM +0000, Linhaifeng wrote:
>>
>>
>> -----Original Message-----
>> From: Ben Pfaff [mailto:blp at ovn.org] 
>> Sent: Wednesday, June 3, 2020 1:28 AM
>> To: Linhaifeng <haifeng.lin at huawei.com>
>> Cc: Yanqin Wei <Yanqin.Wei at arm.com>; dev at openvswitch.org; nd <nd at arm.com>; Lilijun (Jerry) <jerry.lilijun at huawei.com>; chenchanghu <chenchanghu at huawei.com>; Lichunhe <lichunhe at huawei.com>
>> Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
>>
>> On Tue, Jun 02, 2020 at 07:27:59AM +0000, Linhaifeng wrote:
>>> We should update rcu pointer first then use ovsrcu_postpone to free 
>>> otherwise maybe cause use-after-free.
>>> e.g.,reader indicates momentary quiescent and access old pointer after 
>>> writer postpone free old pointer and before setting new pointer.
>>>
>>> Signed-off-by: Linhaifeng <haifeng.lin at huawei.com>
>>
>> I don't see how that's possible, since the writer hasn't quiesced.
>>
>> I think the logic is as follow, Could you help me find out where is incorrect?
>>
>> 1.1 -> 1.2 -> 3.1 -> 3.2 -> 2.1 -> 2.2 -> 2.3 -> 2.1 -> 1.3 -> 1.4 -> 3.3 -> 2.2(use after free)
>>
>> wirter:
>> 1.1 use postone to free old pointer
>> 1.2 flush cbsets to flushed_cbsets
>> 1.3 update new pointer
>> 1.4 quiesced
>>
>> Read:
>> 2.1. read pointer
>> 2.2. use pointer
>> 2.3. quiesced
>>
>> Rcu:
>> 3.1 pop flushed_cbsets
>> 3.2 ovsrcu_synchronize
>> 3.3 call all cb to free
> 
> So you're saying this:
> 
>     1.1 use postone to free old pointer (A)
>     1.2 flush cbsets to flushed_cbsets
> 
>     3.1 pop flushed_cbsets
>     3.2 ovsrcu_synchronize
> 
>     2.1. read pointer (A)
>     2.2. use pointer (A)
>     2.3. quiesced
> 
>     2.1. read pointer (A)

IIUC, 'Read' thread should enter quiescent state here at least
one more time in order to trigger the issue (or, maybe, some other
thread could advance global_seqno before step 2.3 and after
reading target_seqno by RCU thread on step 3.2).  Otherwise,
target_seqno will be equal to thread->seqno of the 'Read' thread
and RCU thread will wait for the next quiescent state before
calling callbacks.

> 
>     1.3 update new pointer (B)
>     1.4 quiesced
> 
>     3.3 call all cb to free (A)
> 
>     2.2. use pointer (A)
> 
> Wow, you are absolutely right.  This had never occurred to me.  Thank
> you!  I'll review your patch.

Sharing my more detailed diagram of the execution flow that should
trigger the issue here:

     ------------------  ------------------  -------------------
     Thread 1            Thread 2            RCU Thread
     ------------------  ------------------  -------------------
     pointer = A
    
     ovsrcu_quiesce():
      thread->seqno = 30
      global_seqno = 31
      quiesced
    
     read pointer A
     postpone(free(A)):
       flush cbset
                                             pop flushed_cbsets
                                             ovsrcu_synchronize:
                                               target_seqno = 31
                         ovsrcu_quiesce():
                          thread->seqno = 31
                          global_seqno = 32
                          quiesced
    
                         read pointer A
                         use pointer A
    
                         ovsrcu_quiesce():
                          thread->seqno = 32
                          global_seqno = 33
                          quiesced
    
                         read pointer A
     pointer = B
    
     ovsrcu_quiesce():
      thread->seqno = 33
      global_seqno = 34
      quiesced
    
                                             target_seqno exceeded
                                             by all threads
                                             call cbs to free A
                         use pointer A
                         (use after free)
     -----------------------------------------------------------

Best regards, Ilya Maximets.