The tt_req_node is added and removed from a list inside a spinlock. But the
locking is sometimes removed even when the object is still referenced and
will be used later via this reference. For example batadv_send_tt_request
can create a new tt_req_node (including add to a list) and later
re-acquires the lock to remove it from the list and to free it. But at this
time another context could have already removed this tt_req_node from the
list and freed it.
CPU#0
batadv_batman_skb_recv from net_device 0
-> batadv_iv_ogm_receive
-> batadv_iv_ogm_process
-> batadv_iv_ogm_process_per_outif
-> batadv_tvlv_ogm_receive
-> batadv_tvlv_ogm_receive
-> batadv_tvlv_containers_process
-> batadv_tvlv_call_handler
-> batadv_tt_tvlv_ogm_handler_v1
-> batadv_tt_update_orig
-> batadv_send_tt_request
-> batadv_tt_req_node_new
spin_lock(...)
allocates new tt_req_node and adds it to list
spin_unlock(...)
return tt_req_node
CPU#1
batadv_batman_skb_recv from net_device 1
-> batadv_recv_unicast_tvlv
-> batadv_tvlv_containers_process
-> batadv_tvlv_call_handler
-> batadv_tt_tvlv_unicast_handler_v1
-> batadv_handle_tt_response
spin_lock(...)
tt_req_node gets removed from list and is freed
spin_unlock(...)
CPU#0
<- returned to batadv_send_tt_request
spin_lock(...)
tt_req_node gets removed from list and is freed
MEMORY CORRUPTION/SEGFAULT/...
spin_unlock(...)
This can only be solved via reference counting to allow multiple contexts
to handle the list manipulation while making sure that only the last
context holding a reference will free the object.
Fixes: cea194d90b11 ("batman-adv: improved client announcement mechanism")
Signed-off-by: Sven Eckelmann <sven(a)narfation.org>
---
v2:
- fixed list->object in commit message
- add example what could have gone wrong in commit message
---
net/batman-adv/translation-table.c | 27 +++++++++++++++++++++++----
net/batman-adv/types.h | 2 ++
2 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index 48adb91..46f90c5 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -2272,6 +2272,19 @@ static u32 batadv_tt_local_crc(struct batadv_priv *bat_priv,
return crc;
}
+/**
+ * batadv_tt_req_node_release - free tt_req node entry
+ * @ref: kref pointer of the tt req_node entry
+ */
+static void batadv_tt_req_node_release(struct kref *ref)
+{
+ struct batadv_tt_req_node *node;
+
+ node = container_of(ref, struct batadv_tt_req_node, refcount);
+
+ kfree(node);
+}
+
static void batadv_tt_req_list_free(struct batadv_priv *bat_priv)
{
struct batadv_tt_req_node *node;
@@ -2281,7 +2294,7 @@ static void batadv_tt_req_list_free(struct batadv_priv *bat_priv)
hlist_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) {
hlist_del_init(&node->list);
- kfree(node);
+ kref_put(&node->refcount, batadv_tt_req_node_release);
}
spin_unlock_bh(&bat_priv->tt.req_list_lock);
@@ -2318,7 +2331,7 @@ static void batadv_tt_req_purge(struct batadv_priv *bat_priv)
if (batadv_has_timed_out(node->issued_at,
BATADV_TT_REQUEST_TIMEOUT)) {
hlist_del_init(&node->list);
- kfree(node);
+ kref_put(&node->refcount, batadv_tt_req_node_release);
}
}
spin_unlock_bh(&bat_priv->tt.req_list_lock);
@@ -2350,9 +2363,11 @@ batadv_tt_req_node_new(struct batadv_priv *bat_priv,
if (!tt_req_node)
goto unlock;
+ kref_init(&tt_req_node->refcount);
ether_addr_copy(tt_req_node->addr, orig_node->orig);
tt_req_node->issued_at = jiffies;
+ kref_get(&tt_req_node->refcount);
hlist_add_head(&tt_req_node->list, &bat_priv->tt.req_list);
unlock:
spin_unlock_bh(&bat_priv->tt.req_list_lock);
@@ -2619,9 +2634,13 @@ out:
spin_lock_bh(&bat_priv->tt.req_list_lock);
/* hlist_del_init() verifies tt_req_node still is in the list */
hlist_del_init(&tt_req_node->list);
+ kref_put(&tt_req_node->refcount, batadv_tt_req_node_release);
spin_unlock_bh(&bat_priv->tt.req_list_lock);
- kfree(tt_req_node);
}
+
+ if (tt_req_node)
+ kref_put(&tt_req_node->refcount, batadv_tt_req_node_release);
+
kfree(tvlv_tt_data);
return ret;
}
@@ -3057,7 +3076,7 @@ static void batadv_handle_tt_response(struct batadv_priv *bat_priv,
if (!batadv_compare_eth(node->addr, resp_src))
continue;
hlist_del_init(&node->list);
- kfree(node);
+ kref_put(&node->refcount, batadv_tt_req_node_release);
}
spin_unlock_bh(&bat_priv->tt.req_list_lock);
diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h
index 1e47fbe..d75beef 100644
--- a/net/batman-adv/types.h
+++ b/net/batman-adv/types.h
@@ -1129,11 +1129,13 @@ struct batadv_tt_change_node {
* struct batadv_tt_req_node - data to keep track of the tt requests in flight
* @addr: mac address address of the originator this request was sent to
* @issued_at: timestamp used for purging stale tt requests
+ * @refcount: number of contexts the object is used by
* @list: list node for batadv_priv_tt::req_list
*/
struct batadv_tt_req_node {
u8 addr[ETH_ALEN];
unsigned long issued_at;
+ struct kref refcount;
struct hlist_node list;
};
--
2.8.1
Hi Patrick,
sorry for the late reply.
On Tuesday 17 May 2016 10:09:27 Patrick Bosch via B.A.T.M.A.N wrote:
> Hi all
>
> As the title suggest, I'm wondering how to read the jsondoc output
> correctly. It seems that every possible link is included in that
> output, but which one is the one that is used? If there is one link,
> it should be the one with the lowest metric, but what if a node has
> more than one link?
>
> For example the following setup:
>
> Node 1
> |
> |
> Node 2 ------- Node 3
>
> Here, node two would have two links. Now, if there would be some more
> nodes that are in the jsondoc output, how can I find out which are the
> links that are in use?
The nodes which appear in the jsondoc are only originators which batman-adv
decided to reach directly (with one hop) - so these are direct neighbors.
Basically, all of these links would be in use from one node to its neighbor,
so all of them are "in use". But you can't find out how much a link is
actually used (i.e. how many packets/bytes are transmitted), or which neighbor
is used to reach a distant neighbor.
Cheers,
Simon
The tt_req_node is added and removed from a list inside a spinlock. But the
locking is sometimes removed even when the object is still referenced and
will be used later via this reference. For example batadv_send_tt_request
can create a new tt_req_node (including add to a list) and later
re-acquires the lock to remove it from the list and to free it. But at
this time another context could have already removed this tt_req_node from
the list and freed it.
This can only be solved via reference counting to allow multiple contexts
to handle the list manipulation while making sure that only the last
context frees the list.
Fixes: cea194d90b11 ("batman-adv: improved client announcement mechanism")
Signed-off-by: Sven Eckelmann <sven(a)narfation.org>
---
Cc: Matthias Schiffer <mschiffer(a)universe-factory.net>:
this can also be interesting for batman-adv-legacy installations on servers
with more than one core
net/batman-adv/translation-table.c | 27 +++++++++++++++++++++++----
net/batman-adv/types.h | 2 ++
2 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index 48adb91..46f90c5 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -2272,6 +2272,19 @@ static u32 batadv_tt_local_crc(struct batadv_priv *bat_priv,
return crc;
}
+/**
+ * batadv_tt_req_node_release - free tt_req node entry
+ * @ref: kref pointer of the tt req_node entry
+ */
+static void batadv_tt_req_node_release(struct kref *ref)
+{
+ struct batadv_tt_req_node *node;
+
+ node = container_of(ref, struct batadv_tt_req_node, refcount);
+
+ kfree(node);
+}
+
static void batadv_tt_req_list_free(struct batadv_priv *bat_priv)
{
struct batadv_tt_req_node *node;
@@ -2281,7 +2294,7 @@ static void batadv_tt_req_list_free(struct batadv_priv *bat_priv)
hlist_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) {
hlist_del_init(&node->list);
- kfree(node);
+ kref_put(&node->refcount, batadv_tt_req_node_release);
}
spin_unlock_bh(&bat_priv->tt.req_list_lock);
@@ -2318,7 +2331,7 @@ static void batadv_tt_req_purge(struct batadv_priv *bat_priv)
if (batadv_has_timed_out(node->issued_at,
BATADV_TT_REQUEST_TIMEOUT)) {
hlist_del_init(&node->list);
- kfree(node);
+ kref_put(&node->refcount, batadv_tt_req_node_release);
}
}
spin_unlock_bh(&bat_priv->tt.req_list_lock);
@@ -2350,9 +2363,11 @@ batadv_tt_req_node_new(struct batadv_priv *bat_priv,
if (!tt_req_node)
goto unlock;
+ kref_init(&tt_req_node->refcount);
ether_addr_copy(tt_req_node->addr, orig_node->orig);
tt_req_node->issued_at = jiffies;
+ kref_get(&tt_req_node->refcount);
hlist_add_head(&tt_req_node->list, &bat_priv->tt.req_list);
unlock:
spin_unlock_bh(&bat_priv->tt.req_list_lock);
@@ -2619,9 +2634,13 @@ out:
spin_lock_bh(&bat_priv->tt.req_list_lock);
/* hlist_del_init() verifies tt_req_node still is in the list */
hlist_del_init(&tt_req_node->list);
+ kref_put(&tt_req_node->refcount, batadv_tt_req_node_release);
spin_unlock_bh(&bat_priv->tt.req_list_lock);
- kfree(tt_req_node);
}
+
+ if (tt_req_node)
+ kref_put(&tt_req_node->refcount, batadv_tt_req_node_release);
+
kfree(tvlv_tt_data);
return ret;
}
@@ -3057,7 +3076,7 @@ static void batadv_handle_tt_response(struct batadv_priv *bat_priv,
if (!batadv_compare_eth(node->addr, resp_src))
continue;
hlist_del_init(&node->list);
- kfree(node);
+ kref_put(&node->refcount, batadv_tt_req_node_release);
}
spin_unlock_bh(&bat_priv->tt.req_list_lock);
diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h
index 1e47fbe..d75beef 100644
--- a/net/batman-adv/types.h
+++ b/net/batman-adv/types.h
@@ -1129,11 +1129,13 @@ struct batadv_tt_change_node {
* struct batadv_tt_req_node - data to keep track of the tt requests in flight
* @addr: mac address address of the originator this request was sent to
* @issued_at: timestamp used for purging stale tt requests
+ * @refcount: number of contexts the object is used by
* @list: list node for batadv_priv_tt::req_list
*/
struct batadv_tt_req_node {
u8 addr[ETH_ALEN];
unsigned long issued_at;
+ struct kref refcount;
struct hlist_node list;
};
--
2.8.1
Dropped patch #1, since it has been merged
Fixed the socket leak
Removed redundant calls to mount debugfs
Andrew Lunn (2):
alfred: Add support for network namespaces
alfred: Mount debugfs before reducing capabilities
batadv_query.c | 19 +------------------
debugfs.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++-
main.c | 4 ++++
vis/vis.h | 2 +-
4 files changed, 54 insertions(+), 20 deletions(-)
--
2.7.0.rc3
This patchset completes netns support, by disabling debugfs entries
when not in the default name space, and correctly handling interface
stack loops when the parent is in a different name space.
It additionally adds netlink support for most of the information found
in debugfs, and is netns aware.
This patchset is based on Andrews v7 series as of May 7th, with the
following changes of Sven and myself:
* rebase on current master
* fix VID output in TT+claim table
* fix kerneldoc for batadv_bla_claim_dump* functions
* fix -ENODEV return in batadv_bla_claim_dump
* added support for bla backbone table
* add own TTVN to mesh_info (required for global translation table)
* add own group id to mesh_info (required for bla claim/backbone table)
* fix soft_iface reference leak when dumping the claim table
* fix function parameter alignments
* fix includes
it applies on master plus the following two patches applied:
* batman-adv: fix returned error in batadv_netlink_tp_meter_cancel
* batman-adv: remove unused vid local variable in tt seq print
We have tested the setup in our VMs and verified functionality of the
provided tables, including big TT tables and bridge loop avoidance.
Corresponding batctl patches will be sent separately.
Cheers,
Simon
Andrew Lunn (5):
batman-adv: Handle parent interfaces in a different netns
batman-adv: Suppress debugfs entries for netns's
batman-adv: add B.A.T.M.A.N. Dump gateways via netlink
batman-adv: add B.A.T.M.A.N. Dump BLA claims via netlink
batman-adv: Indicate netlink socket can be used with netns.
Matthias Schiffer (6):
batman-adv: netlink: add routing_algo query
batman-adv: netlink: hardif query
batman-adv: netlink: add translation table query
batman-adv: netlink: add originator and neighbor table queries
batman-adv: add B.A.T.M.A.N. IV bat_{orig, neigh}_dump implementations
batman-adv: add B.A.T.M.A.N. V bat_{orig, neigh}_dump implementations
Simon Wunderlich (1):
batman-adv: add backbone table netlink support
Sven Eckelmann (2):
batman-adv: Provide TTVN in the mesh_info netlink msg
batman-adv: Provide bla group in the mesh_info netlink msg
compat-include/linux/netlink.h | 18 ++
compat.h | 7 +
include/uapi/linux/batman_adv.h | 94 ++++++++
net/batman-adv/bat_algo.c | 68 ++++++
net/batman-adv/bat_algo.h | 3 +
net/batman-adv/bat_iv_ogm.c | 362 +++++++++++++++++++++++++++++++
net/batman-adv/bat_v.c | 337 +++++++++++++++++++++++++++++
net/batman-adv/bridge_loop_avoidance.c | 333 +++++++++++++++++++++++++++++
net/batman-adv/bridge_loop_avoidance.h | 17 +-
net/batman-adv/debugfs.c | 18 ++
net/batman-adv/gateway_client.c | 148 +++++++++++++
net/batman-adv/gateway_client.h | 2 +
net/batman-adv/hard-interface.c | 50 ++++-
net/batman-adv/netlink.c | 217 ++++++++++++++++++-
net/batman-adv/netlink.h | 6 +
net/batman-adv/originator.c | 160 ++++++++++++++
net/batman-adv/originator.h | 4 +
net/batman-adv/packet.h | 36 ----
net/batman-adv/translation-table.c | 377 +++++++++++++++++++++++++++++++++
net/batman-adv/translation-table.h | 4 +
net/batman-adv/types.h | 9 +
21 files changed, 2222 insertions(+), 48 deletions(-)
--
2.8.1
Hi,
I want to set up bat0 on top of a wifi interface in ap mode
and another machine with bat0 on top of a wifi interface in client mode.
Via iw dev wlan0 station dump I can seen that both are connected.
Via batctl o I see the connected node.
Via batct ping mac addr I can ping each other.
But I can not ping with an IP4 addr.
bat0 was added to a bridge interface with a static IP on both machines.
No DHCP server is running, because only static IPS are needed.
Did I miss something?
Regrads
Hannes
Hi,
Is anyone familiar with the implications of using kmalloc() vs.
kmem_cache_alloc()? Not just allocation speed, but also RAM
fragmentation is something I'm currently wondering about.
We have had issues with the large, bulk allocations from the
debugfs tables before, where if I remember correctly the
experssion "RAM fragmentation" has been mentioned before. With the
fallback from kmalloc() to vmalloc() in the debugfs internals on
more recent kernels, at least that problem has gone for now.
Still, I'm wondering whether a couple of thousand global TT entry
allocations (Freifunk Hamburg currently has more than three
thousand) could lead to a badly fragmented RAM. Too much for a
cute wifi router with just 32MB of RAM, maybe.
I then noticed that the bridge is using kmem_cache_alloc() instead
of kmalloc() for its fdb (forwarding database). Would it make
sense to do the same for global TT entries (and maybe originator
structs, too?)?
If so, I'd be happy to look into providing a patch.
Regards, Linus
PS: Why this thought came up:
https://github.com/freifunk-gluon/gluon/issues/753
Could be a memory leak or something else too though. Investigation
still in progress.