[ovs-dev] ovsdb-idl: Mark row "parsed" in ovsdb_idl_txn_write__
diff mbox series

Message ID 1563390304-15546-1-git-send-email-dceara@redhat.com
State Accepted
Headers show
Series
  • [ovs-dev] ovsdb-idl: Mark row "parsed" in ovsdb_idl_txn_write__
Related show

Commit Message

Dumitru Ceara July 17, 2019, 7:05 p.m. UTC
Once a column is set in a row using ovsdb_idl_txn_write__ we also mark
the row "parsed". Otherwise the memory allocated by
sbrec_<table>_parse_<col>() will never be freed. After marking the row
"parsed", the ovsdb_idl_txn_disassemble function will properly free the
newly allocated memory for the column (ovsdb_idl_row_unparse).

The problem is present only for rows that are inserted by the
running application because rows that are loaded from the database
will always have row->parsed == true.

One way to detect the leak is to start northd with valgrind:

valgrind --tool=memcheck --leak-check=yes ./ovn-northd

Then create a logical switch and bind a logical port to it:

ovn-nbctl ls-add ls1
ovn-nbctl lsp-add ls1 ls1-vm1

The valgrind report:

==9270== Memcheck, a memory error detector
==9270== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==9270== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright
info
==9270== Command: ./ovn-northd
==9270==

<snip>

==9270==
==9270== 8 bytes in 1 blocks are definitely lost in loss record 30 of
292
==9270==    at 0x4C29BC3: malloc (vg_replace_malloc.c:299)
==9270==    by 0x4D31EF: xmalloc (util.c:138)
==9270==    by 0x45CB8E: sbrec_multicast_group_parse_ports (ovn-sb-idl.c:18141)
==9270==    by 0x4BB12D: ovsdb_idl_txn_write__ (ovsdb-idl.c:4489)
==9270==    by 0x4BB1B5: ovsdb_idl_txn_write (ovsdb-idl.c:4527)
==9270==    by 0x45D167: sbrec_multicast_group_set_ports (ovn-sb-idl.c:18561)
==9270==    by 0x40F416: ovn_multicast_update_sbrec (ovn-northd.c:2947)
==9270==    by 0x41FC55: build_lflows (ovn-northd.c:7981)
==9270==    by 0x421830: ovnnb_db_run (ovn-northd.c:8522)
==9270==    by 0x422B2D: ovn_db_run (ovn-northd.c:9089)
==9270==    by 0x423909: main (ovn-northd.c:9409)
==9270==
==9270== 157 (32 direct, 125 indirect) bytes in 1 blocks are definitely
lost in loss record 199 of 292
==9270==    at 0x4C29BC3: malloc (vg_replace_malloc.c:299)
==9270==    by 0x4D31EF: xmalloc (util.c:138)
==9270==    by 0x471E3D: resize (hmap.c:100)
==9270==    by 0x4720C8: hmap_expand_at (hmap.c:175)
==9270==    by 0x4C74F1: hmap_insert_at (hmap.h:277)
==9270==    by 0x4C825A: smap_add__ (smap.c:392)
==9270==    by 0x4C7783: smap_add (smap.c:55)
==9270==    by 0x451054: sbrec_datapath_binding_parse_external_ids (ovn-sb-idl.c:7181)
==9270==    by 0x4BB12D: ovsdb_idl_txn_write__ (ovsdb-idl.c:4489)
==9270==    by 0x4BB1B5: ovsdb_idl_txn_write (ovsdb-idl.c:4527)
==9270==    by 0x451436: sbrec_datapath_binding_set_external_ids (ovn-sb-idl.c:7444)
==9270==    by 0x4090F1: ovn_datapath_update_external_ids (ovn-northd.c:817)
==9270==

<snip>

