On Thu, Jan 12, 2017 at 5:20 PM, Tobias Klauser <tklau...@distanz.ch> wrote:
> On 2017-01-12 at 15:54:31 +0100, Vadim Kochan <vadi...@gmail.com> wrote:
>> On Thu, Jan 12, 2017 at 4:28 PM, Tobias Klauser <tklau...@distanz.ch> wrote:
> [...]
>> >> enum flow_direction {
>> >> @@ -355,15 +357,15 @@ static inline struct flow_entry
>> >> *flow_entry_xalloc(void)
>> >> static inline void flow_entry_xfree(struct flow_entry *n)
>> >> {
>> >> if (n->ct)
>> >> - nfct_destroy(n->ct);
>> >> + xfree(n->ct);
>> >
>> > This would leak memory allocated internally in struct nf_contrack, no?
>> > What's the reason for this change?
>>
>> nfct_destroy(ct) may fail if we free entry after nfct event processing
>> (our free is defered now because of RCU),
>> so using just free(x) is safer because we do just nfct_clone(ct) which
>> just allocated another nfct_conntrack entry for us,
>> but ofcourse it would be better to have something nfct_free(x), so I
>> agree this is not nice but safer.
>
> As long as it causes memory to be leaked IMO it is equally bad.
>
> If flow_entry_xfree() and thus nfct_destroy() is called via the
> call_rcu() wrapper I proposed, it should no longer be unsafe to call
> nfct_destroy()
I send separate v2 (I already send new version but w/o v2 prefix a changelog).
there should be no any leak as now nfct_conntrack will be not freed by
nfct API because I return NFCT_CB_STOLEN,
and it will be freed (destroyed) in defered rcu call.
I checked via valgrind that there is really memleak but after lookup
geoip record which is allocated by
libGeoIP but is not freed by flowtop. But it needs to be fixed in
separate patch.
--
You received this message because you are subscribed to the Google Groups
"netsniff-ng" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to netsniff-ng+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
