[maint,2/2] batman-adv: Fix potential synchronization issues in mcast tvlv handler
Commit Message
So far the mcast tvlv handler did not anticipate the processing of
multiple incoming OGMs from the same originator at the same time. This
can lead to various issues:
* Broken refcounting: For instance two mcast handlers might both assume
that an originator just got multicast capabilities and will together
wrongly decrease mcast.num_disabled by two, potentially leading to
an integer underflow.
* Potential kernel panic on hlist_del_rcu(): Two mcast handlers might
one after another try to do an
hlist_del_rcu(&orig->mcast_want_all_*_node). The second one will
cause memory corruption / crashes.
(Reported by: Sven Eckelmann <sven@narfation.org>)
Right in the beginning the code path makes assumptions about the current
multicast related state of an originator and bases all updates on that. The
easiest and least error prune way to fix the issues in this case is to
serialize multiple mcast handler invocations with a spinlock.
Fixes: 77ec494490d6 ("batman-adv: Announce new capability via multicast TVLV")
Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
---
multicast.c | 2 ++
originator.c | 1 +
types.h | 2 ++
3 files changed, 5 insertions(+)
Comments
On Monday, June 15, 2015 08:22:25 Linus Lüssing wrote:
> So far the mcast tvlv handler did not anticipate the processing of
> multiple incoming OGMs from the same originator at the same time. This
> can lead to various issues:
>
> * Broken refcounting: For instance two mcast handlers might both assume
> that an originator just got multicast capabilities and will together
> wrongly decrease mcast.num_disabled by two, potentially leading to
> an integer underflow.
>
> * Potential kernel panic on hlist_del_rcu(): Two mcast handlers might
> one after another try to do an
> hlist_del_rcu(&orig->mcast_want_all_*_node). The second one will
> cause memory corruption / crashes.
> (Reported by: Sven Eckelmann <sven@narfation.org>)
As far as I can tell from looking at the code your patch does not address the
issue raised by Sven.
The first problem is that the mcast code calls hlist_del_rcu() without
verifying whether or not the element is still in the list. Adding a spinlock
is not going to change that. You can still have a purge event going on while
we just receive a new OGM because the purge caller does not need to hold the
newly added lock.
Cheers,
Marek
@@ -674,6 +674,7 @@ static void batadv_mcast_tvlv_ogm_handler_v1(struct batadv_priv *bat_priv,
uint8_t mcast_flags = BATADV_NO_FLAGS;
bool orig_initialized;
+ spin_lock_bh(&orig->mcast_handler_lock);
orig_initialized = orig->capa_initialized & BATADV_ORIG_CAPA_HAS_MCAST;
/* If mcast support is turned on decrease the disabled mcast node
@@ -707,6 +708,7 @@ static void batadv_mcast_tvlv_ogm_handler_v1(struct batadv_priv *bat_priv,
batadv_mcast_want_ipv6_update(bat_priv, orig, mcast_flags);
orig->mcast_flags = mcast_flags;
+ spin_unlock_bh(&orig->mcast_handler_lock);
}
/**
@@ -663,6 +663,7 @@ struct batadv_orig_node *batadv_orig_node_new(struct batadv_priv *bat_priv,
spin_lock_init(&orig_node->tt_buff_lock);
spin_lock_init(&orig_node->tt_lock);
spin_lock_init(&orig_node->vlan_list_lock);
+ spin_lock_init(&orig_node->mcast_handler_lock);
batadv_nc_init_orig(orig_node);
@@ -251,6 +251,8 @@ struct batadv_orig_node {
unsigned long last_seen;
unsigned long bcast_seqno_reset;
#ifdef CONFIG_BATMAN_ADV_MCAST
+ /* synchronizes mcast tvlv specific orig changes */
+ spinlock_t mcast_handler_lock;
uint8_t mcast_flags;
struct hlist_node mcast_want_all_unsnoopables_node;
struct hlist_node mcast_want_all_ipv4_node;