Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
On 6/10/20 8:41 PM, Ben Pfaff wrote: > On Wed, Jun 10, 2020 at 11:40:14AM -0700, Ben Pfaff wrote: >> On Wed, Jun 03, 2020 at 04:33:28PM +0200, Ilya Maximets wrote: >>> On 6/3/20 1:08 PM, Linhaifeng wrote: >>>> >>>> >>>>> -Original Message- >>>>> From: Ilya Maximets [mailto:i.maxim...@ovn.org] >>>>> Sent: Wednesday, June 3, 2020 6:50 PM >>>>> To: Linhaifeng ; Ben Pfaff >>>>> Cc: i.maxim...@ovn.org; d...@openvswitch.org; Lilijun (Jerry) >>>>> ; Lichunhe ; nd >>>>> ; chenchanghu >>>>> Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first >>>>> >>>>> On 6/3/20 9:04 AM, Linhaifeng wrote: >>>>>> >>>>>> >>>>>>> -Original Message- >>>>>>> From: Ben Pfaff [mailto:b...@ovn.org] >>>>>>> Sent: Wednesday, June 3, 2020 1:26 PM >>>>>>> To: Linhaifeng >>>>>>> Cc: Yanqin Wei ; d...@openvswitch.org; nd >>>>>>> ; Lilijun (Jerry) ; chenchanghu >>>>>>> ; Lichunhe >>>>>>> Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first >>>>>>> >>>>>>> On Wed, Jun 03, 2020 at 01:22:52AM +, Linhaifeng wrote: >>>>>>>> >>>>>>>> >>>>>>>> -Original Message- >>>>>>>> From: Ben Pfaff [mailto:b...@ovn.org] >>>>>>>> Sent: Wednesday, June 3, 2020 1:28 AM >>>>>>>> To: Linhaifeng >>>>>>>> Cc: Yanqin Wei ; d...@openvswitch.org; nd >>>>>>>> ; Lilijun (Jerry) ; >>>>>>>> chenchanghu ; Lichunhe >>>>> >>>>>>>> Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first >>>>>>>> >>>>>>>> On Tue, Jun 02, 2020 at 07:27:59AM +, 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 >>>>>>>> >>>>>>>> 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) >>>>>>> >>>>>>> 1.3 update new pointer (B) >>>>>>> 1.4 quiesced >>>>>>> >>>>>>> 3.3 call all cb to free (A) >>>>>>&g
Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
On Wed, Jun 10, 2020 at 11:40:14AM -0700, Ben Pfaff wrote: > On Wed, Jun 03, 2020 at 04:33:28PM +0200, Ilya Maximets wrote: > > On 6/3/20 1:08 PM, Linhaifeng wrote: > > > > > > > > >> -Original Message- > > >> From: Ilya Maximets [mailto:i.maxim...@ovn.org] > > >> Sent: Wednesday, June 3, 2020 6:50 PM > > >> To: Linhaifeng ; Ben Pfaff > > >> Cc: i.maxim...@ovn.org; d...@openvswitch.org; Lilijun (Jerry) > > >> ; Lichunhe ; nd > > >> ; chenchanghu > > >> Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first > > >> > > >> On 6/3/20 9:04 AM, Linhaifeng wrote: > > >>> > > >>> > > >>>> -Original Message- > > >>>> From: Ben Pfaff [mailto:b...@ovn.org] > > >>>> Sent: Wednesday, June 3, 2020 1:26 PM > > >>>> To: Linhaifeng > > >>>> Cc: Yanqin Wei ; d...@openvswitch.org; nd > > >>>> ; Lilijun (Jerry) ; chenchanghu > > >>>> ; Lichunhe > > >>>> Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first > > >>>> > > >>>> On Wed, Jun 03, 2020 at 01:22:52AM +0000, Linhaifeng wrote: > > >>>>> > > >>>>> > > >>>>> -Original Message- > > >>>>> From: Ben Pfaff [mailto:b...@ovn.org] > > >>>>> Sent: Wednesday, June 3, 2020 1:28 AM > > >>>>> To: Linhaifeng > > >>>>> Cc: Yanqin Wei ; d...@openvswitch.org; nd > > >>>>> ; Lilijun (Jerry) ; > > >>>>> chenchanghu ; Lichunhe > > >> > > >>>>> Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first > > >>>>> > > >>>>> On Tue, Jun 02, 2020 at 07:27:59AM +, 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 > > >>>>> > > >>>>> 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) > > >>>> > > >>>> 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 > > >> yo
Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
On Wed, Jun 03, 2020 at 04:33:28PM +0200, Ilya Maximets wrote: > On 6/3/20 1:08 PM, Linhaifeng wrote: > > > > > >> -Original Message- > >> From: Ilya Maximets [mailto:i.maxim...@ovn.org] > >> Sent: Wednesday, June 3, 2020 6:50 PM > >> To: Linhaifeng ; Ben Pfaff > >> Cc: i.maxim...@ovn.org; d...@openvswitch.org; Lilijun (Jerry) > >> ; Lichunhe ; nd > >> ; chenchanghu > >> Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first > >> > >> On 6/3/20 9:04 AM, Linhaifeng wrote: > >>> > >>> > >>>> -Original Message- > >>>> From: Ben Pfaff [mailto:b...@ovn.org] > >>>> Sent: Wednesday, June 3, 2020 1:26 PM > >>>> To: Linhaifeng > >>>> Cc: Yanqin Wei ; d...@openvswitch.org; nd > >>>> ; Lilijun (Jerry) ; chenchanghu > >>>> ; Lichunhe > >>>> Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first > >>>> > >>>> On Wed, Jun 03, 2020 at 01:22:52AM +, Linhaifeng wrote: > >>>>> > >>>>> > >>>>> -Original Message- > >>>>> From: Ben Pfaff [mailto:b...@ovn.org] > >>>>> Sent: Wednesday, June 3, 2020 1:28 AM > >>>>> To: Linhaifeng > >>>>> Cc: Yanqin Wei ; d...@openvswitch.org; nd > >>>>> ; Lilijun (Jerry) ; > >>>>> chenchanghu ; Lichunhe > >> > >>>>> Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first > >>>>> > >>>>> On Tue, Jun 02, 2020 at 07:27:59AM +, 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 > >>>>> > >>>>> 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) > >>>> > >>>> 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. > >>> > >>> Yes, it's really hard to happen. If it happened it's also hard to find > >>> the reason > >> so I suggest it can be a rule for using rcu. > >> > >> I agree that there is an issue here, but I think that we should not force > >> users to > >> call ovsrcu_set() before ovsrcu_postpone(). Current users doesn't do > >> anything illegal since pointer must not be freed before the next grace > >> period >
Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
Hi, Ben
Will this patch apply ?
> -Original Message-
> From: Ilya Maximets [mailto:i.maxim...@ovn.org]
> Sent: Wednesday, June 3, 2020 10:33 PM
> To: Linhaifeng ; Ilya Maximets
> ; Ben Pfaff
> Cc: d...@openvswitch.org; Lilijun (Jerry) ; Lichunhe
> ; nd ; chenchanghu
>
> Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
>
> The change could look like this:
>
> diff --git a/lib/ovs-rcu.c b/lib/ovs-rcu.c index ebc8120f0..cde1e925b 100644
> --- a/lib/ovs-rcu.c
> +++ b/lib/ovs-rcu.c
> @@ -30,6 +30,8 @@
>
> VLOG_DEFINE_THIS_MODULE(ovs_rcu);
>
> +#define MIN_CBS 16
> +
> struct ovsrcu_cb {
> void (*function)(void *aux);
> void *aux;
> @@ -37,7 +39,8 @@ struct ovsrcu_cb {
>
> struct ovsrcu_cbset {
> struct ovs_list list_node;
> -struct ovsrcu_cb cbs[16];
> +struct ovsrcu_cb *cbs;
> +size_t n_allocated;
> int n_cbs;
> };
>
> @@ -310,16 +313,19 @@ ovsrcu_postpone__(void (*function)(void *aux), void
> *aux)
> cbset = perthread->cbset;
> if (!cbset) {
> cbset = perthread->cbset = xmalloc(sizeof *perthread->cbset);
> +cbset->cbs = xmalloc(MIN_CBS * sizeof *cbset->cbs);
> +cbset->n_allocated = MIN_CBS;
> cbset->n_cbs = 0;
> }
>
> +if (cbset->n_cbs == cbset->n_allocated) {
> +cbset->cbs = x2nrealloc(cbset->cbs, >n_allocated,
> +sizeof *cbset->cbs);
> +}
> +
> cb = >cbs[cbset->n_cbs++];
> cb->function = function;
> cb->aux = aux;
> -
> -if (cbset->n_cbs >= ARRAY_SIZE(cbset->cbs)) {
> -ovsrcu_flush_cbset(perthread);
> -}
> }
>
> static bool
> @@ -341,6 +347,7 @@ ovsrcu_call_postponed(void)
> for (cb = cbset->cbs; cb < >cbs[cbset->n_cbs]; cb++) {
> cb->function(cb->aux);
> }
> +free(cbset->cbs);
> free(cbset);
> }
>
> ---
>
>
> I could submit a formal patch, if agreed.
>
> Ben, what do you think?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
> -Original Message- > From: Ilya Maximets [mailto:i.maxim...@ovn.org] > Sent: Thursday, June 4, 2020 12:01 AM > To: Ben Pfaff ; Linhaifeng > Cc: d...@openvswitch.org; Lilijun (Jerry) ; Lichunhe > ; nd ; chenchanghu > ; i.maxim...@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 +, Linhaifeng wrote: > >> > >> > >> -Original Message- > >> From: Ben Pfaff [mailto:b...@ovn.org] > >> Sent: Wednesday, June 3, 2020 1:28 AM > >> To: Linhaifeng > >> Cc: Yanqin Wei ; d...@openvswitch.org; nd > >> ; Lilijun (Jerry) ; chenchanghu > >> ; Lichunhe > >> Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first > >> > >> On Tue, Jun 02, 2020 at 07:27:59AM +, 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 > >> > >> 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 1Thread 2RCU 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. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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 +, Linhaifeng wrote: >> >> >> -Original Message- >> From: Ben Pfaff [mailto:b...@ovn.org] >> Sent: Wednesday, June 3, 2020 1:28 AM >> To: Linhaifeng >> Cc: Yanqin Wei ; d...@openvswitch.org; nd >> ; Lilijun (Jerry) ; chenchanghu >> ; Lichunhe >> Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first >> >> On Tue, Jun 02, 2020 at 07:27:59AM +, 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 >> >> 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 1Thread 2RCU 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. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
On 6/3/20 1:08 PM, Linhaifeng wrote: > > >> -Original Message- >> From: Ilya Maximets [mailto:i.maxim...@ovn.org] >> Sent: Wednesday, June 3, 2020 6:50 PM >> To: Linhaifeng ; Ben Pfaff >> Cc: i.maxim...@ovn.org; d...@openvswitch.org; Lilijun (Jerry) >> ; Lichunhe ; nd >> ; chenchanghu >> Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first >> >> On 6/3/20 9:04 AM, Linhaifeng wrote: >>> >>> >>>> -Original Message- >>>> From: Ben Pfaff [mailto:b...@ovn.org] >>>> Sent: Wednesday, June 3, 2020 1:26 PM >>>> To: Linhaifeng >>>> Cc: Yanqin Wei ; d...@openvswitch.org; nd >>>> ; Lilijun (Jerry) ; chenchanghu >>>> ; Lichunhe >>>> Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first >>>> >>>> On Wed, Jun 03, 2020 at 01:22:52AM +, Linhaifeng wrote: >>>>> >>>>> >>>>> -Original Message----- >>>>> From: Ben Pfaff [mailto:b...@ovn.org] >>>>> Sent: Wednesday, June 3, 2020 1:28 AM >>>>> To: Linhaifeng >>>>> Cc: Yanqin Wei ; d...@openvswitch.org; nd >>>>> ; Lilijun (Jerry) ; >>>>> chenchanghu ; Lichunhe >> >>>>> Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first >>>>> >>>>> On Tue, Jun 02, 2020 at 07:27:59AM +, 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 >>>>> >>>>> 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) >>>> >>>> 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. >>> >>> Yes, it's really hard to happen. If it happened it's also hard to find the >>> reason >> so I suggest it can be a rule for using rcu. >> >> I agree that there is an issue here, but I think that we should not force >> users to >> call ovsrcu_set() before ovsrcu_postpone(). Current users doesn't do >> anything illegal since pointer must not be freed before the next grace period >> from their point of view. >> >> For me it looks like the main issue is existence of point 1.2, i.e. flushing >> cbsets >> while writer is not quiesced yet. And we need to fix this inside rcu >> library itself. >> For example, we could avoid flushing inside >> ovsrcu_postpone() by making cbs[16] a dynamically allocated array and using >> x2nrealloc instead of flushing. >> >> Thoughts? >> > Hi, Ilya Maximets > > May be this is a good idea therefor the users not need to think about call > ovsrcu_set()
Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
> -Original Message-
> From: Ilya Maximets [mailto:i.maxim...@ovn.org]
> Sent: Wednesday, June 3, 2020 6:50 PM
> To: Linhaifeng ; Ben Pfaff
> Cc: i.maxim...@ovn.org; d...@openvswitch.org; Lilijun (Jerry)
> ; Lichunhe ; nd
> ; chenchanghu
> Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
>
> On 6/3/20 9:04 AM, Linhaifeng wrote:
> >
> >
> >> -Original Message-
> >> From: Ben Pfaff [mailto:b...@ovn.org]
> >> Sent: Wednesday, June 3, 2020 1:26 PM
> >> To: Linhaifeng
> >> Cc: Yanqin Wei ; d...@openvswitch.org; nd
> >> ; Lilijun (Jerry) ; chenchanghu
> >> ; Lichunhe
> >> Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
> >>
> >> On Wed, Jun 03, 2020 at 01:22:52AM +, Linhaifeng wrote:
> >>>
> >>>
> >>> -Original Message-
> >>> From: Ben Pfaff [mailto:b...@ovn.org]
> >>> Sent: Wednesday, June 3, 2020 1:28 AM
> >>> To: Linhaifeng
> >>> Cc: Yanqin Wei ; d...@openvswitch.org; nd
> >>> ; Lilijun (Jerry) ;
> >>> chenchanghu ; Lichunhe
>
> >>> Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
> >>>
> >>> On Tue, Jun 02, 2020 at 07:27:59AM +, 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
> >>>
> >>> 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)
> >>
> >> 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.
> >
> > Yes, it's really hard to happen. If it happened it's also hard to find the
> > reason
> so I suggest it can be a rule for using rcu.
>
> I agree that there is an issue here, but I think that we should not force
> users to
> call ovsrcu_set() before ovsrcu_postpone(). Current users doesn't do
> anything illegal since pointer must not be freed before the next grace period
> from their point of view.
>
> For me it looks like the main issue is existence of point 1.2, i.e. flushing
> cbsets
> while writer is not quiesced yet. And we need to fix this inside rcu library
> itself.
> For example, we could avoid flushing inside
> ovsrcu_postpone() by making cbs[16] a dynamically allocated array and using
> x2nrealloc instead of flushing.
>
> Thoughts?
>
Hi, Ilya Maximets
May be this is a good idea therefor the users not need to think about call
ovsrcu_set() first or ovsrcu_postpone().
How about you think, ben? May be you can send a patch to modify the
ovsrcu_postpone() not to flush cbsets to
replace of my patches.
.
>
> Regarding the patch itself:
>
> > diff --git a/lib/ovs-rcu.h b/lib/ovs-rcu.h index ecc4c9201..98c238aea
> > 100644
> > --- a/lib/ovs-rcu.h
> > +++ b/lib/ovs-rcu.h
> > @@ -118,10 +118,10 @@
> > * void
> > * change_flow(struct flow *new_flow)
> > * {
> > + * struct flow *old_flow = ovsrcu_get_protected(struct flow *,
> )
>
> ovsrcu_get_protected() can not be used outside of the critical section.
>
> > * ovs_mutex_lock();
> > - * ovsrcu_postpone(free,
> > - * ovsrcu_get_protected(struct flow *,
> ));
> > * ovsrcu_set(, new_flow);
> > + * ovsrcu_postpone(free, old_flow);
> > * ovs_mutex_unlock();
> > * }
> >
> Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
On 6/3/20 9:04 AM, Linhaifeng wrote:
>
>
>> -Original Message-
>> From: Ben Pfaff [mailto:b...@ovn.org]
>> Sent: Wednesday, June 3, 2020 1:26 PM
>> To: Linhaifeng
>> Cc: Yanqin Wei ; d...@openvswitch.org; nd
>> ; Lilijun (Jerry) ; chenchanghu
>> ; Lichunhe
>> Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
>>
>> On Wed, Jun 03, 2020 at 01:22:52AM +, Linhaifeng wrote:
>>>
>>>
>>> -Original Message-
>>> From: Ben Pfaff [mailto:b...@ovn.org]
>>> Sent: Wednesday, June 3, 2020 1:28 AM
>>> To: Linhaifeng
>>> Cc: Yanqin Wei ; d...@openvswitch.org; nd
>>> ; Lilijun (Jerry) ; chenchanghu
>>> ; Lichunhe
>>> Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
>>>
>>> On Tue, Jun 02, 2020 at 07:27:59AM +, 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
>>>
>>> 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)
>>
>> 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.
>
> Yes, it's really hard to happen. If it happened it's also hard to find the
> reason so I suggest it can be a rule for using rcu.
I agree that there is an issue here, but I think that we should not force
users to call ovsrcu_set() before ovsrcu_postpone(). Current users doesn't
do anything illegal since pointer must not be freed before the next grace
period from their point of view.
For me it looks like the main issue is existence of point 1.2, i.e. flushing
cbsets while writer is not quiesced yet. And we need to fix this inside
rcu library itself. For example, we could avoid flushing inside
ovsrcu_postpone() by making cbs[16] a dynamically allocated array and using
x2nrealloc instead of flushing.
Thoughts?
Regarding the patch itself:
> diff --git a/lib/ovs-rcu.h b/lib/ovs-rcu.h
> index ecc4c9201..98c238aea 100644
> --- a/lib/ovs-rcu.h
> +++ b/lib/ovs-rcu.h
> @@ -118,10 +118,10 @@
> * void
> * change_flow(struct flow *new_flow)
> * {
> + * struct flow *old_flow = ovsrcu_get_protected(struct flow *,
> )
ovsrcu_get_protected() can not be used outside of the critical section.
> * ovs_mutex_lock();
> - * ovsrcu_postpone(free,
> - * ovsrcu_get_protected(struct flow *, ));
> * ovsrcu_set(, new_flow);
> + * ovsrcu_postpone(free, old_flow);
> * ovs_mutex_unlock();
> * }
>
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
Hi Ben, I read the comments and codes about ovs-rcu. Each thread is added into ovsrcu_threads in the thread start routine. And most of threads quiesce in the poll_block/ xsleep/ ovsrcu_quiesce/ ovsrcu_quiesce_start+ ovsrcu_quiesce_end. Only one thread "afsync_thread" may not be in the ovsrcu_threads list, because it invokes ovsrcu_quiesce_start but does not call ovsrcu_quiesce_end. But this thread does not update/free rcu memory, so no writer is in the quiescent state. I am sorry to make an incorrect comment for Haifeng's patch. He has a correct description in original patch. Please help to correct. https://patchwork.ozlabs.org/project/openvswitch/patch/4099de2e54afad489356c6c9161d53339735a...@dggeml522-mbs.china.huawei.com/ Best Regards, Wei Yanqin > -Original Message- > From: Ben Pfaff > Sent: Wednesday, June 3, 2020 8:48 AM > To: Yanqin Wei > Cc: Linhaifeng ; d...@openvswitch.org; nd > ; Lilijun (Jerry) ; chenchanghu > ; Lichunhe > Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first > > Oh, I apologize that I made a mistake about the author. > > I appreciate feedback from anyone. > > On Wed, Jun 03, 2020 at 12:37:27AM +, Yanqin Wei wrote: > > Hi Ben, > > > > This patch is from Linhai, but I have the same concern about this. I will > > read > ovs-rcu comments and feedback. > > Thanks for your time. > > > > Best Regards, > > Wei Yanqin > > > > > -Original Message- > > > From: Ben Pfaff > > > Sent: Wednesday, June 3, 2020 8:35 AM > > > To: Yanqin Wei > > > Cc: Linhaifeng ; d...@openvswitch.org; nd > > > ; Lilijun (Jerry) ; > > > chenchanghu ; Lichunhe > > > > Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first > > > > > > This is not how RCU works in OVS. Every thread is by default considered > active. > > > They rarely quiesce except implicitly inside poll_block(). > > > Please read the large comment at the top of ovs-rcu.h. > > > > > > Is your patch based on actual bugs that you have found, or is it > > > just some kind of precaution? If it is the latter, then it is not needed. > > > > > > On Tue, Jun 02, 2020 at 11:22:57PM +, Yanqin Wei wrote: > > > > Hi Ben, > > > > > > > > If my understanding is correct, the writer could not be a rcu > > > > thread because it > > > does not need report holding or not holding pointers. > > > > So old memory will be freed after all rcu thread report quiesce. > > > > > > > > Best Regards, > > > > Wei Yanqin > > > > > > > > > -Original Message- > > > > > From: Ben Pfaff > > > > > Sent: Wednesday, June 3, 2020 1:28 AM > > > > > To: Linhaifeng > > > > > Cc: Yanqin Wei ; d...@openvswitch.org; nd > > > > > ; Lilijun (Jerry) ; > > > > > chenchanghu ; Lichunhe > > > > > > > > Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer > > > > > first > > > > > > > > > > On Tue, Jun 02, 2020 at 07:27:59AM +, 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 > > > > > > > > > > I don't see how that's possible, since the writer hasn't quiesced. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
> -Original Message- > From: Ben Pfaff [mailto:b...@ovn.org] > Sent: Wednesday, June 3, 2020 1:26 PM > To: Linhaifeng > Cc: Yanqin Wei ; d...@openvswitch.org; nd > ; Lilijun (Jerry) ; chenchanghu > ; Lichunhe > Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first > > On Wed, Jun 03, 2020 at 01:22:52AM +, Linhaifeng wrote: > > > > > > -Original Message- > > From: Ben Pfaff [mailto:b...@ovn.org] > > Sent: Wednesday, June 3, 2020 1:28 AM > > To: Linhaifeng > > Cc: Yanqin Wei ; d...@openvswitch.org; nd > > ; Lilijun (Jerry) ; chenchanghu > > ; Lichunhe > > Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first > > > > On Tue, Jun 02, 2020 at 07:27:59AM +, 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 > > > > 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) > > 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. Yes, it's really hard to happen. If it happened it's also hard to find the reason so I suggest it can be a rule for using rcu. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
On Wed, Jun 03, 2020 at 01:22:52AM +, Linhaifeng wrote: > > > -Original Message- > From: Ben Pfaff [mailto:b...@ovn.org] > Sent: Wednesday, June 3, 2020 1:28 AM > To: Linhaifeng > Cc: Yanqin Wei ; d...@openvswitch.org; nd ; > Lilijun (Jerry) ; chenchanghu > ; Lichunhe > Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first > > On Tue, Jun 02, 2020 at 07:27:59AM +, 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 > > 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) 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. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
> -Original Message-
> From: Ben Pfaff [mailto:b...@ovn.org]
> Sent: Wednesday, June 3, 2020 8:35 AM
> To: Yanqin Wei
> Cc: Linhaifeng ; d...@openvswitch.org; nd
> ; Lilijun (Jerry) ; chenchanghu
> ; Lichunhe
> Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
>
> This is not how RCU works in OVS. Every thread is by default considered
> active. They rarely quiesce except implicitly inside poll_block().
> Please read the large comment at the top of ovs-rcu.h.
>
> Is your patch based on actual bugs that you have found, or is it just some
> kind
> of precaution? If it is the latter, then it is not needed.
>
Is an actual bug for old version bug it's also suitable for the other codes in
ovs.
Here is the debug info:
linux-mNuKFc:/Images/linhf/830/Euler_compile_env # gdb -p `pidof ovs-vswitchd`
GNU gdb (GDB) Red Hat Enterprise Linux 8.2-3.h2
Copyright (C) 2018 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Type "show copying" and "show warranty" for details.
This GDB was configured as "aarch64-Huawei-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
<http://www.gnu.org/software/gdb/documentation/>.
For help, type "help".
Type "apropos word" to search for commands related to "word".
Attaching to process 102706
[New LWP 109133]
[New LWP 109134]
[New LWP 109297]
[New LWP 109298]
[New LWP 109299]
[New LWP 109300]
[New LWP 109303]
[New LWP 109304]
[New LWP 109308]
[New LWP 109309]
[New LWP 109310]
[New LWP 109311]
[New LWP 109522]
[New LWP 109523]
[New LWP 109603]
[New LWP 109615]
[New LWP 109619]
[New LWP 109655]
[New LWP 109673]
[New LWP 109794]
[New LWP 109795]
[New LWP 113953]
[New LWP 114362]
[New LWP 114364]
[New LWP 114368]
[New LWP 114370]
[New LWP 114373]
[New LWP 114377]
[New LWP 115594]
[New LWP 115595]
[New LWP 115596]
[New LWP 115597]
[New LWP 115598]
[New LWP 115600]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
0x879981ac in poll () from /lib64/libc.so.6
Missing separate debuginfos, use: debuginfo-install glib2-2.54.2-2.h1.aarch64
glibc-2.28-9.h17.aarch64 keyutils-libs-1.5.8-3.aarch64
krb5-libs-1.15.1-34.h2.aarch64 libcgroup-0.41-15.h3.aarch64
libcom_err-1.44.3-1.h4.aarch64 libgcc-7.3.0-20190804.h18.aarch64
libselinux-2.5-12.aarch64 numactl-libs-2.0.9-7.h1.aarch64
openssl-libs-1.0.2k-16.h6.aarch64 pcre-8.32-17.h9.aarch64
uvpkmc-1.0.1-807.aarch64 zlib-1.2.7-17.aarch64
(gdb) b dpcls_destroy_subtable
Breakpoint 1 at 0x508bcc: file lib/dpif-netdev.c, line 6919.
(gdb) b ovsrcu_call_postponed
Breakpoint 2 at 0x5b7d34: file lib/ovs-rcu.c, line 336.
(gdb) c
Continuing.
[Switching to Thread 0x83b97860 (LWP 109304)]
Thread 9 "urcu2" hit Breakpoint 2, ovsrcu_call_postponed () at lib/ovs-rcu.c:336
warning: Source file is more recent than executable.
336 {
(gdb) n
339 int wait_del = 0;
(gdb)
340 while(wait_del);
(gdb) set wait_del = 1
(gdb) c
Continuing.
[Switching to Thread 0x51748860 (LWP 115598)]
Thread 34 "revalidator19" hit Breakpoint 1, dpcls_destroy_subtable
(cls=0x1c00a420, subtable=0x3c009250) at lib/dpif-netdev.c:6919
6919int wait_get = 0;
(gdb) n
6920VLOG_DBG("Destroying subtable %p for in_port %d", subtable,
cls->in_port);
(gdb)
6921pvector_remove(>subtables, subtable);
(gdb)
6922cmap_remove(>subtables_map, >cmap_node,
(gdb) set wait_get = 1
(gdb) n
6924cmap_destroy(>rules);
(gdb) p subtable->rules
$1 = {impl = {p = 0x30008940}}
(gdb) s
cmap_destroy (cmap=0x3c009258) at lib/cmap.c:288
288 if (cmap) {
(gdb) n
289 struct cmap_impl *impl = cmap_get_impl(cmap);
(gdb)
290 if (impl != _cmap) {
(gdb)
291 ovsrcu_postpone(free_cacheline, impl);
(gdb) s
ovsrcu_postpone__ (function=0x6029b0 , aux=0x30008940) at
lib/ovs-rcu.c:315
315 struct ovsrcu_perthread *perthread = ovsrcu_perthread_get();
(gdb) n
318 int size = ARRAY_SIZE(cbset->cbs);
(gdb)
319 cbset = perthread->cbset;
(gdb)
320 if (!cbset) {
(gdb)
325 cb = >cbs[cbset->n_cbs++];
(gdb)
326 cb->function = function;
(gdb)
327 cb->aux = aux;
(gdb)
329 if (cbset->n_cbs >= size) {
(gdb) set size = cbset->n_cbs
(gdb) n
330 ovsrcu_flush_cbset(perthread);
(gdb) s
ovsrcu_flush_cbset (perthread=0x30001210) at lib/ovs-rcu.c:397
397 ovsrcu_flush_
Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
-Original Message- From: Yanqin Wei [mailto:yanqin@arm.com] Sent: Wednesday, June 3, 2020 7:23 AM To: Ben Pfaff ; Linhaifeng Cc: d...@openvswitch.org; nd ; Lilijun (Jerry) ; chenchanghu ; Lichunhe Subject: RE: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first Hi Ben, If my understanding is correct, the writer could not be a rcu thread because it does not need report holding or not holding pointers. So old memory will be freed after all rcu thread report quiesce. The write also is rcu thread. If not first update pointer the reader can also get the old pointer after call ovsrcu_quiesced. Best Regards, Wei Yanqin > -Original Message- > From: Ben Pfaff > Sent: Wednesday, June 3, 2020 1:28 AM > To: Linhaifeng > Cc: Yanqin Wei ; d...@openvswitch.org; nd > ; Lilijun (Jerry) ; chenchanghu > ; Lichunhe > Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first > > On Tue, Jun 02, 2020 at 07:27:59AM +, 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 > > I don't see how that's possible, since the writer hasn't quiesced. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
-Original Message- From: Ben Pfaff [mailto:b...@ovn.org] Sent: Wednesday, June 3, 2020 1:28 AM To: Linhaifeng Cc: Yanqin Wei ; d...@openvswitch.org; nd ; Lilijun (Jerry) ; chenchanghu ; Lichunhe Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first On Tue, Jun 02, 2020 at 07:27:59AM +, 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 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 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
Oh, I apologize that I made a mistake about the author. I appreciate feedback from anyone. On Wed, Jun 03, 2020 at 12:37:27AM +, Yanqin Wei wrote: > Hi Ben, > > This patch is from Linhai, but I have the same concern about this. I will > read ovs-rcu comments and feedback. > Thanks for your time. > > Best Regards, > Wei Yanqin > > > -Original Message- > > From: Ben Pfaff > > Sent: Wednesday, June 3, 2020 8:35 AM > > To: Yanqin Wei > > Cc: Linhaifeng ; d...@openvswitch.org; nd > > ; Lilijun (Jerry) ; chenchanghu > > ; Lichunhe > > Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first > > > > This is not how RCU works in OVS. Every thread is by default considered > > active. > > They rarely quiesce except implicitly inside poll_block(). > > Please read the large comment at the top of ovs-rcu.h. > > > > Is your patch based on actual bugs that you have found, or is it just some > > kind > > of precaution? If it is the latter, then it is not needed. > > > > On Tue, Jun 02, 2020 at 11:22:57PM +, Yanqin Wei wrote: > > > Hi Ben, > > > > > > If my understanding is correct, the writer could not be a rcu thread > > > because it > > does not need report holding or not holding pointers. > > > So old memory will be freed after all rcu thread report quiesce. > > > > > > Best Regards, > > > Wei Yanqin > > > > > > > -Original Message----- > > > > From: Ben Pfaff > > > > Sent: Wednesday, June 3, 2020 1:28 AM > > > > To: Linhaifeng > > > > Cc: Yanqin Wei ; d...@openvswitch.org; nd > > > > ; Lilijun (Jerry) ; > > > > chenchanghu ; Lichunhe > > > > > > Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first > > > > > > > > On Tue, Jun 02, 2020 at 07:27:59AM +, 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 > > > > > > > > I don't see how that's possible, since the writer hasn't quiesced. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
Hi Ben, This patch is from Linhai, but I have the same concern about this. I will read ovs-rcu comments and feedback. Thanks for your time. Best Regards, Wei Yanqin > -Original Message- > From: Ben Pfaff > Sent: Wednesday, June 3, 2020 8:35 AM > To: Yanqin Wei > Cc: Linhaifeng ; d...@openvswitch.org; nd > ; Lilijun (Jerry) ; chenchanghu > ; Lichunhe > Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first > > This is not how RCU works in OVS. Every thread is by default considered > active. > They rarely quiesce except implicitly inside poll_block(). > Please read the large comment at the top of ovs-rcu.h. > > Is your patch based on actual bugs that you have found, or is it just some > kind > of precaution? If it is the latter, then it is not needed. > > On Tue, Jun 02, 2020 at 11:22:57PM +, Yanqin Wei wrote: > > Hi Ben, > > > > If my understanding is correct, the writer could not be a rcu thread > > because it > does not need report holding or not holding pointers. > > So old memory will be freed after all rcu thread report quiesce. > > > > Best Regards, > > Wei Yanqin > > > > > -Original Message- > > > From: Ben Pfaff > > > Sent: Wednesday, June 3, 2020 1:28 AM > > > To: Linhaifeng > > > Cc: Yanqin Wei ; d...@openvswitch.org; nd > > > ; Lilijun (Jerry) ; > > > chenchanghu ; Lichunhe > > > > Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first > > > > > > On Tue, Jun 02, 2020 at 07:27:59AM +, 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 > > > > > > I don't see how that's possible, since the writer hasn't quiesced. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
This is not how RCU works in OVS. Every thread is by default considered active. They rarely quiesce except implicitly inside poll_block(). Please read the large comment at the top of ovs-rcu.h. Is your patch based on actual bugs that you have found, or is it just some kind of precaution? If it is the latter, then it is not needed. On Tue, Jun 02, 2020 at 11:22:57PM +, Yanqin Wei wrote: > Hi Ben, > > If my understanding is correct, the writer could not be a rcu thread because > it does not need report holding or not holding pointers. > So old memory will be freed after all rcu thread report quiesce. > > Best Regards, > Wei Yanqin > > > -Original Message- > > From: Ben Pfaff > > Sent: Wednesday, June 3, 2020 1:28 AM > > To: Linhaifeng > > Cc: Yanqin Wei ; d...@openvswitch.org; nd > > ; Lilijun (Jerry) ; chenchanghu > > ; Lichunhe > > Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first > > > > On Tue, Jun 02, 2020 at 07:27:59AM +, 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 > > > > I don't see how that's possible, since the writer hasn't quiesced. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
Hi Ben, If my understanding is correct, the writer could not be a rcu thread because it does not need report holding or not holding pointers. So old memory will be freed after all rcu thread report quiesce. Best Regards, Wei Yanqin > -Original Message- > From: Ben Pfaff > Sent: Wednesday, June 3, 2020 1:28 AM > To: Linhaifeng > Cc: Yanqin Wei ; d...@openvswitch.org; nd > ; Lilijun (Jerry) ; chenchanghu > ; Lichunhe > Subject: Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first > > On Tue, Jun 02, 2020 at 07:27:59AM +, 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 > > I don't see how that's possible, since the writer hasn't quiesced. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
On Tue, Jun 02, 2020 at 07:27:59AM +, 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 I don't see how that's possible, since the writer hasn't quiesced. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2] ovs rcu: update rcu pointer first
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
---
lib/classifier.c | 4 ++--
lib/ovs-rcu.h | 4 ++--
lib/pvector.c | 15 ---
ofproto/ofproto-dpif-mirror.c | 4 ++--
ofproto/ofproto-dpif-upcall.c | 3 +--
5 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/lib/classifier.c b/lib/classifier.c
index f2c3497c2..6bff76e07 100644
--- a/lib/classifier.c
+++ b/lib/classifier.c
@@ -249,11 +249,11 @@ cls_rule_set_conjunctions(struct cls_rule *cr,
unsigned int old_n = old ? old->n : 0;
if (old_n != n || (n && memcmp(old_conj, conj, n * sizeof *conj))) {
+ovsrcu_set(>conj_set,
+ cls_conjunction_set_alloc(match, conj, n));
if (old) {
ovsrcu_postpone(free, old);
}
-ovsrcu_set(>conj_set,
- cls_conjunction_set_alloc(match, conj, n));
}
}
diff --git a/lib/ovs-rcu.h b/lib/ovs-rcu.h
index ecc4c9201..98c238aea 100644
--- a/lib/ovs-rcu.h
+++ b/lib/ovs-rcu.h
@@ -118,10 +118,10 @@
* void
* change_flow(struct flow *new_flow)
* {
+ * struct flow *old_flow = ovsrcu_get_protected(struct flow *, )
* ovs_mutex_lock();
- * ovsrcu_postpone(free,
- * ovsrcu_get_protected(struct flow *, ));
* ovsrcu_set(, new_flow);
+ * ovsrcu_postpone(free, old_flow);
* ovs_mutex_unlock();
* }
*
diff --git a/lib/pvector.c b/lib/pvector.c
index cc527fdc4..aa8c6cb24 100644
--- a/lib/pvector.c
+++ b/lib/pvector.c
@@ -67,10 +67,11 @@ pvector_init(struct pvector *pvec)
void
pvector_destroy(struct pvector *pvec)
{
+struct pvector_impl *old = pvector_impl_get(pvec);
free(pvec->temp);
pvec->temp = NULL;
-ovsrcu_postpone(free, pvector_impl_get(pvec));
ovsrcu_set(>impl, NULL); /* Poison. */
+ovsrcu_postpone(free, old);
}
/* Iterators for callers that need the 'index' afterward. */
@@ -205,11 +206,11 @@ pvector_change_priority(struct pvector *pvec, void *ptr,
int priority)
/* Make the modified pvector available for iteration. */
void pvector_publish__(struct pvector *pvec)
{
-struct pvector_impl *temp = pvec->temp;
-
+struct pvector_impl *new = pvec->temp;
+struct pvector_impl *old = ovsrcu_get_protected(struct pvector_impl *,
+ >impl);
pvec->temp = NULL;
-pvector_impl_sort(temp); /* Also removes gaps. */
-ovsrcu_postpone(free, ovsrcu_get_protected(struct pvector_impl *,
- >impl));
-ovsrcu_set(>impl, temp);
+pvector_impl_sort(new); /* Also removes gaps. */
+ovsrcu_set(>impl, new);
+ovsrcu_postpone(free, old);
}
diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c
index 343b75f0e..343100c08 100644
--- a/ofproto/ofproto-dpif-mirror.c
+++ b/ofproto/ofproto-dpif-mirror.c
@@ -276,9 +276,9 @@ mirror_set(struct mbridge *mbridge, void *aux, const char
*name,
hmapx_destroy(_map);
if (vlans || src_vlans) {
+unsigned long *new_vlans = vlan_bitmap_clone(src_vlans);
+ovsrcu_set(>vlans, new_vlans);
ovsrcu_postpone(free, vlans);
-vlans = vlan_bitmap_clone(src_vlans);
-ovsrcu_set(>vlans, vlans);
}
mirror->out = out;
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 5e08ef10d..be6dafb78 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -1658,11 +1658,10 @@ ukey_set_actions(struct udpif_key *ukey, const struct
ofpbuf *actions)
struct ofpbuf *old_actions = ovsrcu_get_protected(struct ofpbuf *,
>actions);
+ovsrcu_set(>actions, ofpbuf_clone(actions));
if (old_actions) {
ovsrcu_postpone(ofpbuf_delete, old_actions);
}
-
-ovsrcu_set(>actions, ofpbuf_clone(actions));
}
static struct udpif_key *
--
2.21.0.windows.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