==9270==
==9270== LEAK SUMMARY:
==9270==    definitely lost: 1,322 bytes in 47 blocks
==9270==    indirectly lost: 4,653 bytes in 240 blocks
==9270==      possibly lost: 0 bytes in 0 blocks
==9270==    still reachable: 254,004 bytes in 7,780 blocks
==9270==         suppressed: 0 bytes in 0 blocks
==9270== Reachable blocks (those to which a pointer was found) are not
shown.
==9270== To see them, rerun with: --leak-check=full
--show-leak-kinds=all
==9270==
==9270== For counts of detected and suppressed errors, rerun with: -v
==9270== ERROR SUMMARY: 9 errors from 9 contexts (suppressed: 0 from 0)

Signed-off-by: Dumitru Ceara <dceara@redhat.com>
---
 lib/ovsdb-idl.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Ben Pfaff July 18, 2019, 3:52 p.m. UTC | #1
On Wed, Jul 17, 2019 at 09:05:04PM +0200, Dumitru Ceara wrote:
> Once a column is set in a row using ovsdb_idl_txn_write__ we also mark
> the row "parsed". Otherwise the memory allocated by
> sbrec_<table>_parse_<col>() will never be freed. After marking the row
> "parsed", the ovsdb_idl_txn_disassemble function will properly free the
> newly allocated memory for the column (ovsdb_idl_row_unparse).

Wow, good catch.  I bet that's been there forever.

The OVSDB IDL code is too complicated.  It's too hard to spot the
issues.  I wish I saw a way to make it simpler.
Damijan Skvarc July 18, 2019, 4:58 p.m. UTC | #2
Hmm, the problem of "parsed" flag is that it identifies "all" columns of
certain row have been parsed, however there are CLI tools which modify only
individual colums by calling ovsdb_idl_txn_write_() function. In this case
and in case parsed flag would be set in ovsdb_idl_txn_write() function then
unparsing procedure would be called also for columns which were not parsed.
The problem could be overcome by having individual flag for each column.
This has been addresed in pending pull request. Apparent mail has been sent
to dev list, but obviosly has been somehow overlooked.
br, damijan

On Thu, 18 Jul 2019, 17:52 Ben Pfaff, <blp@ovn.org> wrote:

> On Wed, Jul 17, 2019 at 09:05:04PM +0200, Dumitru Ceara wrote:
> > Once a column is set in a row using ovsdb_idl_txn_write__ we also mark
> > the row "parsed". Otherwise the memory allocated by
> > sbrec_<table>_parse_<col>() will never be freed. After marking the row
> > "parsed", the ovsdb_idl_txn_disassemble function will properly free the
> > newly allocated memory for the column (ovsdb_idl_row_unparse).
>
> Wow, good catch.  I bet that's been there forever.
>
> The OVSDB IDL code is too complicated.  It's too hard to spot the
> issues.  I wish I saw a way to make it simpler.
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Ben Pfaff July 18, 2019, 6:12 p.m. UTC | #3
I guess I missed it.  Will you please point it out to me, for example in
the list archive?

