* [B.A.T.M.A.N.] [PATCH maint] batman-adv: fix potential TT client + orig-node memory leak
@ 2014-12-13 22:32 Linus Lüssing
2014-12-29 3:52 ` Marek Lindner
0 siblings, 1 reply; 6+ messages in thread
From: Linus Lüssing @ 2014-12-13 22:32 UTC (permalink / raw)
To: b.a.t.m.a.n
This patch fixes a potential memory leak which can occur once an
originator times out. On timeout the according global translation table
entry might not get purged correctly. Furthermore, the non purged TT
entry will cause its orig-node to leak, too. Which additionally can lead
to the new multicast optimization feature not kicking in because of a
therefore bogus counter.
In the wild with larger mesh networks we saw this leak quite regularly,
resulting in routers to reboot or killed processes. This was because
of a combination of two bugs: The bug fixed by commit
"batman-adv: fix delayed foreign originator recognition" (8a2ad5204674)
amplified this memory leak heavily. Since that commit I'd expect
it to happen rarely, probably only in paused and resumed VMs and
devices previously in stand-by.
The issue this patch fixes is caused by batadv_orig_node_free_rcu()
never being called because of not yet released references to the
orig-node. References which were supposed to be released through
batadv_orig_node_free_rcu()->batadv_tt_global_del_orig().
Fixing the issue by moving batadv_tt_global_del_orig() out of the rcu
callback.
Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
---
originator.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/originator.c b/originator.c
index 648bdba..bea8198 100644
--- a/originator.c
+++ b/originator.c
@@ -570,9 +570,6 @@ static void batadv_orig_node_free_rcu(struct rcu_head *rcu)
batadv_frag_purge_orig(orig_node, NULL);
- batadv_tt_global_del_orig(orig_node->bat_priv, orig_node, -1,
- "originator timed out");
-
if (orig_node->bat_priv->bat_algo_ops->bat_orig_free)
orig_node->bat_priv->bat_algo_ops->bat_orig_free(orig_node);
@@ -978,6 +975,9 @@ static void _batadv_purge_orig(struct batadv_priv *bat_priv)
if (batadv_purge_orig_node(bat_priv, orig_node)) {
batadv_gw_node_delete(bat_priv, orig_node);
hlist_del_rcu(&orig_node->hash_entry);
+ batadv_tt_global_del_orig(orig_node->bat_priv,
+ orig_node, -1,
+ "originator timed out");
batadv_orig_node_free_ref(orig_node);
continue;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [B.A.T.M.A.N.] [PATCH maint] batman-adv: fix potential TT client + orig-node memory leak
2014-12-13 22:32 [B.A.T.M.A.N.] [PATCH maint] batman-adv: fix potential TT client + orig-node memory leak Linus Lüssing
@ 2014-12-29 3:52 ` Marek Lindner
2014-12-29 14:32 ` Linus Lüssing
2015-01-04 16:05 ` Antonio Quartulli
0 siblings, 2 replies; 6+ messages in thread
From: Marek Lindner @ 2014-12-29 3:52 UTC (permalink / raw)
To: b.a.t.m.a.n
[-- Attachment #1: Type: text/plain, Size: 1806 bytes --]
On Saturday 13 December 2014 23:32:15 Linus Lüssing wrote:
> This patch fixes a potential memory leak which can occur once an
> originator times out. On timeout the according global translation table
> entry might not get purged correctly. Furthermore, the non purged TT
> entry will cause its orig-node to leak, too. Which additionally can lead
> to the new multicast optimization feature not kicking in because of a
> therefore bogus counter.
So far, I am with you ..
> In the wild with larger mesh networks we saw this leak quite regularly,
> resulting in routers to reboot or killed processes. This was because
> of a combination of two bugs: The bug fixed by commit
> "batman-adv: fix delayed foreign originator recognition" (8a2ad5204674)
> amplified this memory leak heavily. Since that commit I'd expect
> it to happen rarely, probably only in paused and resumed VMs and
> devices previously in stand-by.
This section shouldn't be part of the official commit message. It is hardly
relevant to the reviewer how often a memleak occurs and whether or not you
need a VM to trigger it. The provided commit id isn't valid in the Linux tree.
> The issue this patch fixes is caused by batadv_orig_node_free_rcu()
> never being called because of not yet released references to the
> orig-node. References which were supposed to be released through
> batadv_orig_node_free_rcu()->batadv_tt_global_del_orig().
Could you please provide addition insight as to which references are still
held ? I did look around but nothing obvious jumped at me.
Generally, it wouldn't be bad if the commit message went into deeper detail
describing the nature of the bug instead of the middle section above to make
it easy to understand what is being fixed.
Cheers,
Marek
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH maint] batman-adv: fix potential TT client + orig-node memory leak
2014-12-29 3:52 ` Marek Lindner
@ 2014-12-29 14:32 ` Linus Lüssing
2015-01-04 16:05 ` Antonio Quartulli
1 sibling, 0 replies; 6+ messages in thread
From: Linus Lüssing @ 2014-12-29 14:32 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
On Mon, Dec 29, 2014 at 11:52:53AM +0800, Marek Lindner wrote:
> > The issue this patch fixes is caused by batadv_orig_node_free_rcu()
> > never being called because of not yet released references to the
> > orig-node. References which were supposed to be released through
> > batadv_orig_node_free_rcu()->batadv_tt_global_del_orig().
>
> Could you please provide addition insight as to which references are still
> held ? I did look around but nothing obvious jumped at me.
The batadv_tt_global_entry->orig_list holds the reference to the
orig-node. Usually this reference is released after
BATADV_PURGE_TIMEOUT through: _batadv_purge_orig()->
batadv_purge_orig_node()->batadv_update_route()->_batadv_update_route()->
batadv_tt_global_del_orig() which purges this global tt entry and
releases the reference to the orig-node.
However, if between two batadv_purge_orig_node() calls the orig-node
timeout grew to 2*BATADV_PURGE_TIMEOUT then this call path isn't
reached (*). Instead the according orig-node is removed from the
originator hash in _batadv_purge_orig(), the batadv_update_route()
part is skipped and won't be reached anymore.
It seems that in that case batadv_orig_node_free_rcu()->batadv_tt_global_del_orig()
is supposed to purge the global tt entry and to release the orig-node
reference but it's not called because batadv_orig_node_free_rcu() is
only called once all references are freed: A chicken 'n' egg
situation.
>
> Generally, it wouldn't be bad if the commit message went into deeper detail
> describing the nature of the bug instead of the middle section above to make
> it easy to understand what is being fixed.
Hm, hm, my intention was to somehow/somewhere document how these
two, small and rare bugs together created this severe bug.
The issue fixed by 8a2ad5204674 was the main trigger for (*) in
previous releases.
But we can also skip that middle section if you want. Then I'll
just add a note to the ticket on redmine.
>
>
> Cheers,
> Marek
Cheers, Linus
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH maint] batman-adv: fix potential TT client + orig-node memory leak
2014-12-29 3:52 ` Marek Lindner
2014-12-29 14:32 ` Linus Lüssing
@ 2015-01-04 16:05 ` Antonio Quartulli
2015-01-04 16:11 ` Antonio Quartulli
1 sibling, 1 reply; 6+ messages in thread
From: Antonio Quartulli @ 2015-01-04 16:05 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
[-- Attachment #1: Type: text/plain, Size: 2330 bytes --]
On 29/12/14 04:52, Marek Lindner wrote:
> On Saturday 13 December 2014 23:32:15 Linus Lüssing wrote:
>> This patch fixes a potential memory leak which can occur once an
>> originator times out. On timeout the according global translation table
>> entry might not get purged correctly. Furthermore, the non purged TT
>> entry will cause its orig-node to leak, too. Which additionally can lead
>> to the new multicast optimization feature not kicking in because of a
>> therefore bogus counter.
>
> So far, I am with you ..
>
>
>> In the wild with larger mesh networks we saw this leak quite regularly,
>> resulting in routers to reboot or killed processes. This was because
>> of a combination of two bugs: The bug fixed by commit
>> "batman-adv: fix delayed foreign originator recognition" (8a2ad5204674)
>> amplified this memory leak heavily. Since that commit I'd expect
>> it to happen rarely, probably only in paused and resumed VMs and
>> devices previously in stand-by.
>
> This section shouldn't be part of the official commit message. It is hardly
> relevant to the reviewer how often a memleak occurs and whether or not you
> need a VM to trigger it. The provided commit id isn't valid in the Linux tree.
>
>
>> The issue this patch fixes is caused by batadv_orig_node_free_rcu()
>> never being called because of not yet released references to the
>> orig-node. References which were supposed to be released through
>> batadv_orig_node_free_rcu()->batadv_tt_global_del_orig().
>
> Could you please provide addition insight as to which references are still
> held ? I did look around but nothing obvious jumped at me.
>
> Generally, it wouldn't be bad if the commit message went into deeper detail
> describing the nature of the bug instead of the middle section above to make
> it easy to understand what is being fixed.
>
Hi Linus and thanks for fixing this TT bug!
I agree with Marek about extending the commit message with a better
explanation of the actual bug, but at the same time I think it is good
to keep the commit message and ID of the other involved patches.
Moreover, please keep the commit ID of our batman-adv.git tree - I'll
then take care of converting them when sending the patches upstream.
Cheers,
--
Antonio Quartulli
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-01-05 17:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-12-13 22:32 [B.A.T.M.A.N.] [PATCH maint] batman-adv: fix potential TT client + orig-node memory leak Linus Lüssing
2014-12-29 3:52 ` Marek Lindner
2014-12-29 14:32 ` Linus Lüssing
2015-01-04 16:05 ` Antonio Quartulli
2015-01-04 16:11 ` Antonio Quartulli
2015-01-05 17:22 ` Marek Lindner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox