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

[Sven Eckelmann <sven@narfation.org>]

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

On Saturday 28 November 2015 10:49:59 Antonio Quartulli wrote:
> Simon Wunderlich:
> > From: Simon Wunderlich <simon@open-mesh.com>
> > 
> > Signed-off-by: Simon Wunderlich <simon@open-mesh.com>
> > ---
> > 
> >  net/batman-adv/multicast.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/net/batman-adv/multicast.c b/net/batman-adv/multicast.c
> > index 8abf488..d984eee 100644
> > --- a/net/batman-adv/multicast.c
> > +++ b/net/batman-adv/multicast.c
> > @@ -801,7 +801,9 @@ void batadv_mcast_free(struct batadv_priv *bat_priv)
> > 
> >  	batadv_tvlv_container_unregister(bat_priv, BATADV_TVLV_MCAST, 1);
> >  	batadv_tvlv_handler_unregister(bat_priv, BATADV_TVLV_MCAST, 1);
> > 
> > +	spin_lock_bh(&bat_priv->tt.commit_lock);
> > 
> >  	batadv_mcast_mla_tt_retract(bat_priv, NULL);
> > 
> > +	spin_unlock_bh(&bat_priv->tt.commit_lock);
> 
> Linus,
> 
> can you please comment as of why batadv_mcast_mla_tt_retract() requires
> to hold the tt.commit_lock ?

Maybe the relevant parts of the Oops can be added to the commit message:

    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]
     [...]

> I see it calls batadv_tt_local_remove() but this does not really
> requires the lock. Maybe you wanted to perform *all* the removes before
> TT could do a commit ? Or is there any other reason?
> 
> Cheers,

mcast.mla_list is protected by tt.commit_lock (see batadv_mcast_mla_tt_add,
batadv_mcast_mla_list_free and batadv_mcast_mla_tt_retract).

Kind regards,
	Sven

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