On Thu, Jul 18, 2019 at 06:58:34PM +0200, Damijan Skvarc wrote:
> Hmm, the problem of "parsed" flag is that it identifies "all" columns of
> certain row have been parsed, however there are CLI tools which modify only
> individual colums by calling ovsdb_idl_txn_write_() function. In this case
> and in case parsed flag would be set in ovsdb_idl_txn_write() function then
> unparsing procedure would be called also for columns which were not parsed.
> The problem could be overcome by having individual flag for each column.
> This has been addresed in pending pull request. Apparent mail has been sent
> to dev list, but obviosly has been somehow overlooked.
> br, damijan
> 
> On Thu, 18 Jul 2019, 17:52 Ben Pfaff, <blp@ovn.org> wrote:
> 
> > On Wed, Jul 17, 2019 at 09:05:04PM +0200, Dumitru Ceara wrote:
> > > Once a column is set in a row using ovsdb_idl_txn_write__ we also mark
> > > the row "parsed". Otherwise the memory allocated by
> > > sbrec_<table>_parse_<col>() will never be freed. After marking the row
> > > "parsed", the ovsdb_idl_txn_disassemble function will properly free the
> > > newly allocated memory for the column (ovsdb_idl_row_unparse).
> >
> > Wow, good catch.  I bet that's been there forever.
> >
> > The OVSDB IDL code is too complicated.  It's too hard to spot the
> > issues.  I wish I saw a way to make it simpler.
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
Ben Pfaff July 18, 2019, 6:14 p.m. UTC | #4
On Thu, Jul 18, 2019 at 08:52:13AM -0700, Ben Pfaff wrote:
> On Wed, Jul 17, 2019 at 09:05:04PM +0200, Dumitru Ceara wrote:
> > Once a column is set in a row using ovsdb_idl_txn_write__ we also mark
> > the row "parsed". Otherwise the memory allocated by
> > sbrec_<table>_parse_<col>() will never be freed. After marking the row
> > "parsed", the ovsdb_idl_txn_disassemble function will properly free the
> > newly allocated memory for the column (ovsdb_idl_row_unparse).
> 
> Wow, good catch.  I bet that's been there forever.
> 
> The OVSDB IDL code is too complicated.  It's too hard to spot the
> issues.  I wish I saw a way to make it simpler.

Oh, I forgot to say: I applied this to master.
Damijan Skvarc July 19, 2019, 2:50 a.m. UTC | #5
https://mail.openvswitch.org/pipermail/ovs-dev/2019-July/360285.htmlhttps://mail.openvswitch.org/pipermail/ovs-dev/2019-July/360285.html

On Thu, 18 Jul 2019, 20:12 Ben Pfaff, <blp@ovn.org> wrote:

> I guess I missed it.  Will you please point it out to me, for example in
> the list archive?
>
> On Thu, Jul 18, 2019 at 06:58:34PM +0200, Damijan Skvarc wrote:
> > Hmm, the problem of "parsed" flag is that it identifies "all" columns of
> > certain row have been parsed, however there are CLI tools which modify
> only
> > individual colums by calling ovsdb_idl_txn_write_() function. In this
> case
> > and in case parsed flag would be set in ovsdb_idl_txn_write() function
> then
> > unparsing procedure would be called also for columns which were not
> parsed.
> > The problem could be overcome by having individual flag for each column.
> > This has been addresed in pending pull request. Apparent mail has been
> sent
> > to dev list, but obviosly has been somehow overlooked.
> > br, damijan
> >
> > On Thu, 18 Jul 2019, 17:52 Ben Pfaff, <blp@ovn.org> wrote:
> >
> > > On Wed, Jul 17, 2019 at 09:05:04PM +0200, Dumitru Ceara wrote:
> > > > Once a column is set in a row using ovsdb_idl_txn_write__ we also
> mark
> > > > the row "parsed". Otherwise the memory allocated by
> > > > sbrec_<table>_parse_<col>() will never be freed. After marking the
> row
> > > > "parsed", the ovsdb_idl_txn_disassemble function will properly free
> the
> > > > newly allocated memory for the column (ovsdb_idl_row_unparse).
> > >
> > > Wow, good catch.  I bet that's been there forever.
> > >
> > > The OVSDB IDL code is too complicated.  It's too hard to spot the
> > > issues.  I wish I saw a way to make it simpler.
> > > _______________________________________________
> > > dev mailing list
> > > dev@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
>
Dumitru Ceara July 19, 2019, 8:12 a.m. UTC | #6
Hi Damijan, Ben,

Damijan, sorry, I didn't realize you had already reported and fixed
the problem in your pending pull request. I wouldn't have sent my
patch otherwise.

