The kernels for Debian stretch require some special CFLAGS settings which
are only correctly defined when NOSTDINC_FLAGS is defined inside the
execution of the Makefile via kbuild. But batman-adv sets it currently
outside to insert compatibility include headers and compat-sources.
This can be avoided by making the top Makefile kbuild compatible and
redefining the NOSTDINC_FLAGS when kbuild include this Makefile. The actual
build of the batman-adv module is then done by adding the subdirectory to
obj-y.
Signed-off-by: Sven Eckelmann <sven(a)narfation.org>
---
v3:
- append NOSTDINC_FLAGS with our own flags instead of overwriting it
v2:
- Fixed noise from other patches in context (should now apply cleanly on next)
Makefile | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/Makefile b/Makefile
index 5d2c058..2568fb2 100644
--- a/Makefile
+++ b/Makefile
@@ -43,7 +43,7 @@ RM ?= rm -f
REVISION= $(shell if [ -d "$(PWD)/.git" ]; then \
echo $$(git --git-dir="$(PWD)/.git" describe --always --dirty --match "v*" |sed 's/^v//' 2> /dev/null || echo "[unknown]"); \
fi)
-export NOSTDINC_FLAGS := \
+NOSTDINC_FLAGS += \
-I$(PWD)/compat-include/ \
-include $(PWD)/compat.h \
$(CFLAGS)
@@ -52,8 +52,12 @@ ifneq ($(REVISION),)
NOSTDINC_FLAGS += -DBATADV_SOURCE_VERSION=\"$(REVISION)\"
endif
+obj-y += net/batman-adv/
+
BUILD_FLAGS := \
- M=$(PWD)/net/batman-adv \
+ M=$(PWD) \
+ PWD=$(PWD) \
+ REVISION=$(REVISION) \
CONFIG_BATMAN_ADV=m \
CONFIG_BATMAN_ADV_DEBUG=$(CONFIG_BATMAN_ADV_DEBUG) \
CONFIG_BATMAN_ADV_BLA=$(CONFIG_BATMAN_ADV_BLA) \
@@ -61,7 +65,7 @@ BUILD_FLAGS := \
CONFIG_BATMAN_ADV_NC=$(CONFIG_BATMAN_ADV_NC) \
CONFIG_BATMAN_ADV_MCAST=$(CONFIG_BATMAN_ADV_MCAST) \
CONFIG_BATMAN_ADV_BATMAN_V=$(CONFIG_BATMAN_ADV_BATMAN_V) \
- INSTALL_MOD_DIR=updates/net/batman-adv/
+ INSTALL_MOD_DIR=updates/
all: config
$(MAKE) -C $(KERNELPATH) $(BUILD_FLAGS) modules
--
2.8.1
The skb_linearize may reallocate the skb. This makes the calculated pointer
for ethhdr invalid. But it the pointer is used later to fill in the RR
field of the batadv_icmp_packet_rr packet.
Instead re-evaluate eth_hdr after the skb_linearize+skb_cow to fix the
pointer and avoid the invalid read.
Fixes: bb69cb678d37 ("batman-adv: generalize batman-adv icmp packet handling")
Signed-off-by: Sven Eckelmann <sven(a)narfation.org>
---
net/batman-adv/routing.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c
index 0c0c30e..27e07dd 100644
--- a/net/batman-adv/routing.c
+++ b/net/batman-adv/routing.c
@@ -374,6 +374,7 @@ int batadv_recv_icmp_packet(struct sk_buff *skb,
if (skb_cow(skb, ETH_HLEN) < 0)
goto out;
+ ethhdr = eth_hdr(skb);
icmph = (struct batadv_icmp_header *)skb->data;
icmp_packet_rr = (struct batadv_icmp_packet_rr *)icmph;
if (icmp_packet_rr->rr_cur >= BATADV_RR_LEN)
--
2.8.1
Commit a33d970d0b54 "batman-adv: Fix reference counting of vlan object
for tt_local_entry") makes each batadv_tt_local_entry hold a single
reference to a batadv_softif_vlan. In case a new entry cannot be
added to the hash table, the error path puts the reference, but the
reference will also now be dropped by batadv_tt_local_entry_release().
Fixes: a33d970d0b54 ("batman-adv: Fix reference counting of vlan object ...")
Cc: Sven Eckelmann <sven(a)narfation.org>
Signed-off-by: Ben Hutchings <ben(a)decadent.org.uk>
---
This is untested; I just spotted this apparent oversight while trying
to backport the previous fix.
Ben.
net/batman-adv/translation-table.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index feaf492b01ca..2068cf6648cb 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -691,7 +691,6 @@ bool batadv_tt_local_add(struct net_device *soft_iface, const u8 *addr,
if (unlikely(hash_added != 0)) {
/* remove the reference for the hash */
batadv_tt_local_entry_put(tt_local);
- batadv_softif_vlan_put(vlan);
goto out;
}
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>
Tested-by: Martin Weinelt <martin(a)darmstadt.freifunk.net>
---
v3:
- add wrapper function batadv_tt_req_node_put for kref_put(....)
v2:
- fixed list->object in commit message
- add example what could have gone wrong in commit message
---
net/batman-adv/translation-table.c | 37 +++++++++++++++++++++++++++++++++----
net/batman-adv/types.h | 2 ++
2 files changed, 35 insertions(+), 4 deletions(-)
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index 48adb91..5234e53 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -2272,6 +2272,29 @@ 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 *tt_req_node;
+
+ tt_req_node = container_of(ref, struct batadv_tt_req_node, refcount);
+
+ kfree(tt_req_node);
+}
+
+/**
+ * batadv_tt_req_node_put - decrement the tt_req_node refcounter and
+ * possibly release it
+ * @tt_req_node: tt_req_node to be free'd
+ */
+static void batadv_tt_req_node_put(struct batadv_tt_req_node *tt_req_node)
+{
+ kref_put(&tt_req_node->refcount, batadv_tt_req_node_release);
+}
+
static void batadv_tt_req_list_free(struct batadv_priv *bat_priv)
{
struct batadv_tt_req_node *node;
@@ -2281,7 +2304,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);
+ batadv_tt_req_node_put(node);
}
spin_unlock_bh(&bat_priv->tt.req_list_lock);
@@ -2318,7 +2341,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);
+ batadv_tt_req_node_put(node);
}
}
spin_unlock_bh(&bat_priv->tt.req_list_lock);
@@ -2350,9 +2373,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 +2644,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);
+ batadv_tt_req_node_put(tt_req_node);
spin_unlock_bh(&bat_priv->tt.req_list_lock);
- kfree(tt_req_node);
}
+
+ if (tt_req_node)
+ batadv_tt_req_node_put(tt_req_node);
+
kfree(tvlv_tt_data);
return ret;
}
@@ -3057,7 +3086,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);
+ batadv_tt_req_node_put(node);
}
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