Re: [B.A.T.M.A.N.] [PATCH-maint] batman-adv: fix lockdep splat when doing mcast_free

[Sven Eckelmann <sven@narfation.org>]

[-- Attachment #1: Type: text/plain, Size: 2560 bytes --]

On Saturday 19 December 2015 19:09:02 Antonio Quartulli wrote:
> On 19/12/15 17:32, Sven Eckelmann wrote:
> > On Saturday 19 December 2015 09:04:05 Linus Lüssing wrote:
> >>> From: Simon Wunderlich <simon@open-mesh.com>
> >>> 
> >>> While testing, we got something like this:
> >>> 
> >>> WARNING: CPU: 0 PID: 238 at net/batman-adv/multicast.c:142
> >>> batadv_mcast_mla_tt_retract+0x94/0x205 [batman_adv]() [...]
> >>> Call Trace:
> >>> [<ffffffff815fc597>] dump_stack+0x4b/0x64
> >>> [<ffffffff810b34dc>] warn_slowpath_common+0xbc/0x120
> >>> [<ffffffffa0024ec5>] ? batadv_mcast_mla_tt_retract+0x94/0x205
> >>> [batman_adv]
> >>> [<ffffffff810b3705>] warn_slowpath_null+0x15/0x20
> >>> [<ffffffffa0024ec5>] batadv_mcast_mla_tt_retract+0x94/0x205 [batman_adv]
> >>> [<ffffffffa00273fe>] batadv_mcast_free+0x36/0x39 [batman_adv]
> >>> [<ffffffffa0020c77>] batadv_mesh_free+0x7d/0x13f [batman_adv]
> >>> [<ffffffffa0036a6b>] batadv_softif_free+0x15/0x25 [batman_adv]
> >>> [...]
> >>> 
> >>> Signed-off-by: Simon Wunderlich <simon@open-mesh.com>
> >> 
> >> Fixes: 5b95c427d187 ("batman-adv: Annotate deleting functions with
> >> external lock via lockdep") Acked-by: Linus Lüssing
> >> <linus.luessing@c0d3.blue>
> > 
> > I am confused why this should fix a commit which is there to find such
> > problems.
> Am I wrong or also the commit message is misleading? Looks like it is
> "fixing" a bogus lockdep check...but as far as I can say this patch is
> making sure that the lock is held in that particular point.

No, it doesn't say this anywhere. It says that it fixes the "lockdep splat"

> If we want to "fix the lockdep" then we should remove the
> lockdep_assert_held(), but I don't think this is the goal of the whole
> discussion.

No, the lockdep check should not be removed because a lock is necessary for 
other situations when this list is modified as explained earlier [1]. You have 
multiple concurrent calling contexts for this function (hard_if_event with 
something like mtu update + something like batadv_iv_ogm_schedule) which 
require exclusive access to this list. Right now it is just protected by a 
weird "named" lock (from the translation table and not from the mcast).

And no, the solution here is not to add a special codepath which (for no sane 
reason) requires no locks or synchronized frees - even when this seemed to 
have become the new trend in the batman-adv codebase.

Kind regards,
	Sven

[1] https://lists.open-mesh.org/pipermail/b.a.t.m.a.n/2015-December/013876.html

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]