Regarding a single parsed field instead of parsed bits per column I
had the impression that it's ok if unparsing is done for all columns.
Right now, whenever we set a column we call ovsdb_idl_txn_write__()
which unconditionally executes "(column->unparse)(row)" even if there
was no old value set before parsing the new datum. As we zero out rows
when we allocate them I assumed that this unparsing is safe.

What do you guys think?

Thanks,
Dumitru

On Fri, Jul 19, 2019 at 4:50 AM Damijan Skvarc <damjan.skvarc@gmail.com> wrote:
>
> https://mail.openvswitch.org/pipermail/ovs-dev/2019-July/360285.htmlhttps://mail.openvswitch.org/pipermail/ovs-dev/2019-July/360285.html
>
> On Thu, 18 Jul 2019, 20:12 Ben Pfaff, <blp@ovn.org> wrote:
>>
>> I guess I missed it.  Will you please point it out to me, for example in
>> the list archive?
>>
>> On Thu, Jul 18, 2019 at 06:58:34PM +0200, Damijan Skvarc wrote:
>> > Hmm, the problem of "parsed" flag is that it identifies "all" columns of
>> > certain row have been parsed, however there are CLI tools which modify only
>> > individual colums by calling ovsdb_idl_txn_write_() function. In this case
>> > and in case parsed flag would be set in ovsdb_idl_txn_write() function then
>> > unparsing procedure would be called also for columns which were not parsed.
>> > The problem could be overcome by having individual flag for each column.
>> > This has been addresed in pending pull request. Apparent mail has been sent
>> > to dev list, but obviosly has been somehow overlooked.
>> > br, damijan
>> >
>> > On Thu, 18 Jul 2019, 17:52 Ben Pfaff, <blp@ovn.org> wrote:
>> >
>> > > On Wed, Jul 17, 2019 at 09:05:04PM +0200, Dumitru Ceara wrote:
>> > > > Once a column is set in a row using ovsdb_idl_txn_write__ we also mark
>> > > > the row "parsed". Otherwise the memory allocated by
>> > > > sbrec_<table>_parse_<col>() will never be freed. After marking the row
>> > > > "parsed", the ovsdb_idl_txn_disassemble function will properly free the
>> > > > newly allocated memory for the column (ovsdb_idl_row_unparse).
>> > >
>> > > Wow, good catch.  I bet that's been there forever.
>> > >
>> > > The OVSDB IDL code is too complicated.  It's too hard to spot the
>> > > issues.  I wish I saw a way to make it simpler.
>> > > _______________________________________________
>> > > dev mailing list
>> > > dev@openvswitch.org
>> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> > >
Damijan Skvarc July 22, 2019, 7:58 a.m. UTC | #7
Hi Dumitru,

The problem is that ovsdb_idl entity is part of library and can be used by
different application, where each application
instantiates its own parse/unparse callback functions. Library itself does
not know how these parse/unparse functions are
implemented thus it is not reliable to call unparse() function without
calling  apparent parse() function first. This case
happens in case the application modifies one column, while common parse
flag insinuates unparse()
function of some another column is called.  The most problematic case are
parse/unparse pairs where parse()
function allocates memory and its unparse() counterpart deallocates it.
Common parse flag would in this case cause some
memory would be freed despite it has not been allocated or even worse, some
memory could be deallocated multiple times.
I strongly believe this would lead soon or later to the application crash.

However, since I am just a self-invited visitor on this project (just to
gain/discover some practices on github platform) you should
not rely on my opinion. Ben looks to be a moderator/architect of this
project and he should advice how to precede with this issue.

regards,
Damijan


On Fri, Jul 19, 2019 at 10:12 AM Dumitru Ceara <dceara@redhat.com> wrote:

