[ovs-dev] [PATCH ovn] logical-fields: fix memory leak caused by initialize ovnfield_by_name twice

[Numan Siddique]

On Fri, Feb 28, 2020 at 4:11 PM Damijan Skvarc <damjan.skvarc at gmail.com> wrote:
>
> Hi Mark
>
> and thanks you made review of suggested patch.
>
> just a few words of myself...
> I don't work professionally on this project, I started to look into this
> code in my free hours just to satisfy my curiosity about how other people
> work. I am following a couple of other projects just to learn about
> different processes, tools and to keep my brains in good condition while
> studying some others work. And if you get some things/knowledge it is also
> kind to give back something. In case of ovs/ovn I found a nice suite of
> automatic tests,. Unfortunately, by running them under valgrind control the
> apparent reports become useless because of flood of memory leaks. Mostly,
> these are small non-important leaks, however in a flood of duplicated
> reports you can miss some serious issue. And here I found my personal goal
> what I can give back. Unfortunately, I don't have  much knowledge about
> networking, nor about original design. And if you don't have that knowledge
> then from the experience I know it is better not to touch anything for what
> you don't know why it has been designed for. So every my patch is done in a
> way that nothing has been changed in a design, just memory leak in valgrind
> reports disappears.
>
>
> On Thu, Feb 27, 2020 at 10:24 PM Mark Michelson <mmichels at redhat.com> wrote:
>
> > Hi Damijan,
> >
> > Thanks for finding the memory leak and patching it. To me,
> > ovnfield_by_name is a really strangely handled structure. There is an
> > explicit function to destroy the ovnfield_by_name structure, but its
> > creation is a side effect from ovn_symtab_init. The result is that
> > lflow.c calls ovn_symtab_init() and ovn_destroy_ovnfields(). But ofctrl
> > only calls ovn_symtab_init(). It doesn't call ovn_destroy_ovnfields()
> > because it would result in double-freeing ovnfield_by_name.
> >
> >
> yes, I noticed that asymmetry.
>
> This lack of symmetry shows a poor design.
> >
> Your patch fixes the memory
> > leak, but it doesn't fix the lack of symmetry, and it still allows for
> > potential double-freeing.
>
> I have a couple of suggestions for a better design:
> >
> > 1) Instead of calling ovn_symbtab_init() in multiple places, call it
> > once in ovn_controller's main() function. Then when ovn_controller
> > exits, call ovn_destroy_ovnfields(). Then, pass the created symtab as a
> > parameter when it is necessary to. This works because the symtabs
> > created by lflow.c and ofctrl.c are identical, and their use of the
> > symtabs occurs in the same thread.
> >
> > 2) Initialize ovnfield_by_name in a separate function from
> > ovn_symtab_init(). You can call this new ovnfield_by_name_init()
> > function in ovn-controller's main function and then call the already
> > existing ovn_destroy_ovnfields() function when ovn-controller exits.
> > This works because ovnfield_by_name does not rely on any data from the
> > created symtab. It exists completely independently.
> >
> > What do you think about this?
> >
>
> What is strange also is that (If I see correctly) the ovnfield_by_name is a
> hash of one single entry.
> I wouldn't use hash at all in this case. But since I don't know about the
> design, probably some entries
> were deleted in the past and only one remained or probably there is a plan
> some more entries will be
> added in the future. So let's assume we should not get rid of
> ovnfield_by_name hash.
>

This is the commit which added 'ovnfield_by_name' [1]. I only added one field
at the time. The commit message has more details. I think when working on it
I missed out on the fact that ofctrl.c also calls ovn_init_symtab().

I think we can address this issue by having a new function -
ovn_init_ovnfields()
which lflow.c would call. ovn_destroy_ovnfields() is called by lflow.c anyway.

What do you think about this ?


[1] - https://github.com/openvswitch/ovs/commit/086470cdbe66522a1cfff96eb68073e4176684be
Thank

Numan



> I found very similar functionality in meta_flow.c file (ovs). There
> mf_by_name hash is more rational, it is filled with
> plenty of strings. Also initialization of this hash is more rational. It is
> initialized only if it is needed (initialization is hidden
> in mf_from_name() and mf_from_name_len() functions and using the same
> "pthread_once" approach as in my case). What is  strange there is that there
> is no destroy action for mf_by_name and valgrind does not report any leak
> there. Probably test suite does not cover this functionality.
>
> I believe some more alternatives are available, however to reduce the
> number of changes I would vote for
> - automatic and optional initialization similar as in meta_flow case
> - to solve asymmetry issue I would propose atexit() approach, similar as it
> is implemented in ovs/lib/stopwatch.c
>
> btw, I didn't understand your first proposal. ovn_symbtab_init() function
> need to be called twice since two different
> symtab entities are initialized. Is this probably also a design issue that
> we could live with single symtab?
>
> thanks,
> damijan
>
> >
> >
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>