Open-Mesh » batman-adv

I'm wondering if this was related to eth2 flapping up/down, which was added to batman using batctl if add .

Another idea: Could you enable full batman-adv logging and provide the last 10-20 lines ? Might give us a clue where to look.

There are two embedded devices connected using wire both running some linux firmware; the flashed firmware connects using wire or managed wifi and then fetches a stage2 firmware image to be booted using kexec in ram. The stage2 then runs batman-adv on wire and ibss mesh in parallel to hostapd. WiFi-Clients get bridged into a vpn, that runs over wire or mesh. During kexec, the wired link will go down and back up; additionally, the stage2 of one device rebootet every 20min or so using some script (device was not permitted to connect with vpn).

I'll try to get the logs.

I did not make a big review but it seems that the function is missing the locking. This must not be related with your problem but it not the worst place to report this problem.

    /* if this client has been added right now, it is possible to
     * immediately purge it
     */
    batadv_tt_local_event(bat_priv, tt_local_entry, BATADV_TT_CLIENT_DEL);
    hlist_del_rcu(&tt_local_entry->common.hash_entry);
    batadv_tt_local_entry_free_ref(tt_local_entry);

hlist_del_rcu is writing to the list but is not taking any lock... this is wrong. The function has to use the hash bucket spinlock of this element before it can delete it. But it first has to make sure that this is actually in the list (in the same correct locking context). RCU will not help here because it doesn't provide any consistency between these unrelated things. Only because it was found earlier, it is not required to be in the a valid object in the any list.

Same (hlist_del_rcu) for things like

static void
batadv_tt_global_del_orig_entry(struct batadv_tt_global_entry *tt_global_entry,
                struct batadv_tt_orig_list_entry *orig_entry)
{
    batadv_tt_global_size_dec(orig_entry->orig_node,
                  tt_global_entry->common.vid);
    atomic_dec(&tt_global_entry->orig_list_count);
    hlist_del_rcu(&orig_entry->list);
    batadv_tt_orig_list_entry_free_ref(orig_entry);
}

Multiple instances searching and deleting on the same list == bad idea. RCU doesn't help here. See https://git.open-mesh.org/linux-merge.git/blob/2be28ed88835ece650678742be1366fcd445bf3b:/Documentation/RCU/whatisRCU.txt#l702

There are many more (mcast, tvlv_handler, ...) were only the delete (hlist_del_rcu) is locked but not the search for the item or the check if it still in the list. This can also lead to such poisoned list pointer page faults as seen in this ticket

Btw. if it is really needed to do the search + delete in two different locking contexts then you should at least read about the difference of list_del_rcu + hlist_del_init_rcu. But the lock for the list when doing the check if it is in the list (done implicit by hlist_del_init_rcu) in the lock for the hash bucket list where this object would have been expected.

https://git.open-mesh.org/linux-merge.git/blob/2be28ed88835ece650678742be1366fcd445bf3b:/include/linux/rculist.h#l105

Interesting side note:

You are seeing list poison1 https://git.open-mesh.org/linux-merge.git/blob/2be28ed88835ece650678742be1366fcd445bf3b:/include/linux/poison.h#l22 + offset of 4 (maybe to hlist_node::pprev). But I cannot see right now where POISON1 is used in the rcu hlist stuff. POISON2 seems to be used everywhere for pprev. Next is never touched to make forward iterating always possible using the hlist_foreach..._rcu variants.

list_del and hlist_del should only use LIST_POISON1 (when CONFIG_DEBUG_LIST is enabled) and not the _rcu variants. So, it might be something trying to access a pointer of list_head after the next/prev item was deleted via (h)list_del. But this is just wild guessing. The source code + binary objects (*.ko + *.o) of this build could be helpful to analyze this further.

And another hint for Antonio about possible problems in his TT code: batadv_tt_update_orig also never makes sure that tt_req_node is still in a list before calling list_del. It might already have been deleted by batadv_tt_local_event or some other call to list_del on that object.

Sven Eckelmann wrote:

Same (hlist_del_rcu) for things like

static void
batadv_tt_global_del_orig_entry(struct batadv_tt_global_entry *tt_global_entry,
                struct batadv_tt_orig_list_entry *orig_entry)
{
    batadv_tt_global_size_dec(orig_entry->orig_node,
                  tt_global_entry->common.vid);
    atomic_dec(&tt_global_entry->orig_list_count);
    hlist_del_rcu(&orig_entry->list);
    batadv_tt_orig_list_entry_free_ref(orig_entry);
}

@Sven: While following up on your comment I noticed that this function might look similar but does not fall into the same category. The 2 existing callers (batadv_tt_global_del_orig_list() and batadv_tt_global_del_orig_node()) both hold the appropriate locks (tt_global_entry->list_lock) and cycle through the entire list using the hlist_for_each_entry_safe() macro. Looks correct to me - do you agree ?

I could prepare a patch to make this a little more obvious by adding additional kernel doc to batadv_tt_global_del_orig_entry() and maybe adding an underscore to the batadv_tt_global_del_orig_entry() function name ?

Thanks for the more detail check of the code. You are right about batadv_tt_global_del_orig_entry

And a small correction: I didn't mean batadv_tt_update_orig. The function I wanted to point out with a similar problem is batadv_send_tt_request.

Another weird looking function is batadv_tt_global_size_mod

Sven Eckelmann wrote:

And a small correction: I didn't mean batadv_tt_update_orig. The function I wanted to point out with a similar problem is batadv_send_tt_request.

You were right about this section. I already sent a fix to the ml.

Another weird looking function is batadv_tt_global_size_mod

It looks weird but also seems ok. Here the snippet in question:

    if (atomic_add_return(v, &vlan->tt.num_entries) == 0) {
        spin_lock_bh(&orig_node->vlan_list_lock);
        list_del_rcu(&vlan->list);
        spin_unlock_bh(&orig_node->vlan_list_lock);
        batadv_orig_node_vlan_free_ref(vlan);
    }

The tt.num_entries is a counter for the list itself. Only if that counter reaches zero the corresponding list element is removed. I don't quite see how a double delete can be possible. What do you think ?

batadv_tt_local_size_mod is doing the add without any conditions:

static void batadv_tt_local_size_mod(struct batadv_priv *bat_priv,
                     unsigned short vid, int v)
{
    struct batadv_softif_vlan *vlan;

    vlan = batadv_softif_vlan_get(bat_priv, vid);
    if (!vlan)
        return;

    atomic_add(v, &vlan->tt.num_entries);

    batadv_softif_vlan_free_ref(vlan);
}

So it can happen in theory that it gets 0 and then it gets back to 1 -> the thing can be called again.

I've just received new devices to test with and new firmware with patches applied is compiling. Looking forward into testing.

I've applied

0001-batman-adv-avoid-DAT-to-mess-up-LAN-state.patch
0001-batman-adv-lock-crc-access-in-bridge-loop-avoidance.patch
0001-batman-adv-protect-tt_local_entry-from-concurrent-de.patch
0002-batman-adv-DEBUG-track-CRC-changes.patch

from issue #216 and #217 and am still seeing this kernel crash.

Could you please try running the latest release (2016.0) as it contains several bugfixes which might be related to this bug.

Thanks, I will try to. But it might take some time.

The newest release 2016.1 contains more fixes which could be related to this bug

Btw. if you are using OpenWrt with 050-backport_netfilter_rtcache.patch or 120-bridge_allow_receiption_on_disabled_port.patch then please delete these patches and rebuild your images.

Thanks, I will try to. But it might take some time.

Maybe waiting for v2016.2 is also not a bad idea. At least I hope to get following patch (or a variant of it) merged for this release: https://patchwork.open-mesh.org/project/b.a.t.m.a.n./patch/1464588694-19855-1-git-send-email-sven@narfation.org/

It tackles a weird memory corruption problem. A memory corruption problem like the one you may have here.

Thanks for pointing this out.