> Hi Damijan, Ben,
>
> Damijan, sorry, I didn't realize you had already reported and fixed
> the problem in your pending pull request. I wouldn't have sent my
> patch otherwise.
>
> Regarding a single parsed field instead of parsed bits per column I
> had the impression that it's ok if unparsing is done for all columns.
> Right now, whenever we set a column we call ovsdb_idl_txn_write__()
> which unconditionally executes "(column->unparse)(row)" even if there
> was no old value set before parsing the new datum. As we zero out rows
> when we allocate them I assumed that this unparsing is safe.
>
> What do you guys think?
>
> Thanks,
> Dumitru
>
> On Fri, Jul 19, 2019 at 4:50 AM Damijan Skvarc <damjan.skvarc@gmail.com>
> wrote:
> >
> >
> https://mail.openvswitch.org/pipermail/ovs-dev/2019-July/360285.htmlhttps://mail.openvswitch.org/pipermail/ovs-dev/2019-July/360285.html
> >
> > On Thu, 18 Jul 2019, 20:12 Ben Pfaff, <blp@ovn.org> wrote:
> >>
> >> I guess I missed it.  Will you please point it out to me, for example in
> >> the list archive?
> >>
> >> On Thu, Jul 18, 2019 at 06:58:34PM +0200, Damijan Skvarc wrote:
> >> > Hmm, the problem of "parsed" flag is that it identifies "all" columns
> of
> >> > certain row have been parsed, however there are CLI tools which
> modify only
> >> > individual colums by calling ovsdb_idl_txn_write_() function. In this
> case
> >> > and in case parsed flag would be set in ovsdb_idl_txn_write()
> function then
> >> > unparsing procedure would be called also for columns which were not
> parsed.
> >> > The problem could be overcome by having individual flag for each
> column.
> >> > This has been addresed in pending pull request. Apparent mail has
> been sent
> >> > to dev list, but obviosly has been somehow overlooked.
> >> > br, damijan
> >> >
> >> > On Thu, 18 Jul 2019, 17:52 Ben Pfaff, <blp@ovn.org> wrote:
> >> >
> >> > > On Wed, Jul 17, 2019 at 09:05:04PM +0200, Dumitru Ceara wrote:
> >> > > > Once a column is set in a row using ovsdb_idl_txn_write__ we also
> mark
> >> > > > the row "parsed". Otherwise the memory allocated by
> >> > > > sbrec_<table>_parse_<col>() will never be freed. After marking
> the row
> >> > > > "parsed", the ovsdb_idl_txn_disassemble function will properly
> free the
> >> > > > newly allocated memory for the column (ovsdb_idl_row_unparse).
> >> > >
> >> > > Wow, good catch.  I bet that's been there forever.
> >> > >
> >> > > The OVSDB IDL code is too complicated.  It's too hard to spot the
> >> > > issues.  I wish I saw a way to make it simpler.
> >> > > _______________________________________________
> >> > > dev mailing list
> >> > > dev@openvswitch.org
> >> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >> > >
>
Dumitru Ceara July 29, 2019, 8:52 a.m. UTC | #8
Hi Ben,

If we keep this fix as is can we please have it backported all the way
to and including branch-2.10? Right now it's there in master and
branch 2.12.

ovn-nbctl daemon mode is available since branch-2.10 and might be
affected by this leak as it creates new entries in the database.

Thanks,
Dumitru

