On Thu, Mar 7, 2019 at 7:45 PM Han Zhou <[email protected]> wrote:
>
> On Tue, Mar 5, 2019 at 6:17 PM Han Zhou <[email protected]> wrote:
> >
> > From: Han Zhou <[email protected]>
> >
> > When a row is deleted, if it is referenced by another row, the
> > function ovsdb_idl_row_reparse_backrefs() is called but in that
> > function it doesn't destroy the row, because parameter destroy_dsts
> > is false when ovsdb_idl_row_reparse_backrefs() calls
> > ovsdb_idl_row_clear_arcs().
> >
> > Signed-off-by: Han Zhou <[email protected]>
> > ---
> > lib/ovsdb-idl.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> > index 8cfb201..49fd45c 100644
> > --- a/lib/ovsdb-idl.c
> > +++ b/lib/ovsdb-idl.c
> > @@ -3208,6 +3208,7 @@ ovsdb_idl_delete_row(struct ovsdb_idl_row *row)
> > ovsdb_idl_row_destroy(row);
> > } else {
> > ovsdb_idl_row_reparse_backrefs(row);
> > + ovsdb_idl_row_destroy(row);
> > }
> > }
> >
> > --
> > 2.1.0
> >
>
> This patch fails many tests, e.g. set, simple3
> idl-compound-index-with-ref, initially populated - C
>
> It causes SIGSEGV when processing update notification for deletions
> when there are references. E.g. column C1 in row R1 in table T1 refers
> to C2 in R2 in T2. When R1 is deleted, both R1 and R2 will be deleted
> and sent in the same update notification. The IDL processes R2
> deletion first. With this patch, it is destroyed after calling
> ovsdb_idl_row_reparse_backrefs(row). However, when processing R1
> deletion, the function ovsdb_idl_delete_row() calls
> ovsdb_idl_row_clear_arcs(destroy=true) to free the orphan rows.
> Somehow R1 still has arc pointing to R2, and so it is calling
> ovsdb_idl_row_destroy() again for R2, which causes SIGSEGV, with below
> backtrace.
>
> (gdb) bt
> #0 0x000000000043fdd3 in hmap_remove (node=0x16bce30, hmap=0x16b7d88)
> at ../include/openvswitch/hmap.h:287
> #1 ovsdb_idl_row_destroy (row=0x16bce30) at ../lib/ovsdb-idl.c:3104
> #2 0x00000000004400d6 in ovsdb_idl_row_clear_arcs
> (row=row@entry=0x16bcf10, destroy_dsts=destroy_dsts@entry=true) at
> ../lib/ovsdb-idl.c:3041
> #3 0x000000000044028e in ovsdb_idl_delete_row
> (row=row@entry=0x16bcf10) at ../lib/ovsdb-idl.c:3205
> #4 0x0000000000441392 in ovsdb_idl_process_update2
> (json_row=0x16c2370, operation=0x469d7e "delete", uuid=0x7ffff0219980,
> table=0x16b7c90) at ../lib/ovsdb-idl.c:2442
> #5 ovsdb_idl_db_parse_update__ (method=(unknown: 23866864),
> table_updates=<optimized out>, db=<optimized out>) at
> ../lib/ovsdb-idl.c:2302
> #6 ovsdb_idl_db_parse_update (db=db@entry=0x16b7348,
> table_updates=0x16bd1a0,
> method=method@entry=OVSDB_IDL_MM_MONITOR_COND_SINCE) at
> ../lib/ovsdb-idl.c:2354
> #7 0x0000000000441b6e in ovsdb_idl_db_parse_update_rpc
> (db=db@entry=0x16b7348, msg=msg@entry=0x16bd290) at
> ../lib/ovsdb-idl.c:2185
> #8 0x0000000000441f0c in ovsdb_idl_db_parse_update_rpc
> (msg=0x16bd290, db=0x16b7348) at ../lib/ovsdb-idl.c:900
> #9 ovsdb_idl_process_msg (msg=0x16bd290, idl=0x16b7290) at
> ../lib/ovsdb-idl.c:820
> #10 ovsdb_idl_run (idl=0x16b7290) at ../lib/ovsdb-idl.c:899
> #11 0x000000000044682c in ovsdb_idl_txn_commit_block
> (txn=txn@entry=0x16bb600) at ../lib/ovsdb-idl.c:4285
> #12 0x0000000000407f8a in do_idl_compound_index_with_ref
> (ctx=<optimized out>) at ../tests/test-ovsdb.c:2804
> #13 0x0000000000430a84 in ovs_cmdl_run_command__
> (ctx=ctx@entry=0x7ffff0219b80, commands=commands@entry=0x69a800
> <all_commands>, read_only=read_only@entry=false)
> at ../lib/command-line.c:223
> #14 0x0000000000431047 in ovs_cmdl_run_command
> (ctx=ctx@entry=0x7ffff0219b80, commands=commands@entry=0x69a800
> <all_commands>) at ../lib/command-line.c:254
> #15 0x000000000040530c in main (argc=<optimized out>,
> argv=0x7ffff0219c98) at ../tests/test-ovsdb.c:76
>
> My original understanding was that ovsdb_idl_row_reparse_backrefs(row)
> is to cut the link between R1 and R2 when R2 is being deleted.
> However, it turns out in this function it unparse R1, clear arcs from
> R1 to R2 without destroying R2 (i.e.
> ovsdb_idl_row_clear_arcs(destroy=false)), and then it parse R1 again,
> which adds the arcs back. So it seems doesn't cut any arcs, and I am
> totally confused about the purpose of this function
> ovsdb_idl_row_reparse_backrefs() if it doesn't change anything at all.
>
> And still, it seems the memory leak would happen if R2 (the one being
> referenced) is deleted but R1 is never deleted, because the only
> chance that R2 gets destroyed is when R1 is deleted, though
> ovsdb_idl_row_clear_arcs(destroy=true).
>
> Did I misunderstand anything? Could someone who is familiar with this
> code help explain a little bit?
>
I think I understand it now. There is no memory leak here, but the
logic is a tricky. The purpose of ovsdb_idl_row_reparse_backrefs() is
not to cut the arc between R1 and R2, but to make sure R2 disappears
from R1's C structure. The actual destroy of R2 is deferred to when R1
is deleted. If R1 -> R2 is a weak reference and R2 is deleted without
deleting R1, then there will be a "modify" operation for R1, involved
in the same update notification, which will trigger destroying R2 in
ovsdb_idl_modify_row_by_diff()->ovsdb_idl_row_clear_arcs(destroy=true).
So, please ignore this patch.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
