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

[Linhaifeng]

> -----Original Message-----
> From: Ilya Maximets [mailto:i.maximets at ovn.org]
> Sent: Thursday, June 4, 2020 12:01 AM
> To: Ben Pfaff <blp at ovn.org>; Linhaifeng <haifeng.lin at huawei.com>
> Cc: dev at openvswitch.org; Lilijun (Jerry) <jerry.lilijun at huawei.com>; Lichunhe
> <lichunhe at huawei.com>; nd <nd at arm.com>; chenchanghu
> <chenchanghu at huawei.com>; i.maximets at ovn.org
> Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
> 
> 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.
> 

Yes, you are right. The reader's thread->seqno should bigger than global_seqno so it maybe have run one more times before someone's thread->seqno bigger than global_seqno.

> >
> >     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.