On Mon, Jul 22, 2019 at 9:58 AM Damijan Skvarc <damjan.skvarc@gmail.com> wrote:
>
> Hi Dumitru,
>
> The problem is that ovsdb_idl entity is part of library and can be used by different application, where each application
> instantiates its own parse/unparse callback functions. Library itself does not know how these parse/unparse functions are
> implemented thus it is not reliable to call unparse() function without calling  apparent parse() function first. This case
> happens in case the application modifies one column, while common parse flag insinuates unparse()
> function of some another column is called.  The most problematic case are parse/unparse pairs where parse()
> function allocates memory and its unparse() counterpart deallocates it. Common parse flag would in this case cause some
> memory would be freed despite it has not been allocated or even worse, some memory could be deallocated multiple times.
> I strongly believe this would lead soon or later to the application crash.
>
> However, since I am just a self-invited visitor on this project (just to gain/discover some practices on github platform) you should
> not rely on my opinion. Ben looks to be a moderator/architect of this project and he should advice how to precede with this issue.
>
> regards,
> Damijan
>
>
> On Fri, Jul 19, 2019 at 10:12 AM Dumitru Ceara <dceara@redhat.com> wrote:
>>
>> Hi Damijan, Ben,
>>
>> Damijan, sorry, I didn't realize you had already reported and fixed
>> the problem in your pending pull request. I wouldn't have sent my
>> patch otherwise.
>>
>> Regarding a single parsed field instead of parsed bits per column I
>> had the impression that it's ok if unparsing is done for all columns.
>> Right now, whenever we set a column we call ovsdb_idl_txn_write__()
>> which unconditionally executes "(column->unparse)(row)" even if there
>> was no old value set before parsing the new datum. As we zero out rows
>> when we allocate them I assumed that this unparsing is safe.
>>
>> What do you guys think?
>>
>> Thanks,
>> Dumitru
>>
>> On Fri, Jul 19, 2019 at 4:50 AM Damijan Skvarc <damjan.skvarc@gmail.com> wrote:
>> >
>> > https://mail.openvswitch.org/pipermail/ovs-dev/2019-July/360285.htmlhttps://mail.openvswitch.org/pipermail/ovs-dev/2019-July/360285.html
>> >
>> > On Thu, 18 Jul 2019, 20:12 Ben Pfaff, <blp@ovn.org> wrote:
>> >>
>> >> I guess I missed it.  Will you please point it out to me, for example in
>> >> the list archive?
>> >>
>> >> On Thu, Jul 18, 2019 at 06:58:34PM +0200, Damijan Skvarc wrote:
>> >> > Hmm, the problem of "parsed" flag is that it identifies "all" columns of
>> >> > certain row have been parsed, however there are CLI tools which modify only
>> >> > individual colums by calling ovsdb_idl_txn_write_() function. In this case
>> >> > and in case parsed flag would be set in ovsdb_idl_txn_write() function then
>> >> > unparsing procedure would be called also for columns which were not parsed.
>> >> > The problem could be overcome by having individual flag for each column.
>> >> > This has been addresed in pending pull request. Apparent mail has been sent
>> >> > to dev list, but obviosly has been somehow overlooked.
>> >> > br, damijan
>> >> >
>> >> > On Thu, 18 Jul 2019, 17:52 Ben Pfaff, <blp@ovn.org> wrote:
>> >> >
>> >> > > On Wed, Jul 17, 2019 at 09:05:04PM +0200, Dumitru Ceara wrote:
>> >> > > > Once a column is set in a row using ovsdb_idl_txn_write__ we also mark
>> >> > > > the row "parsed". Otherwise the memory allocated by
>> >> > > > sbrec_<table>_parse_<col>() will never be freed. After marking the row
>> >> > > > "parsed", the ovsdb_idl_txn_disassemble function will properly free the
>> >> > > > newly allocated memory for the column (ovsdb_idl_row_unparse).
>> >> > >
>> >> > > Wow, good catch.  I bet that's been there forever.
>> >> > >
>> >> > > The OVSDB IDL code is too complicated.  It's too hard to spot the
>> >> > > issues.  I wish I saw a way to make it simpler.
>> >> > > _______________________________________________
>> >> > > dev mailing list
>> >> > > dev@openvswitch.org
>> >> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> >> > >

Patch
diff mbox series

diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 5c61096..1a6a4af 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -4487,6 +4487,7 @@  ovsdb_idl_txn_write__(const struct ovsdb_idl_row *row_,
     }
     (column->unparse)(row);
     (column->parse)(row, &row->new_datum[column_idx]);
+    row->parsed = true;
     if (!index_row) {
         ovsdb_idl_add_to_indexes(row);
     }