* [B.A.T.M.A.N.] Reordered rcu refcounting
@ 2011-01-30 1:52 Sven Eckelmann
2011-01-30 1:52 ` [B.A.T.M.A.N.] [PATCH 1/4] batman-adv: Correct rcu refcounting for gw_node Sven Eckelmann
` (4 more replies)
0 siblings, 5 replies; 44+ messages in thread
From: Sven Eckelmann @ 2011-01-30 1:52 UTC (permalink / raw)
To: b.a.t.m.a.n
Hi,
I told Linus and Marek that the implementation of some datastructures using
reference counting and RCUs must be redesigned. The problem is that call_rcu
should only be used when the reference count is zero because multiple call_rcu
cannot be initiated at the same time. The next problem is that we may try to
access an element with the refcount 0 when we try to traverse a rcu protected
list. This problem cannot be solved by kref - thus we have to use atomic_t
again to _not_ increase the reference count when it was already zero.
Marek asked me what we must change - I try to summarize those things in the
following patches. They aren't tested and should only be seen as suggestions
what must be done to finish the "remove orig_hash lock"-patches.
If you try these patches then please keep in mind that they will sell your soul
to the next confectioner (for some cookies or a pie).
More information can be found inside Documentation/RCU/rcuref.txt
Btw, maybe I've found a bug inside the find_router with enabled bonding (see
"BUG" in patch 4).
Best regards,
Sven
^ permalink raw reply [flat|nested] 44+ messages in thread
* [B.A.T.M.A.N.] [PATCH 1/4] batman-adv: Correct rcu refcounting for gw_node
2011-01-30 1:52 [B.A.T.M.A.N.] Reordered rcu refcounting Sven Eckelmann
@ 2011-01-30 1:52 ` Sven Eckelmann
2011-02-02 17:37 ` [B.A.T.M.A.N.] [PATCH] " Linus Lüssing
2011-01-30 1:52 ` [B.A.T.M.A.N.] [PATCH 2/4] batman-adv: Correct rcu refcounting for softif_neigh Sven Eckelmann
` (3 subsequent siblings)
4 siblings, 1 reply; 44+ messages in thread
From: Sven Eckelmann @ 2011-01-30 1:52 UTC (permalink / raw)
To: b.a.t.m.a.n
<TODO: write a long monologue about every problem we have or could have or
maybe never had and would have when we not have it>
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
batman-adv/gateway_client.c | 28 +++++++++++++---------------
batman-adv/types.h | 2 +-
2 files changed, 14 insertions(+), 16 deletions(-)
diff --git a/batman-adv/gateway_client.c b/batman-adv/gateway_client.c
index 429a013..8ce3a63 100644
--- a/batman-adv/gateway_client.c
+++ b/batman-adv/gateway_client.c
@@ -28,20 +28,18 @@
#include <linux/udp.h>
#include <linux/if_vlan.h>
-static void gw_node_free_ref(struct kref *refcount)
-{
- struct gw_node *gw_node;
-
- gw_node = container_of(refcount, struct gw_node, refcount);
- kfree(gw_node);
-}
-
static void gw_node_free_rcu(struct rcu_head *rcu)
{
struct gw_node *gw_node;
gw_node = container_of(rcu, struct gw_node, rcu);
- kref_put(&gw_node->refcount, gw_node_free_ref);
+ kfree(gw_node);
+}
+
+static void gw_node_free_ref(struct gw_node *gw_node)
+{
+ if (atomic_dec_and_test(&gw_node->refcount))
+ call_rcu(&gw_node->rcu, gw_node_free_rcu);
}
void *gw_get_selected(struct bat_priv *bat_priv)
@@ -61,7 +59,7 @@ void gw_deselect(struct bat_priv *bat_priv)
bat_priv->curr_gw = NULL;
if (gw_node)
- kref_put(&gw_node->refcount, gw_node_free_ref);
+ gw_node_free_ref(gw_node);
}
static struct gw_node *gw_select(struct bat_priv *bat_priv,
@@ -69,8 +67,8 @@ static struct gw_node *gw_select(struct bat_priv *bat_priv,
{
struct gw_node *curr_gw_node = bat_priv->curr_gw;
- if (new_gw_node)
- kref_get(&new_gw_node->refcount);
+ if (new_gw_node && !atomic_inc_not_zero(&new_gw_node->refcount))
+ return NULL;
bat_priv->curr_gw = new_gw_node;
return curr_gw_node;
@@ -181,7 +179,7 @@ void gw_election(struct bat_priv *bat_priv)
/* the kfree() has to be outside of the rcu lock */
if (old_gw_node)
- kref_put(&old_gw_node->refcount, gw_node_free_ref);
+ gw_node_free_ref(old_gw_node);
}
void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
@@ -242,7 +240,7 @@ static void gw_node_add(struct bat_priv *bat_priv,
memset(gw_node, 0, sizeof(struct gw_node));
INIT_HLIST_NODE(&gw_node->list);
gw_node->orig_node = orig_node;
- kref_init(&gw_node->refcount);
+ atomic_set(&gw_node->refcount, 1);
spin_lock_bh(&bat_priv->gw_list_lock);
hlist_add_head_rcu(&gw_node->list, &bat_priv->gw_list);
@@ -325,7 +323,7 @@ void gw_node_purge(struct bat_priv *bat_priv)
gw_deselect(bat_priv);
hlist_del_rcu(&gw_node->list);
- call_rcu(&gw_node->rcu, gw_node_free_rcu);
+ gw_node_free_ref(gw_node);
}
diff --git a/batman-adv/types.h b/batman-adv/types.h
index e4a0462..ca5f20a 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -100,7 +100,7 @@ struct gw_node {
struct hlist_node list;
struct orig_node *orig_node;
unsigned long deleted;
- struct kref refcount;
+ atomic_t refcount;
struct rcu_head rcu;
};
--
1.7.2.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [B.A.T.M.A.N.] [PATCH 2/4] batman-adv: Correct rcu refcounting for softif_neigh
2011-01-30 1:52 [B.A.T.M.A.N.] Reordered rcu refcounting Sven Eckelmann
2011-01-30 1:52 ` [B.A.T.M.A.N.] [PATCH 1/4] batman-adv: Correct rcu refcounting for gw_node Sven Eckelmann
@ 2011-01-30 1:52 ` Sven Eckelmann
2011-01-30 1:52 ` [B.A.T.M.A.N.] [PATCH 3/4] batman-adv: Correct rcu refcounting for batman_if Sven Eckelmann
` (2 subsequent siblings)
4 siblings, 0 replies; 44+ messages in thread
From: Sven Eckelmann @ 2011-01-30 1:52 UTC (permalink / raw)
To: b.a.t.m.a.n
<TODO: write a long monologue about every problem we have or could have or
maybe never had and would have when we not have it>
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
batman-adv/soft-interface.c | 34 ++++++++++++++++------------------
batman-adv/types.h | 2 +-
2 files changed, 17 insertions(+), 19 deletions(-)
diff --git a/batman-adv/soft-interface.c b/batman-adv/soft-interface.c
index 145e0f7..9fb8a6a 100644
--- a/batman-adv/soft-interface.c
+++ b/batman-adv/soft-interface.c
@@ -79,20 +79,18 @@ int my_skb_head_push(struct sk_buff *skb, unsigned int len)
return 0;
}
-static void softif_neigh_free_ref(struct kref *refcount)
-{
- struct softif_neigh *softif_neigh;
-
- softif_neigh = container_of(refcount, struct softif_neigh, refcount);
- kfree(softif_neigh);
-}
-
static void softif_neigh_free_rcu(struct rcu_head *rcu)
{
struct softif_neigh *softif_neigh;
softif_neigh = container_of(rcu, struct softif_neigh, rcu);
- kref_put(&softif_neigh->refcount, softif_neigh_free_ref);
+ kfree(softif_neigh);
+}
+
+static void softif_neigh_free_ref(struct softif_neigh *softif_neigh)
+{
+ if (atomic_dec_and_test(&softif_neigh->refcount))
+ call_rcu(&softif_neigh->rcu, softif_neigh_free_rcu);
}
void softif_neigh_purge(struct bat_priv *bat_priv)
@@ -119,11 +117,10 @@ void softif_neigh_purge(struct bat_priv *bat_priv)
softif_neigh->addr, softif_neigh->vid);
softif_neigh_tmp = bat_priv->softif_neigh;
bat_priv->softif_neigh = NULL;
- kref_put(&softif_neigh_tmp->refcount,
- softif_neigh_free_ref);
+ softif_neigh_free_ref(softif_neigh_tmp);
}
- call_rcu(&softif_neigh->rcu, softif_neigh_free_rcu);
+ softif_neigh_free_ref(softif_neigh);
}
spin_unlock_bh(&bat_priv->softif_neigh_lock);
@@ -144,8 +141,11 @@ static struct softif_neigh *softif_neigh_get(struct bat_priv *bat_priv,
if (softif_neigh->vid != vid)
continue;
+ if (!atomic_inc_not_zero(&softif_neigh->refcount))
+ continue;
+
softif_neigh->last_seen = jiffies;
- goto found;
+ goto out;
}
softif_neigh = kzalloc(sizeof(struct softif_neigh), GFP_ATOMIC);
@@ -155,15 +155,13 @@ static struct softif_neigh *softif_neigh_get(struct bat_priv *bat_priv,
memcpy(softif_neigh->addr, addr, ETH_ALEN);
softif_neigh->vid = vid;
softif_neigh->last_seen = jiffies;
- kref_init(&softif_neigh->refcount);
+ atomic_set(&softif_neigh->refcount, 1);
INIT_HLIST_NODE(&softif_neigh->list);
spin_lock_bh(&bat_priv->softif_neigh_lock);
hlist_add_head_rcu(&softif_neigh->list, &bat_priv->softif_neigh_list);
spin_unlock_bh(&bat_priv->softif_neigh_lock);
-found:
- kref_get(&softif_neigh->refcount);
out:
rcu_read_unlock();
return softif_neigh;
@@ -267,7 +265,7 @@ static void softif_batman_recv(struct sk_buff *skb, struct net_device *dev,
softif_neigh->addr, softif_neigh->vid);
softif_neigh_tmp = bat_priv->softif_neigh;
bat_priv->softif_neigh = softif_neigh;
- kref_put(&softif_neigh_tmp->refcount, softif_neigh_free_ref);
+ softif_neigh_free_ref(softif_neigh_tmp);
/* we need to hold the additional reference */
goto err;
}
@@ -285,7 +283,7 @@ static void softif_batman_recv(struct sk_buff *skb, struct net_device *dev,
}
out:
- kref_put(&softif_neigh->refcount, softif_neigh_free_ref);
+ softif_neigh_free_ref(softif_neigh);
err:
kfree_skb(skb);
return;
diff --git a/batman-adv/types.h b/batman-adv/types.h
index ca5f20a..8df6d9d 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -270,7 +270,7 @@ struct softif_neigh {
uint8_t addr[ETH_ALEN];
unsigned long last_seen;
short vid;
- struct kref refcount;
+ atomic_t refcount;
struct rcu_head rcu;
};
--
1.7.2.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [B.A.T.M.A.N.] [PATCH 3/4] batman-adv: Correct rcu refcounting for batman_if
2011-01-30 1:52 [B.A.T.M.A.N.] Reordered rcu refcounting Sven Eckelmann
2011-01-30 1:52 ` [B.A.T.M.A.N.] [PATCH 1/4] batman-adv: Correct rcu refcounting for gw_node Sven Eckelmann
2011-01-30 1:52 ` [B.A.T.M.A.N.] [PATCH 2/4] batman-adv: Correct rcu refcounting for softif_neigh Sven Eckelmann
@ 2011-01-30 1:52 ` Sven Eckelmann
2011-01-30 1:52 ` [B.A.T.M.A.N.] [PATCH 4/4] batman-adv: Correct rcu refcounting for neigh_node Sven Eckelmann
2011-02-03 17:02 ` [B.A.T.M.A.N.] Reordered rcu refcounting Marek Lindner
4 siblings, 0 replies; 44+ messages in thread
From: Sven Eckelmann @ 2011-01-30 1:52 UTC (permalink / raw)
To: b.a.t.m.a.n
<TODO: write a long monologue about every problem we have or could have or
maybe never had and would have when we not have it>
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
batman-adv/bat_sysfs.c | 12 ++++++------
batman-adv/hard-interface.c | 40 +++++++++++++++++++---------------------
batman-adv/hard-interface.h | 9 ++++-----
batman-adv/types.h | 2 +-
4 files changed, 30 insertions(+), 33 deletions(-)
diff --git a/batman-adv/bat_sysfs.c b/batman-adv/bat_sysfs.c
index f7b93a0..08f9251 100644
--- a/batman-adv/bat_sysfs.c
+++ b/batman-adv/bat_sysfs.c
@@ -450,7 +450,7 @@ static ssize_t show_mesh_iface(struct kobject *kobj, struct attribute *attr,
length = sprintf(buff, "%s\n", batman_if->if_status == IF_NOT_IN_USE ?
"none" : batman_if->soft_iface->name);
- kref_put(&batman_if->refcount, hardif_free_ref);
+ hardif_free_ref(batman_if);
return length;
}
@@ -472,7 +472,7 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr,
if (strlen(buff) >= IFNAMSIZ) {
pr_err("Invalid parameter for 'mesh_iface' setting received: "
"interface name too long '%s'\n", buff);
- kref_put(&batman_if->refcount, hardif_free_ref);
+ hardif_free_ref(batman_if);
return -EINVAL;
}
@@ -483,7 +483,7 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr,
if ((batman_if->if_status == status_tmp) || ((batman_if->soft_iface) &&
(strncmp(batman_if->soft_iface->name, buff, IFNAMSIZ) == 0))) {
- kref_put(&batman_if->refcount, hardif_free_ref);
+ hardif_free_ref(batman_if);
return count;
}
@@ -491,7 +491,7 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr,
rtnl_lock();
hardif_disable_interface(batman_if);
rtnl_unlock();
- kref_put(&batman_if->refcount, hardif_free_ref);
+ hardif_free_ref(batman_if);
return count;
}
@@ -503,7 +503,7 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr,
}
ret = hardif_enable_interface(batman_if, buff);
- kref_put(&batman_if->refcount, hardif_free_ref);
+ hardif_free_ref(batman_if);
return ret;
}
@@ -537,7 +537,7 @@ static ssize_t show_iface_status(struct kobject *kobj, struct attribute *attr,
break;
}
- kref_put(&batman_if->refcount, hardif_free_ref);
+ hardif_free_ref(batman_if);
return length;
}
diff --git a/batman-adv/hard-interface.c b/batman-adv/hard-interface.c
index e2b001a..3f59c52 100644
--- a/batman-adv/hard-interface.c
+++ b/batman-adv/hard-interface.c
@@ -40,13 +40,13 @@ static int batman_skb_recv(struct sk_buff *skb,
struct packet_type *ptype,
struct net_device *orig_dev);
-static void hardif_free_rcu(struct rcu_head *rcu)
+void hardif_free_rcu(struct rcu_head *rcu)
{
struct batman_if *batman_if;
batman_if = container_of(rcu, struct batman_if, rcu);
dev_put(batman_if->net_dev);
- kref_put(&batman_if->refcount, hardif_free_ref);
+ kfree(batman_if);
}
struct batman_if *get_batman_if_by_netdev(struct net_device *net_dev)
@@ -55,16 +55,14 @@ struct batman_if *get_batman_if_by_netdev(struct net_device *net_dev)
rcu_read_lock();
list_for_each_entry_rcu(batman_if, &if_list, list) {
- if (batman_if->net_dev == net_dev)
+ if (batman_if->net_dev == net_dev &&
+ atomic_inc_not_zero(&batman_if->refcount))
goto out;
}
batman_if = NULL;
out:
- if (batman_if)
- kref_get(&batman_if->refcount);
-
rcu_read_unlock();
return batman_if;
}
@@ -105,16 +103,14 @@ static struct batman_if *get_active_batman_if(struct net_device *soft_iface)
if (batman_if->soft_iface != soft_iface)
continue;
- if (batman_if->if_status == IF_ACTIVE)
+ if (batman_if->if_status == IF_ACTIVE &&
+ atomic_inc_not_zero(&batman_if->refcount))
goto out;
}
batman_if = NULL;
out:
- if (batman_if)
- kref_get(&batman_if->refcount);
-
rcu_read_unlock();
return batman_if;
}
@@ -137,14 +133,14 @@ static void set_primary_if(struct bat_priv *bat_priv,
struct batman_packet *batman_packet;
struct batman_if *old_if;
- if (batman_if)
- kref_get(&batman_if->refcount);
+ if (batman_if && !atomic_inc_not_zero(&batman_if->refcount))
+ batman_if = NULL;
old_if = bat_priv->primary_if;
bat_priv->primary_if = batman_if;
if (old_if)
- kref_put(&old_if->refcount, hardif_free_ref);
+ hardif_free_ref(old_if);
if (!bat_priv->primary_if)
return;
@@ -290,6 +286,9 @@ int hardif_enable_interface(struct batman_if *batman_if, char *iface_name)
if (batman_if->if_status != IF_NOT_IN_USE)
goto out;
+ if (!atomic_inc_not_zero(&batman_if->refcount))
+ goto out;
+
batman_if->soft_iface = dev_get_by_name(&init_net, iface_name);
if (!batman_if->soft_iface) {
@@ -328,7 +327,6 @@ int hardif_enable_interface(struct batman_if *batman_if, char *iface_name)
batman_if->batman_adv_ptype.type = __constant_htons(ETH_P_BATMAN);
batman_if->batman_adv_ptype.func = batman_skb_recv;
batman_if->batman_adv_ptype.dev = batman_if->net_dev;
- kref_get(&batman_if->refcount);
dev_add_pack(&batman_if->batman_adv_ptype);
atomic_set(&batman_if->seqno, 1);
@@ -371,6 +369,7 @@ out:
return 0;
err:
+ hardif_free_ref(batman_if);
return -ENOMEM;
}
@@ -387,7 +386,7 @@ void hardif_disable_interface(struct batman_if *batman_if)
bat_info(batman_if->soft_iface, "Removing interface: %s\n",
batman_if->net_dev->name);
dev_remove_pack(&batman_if->batman_adv_ptype);
- kref_put(&batman_if->refcount, hardif_free_ref);
+ hardif_free_ref(batman_if);
bat_priv->num_ifaces--;
orig_hash_del_if(batman_if, bat_priv->num_ifaces);
@@ -399,7 +398,7 @@ void hardif_disable_interface(struct batman_if *batman_if)
set_primary_if(bat_priv, new_if);
if (new_if)
- kref_put(&new_if->refcount, hardif_free_ref);
+ hardif_free_ref(new_if);
}
kfree(batman_if->packet_buff);
@@ -445,7 +444,8 @@ static struct batman_if *hardif_add_interface(struct net_device *net_dev)
batman_if->soft_iface = NULL;
batman_if->if_status = IF_NOT_IN_USE;
INIT_LIST_HEAD(&batman_if->list);
- kref_init(&batman_if->refcount);
+ /* extra reference for return */
+ atomic_set(&batman_if->refcount, 2);
check_known_mac_addr(batman_if->net_dev);
@@ -453,8 +453,6 @@ static struct batman_if *hardif_add_interface(struct net_device *net_dev)
list_add_tail_rcu(&batman_if->list, &if_list);
spin_unlock(&if_list_lock);
- /* extra reference for return */
- kref_get(&batman_if->refcount);
return batman_if;
free_if:
@@ -476,7 +474,7 @@ static void hardif_remove_interface(struct batman_if *batman_if)
batman_if->if_status = IF_TO_BE_REMOVED;
sysfs_del_hardif(&batman_if->hardif_obj);
- call_rcu(&batman_if->rcu, hardif_free_rcu);
+ hardif_free_ref(batman_if);
}
void hardif_remove_interfaces(void)
@@ -548,7 +546,7 @@ static int hard_if_event(struct notifier_block *this,
};
hardif_put:
- kref_put(&batman_if->refcount, hardif_free_ref);
+ hardif_free_ref(batman_if);
out:
return NOTIFY_DONE;
}
diff --git a/batman-adv/hard-interface.h b/batman-adv/hard-interface.h
index ad19543..e488b90 100644
--- a/batman-adv/hard-interface.h
+++ b/batman-adv/hard-interface.h
@@ -37,13 +37,12 @@ void hardif_disable_interface(struct batman_if *batman_if);
void hardif_remove_interfaces(void);
int hardif_min_mtu(struct net_device *soft_iface);
void update_min_mtu(struct net_device *soft_iface);
+void hardif_free_rcu(struct rcu_head *rcu);
-static inline void hardif_free_ref(struct kref *refcount)
+static inline void hardif_free_ref(struct batman_if *batman_if)
{
- struct batman_if *batman_if;
-
- batman_if = container_of(refcount, struct batman_if, refcount);
- kfree(batman_if);
+ if (atomic_dec_and_test(&batman_if->refcount))
+ call_rcu(&batman_if->rcu, hardif_free_rcu);
}
#endif /* _NET_BATMAN_ADV_HARD_INTERFACE_H_ */
diff --git a/batman-adv/types.h b/batman-adv/types.h
index 8df6d9d..317ede8 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -43,7 +43,7 @@ struct batman_if {
unsigned char *packet_buff;
int packet_len;
struct kobject *hardif_obj;
- struct kref refcount;
+ atomic_t refcount;
struct packet_type batman_adv_ptype;
struct net_device *soft_iface;
struct rcu_head rcu;
--
1.7.2.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [B.A.T.M.A.N.] [PATCH 4/4] batman-adv: Correct rcu refcounting for neigh_node
2011-01-30 1:52 [B.A.T.M.A.N.] Reordered rcu refcounting Sven Eckelmann
` (2 preceding siblings ...)
2011-01-30 1:52 ` [B.A.T.M.A.N.] [PATCH 3/4] batman-adv: Correct rcu refcounting for batman_if Sven Eckelmann
@ 2011-01-30 1:52 ` Sven Eckelmann
2011-01-30 15:20 ` [B.A.T.M.A.N.] [PATCHv2 " Sven Eckelmann
2011-02-03 17:02 ` [B.A.T.M.A.N.] Reordered rcu refcounting Marek Lindner
4 siblings, 1 reply; 44+ messages in thread
From: Sven Eckelmann @ 2011-01-30 1:52 UTC (permalink / raw)
To: b.a.t.m.a.n
<TODO: write a long monologue about every problem we have or could have or
maybe never had and would have when we not have it>
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
batman-adv/icmp_socket.c | 7 ++--
batman-adv/originator.c | 26 +++++----------
batman-adv/originator.h | 3 +-
batman-adv/routing.c | 79 ++++++++++++++++++++++++++++-----------------
batman-adv/types.h | 3 +-
batman-adv/unicast.c | 2 +-
batman-adv/vis.c | 7 ++--
7 files changed, 68 insertions(+), 59 deletions(-)
diff --git a/batman-adv/icmp_socket.c b/batman-adv/icmp_socket.c
index 10a969c..046f976 100644
--- a/batman-adv/icmp_socket.c
+++ b/batman-adv/icmp_socket.c
@@ -230,10 +230,11 @@ static ssize_t bat_socket_write(struct file *file, const char __user *buff,
kref_get(&orig_node->refcount);
neigh_node = orig_node->router;
- if (!neigh_node)
+ if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
goto unlock;
+ }
- kref_get(&neigh_node->refcount);
rcu_read_unlock();
if (!neigh_node->if_incoming)
@@ -261,7 +262,7 @@ free_skb:
kfree_skb(skb);
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return len;
diff --git a/batman-adv/originator.c b/batman-adv/originator.c
index e8a8473..bde9778 100644
--- a/batman-adv/originator.c
+++ b/batman-adv/originator.c
@@ -56,28 +56,18 @@ err:
return 0;
}
-void neigh_node_free_ref(struct kref *refcount)
-{
- struct neigh_node *neigh_node;
-
- neigh_node = container_of(refcount, struct neigh_node, refcount);
- kfree(neigh_node);
-}
-
static void neigh_node_free_rcu(struct rcu_head *rcu)
{
struct neigh_node *neigh_node;
neigh_node = container_of(rcu, struct neigh_node, rcu);
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ kfree(neigh_node);
}
-void neigh_node_free_rcu_bond(struct rcu_head *rcu)
+void neigh_node_free_ref(struct neigh_node *neigh_node)
{
- struct neigh_node *neigh_node;
-
- neigh_node = container_of(rcu, struct neigh_node, rcu_bond);
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ if (atomic_dec_and_test(&neigh_node->refcount))
+ call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
}
struct neigh_node *create_neighbor(struct orig_node *orig_node,
@@ -101,7 +91,7 @@ struct neigh_node *create_neighbor(struct orig_node *orig_node,
memcpy(neigh_node->addr, neigh, ETH_ALEN);
neigh_node->orig_node = orig_neigh_node;
neigh_node->if_incoming = if_incoming;
- kref_init(&neigh_node->refcount);
+ atomic_set(&neigh_node->refcount, 1);
spin_lock_bh(&orig_node->neigh_list_lock);
hlist_add_head_rcu(&neigh_node->list, &orig_node->neigh_list);
@@ -123,14 +113,14 @@ void orig_node_free_ref(struct kref *refcount)
list_for_each_entry_safe(neigh_node, tmp_neigh_node,
&orig_node->bond_list, bonding_list) {
list_del_rcu(&neigh_node->bonding_list);
- call_rcu(&neigh_node->rcu_bond, neigh_node_free_rcu_bond);
+ neigh_node_free_ref(neigh_node);
}
/* for all neighbors towards this originator ... */
hlist_for_each_entry_safe(neigh_node, node, node_tmp,
&orig_node->neigh_list, list) {
hlist_del_rcu(&neigh_node->list);
- call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
+ neigh_node_free_ref(neigh_node);
}
spin_unlock_bh(&orig_node->neigh_list_lock);
@@ -311,7 +301,7 @@ static bool purge_orig_neighbors(struct bat_priv *bat_priv,
hlist_del_rcu(&neigh_node->list);
bonding_candidate_del(orig_node, neigh_node);
- call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
+ neigh_node_free_ref(neigh_node);
} else {
if ((!*best_neigh_node) ||
(neigh_node->tq_avg > (*best_neigh_node)->tq_avg))
diff --git a/batman-adv/originator.h b/batman-adv/originator.h
index 360dfd1..84d96e2 100644
--- a/batman-adv/originator.h
+++ b/batman-adv/originator.h
@@ -26,13 +26,12 @@ int originator_init(struct bat_priv *bat_priv);
void originator_free(struct bat_priv *bat_priv);
void purge_orig_ref(struct bat_priv *bat_priv);
void orig_node_free_ref(struct kref *refcount);
-void neigh_node_free_rcu_bond(struct rcu_head *rcu);
struct orig_node *get_orig_node(struct bat_priv *bat_priv, uint8_t *addr);
struct neigh_node *create_neighbor(struct orig_node *orig_node,
struct orig_node *orig_neigh_node,
uint8_t *neigh,
struct batman_if *if_incoming);
-void neigh_node_free_ref(struct kref *refcount);
+void neigh_node_free_ref(struct neigh_node *neigh_node);
int orig_seq_print_text(struct seq_file *seq, void *offset);
int orig_hash_add_if(struct batman_if *batman_if, int max_if_num);
int orig_hash_del_if(struct batman_if *batman_if, int max_if_num);
diff --git a/batman-adv/routing.c b/batman-adv/routing.c
index 2861f18..29998cf 100644
--- a/batman-adv/routing.c
+++ b/batman-adv/routing.c
@@ -118,12 +118,12 @@ static void update_route(struct bat_priv *bat_priv,
orig_node->router->addr);
}
- if (neigh_node)
- kref_get(&neigh_node->refcount);
+ if (neigh_node && !atomic_inc_not_zero(&neigh_node->refcount))
+ neigh_node = NULL;
neigh_node_tmp = orig_node->router;
orig_node->router = neigh_node;
if (neigh_node_tmp)
- kref_put(&neigh_node_tmp->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node_tmp);
}
@@ -172,10 +172,12 @@ static int is_bidirectional_neigh(struct orig_node *orig_node,
orig_neigh_node->orig,
if_incoming);
/* create_neighbor failed, return 0 */
- if (!neigh_node)
+ if (!neigh_node ||
+ !atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
goto unlock;
+ }
- kref_get(&neigh_node->refcount);
rcu_read_unlock();
neigh_node->last_valid = jiffies;
@@ -197,10 +199,12 @@ static int is_bidirectional_neigh(struct orig_node *orig_node,
orig_neigh_node->orig,
if_incoming);
/* create_neighbor failed, return 0 */
- if (!neigh_node)
+ if (!neigh_node ||
+ !atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
goto unlock;
+ }
- kref_get(&neigh_node->refcount);
rcu_read_unlock();
}
@@ -262,7 +266,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
return ret;
}
@@ -275,7 +279,7 @@ void bonding_candidate_del(struct orig_node *orig_node,
goto out;
list_del_rcu(&neigh_node->bonding_list);
- call_rcu(&neigh_node->rcu_bond, neigh_node_free_rcu_bond);
+ neigh_node_free_ref(neigh_node);
INIT_LIST_HEAD(&neigh_node->bonding_list);
atomic_dec(&orig_node->bond_candidates);
@@ -337,8 +341,10 @@ static void bonding_candidate_add(struct orig_node *orig_node,
if (!list_empty(&neigh_node->bonding_list))
goto out;
+ if (!atomic_inc_not_zero(&neigh_node->refcount))
+ goto out;
+
list_add_rcu(&neigh_node->bonding_list, &orig_node->bond_list);
- kref_get(&neigh_node->refcount);
atomic_inc(&orig_node->bond_candidates);
goto out;
@@ -382,7 +388,10 @@ static void update_orig(struct bat_priv *bat_priv,
hlist_for_each_entry_rcu(tmp_neigh_node, node,
&orig_node->neigh_list, list) {
if (compare_orig(tmp_neigh_node->addr, ethhdr->h_source) &&
- (tmp_neigh_node->if_incoming == if_incoming)) {
+ (tmp_neigh_node->if_incoming == if_incoming) &&
+ atomic_inc_not_zero(&tmp_neigh_node->refcount)) {
+ if (neigh_node)
+ neigh_node_free_ref(neigh_node);
neigh_node = tmp_neigh_node;
continue;
}
@@ -407,13 +416,15 @@ static void update_orig(struct bat_priv *bat_priv,
ethhdr->h_source, if_incoming);
kref_put(&orig_tmp->refcount, orig_node_free_ref);
- if (!neigh_node)
+ if (!neigh_node ||
+ !atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
goto unlock;
+ }
} else
bat_dbg(DBG_BATMAN, bat_priv,
"Updating existing last-hop neighbor of originator\n");
- kref_get(&neigh_node->refcount);
rcu_read_unlock();
orig_node->flags = batman_packet->flags;
@@ -490,7 +501,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
}
/* checks whether the host restarted and is in the protection time.
@@ -891,10 +902,11 @@ static int recv_my_icmp_packet(struct bat_priv *bat_priv,
kref_get(&orig_node->refcount);
neigh_node = orig_node->router;
- if (!neigh_node)
+ if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
goto unlock;
+ }
- kref_get(&neigh_node->refcount);
rcu_read_unlock();
/* create a copy of the skb, if needed, to modify it. */
@@ -917,7 +929,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return ret;
@@ -955,10 +967,11 @@ static int recv_icmp_ttl_exceeded(struct bat_priv *bat_priv,
kref_get(&orig_node->refcount);
neigh_node = orig_node->router;
- if (!neigh_node)
+ if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
goto unlock;
+ }
- kref_get(&neigh_node->refcount);
rcu_read_unlock();
/* create a copy of the skb, if needed, to modify it. */
@@ -981,7 +994,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return ret;
@@ -1051,10 +1064,11 @@ int recv_icmp_packet(struct sk_buff *skb, struct batman_if *recv_if)
kref_get(&orig_node->refcount);
neigh_node = orig_node->router;
- if (!neigh_node)
+ if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
goto unlock;
+ }
- kref_get(&neigh_node->refcount);
rcu_read_unlock();
/* create a copy of the skb, if needed, to modify it. */
@@ -1075,7 +1089,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return ret;
@@ -1108,7 +1122,7 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
/* select default router to output */
router = orig_node->router;
router_orig = orig_node->router->orig_node;
- if (!router_orig) {
+ if (!router_orig || !atomic_inc_not_zero(&router->refcount)) {
rcu_read_unlock();
return NULL;
}
@@ -1146,6 +1160,7 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
* is is not on the interface where the packet came
* in. */
+ neigh_node_free_ref(router);
first_candidate = NULL;
router = NULL;
@@ -1158,14 +1173,16 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
if (!first_candidate)
first_candidate = tmp_neigh_node;
/* recv_if == NULL on the first node. */
- if (tmp_neigh_node->if_incoming != recv_if) {
+ if (tmp_neigh_node->if_incoming != recv_if &&
+ atomic_inc_not_zero(&tmp_neigh_node->refcount)) {
router = tmp_neigh_node;
break;
}
}
/* use the first candidate if nothing was found. */
- if (!router)
+ if (!router && first_candidate &&
+ atomic_inc_not_zero(&first_candidate->refcount))
router = first_candidate;
/* selected should point to the next element
@@ -1174,6 +1191,7 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
/* this is a list_move(), which unfortunately
* does not exist as rcu version */
list_del_rcu(&primary_orig_node->bond_list);
+ /* BUG: router can be NULL */
list_add_rcu(&primary_orig_node->bond_list,
&router->bonding_list);
spin_unlock_bh(&primary_orig_node->neigh_list_lock);
@@ -1188,7 +1206,8 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
first_candidate = tmp_neigh_node;
/* recv_if == NULL on the first node. */
- if (tmp_neigh_node->if_incoming != recv_if)
+ if (tmp_neigh_node->if_incoming != recv_if &&
+ atomic_inc_not_zero(&tmp_neigh_node->refcount))
/* if we don't have a router yet
* or this one is better, choose it. */
if ((!router) ||
@@ -1198,11 +1217,11 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
}
/* use the first candidate if nothing was found. */
- if (!router)
+ if (!router && first_candidate &&
+ atomic_inc_not_zero(&first_candidate->refcount))
router = first_candidate;
}
return_router:
- kref_get(&router->refcount);
rcu_read_unlock();
return router;
}
@@ -1315,7 +1334,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return ret;
diff --git a/batman-adv/types.h b/batman-adv/types.h
index 317ede8..ee77d48 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -119,9 +119,8 @@ struct neigh_node {
struct list_head bonding_list;
unsigned long last_valid;
unsigned long real_bits[NUM_WORDS];
- struct kref refcount;
+ atomic_t refcount;
struct rcu_head rcu;
- struct rcu_head rcu_bond;
struct orig_node *orig_node;
struct batman_if *if_incoming;
};
diff --git a/batman-adv/unicast.c b/batman-adv/unicast.c
index 6a9ab61..580b547 100644
--- a/batman-adv/unicast.c
+++ b/batman-adv/unicast.c
@@ -363,7 +363,7 @@ find_router:
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
if (ret == 1)
diff --git a/batman-adv/vis.c b/batman-adv/vis.c
index 191401b..fe32de3 100644
--- a/batman-adv/vis.c
+++ b/batman-adv/vis.c
@@ -773,10 +773,11 @@ static void unicast_vis_packet(struct bat_priv *bat_priv,
kref_get(&orig_node->refcount);
neigh_node = orig_node->router;
- if (!neigh_node)
+ if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
goto unlock;
+ }
- kref_get(&neigh_node->refcount);
rcu_read_unlock();
skb = skb_clone(info->skb_packet, GFP_ATOMIC);
@@ -790,7 +791,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return;
--
1.7.2.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [B.A.T.M.A.N.] [PATCHv2 4/4] batman-adv: Correct rcu refcounting for neigh_node
2011-01-30 1:52 ` [B.A.T.M.A.N.] [PATCH 4/4] batman-adv: Correct rcu refcounting for neigh_node Sven Eckelmann
@ 2011-01-30 15:20 ` Sven Eckelmann
0 siblings, 0 replies; 44+ messages in thread
From: Sven Eckelmann @ 2011-01-30 15:20 UTC (permalink / raw)
To: b.a.t.m.a.n
<TODO: write a long monologue about every problem we have or could have or
maybe never had and would have when we not have it>
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
Removed the "BUG" after a small conversation with d0tslash
batman-adv/icmp_socket.c | 7 ++--
batman-adv/originator.c | 26 ++++----------
batman-adv/originator.h | 3 +-
batman-adv/routing.c | 83 +++++++++++++++++++++++++++++----------------
batman-adv/types.h | 3 +-
batman-adv/unicast.c | 2 +-
batman-adv/vis.c | 7 ++--
7 files changed, 72 insertions(+), 59 deletions(-)
diff --git a/batman-adv/icmp_socket.c b/batman-adv/icmp_socket.c
index 10a969c..046f976 100644
--- a/batman-adv/icmp_socket.c
+++ b/batman-adv/icmp_socket.c
@@ -230,10 +230,11 @@ static ssize_t bat_socket_write(struct file *file, const char __user *buff,
kref_get(&orig_node->refcount);
neigh_node = orig_node->router;
- if (!neigh_node)
+ if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
goto unlock;
+ }
- kref_get(&neigh_node->refcount);
rcu_read_unlock();
if (!neigh_node->if_incoming)
@@ -261,7 +262,7 @@ free_skb:
kfree_skb(skb);
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return len;
diff --git a/batman-adv/originator.c b/batman-adv/originator.c
index e8a8473..bde9778 100644
--- a/batman-adv/originator.c
+++ b/batman-adv/originator.c
@@ -56,28 +56,18 @@ err:
return 0;
}
-void neigh_node_free_ref(struct kref *refcount)
-{
- struct neigh_node *neigh_node;
-
- neigh_node = container_of(refcount, struct neigh_node, refcount);
- kfree(neigh_node);
-}
-
static void neigh_node_free_rcu(struct rcu_head *rcu)
{
struct neigh_node *neigh_node;
neigh_node = container_of(rcu, struct neigh_node, rcu);
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ kfree(neigh_node);
}
-void neigh_node_free_rcu_bond(struct rcu_head *rcu)
+void neigh_node_free_ref(struct neigh_node *neigh_node)
{
- struct neigh_node *neigh_node;
-
- neigh_node = container_of(rcu, struct neigh_node, rcu_bond);
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ if (atomic_dec_and_test(&neigh_node->refcount))
+ call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
}
struct neigh_node *create_neighbor(struct orig_node *orig_node,
@@ -101,7 +91,7 @@ struct neigh_node *create_neighbor(struct orig_node *orig_node,
memcpy(neigh_node->addr, neigh, ETH_ALEN);
neigh_node->orig_node = orig_neigh_node;
neigh_node->if_incoming = if_incoming;
- kref_init(&neigh_node->refcount);
+ atomic_set(&neigh_node->refcount, 1);
spin_lock_bh(&orig_node->neigh_list_lock);
hlist_add_head_rcu(&neigh_node->list, &orig_node->neigh_list);
@@ -123,14 +113,14 @@ void orig_node_free_ref(struct kref *refcount)
list_for_each_entry_safe(neigh_node, tmp_neigh_node,
&orig_node->bond_list, bonding_list) {
list_del_rcu(&neigh_node->bonding_list);
- call_rcu(&neigh_node->rcu_bond, neigh_node_free_rcu_bond);
+ neigh_node_free_ref(neigh_node);
}
/* for all neighbors towards this originator ... */
hlist_for_each_entry_safe(neigh_node, node, node_tmp,
&orig_node->neigh_list, list) {
hlist_del_rcu(&neigh_node->list);
- call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
+ neigh_node_free_ref(neigh_node);
}
spin_unlock_bh(&orig_node->neigh_list_lock);
@@ -311,7 +301,7 @@ static bool purge_orig_neighbors(struct bat_priv *bat_priv,
hlist_del_rcu(&neigh_node->list);
bonding_candidate_del(orig_node, neigh_node);
- call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
+ neigh_node_free_ref(neigh_node);
} else {
if ((!*best_neigh_node) ||
(neigh_node->tq_avg > (*best_neigh_node)->tq_avg))
diff --git a/batman-adv/originator.h b/batman-adv/originator.h
index 360dfd1..84d96e2 100644
--- a/batman-adv/originator.h
+++ b/batman-adv/originator.h
@@ -26,13 +26,12 @@ int originator_init(struct bat_priv *bat_priv);
void originator_free(struct bat_priv *bat_priv);
void purge_orig_ref(struct bat_priv *bat_priv);
void orig_node_free_ref(struct kref *refcount);
-void neigh_node_free_rcu_bond(struct rcu_head *rcu);
struct orig_node *get_orig_node(struct bat_priv *bat_priv, uint8_t *addr);
struct neigh_node *create_neighbor(struct orig_node *orig_node,
struct orig_node *orig_neigh_node,
uint8_t *neigh,
struct batman_if *if_incoming);
-void neigh_node_free_ref(struct kref *refcount);
+void neigh_node_free_ref(struct neigh_node *neigh_node);
int orig_seq_print_text(struct seq_file *seq, void *offset);
int orig_hash_add_if(struct batman_if *batman_if, int max_if_num);
int orig_hash_del_if(struct batman_if *batman_if, int max_if_num);
diff --git a/batman-adv/routing.c b/batman-adv/routing.c
index 2861f18..94e9c1c 100644
--- a/batman-adv/routing.c
+++ b/batman-adv/routing.c
@@ -118,12 +118,12 @@ static void update_route(struct bat_priv *bat_priv,
orig_node->router->addr);
}
- if (neigh_node)
- kref_get(&neigh_node->refcount);
+ if (neigh_node && !atomic_inc_not_zero(&neigh_node->refcount))
+ neigh_node = NULL;
neigh_node_tmp = orig_node->router;
orig_node->router = neigh_node;
if (neigh_node_tmp)
- kref_put(&neigh_node_tmp->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node_tmp);
}
@@ -172,10 +172,12 @@ static int is_bidirectional_neigh(struct orig_node *orig_node,
orig_neigh_node->orig,
if_incoming);
/* create_neighbor failed, return 0 */
- if (!neigh_node)
+ if (!neigh_node ||
+ !atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
goto unlock;
+ }
- kref_get(&neigh_node->refcount);
rcu_read_unlock();
neigh_node->last_valid = jiffies;
@@ -197,10 +199,12 @@ static int is_bidirectional_neigh(struct orig_node *orig_node,
orig_neigh_node->orig,
if_incoming);
/* create_neighbor failed, return 0 */
- if (!neigh_node)
+ if (!neigh_node ||
+ !atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
goto unlock;
+ }
- kref_get(&neigh_node->refcount);
rcu_read_unlock();
}
@@ -262,7 +266,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
return ret;
}
@@ -275,7 +279,7 @@ void bonding_candidate_del(struct orig_node *orig_node,
goto out;
list_del_rcu(&neigh_node->bonding_list);
- call_rcu(&neigh_node->rcu_bond, neigh_node_free_rcu_bond);
+ neigh_node_free_ref(neigh_node);
INIT_LIST_HEAD(&neigh_node->bonding_list);
atomic_dec(&orig_node->bond_candidates);
@@ -337,8 +341,10 @@ static void bonding_candidate_add(struct orig_node *orig_node,
if (!list_empty(&neigh_node->bonding_list))
goto out;
+ if (!atomic_inc_not_zero(&neigh_node->refcount))
+ goto out;
+
list_add_rcu(&neigh_node->bonding_list, &orig_node->bond_list);
- kref_get(&neigh_node->refcount);
atomic_inc(&orig_node->bond_candidates);
goto out;
@@ -382,7 +388,10 @@ static void update_orig(struct bat_priv *bat_priv,
hlist_for_each_entry_rcu(tmp_neigh_node, node,
&orig_node->neigh_list, list) {
if (compare_orig(tmp_neigh_node->addr, ethhdr->h_source) &&
- (tmp_neigh_node->if_incoming == if_incoming)) {
+ (tmp_neigh_node->if_incoming == if_incoming) &&
+ atomic_inc_not_zero(&tmp_neigh_node->refcount)) {
+ if (neigh_node)
+ neigh_node_free_ref(neigh_node);
neigh_node = tmp_neigh_node;
continue;
}
@@ -407,13 +416,15 @@ static void update_orig(struct bat_priv *bat_priv,
ethhdr->h_source, if_incoming);
kref_put(&orig_tmp->refcount, orig_node_free_ref);
- if (!neigh_node)
+ if (!neigh_node ||
+ !atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
goto unlock;
+ }
} else
bat_dbg(DBG_BATMAN, bat_priv,
"Updating existing last-hop neighbor of originator\n");
- kref_get(&neigh_node->refcount);
rcu_read_unlock();
orig_node->flags = batman_packet->flags;
@@ -490,7 +501,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
}
/* checks whether the host restarted and is in the protection time.
@@ -891,10 +902,11 @@ static int recv_my_icmp_packet(struct bat_priv *bat_priv,
kref_get(&orig_node->refcount);
neigh_node = orig_node->router;
- if (!neigh_node)
+ if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
goto unlock;
+ }
- kref_get(&neigh_node->refcount);
rcu_read_unlock();
/* create a copy of the skb, if needed, to modify it. */
@@ -917,7 +929,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return ret;
@@ -955,10 +967,11 @@ static int recv_icmp_ttl_exceeded(struct bat_priv *bat_priv,
kref_get(&orig_node->refcount);
neigh_node = orig_node->router;
- if (!neigh_node)
+ if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
goto unlock;
+ }
- kref_get(&neigh_node->refcount);
rcu_read_unlock();
/* create a copy of the skb, if needed, to modify it. */
@@ -981,7 +994,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return ret;
@@ -1051,10 +1064,11 @@ int recv_icmp_packet(struct sk_buff *skb, struct batman_if *recv_if)
kref_get(&orig_node->refcount);
neigh_node = orig_node->router;
- if (!neigh_node)
+ if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
goto unlock;
+ }
- kref_get(&neigh_node->refcount);
rcu_read_unlock();
/* create a copy of the skb, if needed, to modify it. */
@@ -1075,7 +1089,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return ret;
@@ -1108,7 +1122,7 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
/* select default router to output */
router = orig_node->router;
router_orig = orig_node->router->orig_node;
- if (!router_orig) {
+ if (!router_orig || !atomic_inc_not_zero(&router->refcount)) {
rcu_read_unlock();
return NULL;
}
@@ -1146,6 +1160,7 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
* is is not on the interface where the packet came
* in. */
+ neigh_node_free_ref(router);
first_candidate = NULL;
router = NULL;
@@ -1158,16 +1173,23 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
if (!first_candidate)
first_candidate = tmp_neigh_node;
/* recv_if == NULL on the first node. */
- if (tmp_neigh_node->if_incoming != recv_if) {
+ if (tmp_neigh_node->if_incoming != recv_if &&
+ atomic_inc_not_zero(&tmp_neigh_node->refcount)) {
router = tmp_neigh_node;
break;
}
}
/* use the first candidate if nothing was found. */
- if (!router)
+ if (!router && first_candidate &&
+ atomic_inc_not_zero(&first_candidate->refcount))
router = first_candidate;
+ if (!router) {
+ rcu_read_unlock();
+ return NULL;
+ }
+
/* selected should point to the next element
* after the current router */
spin_lock_bh(&primary_orig_node->neigh_list_lock);
@@ -1188,7 +1210,8 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
first_candidate = tmp_neigh_node;
/* recv_if == NULL on the first node. */
- if (tmp_neigh_node->if_incoming != recv_if)
+ if (tmp_neigh_node->if_incoming != recv_if &&
+ atomic_inc_not_zero(&tmp_neigh_node->refcount))
/* if we don't have a router yet
* or this one is better, choose it. */
if ((!router) ||
@@ -1198,11 +1221,11 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
}
/* use the first candidate if nothing was found. */
- if (!router)
+ if (!router && first_candidate &&
+ atomic_inc_not_zero(&first_candidate->refcount))
router = first_candidate;
}
return_router:
- kref_get(&router->refcount);
rcu_read_unlock();
return router;
}
@@ -1315,7 +1338,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return ret;
diff --git a/batman-adv/types.h b/batman-adv/types.h
index 317ede8..ee77d48 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -119,9 +119,8 @@ struct neigh_node {
struct list_head bonding_list;
unsigned long last_valid;
unsigned long real_bits[NUM_WORDS];
- struct kref refcount;
+ atomic_t refcount;
struct rcu_head rcu;
- struct rcu_head rcu_bond;
struct orig_node *orig_node;
struct batman_if *if_incoming;
};
diff --git a/batman-adv/unicast.c b/batman-adv/unicast.c
index 6a9ab61..580b547 100644
--- a/batman-adv/unicast.c
+++ b/batman-adv/unicast.c
@@ -363,7 +363,7 @@ find_router:
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
if (ret == 1)
diff --git a/batman-adv/vis.c b/batman-adv/vis.c
index 191401b..fe32de3 100644
--- a/batman-adv/vis.c
+++ b/batman-adv/vis.c
@@ -773,10 +773,11 @@ static void unicast_vis_packet(struct bat_priv *bat_priv,
kref_get(&orig_node->refcount);
neigh_node = orig_node->router;
- if (!neigh_node)
+ if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
goto unlock;
+ }
- kref_get(&neigh_node->refcount);
rcu_read_unlock();
skb = skb_clone(info->skb_packet, GFP_ATOMIC);
@@ -790,7 +791,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return;
--
1.7.2.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [B.A.T.M.A.N.] [PATCH] Re: batman-adv: Correct rcu refcounting for gw_node
2011-01-30 1:52 ` [B.A.T.M.A.N.] [PATCH 1/4] batman-adv: Correct rcu refcounting for gw_node Sven Eckelmann
@ 2011-02-02 17:37 ` Linus Lüssing
2011-02-02 19:49 ` Marek Lindner
` (4 more replies)
0 siblings, 5 replies; 44+ messages in thread
From: Linus Lüssing @ 2011-02-02 17:37 UTC (permalink / raw)
To: b.a.t.m.a.n
From: Sven Eckelmann <sven@narfation.org>
Was:
---
<TODO: write a long monologue about every problem we have or could have or
maybe never had and would have when we not have it>
Signed-off-by: Sven Eckelmann <sven@narfation.org>
---
So after some more discussions with Marek and Sven, it looks like we
have to use the rcu protected macros rcu_dereference() and
rcu_assign_pointer() for the bat_priv->curr_gw and curr_gw->orig_node.
Changes here also include moving the kref_get() from unicast_send_skb()
into gw_get_selected(). The orig_node could have been freed already at
the time the kref_get() was called in unicast_send_skb().
Some things that are still not that clear to me:
gw_election():
* can the if-block before gw_deselect() be ommited, we had a nullpointer
check for curr_gw just a couple of lines before during the rcu-lock.
gw_deselet():
* is the refcount at this time always 1 for gw_node, can the null
pointer check + a rcu_dereference be ommited? (at least that's what
it looks like when comparing to the rcuref.txt example)
gw_get_selected():
* Probably the orig_node's refcounting has to be made atomic, too?
Cheers, Linus
Not-Signed-off-by: Linus Lüssing <linus.luessing@ascom.ch>
---
gateway_client.c | 169 +++++++++++++++++++++++++++++++++---------------------
main.c | 1 +
types.h | 7 +-
unicast.c | 1 -
4 files changed, 109 insertions(+), 69 deletions(-)
diff --git a/gateway_client.c b/gateway_client.c
index 429a013..96a67bc 100644
--- a/gateway_client.c
+++ b/gateway_client.c
@@ -28,40 +28,54 @@
#include <linux/udp.h>
#include <linux/if_vlan.h>
-static void gw_node_free_ref(struct kref *refcount)
+static void gw_node_free_rcu(struct rcu_head *rcu)
{
struct gw_node *gw_node;
- gw_node = container_of(refcount, struct gw_node, refcount);
+ gw_node = container_of(rcu, struct gw_node, rcu);
kfree(gw_node);
}
-static void gw_node_free_rcu(struct rcu_head *rcu)
+static void gw_node_free_ref(struct gw_node *gw_node)
{
- struct gw_node *gw_node;
-
- gw_node = container_of(rcu, struct gw_node, rcu);
- kref_put(&gw_node->refcount, gw_node_free_ref);
+ if (atomic_dec_and_test(&gw_node->refcount))
+ call_rcu(&gw_node->rcu, gw_node_free_rcu);
}
+/* increases the returned orig_node's refcount */
void *gw_get_selected(struct bat_priv *bat_priv)
{
- struct gw_node *curr_gateway_tmp = bat_priv->curr_gw;
+ struct gw_node *curr_gateway_tmp;
+ struct orig_node *orig_node;
- if (!curr_gateway_tmp)
+ rcu_read_lock();
+ curr_gateway_tmp = rcu_dereference(bat_priv->curr_gw);
+ if (!curr_gateway_tmp) {
+ rcu_read_unlock();
return NULL;
+ }
- return curr_gateway_tmp->orig_node;
+ orig_node = rcu_dereference(curr_gateway_tmp->orig_node);
+ if (orig_node) {
+ kref_get(&orig_node->refcount);
+ rcu_read_unlock();
+ return NULL;
+ }
+
+ rcu_read_unlock();
+ return orig_node;
}
void gw_deselect(struct bat_priv *bat_priv)
{
- struct gw_node *gw_node = bat_priv->curr_gw;
+ struct gw_node *gw_node;
- bat_priv->curr_gw = NULL;
+ spin_lock_bh(&bat_priv->curr_gw_lock);
+ gw_node = bat_priv->curr_gw;
+ rcu_assign_pointer(bat_priv->curr_gw, NULL);
+ spin_unlock_bh(&bat_priv->curr_gw_lock);
- if (gw_node)
- kref_put(&gw_node->refcount, gw_node_free_ref);
+ gw_node_free_ref(gw_node);
}
static struct gw_node *gw_select(struct bat_priv *bat_priv,
@@ -69,17 +83,21 @@ static struct gw_node *gw_select(struct bat_priv *bat_priv,
{
struct gw_node *curr_gw_node = bat_priv->curr_gw;
- if (new_gw_node)
- kref_get(&new_gw_node->refcount);
+ if (new_gw_node && !atomic_inc_not_zero(&new_gw_node->refcount))
+ return NULL;
+
+ spin_lock_bh(&bat_priv->curr_gw_lock);
+ rcu_assign_pointer(bat_priv->curr_gw, new_gw_node);
+ spin_unlock_bh(&bat_priv->curr_gw_lock);
- bat_priv->curr_gw = new_gw_node;
return curr_gw_node;
}
void gw_election(struct bat_priv *bat_priv)
{
struct hlist_node *node;
- struct gw_node *gw_node, *curr_gw_tmp = NULL, *old_gw_node = NULL;
+ struct gw_node *gw_node, *curr_gw, *curr_gw_tmp = NULL, *old_gw_node = NULL;
+ struct orig_node *orig_node;
uint8_t max_tq = 0;
uint32_t max_gw_factor = 0, tmp_gw_factor = 0;
int down, up;
@@ -93,25 +111,28 @@ void gw_election(struct bat_priv *bat_priv)
if (atomic_read(&bat_priv->gw_mode) != GW_MODE_CLIENT)
return;
- if (bat_priv->curr_gw)
- return;
-
rcu_read_lock();
- if (hlist_empty(&bat_priv->gw_list)) {
+ curr_gw = rcu_dereference(bat_priv->curr_gw);
+ if (curr_gw) {
rcu_read_unlock();
+ return;
+ }
- if (bat_priv->curr_gw) {
+ if (hlist_empty(&bat_priv->gw_list)) {
+ if (curr_gw) {
bat_dbg(DBG_BATMAN, bat_priv,
"Removing selected gateway - "
"no gateway in range\n");
gw_deselect(bat_priv);
}
+ rcu_read_unlock();
return;
}
hlist_for_each_entry_rcu(gw_node, node, &bat_priv->gw_list, list) {
- if (!gw_node->orig_node->router)
+ orig_node = rcu_dereference(gw_node->orig_node);
+ if (!orig_node->router)
continue;
if (gw_node->deleted)
@@ -119,18 +140,17 @@ void gw_election(struct bat_priv *bat_priv)
switch (atomic_read(&bat_priv->gw_sel_class)) {
case 1: /* fast connection */
- gw_bandwidth_to_kbit(gw_node->orig_node->gw_flags,
- &down, &up);
+ gw_bandwidth_to_kbit(orig_node->gw_flags, &down, &up);
- tmp_gw_factor = (gw_node->orig_node->router->tq_avg *
- gw_node->orig_node->router->tq_avg *
+ tmp_gw_factor = (orig_node->router->tq_avg *
+ orig_node->router->tq_avg *
down * 100 * 100) /
(TQ_LOCAL_WINDOW_SIZE *
TQ_LOCAL_WINDOW_SIZE * 64);
if ((tmp_gw_factor > max_gw_factor) ||
((tmp_gw_factor == max_gw_factor) &&
- (gw_node->orig_node->router->tq_avg > max_tq)))
+ (orig_node->router->tq_avg > max_tq)))
curr_gw_tmp = gw_node;
break;
@@ -142,37 +162,38 @@ void gw_election(struct bat_priv *bat_priv)
* soon as a better gateway appears which has
* $routing_class more tq points)
**/
- if (gw_node->orig_node->router->tq_avg > max_tq)
+ if (orig_node->router->tq_avg > max_tq)
curr_gw_tmp = gw_node;
break;
}
- if (gw_node->orig_node->router->tq_avg > max_tq)
- max_tq = gw_node->orig_node->router->tq_avg;
+ if (orig_node->router->tq_avg > max_tq)
+ max_tq = orig_node->router->tq_avg;
if (tmp_gw_factor > max_gw_factor)
max_gw_factor = tmp_gw_factor;
}
- if (bat_priv->curr_gw != curr_gw_tmp) {
- if ((bat_priv->curr_gw) && (!curr_gw_tmp))
+ if (curr_gw != curr_gw_tmp) {
+ orig_node = rcu_dereference(curr_gw_tmp->orig_node);
+ if ((curr_gw) && (!curr_gw_tmp))
bat_dbg(DBG_BATMAN, bat_priv,
"Removing selected gateway - "
"no gateway in range\n");
- else if ((!bat_priv->curr_gw) && (curr_gw_tmp))
+ else if ((!curr_gw) && (curr_gw_tmp))
bat_dbg(DBG_BATMAN, bat_priv,
"Adding route to gateway %pM "
"(gw_flags: %i, tq: %i)\n",
- curr_gw_tmp->orig_node->orig,
- curr_gw_tmp->orig_node->gw_flags,
- curr_gw_tmp->orig_node->router->tq_avg);
+ orig_node->orig,
+ orig_node->gw_flags,
+ orig_node->router->tq_avg);
else
bat_dbg(DBG_BATMAN, bat_priv,
"Changing route to gateway %pM "
"(gw_flags: %i, tq: %i)\n",
- curr_gw_tmp->orig_node->orig,
- curr_gw_tmp->orig_node->gw_flags,
- curr_gw_tmp->orig_node->router->tq_avg);
+ orig_node->orig,
+ orig_node->gw_flags,
+ orig_node->router->tq_avg);
old_gw_node = gw_select(bat_priv, curr_gw_tmp);
}
@@ -181,36 +202,40 @@ void gw_election(struct bat_priv *bat_priv)
/* the kfree() has to be outside of the rcu lock */
if (old_gw_node)
- kref_put(&old_gw_node->refcount, gw_node_free_ref);
+ gw_node_free_ref(old_gw_node);
}
void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
{
- struct gw_node *curr_gateway_tmp = bat_priv->curr_gw;
+ struct gw_node *curr_gateway_tmp;
+ struct orig_node *curr_gw_orig;
uint8_t gw_tq_avg, orig_tq_avg;
+ rcu_read_lock();
+ curr_gateway_tmp = rcu_dereference(bat_priv->curr_gw);
if (!curr_gateway_tmp)
- return;
+ goto rcu_unlock;
- if (!curr_gateway_tmp->orig_node)
+ curr_gw_orig = rcu_dereference(curr_gateway_tmp->orig_node);
+ if (!curr_gw_orig)
goto deselect;
- if (!curr_gateway_tmp->orig_node->router)
+ if (!curr_gw_orig->router)
goto deselect;
/* this node already is the gateway */
- if (curr_gateway_tmp->orig_node == orig_node)
- return;
+ if (curr_gw_orig == orig_node)
+ goto deselect;
if (!orig_node->router)
- return;
+ goto rcu_unlock;
- gw_tq_avg = curr_gateway_tmp->orig_node->router->tq_avg;
+ gw_tq_avg = curr_gw_orig ->router->tq_avg;
orig_tq_avg = orig_node->router->tq_avg;
/* the TQ value has to be better */
if (orig_tq_avg < gw_tq_avg)
- return;
+ goto rcu_unlock;
/**
* if the routing class is greater than 3 the value tells us how much
@@ -218,7 +243,7 @@ void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
**/
if ((atomic_read(&bat_priv->gw_sel_class) > 3) &&
(orig_tq_avg - gw_tq_avg < atomic_read(&bat_priv->gw_sel_class)))
- return;
+ goto rcu_unlock;
bat_dbg(DBG_BATMAN, bat_priv,
"Restarting gateway selection: better gateway found (tq curr: "
@@ -227,6 +252,8 @@ void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
deselect:
gw_deselect(bat_priv);
+rcu_unlock:
+ rcu_read_unlock();
}
static void gw_node_add(struct bat_priv *bat_priv,
@@ -242,7 +269,7 @@ static void gw_node_add(struct bat_priv *bat_priv,
memset(gw_node, 0, sizeof(struct gw_node));
INIT_HLIST_NODE(&gw_node->list);
gw_node->orig_node = orig_node;
- kref_init(&gw_node->refcount);
+ atomic_set(&gw_node->refcount, 1);
spin_lock_bh(&bat_priv->gw_list_lock);
hlist_add_head_rcu(&gw_node->list, &bat_priv->gw_list);
@@ -325,7 +352,7 @@ void gw_node_purge(struct bat_priv *bat_priv)
gw_deselect(bat_priv);
hlist_del_rcu(&gw_node->list);
- call_rcu(&gw_node->rcu, gw_node_free_rcu);
+ gw_node_free_ref(gw_node);
}
@@ -335,21 +362,29 @@ void gw_node_purge(struct bat_priv *bat_priv)
static int _write_buffer_text(struct bat_priv *bat_priv,
struct seq_file *seq, struct gw_node *gw_node)
{
- int down, up;
+ struct gw_node *curr_gw;
+ struct orig_node *orig_node;
+ int down, up, ret;
- gw_bandwidth_to_kbit(gw_node->orig_node->gw_flags, &down, &up);
-
- return seq_printf(seq, "%s %pM (%3i) %pM [%10s]: %3i - %i%s/%i%s\n",
- (bat_priv->curr_gw == gw_node ? "=>" : " "),
- gw_node->orig_node->orig,
- gw_node->orig_node->router->tq_avg,
- gw_node->orig_node->router->addr,
- gw_node->orig_node->router->if_incoming->net_dev->name,
- gw_node->orig_node->gw_flags,
+ rcu_read_lock();
+ curr_gw = rcu_dereference(bat_priv->curr_gw);
+ orig_node = rcu_dereference(gw_node->orig_node);
+ gw_bandwidth_to_kbit(orig_node->gw_flags, &down, &up);
+
+ ret = seq_printf(seq, "%s %pM (%3i) %pM [%10s]: %3i - %i%s/%i%s\n",
+ (curr_gw == gw_node ? "=>" : " "),
+ orig_node->orig,
+ orig_node->router->tq_avg,
+ orig_node->router->addr,
+ orig_node->router->if_incoming->net_dev->name,
+ orig_node->gw_flags,
(down > 2048 ? down / 1024 : down),
(down > 2048 ? "MBit" : "KBit"),
(up > 2048 ? up / 1024 : up),
(up > 2048 ? "MBit" : "KBit"));
+ rcu_read_unlock();
+
+ return ret;
}
int gw_client_seq_print_text(struct seq_file *seq, void *offset)
@@ -470,8 +505,12 @@ int gw_is_target(struct bat_priv *bat_priv, struct sk_buff *skb)
if (atomic_read(&bat_priv->gw_mode) == GW_MODE_SERVER)
return -1;
- if (!bat_priv->curr_gw)
+ rcu_read_lock();
+ if (!rcu_dereference(bat_priv->curr_gw)) {
+ rcu_read_unlock();
return 0;
+ }
+ rcu_read_unlock();
return 1;
}
diff --git a/main.c b/main.c
index e687e7f..8679260 100644
--- a/main.c
+++ b/main.c
@@ -85,6 +85,7 @@ int mesh_init(struct net_device *soft_iface)
spin_lock_init(&bat_priv->hna_lhash_lock);
spin_lock_init(&bat_priv->hna_ghash_lock);
spin_lock_init(&bat_priv->gw_list_lock);
+ spin_lock_init(&bat_priv->curr_gw_lock);
spin_lock_init(&bat_priv->vis_hash_lock);
spin_lock_init(&bat_priv->vis_list_lock);
spin_lock_init(&bat_priv->softif_neigh_lock);
diff --git a/types.h b/types.h
index e4a0462..b9b20b6 100644
--- a/types.h
+++ b/types.h
@@ -98,9 +98,9 @@ struct orig_node {
struct gw_node {
struct hlist_node list;
- struct orig_node *orig_node;
+ struct orig_node *orig_node; /* rcu protected pointer */
unsigned long deleted;
- struct kref refcount;
+ atomic_t refcount;
struct rcu_head rcu;
};
@@ -163,6 +163,7 @@ struct bat_priv {
spinlock_t hna_lhash_lock; /* protects hna_local_hash */
spinlock_t hna_ghash_lock; /* protects hna_global_hash */
spinlock_t gw_list_lock; /* protects gw_list */
+ spinlock_t curr_gw_lock; /* protects curr_gw updates */
spinlock_t vis_hash_lock; /* protects vis_hash */
spinlock_t vis_list_lock; /* protects vis_info::recv_list */
spinlock_t softif_neigh_lock; /* protects soft-interface neigh list */
@@ -171,7 +172,7 @@ struct bat_priv {
struct delayed_work hna_work;
struct delayed_work orig_work;
struct delayed_work vis_work;
- struct gw_node *curr_gw;
+ struct gw_node *curr_gw; /* rcu protected pointer */
struct vis_info *my_vis_info;
};
diff --git a/unicast.c b/unicast.c
index 6a9ab61..8816102 100644
--- a/unicast.c
+++ b/unicast.c
@@ -298,7 +298,6 @@ int unicast_send_skb(struct sk_buff *skb, struct bat_priv *bat_priv)
if (!orig_node)
goto trans_search;
- kref_get(&orig_node->refcount);
goto find_router;
} else {
rcu_read_lock();
--
1.7.2.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH] Re: batman-adv: Correct rcu refcounting for gw_node
2011-02-02 17:37 ` [B.A.T.M.A.N.] [PATCH] " Linus Lüssing
@ 2011-02-02 19:49 ` Marek Lindner
2011-02-02 20:43 ` Linus Lüssing
2011-02-02 21:42 ` Sven Eckelmann
` (3 subsequent siblings)
4 siblings, 1 reply; 44+ messages in thread
From: Marek Lindner @ 2011-02-02 19:49 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
On Wednesday 02 February 2011 18:37:18 Linus Lüssing wrote:
> So after some more discussions with Marek and Sven, it looks like we
> have to use the rcu protected macros rcu_dereference() and
> rcu_assign_pointer() for the bat_priv->curr_gw and curr_gw->orig_node.
>
> Changes here also include moving the kref_get() from unicast_send_skb()
> into gw_get_selected(). The orig_node could have been freed already at
> the time the kref_get() was called in unicast_send_skb().
I'd suggest you make a standalone patch because the patches address different
problems.
Thanks,
Marek
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH] Re: batman-adv: Correct rcu refcounting for gw_node
2011-02-02 19:49 ` Marek Lindner
@ 2011-02-02 20:43 ` Linus Lüssing
0 siblings, 0 replies; 44+ messages in thread
From: Linus Lüssing @ 2011-02-02 20:43 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
On Wed, Feb 02, 2011 at 08:49:17PM +0100, Marek Lindner wrote:
> On Wednesday 02 February 2011 18:37:18 Linus Lüssing wrote:
> > So after some more discussions with Marek and Sven, it looks like we
> > have to use the rcu protected macros rcu_dereference() and
> > rcu_assign_pointer() for the bat_priv->curr_gw and curr_gw->orig_node.
> >
> > Changes here also include moving the kref_get() from unicast_send_skb()
> > into gw_get_selected(). The orig_node could have been freed already at
> > the time the kref_get() was called in unicast_send_skb().
>
> I'd suggest you make a standalone patch because the patches address different
> problems.
>
> Thanks,
> Marek
>
Oki doki, will do that (wasn't ment to be a clean patch yet anyway
- I wanted to know first if the usage of
rcu_dereference/rcu_assign_pointer() was going in the right
direction).
Cheers, Linus
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH] Re: batman-adv: Correct rcu refcounting for gw_node
2011-02-02 17:37 ` [B.A.T.M.A.N.] [PATCH] " Linus Lüssing
2011-02-02 19:49 ` Marek Lindner
@ 2011-02-02 21:42 ` Sven Eckelmann
2011-02-03 0:19 ` Marek Lindner
2011-02-03 9:55 ` Linus Lüssing
2011-02-03 14:43 ` [B.A.T.M.A.N.] [PATCH 1/3] " Linus Lüssing
` (2 subsequent siblings)
4 siblings, 2 replies; 44+ messages in thread
From: Sven Eckelmann @ 2011-02-02 21:42 UTC (permalink / raw)
To: Linus Lüssing; +Cc: b.a.t.m.a.n
[-- Attachment #1: Type: Text/Plain, Size: 4106 bytes --]
On Wednesday 02 February 2011 18:37:18 Linus Lüssing wrote:
> From: Sven Eckelmann <sven@narfation.org>
>
> Was:
> ---
> <TODO: write a long monologue about every problem we have or could have or
> maybe never had and would have when we not have it>
>
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
>
> So after some more discussions with Marek and Sven, it looks like we
> have to use the rcu protected macros rcu_dereference() and
> rcu_assign_pointer() for the bat_priv->curr_gw and curr_gw->orig_node.
>
> Changes here also include moving the kref_get() from unicast_send_skb()
> into gw_get_selected(). The orig_node could have been freed already at
> the time the kref_get() was called in unicast_send_skb().
>
> Some things that are still not that clear to me:
>
> gw_election():
> * can the if-block before gw_deselect() be ommited, we had a nullpointer
> check for curr_gw just a couple of lines before during the rcu-lock.
I thought that this if block should be moved to gw_select. And your gw_select
still has the bug that the bat_priv->curr_gw isn't set to NULL when
new_gw_node is NULL.
> gw_deselet():
> * is the refcount at this time always 1 for gw_node, can the null
> pointer check + a rcu_dereference be ommited? (at least that's what
> it looks like when comparing to the rcuref.txt example)
Why can't it be NULL? And _always_ use rcu_dereference. What example tells you
that it isn't needed? None of the examples has any kind of rcu pointer in it
(just el as pointer which is stored in a struct were the pointer inside the
struct is rcu protected).
> gw_get_selected():
> * Probably the orig_node's refcounting has to be made atomic, too?
This part is still a little bit ugly and I cannot give you an easy answer.
Just think about following:
* Hash list is a bunch of rcu protected lists
* pointer to originator is stored inside a bucket (list elements inside the
hash)
* hash bucket wants to get removed - call_rcu; reference count of the
originator is decremented immediately
* (!!!! lots of reordering of read and write commands inside the cpu!!!! -
aren't we happy about the added complexity which tries to hide the memory
latency?)
* the originator was removed, the bucket which is removed in the call_rcu
still points to the removed originator
* a parallel running operation tries to find a originator, the rcu list
iterator gets the to-be-deleted bucket to the originator
* the pointer to the already removed originator inside the bucket is
dereferenced, data is read/written -> Kernel Oops
Does this sound scary? At least it could be used in some horror movies (and I
would watch them).
But that is the other problem I currently have with the state of batman-adv in
trunk - and I think I forget to tell you about it after the release of
v2011.0.0.
So, a good idea would be the removal of the buckets for the hash. Usage of
"struct hlist_node" inside the hash elements should be a good starting point.
But think about the problem that the different hashes could have the same
element. So you need for each distinct hash an extra "struct hlist_node"
inside the element which should be part of the hash. The hash_add (and
related) functions don't get the actual pointer to the element, but the
pointer to the correct "struct hlist_node" inside the element/struct. The
comparison and hashing function would also receive "struct hlist_node" as
parameter and must get the pointer to the element using the container_of
macro.
> @@ -171,7 +172,7 @@ struct bat_priv {
> struct delayed_work hna_work;
> struct delayed_work orig_work;
> struct delayed_work vis_work;
> - struct gw_node *curr_gw;
> + struct gw_node *curr_gw; /* rcu protected pointer */
> struct vis_info *my_vis_info;
> };
Sry, but I have to say that: FAIL ;)
I think it should look that way:
> - struct gw_node *curr_gw;
> + struct gw_node __rcu *curr_gw;
Best regards,
Sven
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH] Re: batman-adv: Correct rcu refcounting for gw_node
2011-02-02 21:42 ` Sven Eckelmann
@ 2011-02-03 0:19 ` Marek Lindner
2011-02-03 9:55 ` Linus Lüssing
1 sibling, 0 replies; 44+ messages in thread
From: Marek Lindner @ 2011-02-03 0:19 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
On Wednesday 02 February 2011 22:42:46 Sven Eckelmann wrote:
> > gw_election():
> > * can the if-block before gw_deselect() be ommited, we had a nullpointer
> >
> > check for curr_gw just a couple of lines before during the rcu-lock.
>
> I thought that this if block should be moved to gw_select. And your
> gw_select still has the bug that the bat_priv->curr_gw isn't set to NULL
> when new_gw_node is NULL.
Yes, but we discussed this without Linus.
@Linus: This section will be changed - I'm working on this.
Regards,
Marek
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH] Re: batman-adv: Correct rcu refcounting for gw_node
2011-02-02 21:42 ` Sven Eckelmann
2011-02-03 0:19 ` Marek Lindner
@ 2011-02-03 9:55 ` Linus Lüssing
2011-02-03 10:01 ` Sven Eckelmann
1 sibling, 1 reply; 44+ messages in thread
From: Linus Lüssing @ 2011-02-03 9:55 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
Hi Sven,
On Wed, Feb 02, 2011 at 10:42:46PM +0100, Sven Eckelmann wrote:
> > gw_deselet():
> > * is the refcount at this time always 1 for gw_node, can the null
> > pointer check + a rcu_dereference be ommited? (at least that's what
> > it looks like when comparing to the rcuref.txt example)
>
> Why can't it be NULL? And _always_ use rcu_dereference. What example tells you
> that it isn't needed? None of the examples has any kind of rcu pointer in it
> (just el as pointer which is stored in a struct were the pointer inside the
> struct is rcu protected).
Ok, you got a point there with the always-rcu-dereference pointers. I
somehow was thinking that in between the spin-lock/unlock there
could possibly be no other thread reading/writing to it then - but
I guess at that moment I forgot about the reordering and the whole
point of using the rcu macros between the spinlock there :). So,
yes, you're right with that one, will change it.
For the NULL pointer, guess you're right again. I was looking at
the delete() example in rcuref.txt which was not doing any NULL
pointer check. But either that's the case there because it's more
pseudo-code there or because it's more related to lists, meaning
that after the delete_element there it's not in the list anymore
and not possible for any other thread to have the idea to free the
same thing again.
>
>
> > gw_get_selected():
> > * Probably the orig_node's refcounting has to be made atomic, too?
>
> This part is still a little bit ugly and I cannot give you an easy answer.
> Just think about following:
> * Hash list is a bunch of rcu protected lists
> * pointer to originator is stored inside a bucket (list elements inside the
> hash)
> * hash bucket wants to get removed - call_rcu; reference count of the
> originator is decremented immediately
> * (!!!! lots of reordering of read and write commands inside the cpu!!!! -
> aren't we happy about the added complexity which tries to hide the memory
> latency?)
> * the originator was removed, the bucket which is removed in the call_rcu
> still points to the removed originator
> * a parallel running operation tries to find a originator, the rcu list
> iterator gets the to-be-deleted bucket to the originator
> * the pointer to the already removed originator inside the bucket is
> dereferenced, data is read/written -> Kernel Oops
>
> Does this sound scary? At least it could be used in some horror movies (and I
> would watch them).
>
> But that is the other problem I currently have with the state of batman-adv in
> trunk - and I think I forget to tell you about it after the release of
> v2011.0.0.
>
> So, a good idea would be the removal of the buckets for the hash. Usage of
> "struct hlist_node" inside the hash elements should be a good starting point.
> But think about the problem that the different hashes could have the same
> element. So you need for each distinct hash an extra "struct hlist_node"
> inside the element which should be part of the hash. The hash_add (and
> related) functions don't get the actual pointer to the element, but the
> pointer to the correct "struct hlist_node" inside the element/struct. The
> comparison and hashing function would also receive "struct hlist_node" as
> parameter and must get the pointer to the element using the container_of
> macro.
>
>
> > @@ -171,7 +172,7 @@ struct bat_priv {
> > struct delayed_work hna_work;
> > struct delayed_work orig_work;
> > struct delayed_work vis_work;
> > - struct gw_node *curr_gw;
> > + struct gw_node *curr_gw; /* rcu protected pointer */
> > struct vis_info *my_vis_info;
> > };
>
> Sry, but I have to say that: FAIL ;)
>
> I think it should look that way:
> > - struct gw_node *curr_gw;
> > + struct gw_node __rcu *curr_gw;
Eh, had been looking at whatisRCU.txt and there gbl_foo in section
3 did not have a "__rcu" (actually I hadn't seen that in any of the
documentations before).
>
> Best regards,
> Sven
Cheers, Linus
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH] Re: batman-adv: Correct rcu refcounting for gw_node
2011-02-03 9:55 ` Linus Lüssing
@ 2011-02-03 10:01 ` Sven Eckelmann
0 siblings, 0 replies; 44+ messages in thread
From: Sven Eckelmann @ 2011-02-03 10:01 UTC (permalink / raw)
To: b.a.t.m.a.n
[-- Attachment #1: Type: Text/Plain, Size: 567 bytes --]
Linus Lüssing wrote:
> > I think it should look that way:
> > > - struct gw_node *curr_gw;
> > > + struct gw_node __rcu *curr_gw;
>
> Eh, had been looking at whatisRCU.txt and there gbl_foo in section
> 3 did not have a "__rcu" (actually I hadn't seen that in any of the
> documentations before).
$ git show v2.6.37:Documentation/RCU/checklist.txt|grep __rcu
That document explains it quite well how the sparse check works and why you
must use __rcu to say "hey, this is a pointer which is used in a special way).
Best regards,
Sven
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* [B.A.T.M.A.N.] [PATCH 1/3] batman-adv: Correct rcu refcounting for gw_node
2011-02-02 17:37 ` [B.A.T.M.A.N.] [PATCH] " Linus Lüssing
2011-02-02 19:49 ` Marek Lindner
2011-02-02 21:42 ` Sven Eckelmann
@ 2011-02-03 14:43 ` Linus Lüssing
2011-02-03 17:56 ` Sven Eckelmann
2011-02-03 14:43 ` [B.A.T.M.A.N.] [PATCH 2/3] batman-adv: add __rcu annotations " Linus Lüssing
2011-02-03 14:43 ` [B.A.T.M.A.N.] [PATCH 3/3] batman-adv: Increase orig_node refcount before releasing rcu read lock Linus Lüssing
4 siblings, 1 reply; 44+ messages in thread
From: Linus Lüssing @ 2011-02-03 14:43 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Linus Lüssing
For one thing, atomic refcounters for the gw_list need to be used when
using both refcounting and rcu locking (see
Documentation/RCU/rcuref.txt for details).
For another the rcu protected macros rcu_dereference() and
rcu_assign_pointer() for the bat_priv->curr_gw and curr_gw->orig_node
need to be used.
Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Linus Lüssing <linus.luessing@ascom.ch>
---
gateway_client.c | 182 +++++++++++++++++++++++++++++++++---------------------
main.c | 1 +
types.h | 3 +-
unicast.c | 1 -
4 files changed, 114 insertions(+), 73 deletions(-)
diff --git a/batman-adv/gateway_client.c b/batman-adv/gateway_client.c
index 429a013..ee71335 100644
--- a/batman-adv/gateway_client.c
+++ b/batman-adv/gateway_client.c
@@ -28,58 +28,77 @@
#include <linux/udp.h>
#include <linux/if_vlan.h>
-static void gw_node_free_ref(struct kref *refcount)
-{
- struct gw_node *gw_node;
-
- gw_node = container_of(refcount, struct gw_node, refcount);
- kfree(gw_node);
-}
-
static void gw_node_free_rcu(struct rcu_head *rcu)
{
struct gw_node *gw_node;
gw_node = container_of(rcu, struct gw_node, rcu);
- kref_put(&gw_node->refcount, gw_node_free_ref);
+ kfree(gw_node);
}
+static void gw_node_free_ref(struct gw_node *gw_node)
+{
+ if (atomic_dec_and_test(&gw_node->refcount))
+ call_rcu(&gw_node->rcu, gw_node_free_rcu);
+}
+
+/* increases the returned orig_node's refcount */
void *gw_get_selected(struct bat_priv *bat_priv)
{
- struct gw_node *curr_gateway_tmp = bat_priv->curr_gw;
+ struct gw_node *curr_gateway_tmp;
+ struct orig_node *orig_node;
- if (!curr_gateway_tmp)
+ rcu_read_lock();
+ curr_gateway_tmp = rcu_dereference(bat_priv->curr_gw);
+ if (!curr_gateway_tmp) {
+ rcu_read_unlock();
return NULL;
+ }
- return curr_gateway_tmp->orig_node;
+ orig_node = rcu_dereference(curr_gateway_tmp->orig_node);
+ if (orig_node) {
+ rcu_read_unlock();
+ return NULL;
+ }
+
+ rcu_read_unlock();
+ return orig_node;
}
void gw_deselect(struct bat_priv *bat_priv)
{
- struct gw_node *gw_node = bat_priv->curr_gw;
+ struct gw_node *gw_node;
- bat_priv->curr_gw = NULL;
+ spin_lock_bh(&bat_priv->curr_gw_lock);
+ gw_node = rcu_dereference(bat_priv->curr_gw);
+ rcu_assign_pointer(bat_priv->curr_gw, NULL);
+ spin_unlock_bh(&bat_priv->curr_gw_lock);
if (gw_node)
- kref_put(&gw_node->refcount, gw_node_free_ref);
+ gw_node_free_ref(gw_node);
}
static struct gw_node *gw_select(struct bat_priv *bat_priv,
struct gw_node *new_gw_node)
{
- struct gw_node *curr_gw_node = bat_priv->curr_gw;
+ struct gw_node *curr_gw_node;
- if (new_gw_node)
- kref_get(&new_gw_node->refcount);
+ if (new_gw_node && !atomic_inc_not_zero(&new_gw_node->refcount))
+ return NULL;
+
+ spin_lock_bh(&bat_priv->curr_gw_lock);
+ curr_gw_node = rcu_dereference(bat_priv->curr_gw);
+ rcu_assign_pointer(bat_priv->curr_gw, new_gw_node);
+ spin_unlock_bh(&bat_priv->curr_gw_lock);
- bat_priv->curr_gw = new_gw_node;
return curr_gw_node;
}
void gw_election(struct bat_priv *bat_priv)
{
struct hlist_node *node;
- struct gw_node *gw_node, *curr_gw_tmp = NULL, *old_gw_node = NULL;
+ struct gw_node *gw_node, *curr_gw, *curr_gw_tmp = NULL, *old_gw_node = NULL;
+ struct orig_node *orig_node;
uint8_t max_tq = 0;
uint32_t max_gw_factor = 0, tmp_gw_factor = 0;
int down, up;
@@ -93,25 +112,28 @@ void gw_election(struct bat_priv *bat_priv)
if (atomic_read(&bat_priv->gw_mode) != GW_MODE_CLIENT)
return;
- if (bat_priv->curr_gw)
+ rcu_read_lock();
+ curr_gw = rcu_dereference(bat_priv->curr_gw);
+ if (curr_gw) {
+ rcu_read_unlock();
return;
+ }
- rcu_read_lock();
if (hlist_empty(&bat_priv->gw_list)) {
- rcu_read_unlock();
-
- if (bat_priv->curr_gw) {
+ if (curr_gw) {
bat_dbg(DBG_BATMAN, bat_priv,
"Removing selected gateway - "
"no gateway in range\n");
gw_deselect(bat_priv);
}
+ rcu_read_unlock();
return;
}
hlist_for_each_entry_rcu(gw_node, node, &bat_priv->gw_list, list) {
- if (!gw_node->orig_node->router)
+ orig_node = rcu_dereference(gw_node->orig_node);
+ if (!orig_node->router)
continue;
if (gw_node->deleted)
@@ -119,18 +141,17 @@ void gw_election(struct bat_priv *bat_priv)
switch (atomic_read(&bat_priv->gw_sel_class)) {
case 1: /* fast connection */
- gw_bandwidth_to_kbit(gw_node->orig_node->gw_flags,
- &down, &up);
+ gw_bandwidth_to_kbit(orig_node->gw_flags, &down, &up);
- tmp_gw_factor = (gw_node->orig_node->router->tq_avg *
- gw_node->orig_node->router->tq_avg *
+ tmp_gw_factor = (orig_node->router->tq_avg *
+ orig_node->router->tq_avg *
down * 100 * 100) /
(TQ_LOCAL_WINDOW_SIZE *
TQ_LOCAL_WINDOW_SIZE * 64);
if ((tmp_gw_factor > max_gw_factor) ||
((tmp_gw_factor == max_gw_factor) &&
- (gw_node->orig_node->router->tq_avg > max_tq)))
+ (orig_node->router->tq_avg > max_tq)))
curr_gw_tmp = gw_node;
break;
@@ -142,37 +163,38 @@ void gw_election(struct bat_priv *bat_priv)
* soon as a better gateway appears which has
* $routing_class more tq points)
**/
- if (gw_node->orig_node->router->tq_avg > max_tq)
+ if (orig_node->router->tq_avg > max_tq)
curr_gw_tmp = gw_node;
break;
}
- if (gw_node->orig_node->router->tq_avg > max_tq)
- max_tq = gw_node->orig_node->router->tq_avg;
+ if (orig_node->router->tq_avg > max_tq)
+ max_tq = orig_node->router->tq_avg;
if (tmp_gw_factor > max_gw_factor)
max_gw_factor = tmp_gw_factor;
}
- if (bat_priv->curr_gw != curr_gw_tmp) {
- if ((bat_priv->curr_gw) && (!curr_gw_tmp))
+ if (curr_gw != curr_gw_tmp) {
+ orig_node = rcu_dereference(curr_gw_tmp->orig_node);
+ if ((curr_gw) && (!curr_gw_tmp))
bat_dbg(DBG_BATMAN, bat_priv,
"Removing selected gateway - "
"no gateway in range\n");
- else if ((!bat_priv->curr_gw) && (curr_gw_tmp))
+ else if ((!curr_gw) && (curr_gw_tmp))
bat_dbg(DBG_BATMAN, bat_priv,
"Adding route to gateway %pM "
"(gw_flags: %i, tq: %i)\n",
- curr_gw_tmp->orig_node->orig,
- curr_gw_tmp->orig_node->gw_flags,
- curr_gw_tmp->orig_node->router->tq_avg);
+ orig_node->orig,
+ orig_node->gw_flags,
+ orig_node->router->tq_avg);
else
bat_dbg(DBG_BATMAN, bat_priv,
"Changing route to gateway %pM "
"(gw_flags: %i, tq: %i)\n",
- curr_gw_tmp->orig_node->orig,
- curr_gw_tmp->orig_node->gw_flags,
- curr_gw_tmp->orig_node->router->tq_avg);
+ orig_node->orig,
+ orig_node->gw_flags,
+ orig_node->router->tq_avg);
old_gw_node = gw_select(bat_priv, curr_gw_tmp);
}
@@ -181,36 +203,40 @@ void gw_election(struct bat_priv *bat_priv)
/* the kfree() has to be outside of the rcu lock */
if (old_gw_node)
- kref_put(&old_gw_node->refcount, gw_node_free_ref);
+ gw_node_free_ref(old_gw_node);
}
void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
{
- struct gw_node *curr_gateway_tmp = bat_priv->curr_gw;
+ struct gw_node *curr_gateway_tmp;
+ struct orig_node *curr_gw_orig;
uint8_t gw_tq_avg, orig_tq_avg;
+ rcu_read_lock();
+ curr_gateway_tmp = rcu_dereference(bat_priv->curr_gw);
if (!curr_gateway_tmp)
- return;
+ goto rcu_unlock;
- if (!curr_gateway_tmp->orig_node)
+ curr_gw_orig = rcu_dereference(curr_gateway_tmp->orig_node);
+ if (!curr_gw_orig)
goto deselect;
- if (!curr_gateway_tmp->orig_node->router)
+ if (!curr_gw_orig->router)
goto deselect;
/* this node already is the gateway */
- if (curr_gateway_tmp->orig_node == orig_node)
- return;
+ if (curr_gw_orig == orig_node)
+ goto deselect;
if (!orig_node->router)
- return;
+ goto rcu_unlock;
- gw_tq_avg = curr_gateway_tmp->orig_node->router->tq_avg;
+ gw_tq_avg = curr_gw_orig ->router->tq_avg;
orig_tq_avg = orig_node->router->tq_avg;
/* the TQ value has to be better */
if (orig_tq_avg < gw_tq_avg)
- return;
+ goto rcu_unlock;
/**
* if the routing class is greater than 3 the value tells us how much
@@ -218,7 +244,7 @@ void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
**/
if ((atomic_read(&bat_priv->gw_sel_class) > 3) &&
(orig_tq_avg - gw_tq_avg < atomic_read(&bat_priv->gw_sel_class)))
- return;
+ goto rcu_unlock;
bat_dbg(DBG_BATMAN, bat_priv,
"Restarting gateway selection: better gateway found (tq curr: "
@@ -227,6 +253,8 @@ void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
deselect:
gw_deselect(bat_priv);
+rcu_unlock:
+ rcu_read_unlock();
}
static void gw_node_add(struct bat_priv *bat_priv,
@@ -242,7 +270,7 @@ static void gw_node_add(struct bat_priv *bat_priv,
memset(gw_node, 0, sizeof(struct gw_node));
INIT_HLIST_NODE(&gw_node->list);
gw_node->orig_node = orig_node;
- kref_init(&gw_node->refcount);
+ atomic_set(&gw_node->refcount, 1);
spin_lock_bh(&bat_priv->gw_list_lock);
hlist_add_head_rcu(&gw_node->list, &bat_priv->gw_list);
@@ -266,13 +294,13 @@ void gw_node_update(struct bat_priv *bat_priv,
rcu_read_lock();
hlist_for_each_entry_rcu(gw_node, node, &bat_priv->gw_list, list) {
- if (gw_node->orig_node != orig_node)
+ if (rcu_dereference(gw_node->orig_node) != orig_node)
continue;
bat_dbg(DBG_BATMAN, bat_priv,
"Gateway class of originator %pM changed from "
- "%i to %i\n",
- orig_node->orig, gw_node->orig_node->gw_flags,
+ "%i to %i\n", orig_node->orig,
+ rcu_dereference(gw_node->orig_node)->gw_flags,
new_gwflags);
gw_node->deleted = 0;
@@ -283,7 +311,7 @@ void gw_node_update(struct bat_priv *bat_priv,
"Gateway %pM removed from gateway list\n",
orig_node->orig);
- if (gw_node == bat_priv->curr_gw) {
+ if (gw_node == rcu_dereference(bat_priv->curr_gw)) {
rcu_read_unlock();
gw_deselect(bat_priv);
return;
@@ -321,11 +349,11 @@ void gw_node_purge(struct bat_priv *bat_priv)
atomic_read(&bat_priv->mesh_state) == MESH_ACTIVE)
continue;
- if (bat_priv->curr_gw == gw_node)
+ if (rcu_dereference(bat_priv->curr_gw) == gw_node)
gw_deselect(bat_priv);
hlist_del_rcu(&gw_node->list);
- call_rcu(&gw_node->rcu, gw_node_free_rcu);
+ gw_node_free_ref(gw_node);
}
@@ -335,21 +363,29 @@ void gw_node_purge(struct bat_priv *bat_priv)
static int _write_buffer_text(struct bat_priv *bat_priv,
struct seq_file *seq, struct gw_node *gw_node)
{
- int down, up;
+ struct gw_node *curr_gw;
+ struct orig_node *orig_node;
+ int down, up, ret;
- gw_bandwidth_to_kbit(gw_node->orig_node->gw_flags, &down, &up);
+ rcu_read_lock();
+ curr_gw = rcu_dereference(bat_priv->curr_gw);
+ orig_node = rcu_dereference(gw_node->orig_node);
+ gw_bandwidth_to_kbit(orig_node->gw_flags, &down, &up);
- return seq_printf(seq, "%s %pM (%3i) %pM [%10s]: %3i - %i%s/%i%s\n",
- (bat_priv->curr_gw == gw_node ? "=>" : " "),
- gw_node->orig_node->orig,
- gw_node->orig_node->router->tq_avg,
- gw_node->orig_node->router->addr,
- gw_node->orig_node->router->if_incoming->net_dev->name,
- gw_node->orig_node->gw_flags,
+ ret = seq_printf(seq, "%s %pM (%3i) %pM [%10s]: %3i - %i%s/%i%s\n",
+ (curr_gw == gw_node ? "=>" : " "),
+ orig_node->orig,
+ orig_node->router->tq_avg,
+ orig_node->router->addr,
+ orig_node->router->if_incoming->net_dev->name,
+ orig_node->gw_flags,
(down > 2048 ? down / 1024 : down),
(down > 2048 ? "MBit" : "KBit"),
(up > 2048 ? up / 1024 : up),
(up > 2048 ? "MBit" : "KBit"));
+ rcu_read_unlock();
+
+ return ret;
}
int gw_client_seq_print_text(struct seq_file *seq, void *offset)
@@ -470,8 +506,12 @@ int gw_is_target(struct bat_priv *bat_priv, struct sk_buff *skb)
if (atomic_read(&bat_priv->gw_mode) == GW_MODE_SERVER)
return -1;
- if (!bat_priv->curr_gw)
+ rcu_read_lock();
+ if (!rcu_dereference(bat_priv->curr_gw)) {
+ rcu_read_unlock();
return 0;
+ }
+ rcu_read_unlock();
return 1;
}
diff --git a/batman-adv/main.c b/batman-adv/main.c
index e687e7f..8679260 100644
--- a/batman-adv/main.c
+++ b/batman-adv/main.c
@@ -85,6 +85,7 @@ int mesh_init(struct net_device *soft_iface)
spin_lock_init(&bat_priv->hna_lhash_lock);
spin_lock_init(&bat_priv->hna_ghash_lock);
spin_lock_init(&bat_priv->gw_list_lock);
+ spin_lock_init(&bat_priv->curr_gw_lock);
spin_lock_init(&bat_priv->vis_hash_lock);
spin_lock_init(&bat_priv->vis_list_lock);
spin_lock_init(&bat_priv->softif_neigh_lock);
diff --git a/batman-adv/types.h b/batman-adv/types.h
index e4a0462..30d10c0 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -100,7 +100,7 @@ struct gw_node {
struct hlist_node list;
struct orig_node *orig_node;
unsigned long deleted;
- struct kref refcount;
+ atomic_t refcount;
struct rcu_head rcu;
};
@@ -163,6 +163,7 @@ struct bat_priv {
spinlock_t hna_lhash_lock; /* protects hna_local_hash */
spinlock_t hna_ghash_lock; /* protects hna_global_hash */
spinlock_t gw_list_lock; /* protects gw_list */
+ spinlock_t curr_gw_lock; /* protects curr_gw updates */
spinlock_t vis_hash_lock; /* protects vis_hash */
spinlock_t vis_list_lock; /* protects vis_info::recv_list */
spinlock_t softif_neigh_lock; /* protects soft-interface neigh list */
diff --git a/batman-adv/unicast.c b/batman-adv/unicast.c
index 6a9ab61..8816102 100644
--- a/batman-adv/unicast.c
+++ b/batman-adv/unicast.c
@@ -298,7 +298,6 @@ int unicast_send_skb(struct sk_buff *skb, struct bat_priv *bat_priv)
if (!orig_node)
goto trans_search;
- kref_get(&orig_node->refcount);
goto find_router;
} else {
rcu_read_lock();
--
1.7.2.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [B.A.T.M.A.N.] [PATCH 2/3] batman-adv: add __rcu annotations for gw_node
2011-02-02 17:37 ` [B.A.T.M.A.N.] [PATCH] " Linus Lüssing
` (2 preceding siblings ...)
2011-02-03 14:43 ` [B.A.T.M.A.N.] [PATCH 1/3] " Linus Lüssing
@ 2011-02-03 14:43 ` Linus Lüssing
2011-02-03 14:43 ` [B.A.T.M.A.N.] [PATCH 3/3] batman-adv: Increase orig_node refcount before releasing rcu read lock Linus Lüssing
4 siblings, 0 replies; 44+ messages in thread
From: Linus Lüssing @ 2011-02-03 14:43 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Linus Lüssing
Add __rcu annotations for rcu protected pointers in the gateway code
to allow sparse checking.
Signed-off-by: Linus Lüssing <linus.luessing@ascom.ch>
---
compat.h | 6 ++++++
types.h | 4 ++--
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/batman-adv/compat.h b/batman-adv/compat.h
index a76d0be..4e89049 100644
--- a/batman-adv/compat.h
+++ b/batman-adv/compat.h
@@ -270,4 +270,10 @@ int bat_seq_printf(struct seq_file *m, const char *f, ...);
#endif /* < KERNEL_VERSION(2, 6, 33) */
+#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 36)
+
+#define __rcu
+
+#endif /* < KERNEL_VERSION(2, 6, 36) */
+
#endif /* _NET_BATMAN_ADV_COMPAT_H_ */
diff --git a/batman-adv/types.h b/batman-adv/types.h
index 30d10c0..2cb0c31 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -98,7 +98,7 @@ struct orig_node {
struct gw_node {
struct hlist_node list;
- struct orig_node *orig_node;
+ struct orig_node __rcu *orig_node; /* rcu protected pointer */
unsigned long deleted;
atomic_t refcount;
struct rcu_head rcu;
@@ -172,7 +172,7 @@ struct bat_priv {
struct delayed_work hna_work;
struct delayed_work orig_work;
struct delayed_work vis_work;
- struct gw_node *curr_gw;
+ struct gw_node __rcu *curr_gw; /* rcu protected pointer */
struct vis_info *my_vis_info;
};
--
1.7.2.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [B.A.T.M.A.N.] [PATCH 3/3] batman-adv: Increase orig_node refcount before releasing rcu read lock
2011-02-02 17:37 ` [B.A.T.M.A.N.] [PATCH] " Linus Lüssing
` (3 preceding siblings ...)
2011-02-03 14:43 ` [B.A.T.M.A.N.] [PATCH 2/3] batman-adv: add __rcu annotations " Linus Lüssing
@ 2011-02-03 14:43 ` Linus Lüssing
4 siblings, 0 replies; 44+ messages in thread
From: Linus Lüssing @ 2011-02-03 14:43 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Linus Lüssing
When unicast_send_skb() is increasing the orig_node's refcount another
thread might have been freeing this orig_node already. We need to
increase the refcount in the rcu read lock protected area to avoid that.
Signed-off-by: Linus Lüssing <linus.luessing@ascom.ch>
---
gateway_client.c | 1 +
unicast.c | 1 -
2 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/batman-adv/gateway_client.c b/batman-adv/gateway_client.c
index ee71335..15ea268 100644
--- a/batman-adv/gateway_client.c
+++ b/batman-adv/gateway_client.c
@@ -57,6 +57,7 @@ void *gw_get_selected(struct bat_priv *bat_priv)
orig_node = rcu_dereference(curr_gateway_tmp->orig_node);
if (orig_node) {
+ kref_get(&orig_node->refcount);
rcu_read_unlock();
return NULL;
}
diff --git a/batman-adv/unicast.c b/batman-adv/unicast.c
index 8816102..b42e40e 100644
--- a/batman-adv/unicast.c
+++ b/batman-adv/unicast.c
@@ -310,7 +310,6 @@ int unicast_send_skb(struct sk_buff *skb, struct bat_priv *bat_priv)
goto trans_search;
}
- kref_get(&orig_node->refcount);
rcu_read_unlock();
goto find_router;
}
--
1.7.2.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [B.A.T.M.A.N.] Reordered rcu refcounting
2011-01-30 1:52 [B.A.T.M.A.N.] Reordered rcu refcounting Sven Eckelmann
` (3 preceding siblings ...)
2011-01-30 1:52 ` [B.A.T.M.A.N.] [PATCH 4/4] batman-adv: Correct rcu refcounting for neigh_node Sven Eckelmann
@ 2011-02-03 17:02 ` Marek Lindner
2011-02-03 17:03 ` [B.A.T.M.A.N.] [PATCH 1/4] batman-adv: Correct rcu refcounting for gw_node Marek Lindner
` (11 more replies)
4 siblings, 12 replies; 44+ messages in thread
From: Marek Lindner @ 2011-02-03 17:02 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
Hi,
> I told Linus and Marek that the implementation of some datastructures using
> reference counting and RCUs must be redesigned. The problem is that
> call_rcu should only be used when the reference count is zero because
> multiple call_rcu cannot be initiated at the same time. The next problem
> is that we may try to access an element with the refcount 0 when we try to
> traverse a rcu protected list. This problem cannot be solved by kref -
> thus we have to use atomic_t again to not increase the reference count
> when it was already zero.
I reviewed the posted patches, filled in the missing parts and fixed some bugs
along the way. Feel free to comment.
@Linus: I believe it would be a good idea if your gateway patch was based on
this patchset.
Cheers,
Marek
^ permalink raw reply [flat|nested] 44+ messages in thread
* [B.A.T.M.A.N.] [PATCH 1/4] batman-adv: Correct rcu refcounting for gw_node
2011-02-03 17:02 ` [B.A.T.M.A.N.] Reordered rcu refcounting Marek Lindner
@ 2011-02-03 17:03 ` Marek Lindner
2011-02-03 17:03 ` [B.A.T.M.A.N.] [PATCH 2/4] batman-adv: Correct rcu refcounting for softif_neigh Marek Lindner
` (10 subsequent siblings)
11 siblings, 0 replies; 44+ messages in thread
From: Marek Lindner @ 2011-02-03 17:03 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Marek Lindner
From: Sven Eckelmann <sven@narfation.org>
It might be possible that 2 threads access the same data in the same
rcu grace period. The first thread calls call_rcu() to decrement the
refcount and free the data while the second thread increases the
refcount to use the data. To avoid this race condition all refcount
operations have to be atomic.
Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
---
batman-adv/gateway_client.c | 37 ++++++++++++++++---------------------
batman-adv/types.h | 2 +-
2 files changed, 17 insertions(+), 22 deletions(-)
diff --git a/batman-adv/gateway_client.c b/batman-adv/gateway_client.c
index 429a013..517e001 100644
--- a/batman-adv/gateway_client.c
+++ b/batman-adv/gateway_client.c
@@ -28,20 +28,18 @@
#include <linux/udp.h>
#include <linux/if_vlan.h>
-static void gw_node_free_ref(struct kref *refcount)
+static void gw_node_free_rcu(struct rcu_head *rcu)
{
struct gw_node *gw_node;
- gw_node = container_of(refcount, struct gw_node, refcount);
+ gw_node = container_of(rcu, struct gw_node, rcu);
kfree(gw_node);
}
-static void gw_node_free_rcu(struct rcu_head *rcu)
+static void gw_node_free_ref(struct gw_node *gw_node)
{
- struct gw_node *gw_node;
-
- gw_node = container_of(rcu, struct gw_node, rcu);
- kref_put(&gw_node->refcount, gw_node_free_ref);
+ if (atomic_dec_and_test(&gw_node->refcount))
+ call_rcu(&gw_node->rcu, gw_node_free_rcu);
}
void *gw_get_selected(struct bat_priv *bat_priv)
@@ -61,25 +59,26 @@ void gw_deselect(struct bat_priv *bat_priv)
bat_priv->curr_gw = NULL;
if (gw_node)
- kref_put(&gw_node->refcount, gw_node_free_ref);
+ gw_node_free_ref(gw_node);
}
-static struct gw_node *gw_select(struct bat_priv *bat_priv,
- struct gw_node *new_gw_node)
+static void gw_select(struct bat_priv *bat_priv, struct gw_node *new_gw_node)
{
struct gw_node *curr_gw_node = bat_priv->curr_gw;
- if (new_gw_node)
- kref_get(&new_gw_node->refcount);
+ if (new_gw_node && !atomic_inc_not_zero(&new_gw_node->refcount))
+ new_gw_node = NULL;
bat_priv->curr_gw = new_gw_node;
- return curr_gw_node;
+
+ if (curr_gw_node)
+ gw_node_free_ref(curr_gw_node);
}
void gw_election(struct bat_priv *bat_priv)
{
struct hlist_node *node;
- struct gw_node *gw_node, *curr_gw_tmp = NULL, *old_gw_node = NULL;
+ struct gw_node *gw_node, *curr_gw_tmp = NULL;
uint8_t max_tq = 0;
uint32_t max_gw_factor = 0, tmp_gw_factor = 0;
int down, up;
@@ -174,14 +173,10 @@ void gw_election(struct bat_priv *bat_priv)
curr_gw_tmp->orig_node->gw_flags,
curr_gw_tmp->orig_node->router->tq_avg);
- old_gw_node = gw_select(bat_priv, curr_gw_tmp);
+ gw_select(bat_priv, curr_gw_tmp);
}
rcu_read_unlock();
-
- /* the kfree() has to be outside of the rcu lock */
- if (old_gw_node)
- kref_put(&old_gw_node->refcount, gw_node_free_ref);
}
void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
@@ -242,7 +237,7 @@ static void gw_node_add(struct bat_priv *bat_priv,
memset(gw_node, 0, sizeof(struct gw_node));
INIT_HLIST_NODE(&gw_node->list);
gw_node->orig_node = orig_node;
- kref_init(&gw_node->refcount);
+ atomic_set(&gw_node->refcount, 1);
spin_lock_bh(&bat_priv->gw_list_lock);
hlist_add_head_rcu(&gw_node->list, &bat_priv->gw_list);
@@ -325,7 +320,7 @@ void gw_node_purge(struct bat_priv *bat_priv)
gw_deselect(bat_priv);
hlist_del_rcu(&gw_node->list);
- call_rcu(&gw_node->rcu, gw_node_free_rcu);
+ gw_node_free_ref(gw_node);
}
diff --git a/batman-adv/types.h b/batman-adv/types.h
index e4a0462..ca5f20a 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -100,7 +100,7 @@ struct gw_node {
struct hlist_node list;
struct orig_node *orig_node;
unsigned long deleted;
- struct kref refcount;
+ atomic_t refcount;
struct rcu_head rcu;
};
--
1.7.2.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [B.A.T.M.A.N.] [PATCH 2/4] batman-adv: Correct rcu refcounting for softif_neigh
2011-02-03 17:02 ` [B.A.T.M.A.N.] Reordered rcu refcounting Marek Lindner
2011-02-03 17:03 ` [B.A.T.M.A.N.] [PATCH 1/4] batman-adv: Correct rcu refcounting for gw_node Marek Lindner
@ 2011-02-03 17:03 ` Marek Lindner
2011-02-03 17:03 ` [B.A.T.M.A.N.] [PATCH 3/4] batman-adv: Correct rcu refcounting for batman_if Marek Lindner
` (9 subsequent siblings)
11 siblings, 0 replies; 44+ messages in thread
From: Marek Lindner @ 2011-02-03 17:03 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Marek Lindner
From: Sven Eckelmann <sven@narfation.org>
It might be possible that 2 threads access the same data in the same
rcu grace period. The first thread calls call_rcu() to decrement the
refcount and free the data while the second thread increases the
refcount to use the data. To avoid this race condition all refcount
operations have to be atomic.
Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
---
batman-adv/soft-interface.c | 31 +++++++++++++++----------------
batman-adv/types.h | 2 +-
2 files changed, 16 insertions(+), 17 deletions(-)
diff --git a/batman-adv/soft-interface.c b/batman-adv/soft-interface.c
index 145e0f7..83dcccd 100644
--- a/batman-adv/soft-interface.c
+++ b/batman-adv/soft-interface.c
@@ -79,20 +79,18 @@ int my_skb_head_push(struct sk_buff *skb, unsigned int len)
return 0;
}
-static void softif_neigh_free_ref(struct kref *refcount)
+static void softif_neigh_free_rcu(struct rcu_head *rcu)
{
struct softif_neigh *softif_neigh;
- softif_neigh = container_of(refcount, struct softif_neigh, refcount);
+ softif_neigh = container_of(rcu, struct softif_neigh, rcu);
kfree(softif_neigh);
}
-static void softif_neigh_free_rcu(struct rcu_head *rcu)
+static void softif_neigh_free_ref(struct softif_neigh *softif_neigh)
{
- struct softif_neigh *softif_neigh;
-
- softif_neigh = container_of(rcu, struct softif_neigh, rcu);
- kref_put(&softif_neigh->refcount, softif_neigh_free_ref);
+ if (atomic_dec_and_test(&softif_neigh->refcount))
+ call_rcu(&softif_neigh->rcu, softif_neigh_free_rcu);
}
void softif_neigh_purge(struct bat_priv *bat_priv)
@@ -119,11 +117,10 @@ void softif_neigh_purge(struct bat_priv *bat_priv)
softif_neigh->addr, softif_neigh->vid);
softif_neigh_tmp = bat_priv->softif_neigh;
bat_priv->softif_neigh = NULL;
- kref_put(&softif_neigh_tmp->refcount,
- softif_neigh_free_ref);
+ softif_neigh_free_ref(softif_neigh_tmp);
}
- call_rcu(&softif_neigh->rcu, softif_neigh_free_rcu);
+ softif_neigh_free_ref(softif_neigh);
}
spin_unlock_bh(&bat_priv->softif_neigh_lock);
@@ -144,8 +141,11 @@ static struct softif_neigh *softif_neigh_get(struct bat_priv *bat_priv,
if (softif_neigh->vid != vid)
continue;
+ if (!atomic_inc_not_zero(&softif_neigh->refcount))
+ continue;
+
softif_neigh->last_seen = jiffies;
- goto found;
+ goto out;
}
softif_neigh = kzalloc(sizeof(struct softif_neigh), GFP_ATOMIC);
@@ -155,15 +155,14 @@ static struct softif_neigh *softif_neigh_get(struct bat_priv *bat_priv,
memcpy(softif_neigh->addr, addr, ETH_ALEN);
softif_neigh->vid = vid;
softif_neigh->last_seen = jiffies;
- kref_init(&softif_neigh->refcount);
+ /* initialize with 2 - caller decrements counter by one */
+ atomic_set(&softif_neigh->refcount, 2);
INIT_HLIST_NODE(&softif_neigh->list);
spin_lock_bh(&bat_priv->softif_neigh_lock);
hlist_add_head_rcu(&softif_neigh->list, &bat_priv->softif_neigh_list);
spin_unlock_bh(&bat_priv->softif_neigh_lock);
-found:
- kref_get(&softif_neigh->refcount);
out:
rcu_read_unlock();
return softif_neigh;
@@ -267,7 +266,7 @@ static void softif_batman_recv(struct sk_buff *skb, struct net_device *dev,
softif_neigh->addr, softif_neigh->vid);
softif_neigh_tmp = bat_priv->softif_neigh;
bat_priv->softif_neigh = softif_neigh;
- kref_put(&softif_neigh_tmp->refcount, softif_neigh_free_ref);
+ softif_neigh_free_ref(softif_neigh_tmp);
/* we need to hold the additional reference */
goto err;
}
@@ -285,7 +284,7 @@ static void softif_batman_recv(struct sk_buff *skb, struct net_device *dev,
}
out:
- kref_put(&softif_neigh->refcount, softif_neigh_free_ref);
+ softif_neigh_free_ref(softif_neigh);
err:
kfree_skb(skb);
return;
diff --git a/batman-adv/types.h b/batman-adv/types.h
index ca5f20a..8df6d9d 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -270,7 +270,7 @@ struct softif_neigh {
uint8_t addr[ETH_ALEN];
unsigned long last_seen;
short vid;
- struct kref refcount;
+ atomic_t refcount;
struct rcu_head rcu;
};
--
1.7.2.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [B.A.T.M.A.N.] [PATCH 3/4] batman-adv: Correct rcu refcounting for batman_if
2011-02-03 17:02 ` [B.A.T.M.A.N.] Reordered rcu refcounting Marek Lindner
2011-02-03 17:03 ` [B.A.T.M.A.N.] [PATCH 1/4] batman-adv: Correct rcu refcounting for gw_node Marek Lindner
2011-02-03 17:03 ` [B.A.T.M.A.N.] [PATCH 2/4] batman-adv: Correct rcu refcounting for softif_neigh Marek Lindner
@ 2011-02-03 17:03 ` Marek Lindner
2011-02-03 17:03 ` [B.A.T.M.A.N.] [PATCH 4/4] batman-adv: Correct rcu refcounting for neigh_node Marek Lindner
` (8 subsequent siblings)
11 siblings, 0 replies; 44+ messages in thread
From: Marek Lindner @ 2011-02-03 17:03 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Marek Lindner
From: Sven Eckelmann <sven@narfation.org>
It might be possible that 2 threads access the same data in the same
rcu grace period. The first thread calls call_rcu() to decrement the
refcount and free the data while the second thread increases the
refcount to use the data. To avoid this race condition all refcount
operations have to be atomic.
Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
---
batman-adv/bat_sysfs.c | 20 +++++++++-----------
batman-adv/hard-interface.c | 40 +++++++++++++++++++---------------------
batman-adv/hard-interface.h | 9 ++++-----
batman-adv/types.h | 2 +-
4 files changed, 33 insertions(+), 38 deletions(-)
diff --git a/batman-adv/bat_sysfs.c b/batman-adv/bat_sysfs.c
index f7b93a0..93ae20a 100644
--- a/batman-adv/bat_sysfs.c
+++ b/batman-adv/bat_sysfs.c
@@ -450,7 +450,7 @@ static ssize_t show_mesh_iface(struct kobject *kobj, struct attribute *attr,
length = sprintf(buff, "%s\n", batman_if->if_status == IF_NOT_IN_USE ?
"none" : batman_if->soft_iface->name);
- kref_put(&batman_if->refcount, hardif_free_ref);
+ hardif_free_ref(batman_if);
return length;
}
@@ -461,7 +461,7 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr,
struct net_device *net_dev = kobj_to_netdev(kobj);
struct batman_if *batman_if = get_batman_if_by_netdev(net_dev);
int status_tmp = -1;
- int ret;
+ int ret = count;
if (!batman_if)
return count;
@@ -472,7 +472,7 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr,
if (strlen(buff) >= IFNAMSIZ) {
pr_err("Invalid parameter for 'mesh_iface' setting received: "
"interface name too long '%s'\n", buff);
- kref_put(&batman_if->refcount, hardif_free_ref);
+ hardif_free_ref(batman_if);
return -EINVAL;
}
@@ -482,17 +482,14 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr,
status_tmp = IF_I_WANT_YOU;
if ((batman_if->if_status == status_tmp) || ((batman_if->soft_iface) &&
- (strncmp(batman_if->soft_iface->name, buff, IFNAMSIZ) == 0))) {
- kref_put(&batman_if->refcount, hardif_free_ref);
- return count;
- }
+ (strncmp(batman_if->soft_iface->name, buff, IFNAMSIZ) == 0)))
+ goto out;
if (status_tmp == IF_NOT_IN_USE) {
rtnl_lock();
hardif_disable_interface(batman_if);
rtnl_unlock();
- kref_put(&batman_if->refcount, hardif_free_ref);
- return count;
+ goto out;
}
/* if the interface already is in use */
@@ -503,8 +500,9 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr,
}
ret = hardif_enable_interface(batman_if, buff);
- kref_put(&batman_if->refcount, hardif_free_ref);
+out:
+ hardif_free_ref(batman_if);
return ret;
}
@@ -537,7 +535,7 @@ static ssize_t show_iface_status(struct kobject *kobj, struct attribute *attr,
break;
}
- kref_put(&batman_if->refcount, hardif_free_ref);
+ hardif_free_ref(batman_if);
return length;
}
diff --git a/batman-adv/hard-interface.c b/batman-adv/hard-interface.c
index e2b001a..8982485 100644
--- a/batman-adv/hard-interface.c
+++ b/batman-adv/hard-interface.c
@@ -40,13 +40,13 @@ static int batman_skb_recv(struct sk_buff *skb,
struct packet_type *ptype,
struct net_device *orig_dev);
-static void hardif_free_rcu(struct rcu_head *rcu)
+void hardif_free_rcu(struct rcu_head *rcu)
{
struct batman_if *batman_if;
batman_if = container_of(rcu, struct batman_if, rcu);
dev_put(batman_if->net_dev);
- kref_put(&batman_if->refcount, hardif_free_ref);
+ kfree(batman_if);
}
struct batman_if *get_batman_if_by_netdev(struct net_device *net_dev)
@@ -55,16 +55,14 @@ struct batman_if *get_batman_if_by_netdev(struct net_device *net_dev)
rcu_read_lock();
list_for_each_entry_rcu(batman_if, &if_list, list) {
- if (batman_if->net_dev == net_dev)
+ if (batman_if->net_dev == net_dev &&
+ atomic_inc_not_zero(&batman_if->refcount))
goto out;
}
batman_if = NULL;
out:
- if (batman_if)
- kref_get(&batman_if->refcount);
-
rcu_read_unlock();
return batman_if;
}
@@ -105,16 +103,14 @@ static struct batman_if *get_active_batman_if(struct net_device *soft_iface)
if (batman_if->soft_iface != soft_iface)
continue;
- if (batman_if->if_status == IF_ACTIVE)
+ if (batman_if->if_status == IF_ACTIVE &&
+ atomic_inc_not_zero(&batman_if->refcount))
goto out;
}
batman_if = NULL;
out:
- if (batman_if)
- kref_get(&batman_if->refcount);
-
rcu_read_unlock();
return batman_if;
}
@@ -137,14 +133,14 @@ static void set_primary_if(struct bat_priv *bat_priv,
struct batman_packet *batman_packet;
struct batman_if *old_if;
- if (batman_if)
- kref_get(&batman_if->refcount);
+ if (batman_if && !atomic_inc_not_zero(&batman_if->refcount))
+ batman_if = NULL;
old_if = bat_priv->primary_if;
bat_priv->primary_if = batman_if;
if (old_if)
- kref_put(&old_if->refcount, hardif_free_ref);
+ hardif_free_ref(old_if);
if (!bat_priv->primary_if)
return;
@@ -290,6 +286,9 @@ int hardif_enable_interface(struct batman_if *batman_if, char *iface_name)
if (batman_if->if_status != IF_NOT_IN_USE)
goto out;
+ if (!atomic_inc_not_zero(&batman_if->refcount))
+ goto out;
+
batman_if->soft_iface = dev_get_by_name(&init_net, iface_name);
if (!batman_if->soft_iface) {
@@ -328,7 +327,6 @@ int hardif_enable_interface(struct batman_if *batman_if, char *iface_name)
batman_if->batman_adv_ptype.type = __constant_htons(ETH_P_BATMAN);
batman_if->batman_adv_ptype.func = batman_skb_recv;
batman_if->batman_adv_ptype.dev = batman_if->net_dev;
- kref_get(&batman_if->refcount);
dev_add_pack(&batman_if->batman_adv_ptype);
atomic_set(&batman_if->seqno, 1);
@@ -371,6 +369,7 @@ out:
return 0;
err:
+ hardif_free_ref(batman_if);
return -ENOMEM;
}
@@ -387,7 +386,6 @@ void hardif_disable_interface(struct batman_if *batman_if)
bat_info(batman_if->soft_iface, "Removing interface: %s\n",
batman_if->net_dev->name);
dev_remove_pack(&batman_if->batman_adv_ptype);
- kref_put(&batman_if->refcount, hardif_free_ref);
bat_priv->num_ifaces--;
orig_hash_del_if(batman_if, bat_priv->num_ifaces);
@@ -399,7 +397,7 @@ void hardif_disable_interface(struct batman_if *batman_if)
set_primary_if(bat_priv, new_if);
if (new_if)
- kref_put(&new_if->refcount, hardif_free_ref);
+ hardif_free_ref(new_if);
}
kfree(batman_if->packet_buff);
@@ -416,6 +414,7 @@ void hardif_disable_interface(struct batman_if *batman_if)
softif_destroy(batman_if->soft_iface);
batman_if->soft_iface = NULL;
+ hardif_free_ref(batman_if);
}
static struct batman_if *hardif_add_interface(struct net_device *net_dev)
@@ -445,7 +444,8 @@ static struct batman_if *hardif_add_interface(struct net_device *net_dev)
batman_if->soft_iface = NULL;
batman_if->if_status = IF_NOT_IN_USE;
INIT_LIST_HEAD(&batman_if->list);
- kref_init(&batman_if->refcount);
+ /* extra reference for return */
+ atomic_set(&batman_if->refcount, 2);
check_known_mac_addr(batman_if->net_dev);
@@ -453,8 +453,6 @@ static struct batman_if *hardif_add_interface(struct net_device *net_dev)
list_add_tail_rcu(&batman_if->list, &if_list);
spin_unlock(&if_list_lock);
- /* extra reference for return */
- kref_get(&batman_if->refcount);
return batman_if;
free_if:
@@ -476,7 +474,7 @@ static void hardif_remove_interface(struct batman_if *batman_if)
batman_if->if_status = IF_TO_BE_REMOVED;
sysfs_del_hardif(&batman_if->hardif_obj);
- call_rcu(&batman_if->rcu, hardif_free_rcu);
+ hardif_free_ref(batman_if);
}
void hardif_remove_interfaces(void)
@@ -548,7 +546,7 @@ static int hard_if_event(struct notifier_block *this,
};
hardif_put:
- kref_put(&batman_if->refcount, hardif_free_ref);
+ hardif_free_ref(batman_if);
out:
return NOTIFY_DONE;
}
diff --git a/batman-adv/hard-interface.h b/batman-adv/hard-interface.h
index ad19543..e488b90 100644
--- a/batman-adv/hard-interface.h
+++ b/batman-adv/hard-interface.h
@@ -37,13 +37,12 @@ void hardif_disable_interface(struct batman_if *batman_if);
void hardif_remove_interfaces(void);
int hardif_min_mtu(struct net_device *soft_iface);
void update_min_mtu(struct net_device *soft_iface);
+void hardif_free_rcu(struct rcu_head *rcu);
-static inline void hardif_free_ref(struct kref *refcount)
+static inline void hardif_free_ref(struct batman_if *batman_if)
{
- struct batman_if *batman_if;
-
- batman_if = container_of(refcount, struct batman_if, refcount);
- kfree(batman_if);
+ if (atomic_dec_and_test(&batman_if->refcount))
+ call_rcu(&batman_if->rcu, hardif_free_rcu);
}
#endif /* _NET_BATMAN_ADV_HARD_INTERFACE_H_ */
diff --git a/batman-adv/types.h b/batman-adv/types.h
index 8df6d9d..317ede8 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -43,7 +43,7 @@ struct batman_if {
unsigned char *packet_buff;
int packet_len;
struct kobject *hardif_obj;
- struct kref refcount;
+ atomic_t refcount;
struct packet_type batman_adv_ptype;
struct net_device *soft_iface;
struct rcu_head rcu;
--
1.7.2.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [B.A.T.M.A.N.] [PATCH 4/4] batman-adv: Correct rcu refcounting for neigh_node
2011-02-03 17:02 ` [B.A.T.M.A.N.] Reordered rcu refcounting Marek Lindner
` (2 preceding siblings ...)
2011-02-03 17:03 ` [B.A.T.M.A.N.] [PATCH 3/4] batman-adv: Correct rcu refcounting for batman_if Marek Lindner
@ 2011-02-03 17:03 ` Marek Lindner
2011-02-04 12:59 ` [B.A.T.M.A.N.] [PATCHv2 " Marek Lindner
2011-02-04 15:21 ` [B.A.T.M.A.N.] Reordered rcu refcounting, v3 Linus Lüssing
` (7 subsequent siblings)
11 siblings, 1 reply; 44+ messages in thread
From: Marek Lindner @ 2011-02-03 17:03 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Marek Lindner
From: Sven Eckelmann <sven@narfation.org>
It might be possible that 2 threads access the same data in the same
rcu grace period. The first thread calls call_rcu() to decrement the
refcount and free the data while the second thread increases the
refcount to use the data. To avoid this race condition all refcount
operations have to be atomic.
Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
---
batman-adv/icmp_socket.c | 7 ++-
batman-adv/originator.c | 26 ++++---------
batman-adv/originator.h | 3 +-
batman-adv/routing.c | 93 ++++++++++++++++++++++++++++++++-------------
batman-adv/types.h | 3 +-
batman-adv/unicast.c | 2 +-
batman-adv/vis.c | 8 +++-
7 files changed, 87 insertions(+), 55 deletions(-)
diff --git a/batman-adv/icmp_socket.c b/batman-adv/icmp_socket.c
index 10a969c..046f976 100644
--- a/batman-adv/icmp_socket.c
+++ b/batman-adv/icmp_socket.c
@@ -230,10 +230,11 @@ static ssize_t bat_socket_write(struct file *file, const char __user *buff,
kref_get(&orig_node->refcount);
neigh_node = orig_node->router;
- if (!neigh_node)
+ if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
goto unlock;
+ }
- kref_get(&neigh_node->refcount);
rcu_read_unlock();
if (!neigh_node->if_incoming)
@@ -261,7 +262,7 @@ free_skb:
kfree_skb(skb);
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return len;
diff --git a/batman-adv/originator.c b/batman-adv/originator.c
index e8a8473..bde9778 100644
--- a/batman-adv/originator.c
+++ b/batman-adv/originator.c
@@ -56,28 +56,18 @@ err:
return 0;
}
-void neigh_node_free_ref(struct kref *refcount)
-{
- struct neigh_node *neigh_node;
-
- neigh_node = container_of(refcount, struct neigh_node, refcount);
- kfree(neigh_node);
-}
-
static void neigh_node_free_rcu(struct rcu_head *rcu)
{
struct neigh_node *neigh_node;
neigh_node = container_of(rcu, struct neigh_node, rcu);
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ kfree(neigh_node);
}
-void neigh_node_free_rcu_bond(struct rcu_head *rcu)
+void neigh_node_free_ref(struct neigh_node *neigh_node)
{
- struct neigh_node *neigh_node;
-
- neigh_node = container_of(rcu, struct neigh_node, rcu_bond);
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ if (atomic_dec_and_test(&neigh_node->refcount))
+ call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
}
struct neigh_node *create_neighbor(struct orig_node *orig_node,
@@ -101,7 +91,7 @@ struct neigh_node *create_neighbor(struct orig_node *orig_node,
memcpy(neigh_node->addr, neigh, ETH_ALEN);
neigh_node->orig_node = orig_neigh_node;
neigh_node->if_incoming = if_incoming;
- kref_init(&neigh_node->refcount);
+ atomic_set(&neigh_node->refcount, 1);
spin_lock_bh(&orig_node->neigh_list_lock);
hlist_add_head_rcu(&neigh_node->list, &orig_node->neigh_list);
@@ -123,14 +113,14 @@ void orig_node_free_ref(struct kref *refcount)
list_for_each_entry_safe(neigh_node, tmp_neigh_node,
&orig_node->bond_list, bonding_list) {
list_del_rcu(&neigh_node->bonding_list);
- call_rcu(&neigh_node->rcu_bond, neigh_node_free_rcu_bond);
+ neigh_node_free_ref(neigh_node);
}
/* for all neighbors towards this originator ... */
hlist_for_each_entry_safe(neigh_node, node, node_tmp,
&orig_node->neigh_list, list) {
hlist_del_rcu(&neigh_node->list);
- call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
+ neigh_node_free_ref(neigh_node);
}
spin_unlock_bh(&orig_node->neigh_list_lock);
@@ -311,7 +301,7 @@ static bool purge_orig_neighbors(struct bat_priv *bat_priv,
hlist_del_rcu(&neigh_node->list);
bonding_candidate_del(orig_node, neigh_node);
- call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
+ neigh_node_free_ref(neigh_node);
} else {
if ((!*best_neigh_node) ||
(neigh_node->tq_avg > (*best_neigh_node)->tq_avg))
diff --git a/batman-adv/originator.h b/batman-adv/originator.h
index 360dfd1..84d96e2 100644
--- a/batman-adv/originator.h
+++ b/batman-adv/originator.h
@@ -26,13 +26,12 @@ int originator_init(struct bat_priv *bat_priv);
void originator_free(struct bat_priv *bat_priv);
void purge_orig_ref(struct bat_priv *bat_priv);
void orig_node_free_ref(struct kref *refcount);
-void neigh_node_free_rcu_bond(struct rcu_head *rcu);
struct orig_node *get_orig_node(struct bat_priv *bat_priv, uint8_t *addr);
struct neigh_node *create_neighbor(struct orig_node *orig_node,
struct orig_node *orig_neigh_node,
uint8_t *neigh,
struct batman_if *if_incoming);
-void neigh_node_free_ref(struct kref *refcount);
+void neigh_node_free_ref(struct neigh_node *neigh_node);
int orig_seq_print_text(struct seq_file *seq, void *offset);
int orig_hash_add_if(struct batman_if *batman_if, int max_if_num);
int orig_hash_del_if(struct batman_if *batman_if, int max_if_num);
diff --git a/batman-adv/routing.c b/batman-adv/routing.c
index 2861f18..0d6a629 100644
--- a/batman-adv/routing.c
+++ b/batman-adv/routing.c
@@ -118,12 +118,12 @@ static void update_route(struct bat_priv *bat_priv,
orig_node->router->addr);
}
- if (neigh_node)
- kref_get(&neigh_node->refcount);
+ if (neigh_node && !atomic_inc_not_zero(&neigh_node->refcount))
+ neigh_node = NULL;
neigh_node_tmp = orig_node->router;
orig_node->router = neigh_node;
if (neigh_node_tmp)
- kref_put(&neigh_node_tmp->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node_tmp);
}
@@ -175,7 +175,11 @@ static int is_bidirectional_neigh(struct orig_node *orig_node,
if (!neigh_node)
goto unlock;
- kref_get(&neigh_node->refcount);
+ if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
+ goto unlock;
+ }
+
rcu_read_unlock();
neigh_node->last_valid = jiffies;
@@ -200,7 +204,11 @@ static int is_bidirectional_neigh(struct orig_node *orig_node,
if (!neigh_node)
goto unlock;
- kref_get(&neigh_node->refcount);
+ if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
+ goto unlock;
+ }
+
rcu_read_unlock();
}
@@ -262,7 +270,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
return ret;
}
@@ -275,8 +283,8 @@ void bonding_candidate_del(struct orig_node *orig_node,
goto out;
list_del_rcu(&neigh_node->bonding_list);
- call_rcu(&neigh_node->rcu_bond, neigh_node_free_rcu_bond);
INIT_LIST_HEAD(&neigh_node->bonding_list);
+ neigh_node_free_ref(neigh_node);
atomic_dec(&orig_node->bond_candidates);
out:
@@ -337,8 +345,10 @@ static void bonding_candidate_add(struct orig_node *orig_node,
if (!list_empty(&neigh_node->bonding_list))
goto out;
+ if (!atomic_inc_not_zero(&neigh_node->refcount))
+ goto out;
+
list_add_rcu(&neigh_node->bonding_list, &orig_node->bond_list);
- kref_get(&neigh_node->refcount);
atomic_inc(&orig_node->bond_candidates);
goto out;
@@ -382,7 +392,10 @@ static void update_orig(struct bat_priv *bat_priv,
hlist_for_each_entry_rcu(tmp_neigh_node, node,
&orig_node->neigh_list, list) {
if (compare_orig(tmp_neigh_node->addr, ethhdr->h_source) &&
- (tmp_neigh_node->if_incoming == if_incoming)) {
+ (tmp_neigh_node->if_incoming == if_incoming) &&
+ atomic_inc_not_zero(&tmp_neigh_node->refcount)) {
+ if (neigh_node)
+ neigh_node_free_ref(neigh_node);
neigh_node = tmp_neigh_node;
continue;
}
@@ -409,11 +422,15 @@ static void update_orig(struct bat_priv *bat_priv,
kref_put(&orig_tmp->refcount, orig_node_free_ref);
if (!neigh_node)
goto unlock;
+
+ if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
+ goto unlock;
+ }
} else
bat_dbg(DBG_BATMAN, bat_priv,
"Updating existing last-hop neighbor of originator\n");
- kref_get(&neigh_node->refcount);
rcu_read_unlock();
orig_node->flags = batman_packet->flags;
@@ -490,7 +507,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
}
/* checks whether the host restarted and is in the protection time.
@@ -894,7 +911,11 @@ static int recv_my_icmp_packet(struct bat_priv *bat_priv,
if (!neigh_node)
goto unlock;
- kref_get(&neigh_node->refcount);
+ if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
+ goto unlock;
+ }
+
rcu_read_unlock();
/* create a copy of the skb, if needed, to modify it. */
@@ -917,7 +938,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return ret;
@@ -958,7 +979,11 @@ static int recv_icmp_ttl_exceeded(struct bat_priv *bat_priv,
if (!neigh_node)
goto unlock;
- kref_get(&neigh_node->refcount);
+ if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
+ goto unlock;
+ }
+
rcu_read_unlock();
/* create a copy of the skb, if needed, to modify it. */
@@ -981,7 +1006,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return ret;
@@ -1054,7 +1079,11 @@ int recv_icmp_packet(struct sk_buff *skb, struct batman_if *recv_if)
if (!neigh_node)
goto unlock;
- kref_get(&neigh_node->refcount);
+ if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
+ goto unlock;
+ }
+
rcu_read_unlock();
/* create a copy of the skb, if needed, to modify it. */
@@ -1075,7 +1104,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return ret;
@@ -1108,12 +1137,11 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
/* select default router to output */
router = orig_node->router;
router_orig = orig_node->router->orig_node;
- if (!router_orig) {
+ if (!router_orig || !atomic_inc_not_zero(&router->refcount)) {
rcu_read_unlock();
return NULL;
}
-
if ((!recv_if) && (!bonding_enabled))
goto return_router;
@@ -1146,6 +1174,7 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
* is is not on the interface where the packet came
* in. */
+ neigh_node_free_ref(router);
first_candidate = NULL;
router = NULL;
@@ -1158,16 +1187,23 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
if (!first_candidate)
first_candidate = tmp_neigh_node;
/* recv_if == NULL on the first node. */
- if (tmp_neigh_node->if_incoming != recv_if) {
+ if (tmp_neigh_node->if_incoming != recv_if &&
+ atomic_inc_not_zero(&tmp_neigh_node->refcount)) {
router = tmp_neigh_node;
break;
}
}
/* use the first candidate if nothing was found. */
- if (!router)
+ if (!router && first_candidate &&
+ atomic_inc_not_zero(&first_candidate->refcount))
router = first_candidate;
+ if (!router) {
+ rcu_read_unlock();
+ return NULL;
+ }
+
/* selected should point to the next element
* after the current router */
spin_lock_bh(&primary_orig_node->neigh_list_lock);
@@ -1188,21 +1224,24 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
first_candidate = tmp_neigh_node;
/* recv_if == NULL on the first node. */
- if (tmp_neigh_node->if_incoming != recv_if)
+ if (tmp_neigh_node->if_incoming != recv_if &&
+ atomic_inc_not_zero(&tmp_neigh_node->refcount)) {
/* if we don't have a router yet
* or this one is better, choose it. */
if ((!router) ||
- (tmp_neigh_node->tq_avg > router->tq_avg)) {
+ (tmp_neigh_node->tq_avg > router->tq_avg))
router = tmp_neigh_node;
- }
+ else
+ neigh_node_free_ref(tmp_neigh_node);
+ }
}
/* use the first candidate if nothing was found. */
- if (!router)
+ if (!router && first_candidate &&
+ atomic_inc_not_zero(&first_candidate->refcount))
router = first_candidate;
}
return_router:
- kref_get(&router->refcount);
rcu_read_unlock();
return router;
}
@@ -1315,7 +1354,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return ret;
diff --git a/batman-adv/types.h b/batman-adv/types.h
index 317ede8..ee77d48 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -119,9 +119,8 @@ struct neigh_node {
struct list_head bonding_list;
unsigned long last_valid;
unsigned long real_bits[NUM_WORDS];
- struct kref refcount;
+ atomic_t refcount;
struct rcu_head rcu;
- struct rcu_head rcu_bond;
struct orig_node *orig_node;
struct batman_if *if_incoming;
};
diff --git a/batman-adv/unicast.c b/batman-adv/unicast.c
index 6a9ab61..580b547 100644
--- a/batman-adv/unicast.c
+++ b/batman-adv/unicast.c
@@ -363,7 +363,7 @@ find_router:
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
if (ret == 1)
diff --git a/batman-adv/vis.c b/batman-adv/vis.c
index 191401b..c1c3258 100644
--- a/batman-adv/vis.c
+++ b/batman-adv/vis.c
@@ -776,7 +776,11 @@ static void unicast_vis_packet(struct bat_priv *bat_priv,
if (!neigh_node)
goto unlock;
- kref_get(&neigh_node->refcount);
+ if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
+ goto unlock;
+ }
+
rcu_read_unlock();
skb = skb_clone(info->skb_packet, GFP_ATOMIC);
@@ -790,7 +794,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return;
--
1.7.2.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH 1/3] batman-adv: Correct rcu refcounting for gw_node
2011-02-03 14:43 ` [B.A.T.M.A.N.] [PATCH 1/3] " Linus Lüssing
@ 2011-02-03 17:56 ` Sven Eckelmann
0 siblings, 0 replies; 44+ messages in thread
From: Sven Eckelmann @ 2011-02-03 17:56 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Linus Lüssing
[-- Attachment #1: Type: Text/Plain, Size: 2306 bytes --]
> For another the rcu protected macros rcu_dereference() and
> rcu_assign_pointer() for the bat_priv->curr_gw and curr_gw->orig_node
> need to be used.
What? Since when is curr_gw::orig_node rcu protected?
> - return curr_gateway_tmp->orig_node;
> + orig_node = rcu_dereference(curr_gateway_tmp->orig_node);
> + if (orig_node) {
> + rcu_read_unlock();
> + return NULL;
> + }
> +
> + rcu_read_unlock();
> + return orig_node;
> }
And you cannot return the (it is not, but when it would be) rcu protected
element without increasing the refcounter.
> + orig_node = rcu_dereference(gw_node->orig_node);
> + if (!orig_node->router)
> continue;
Same here
> - if (bat_priv->curr_gw != curr_gw_tmp) {
> - if ((bat_priv->curr_gw) && (!curr_gw_tmp))
> + if (curr_gw != curr_gw_tmp) {
> + orig_node = rcu_dereference(curr_gw_tmp->orig_node);
> + if ((curr_gw) && (!curr_gw_tmp))
And here
> - if (bat_priv->curr_gw != curr_gw_tmp) {
> - if ((bat_priv->curr_gw) && (!curr_gw_tmp))
> + if (curr_gw != curr_gw_tmp) {
> + orig_node = rcu_dereference(curr_gw_tmp->orig_node);
> + if ((curr_gw) && (!curr_gw_tmp))
Here again
> - if (bat_priv->curr_gw != curr_gw_tmp) {
> - if ((bat_priv->curr_gw) && (!curr_gw_tmp))
> + if (curr_gw != curr_gw_tmp) {
> + orig_node = rcu_dereference(curr_gw_tmp->orig_node);
> + if ((curr_gw) && (!curr_gw_tmp))
Here....
> - gw_bandwidth_to_kbit(gw_node->orig_node->gw_flags, &down, &up);
> + rcu_read_lock();
> + curr_gw = rcu_dereference(bat_priv->curr_gw);
> + orig_node = rcu_dereference(gw_node->orig_node);
> + gw_bandwidth_to_kbit(orig_node->gw_flags, &down, &up);
....
> diff --git a/batman-adv/unicast.c b/batman-adv/unicast.c
> index 6a9ab61..8816102 100644
> --- a/batman-adv/unicast.c
> +++ b/batman-adv/unicast.c
> @@ -298,7 +298,6 @@ int unicast_send_skb(struct sk_buff *skb, struct
> bat_priv *bat_priv) if (!orig_node)
> goto trans_search;
>
> - kref_get(&orig_node->refcount);
> goto find_router;
> } else {
> rcu_read_lock();
Why? Maybe this should be part of the patch 3/3?
And please to merge the refcounting stuff with the rcu pointer in one patch...
and maybe it is better to merge patch 2/3 with the rcu pointer patch.
Best regards,
Sven
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* [B.A.T.M.A.N.] [PATCHv2 4/4] batman-adv: Correct rcu refcounting for neigh_node
2011-02-03 17:03 ` [B.A.T.M.A.N.] [PATCH 4/4] batman-adv: Correct rcu refcounting for neigh_node Marek Lindner
@ 2011-02-04 12:59 ` Marek Lindner
0 siblings, 0 replies; 44+ messages in thread
From: Marek Lindner @ 2011-02-04 12:59 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Marek Lindner
From: Sven Eckelmann <sven@narfation.org>
It might be possible that 2 threads access the same data in the same
rcu grace period. The first thread calls call_rcu() to decrement the
refcount and free the data while the second thread increases the
refcount to use the data. To avoid this race condition all refcount
operations have to be atomic.
Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
---
Fixed refcounting in find_router().
batman-adv/icmp_socket.c | 7 ++-
batman-adv/originator.c | 26 +++-------
batman-adv/originator.h | 3 +-
batman-adv/routing.c | 111 +++++++++++++++++++++++++++++++++-------------
batman-adv/types.h | 3 +-
batman-adv/unicast.c | 2 +-
batman-adv/vis.c | 8 +++-
7 files changed, 101 insertions(+), 59 deletions(-)
diff --git a/batman-adv/icmp_socket.c b/batman-adv/icmp_socket.c
index 10a969c..046f976 100644
--- a/batman-adv/icmp_socket.c
+++ b/batman-adv/icmp_socket.c
@@ -230,10 +230,11 @@ static ssize_t bat_socket_write(struct file *file, const char __user *buff,
kref_get(&orig_node->refcount);
neigh_node = orig_node->router;
- if (!neigh_node)
+ if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
goto unlock;
+ }
- kref_get(&neigh_node->refcount);
rcu_read_unlock();
if (!neigh_node->if_incoming)
@@ -261,7 +262,7 @@ free_skb:
kfree_skb(skb);
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return len;
diff --git a/batman-adv/originator.c b/batman-adv/originator.c
index e8a8473..bde9778 100644
--- a/batman-adv/originator.c
+++ b/batman-adv/originator.c
@@ -56,28 +56,18 @@ err:
return 0;
}
-void neigh_node_free_ref(struct kref *refcount)
-{
- struct neigh_node *neigh_node;
-
- neigh_node = container_of(refcount, struct neigh_node, refcount);
- kfree(neigh_node);
-}
-
static void neigh_node_free_rcu(struct rcu_head *rcu)
{
struct neigh_node *neigh_node;
neigh_node = container_of(rcu, struct neigh_node, rcu);
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ kfree(neigh_node);
}
-void neigh_node_free_rcu_bond(struct rcu_head *rcu)
+void neigh_node_free_ref(struct neigh_node *neigh_node)
{
- struct neigh_node *neigh_node;
-
- neigh_node = container_of(rcu, struct neigh_node, rcu_bond);
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ if (atomic_dec_and_test(&neigh_node->refcount))
+ call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
}
struct neigh_node *create_neighbor(struct orig_node *orig_node,
@@ -101,7 +91,7 @@ struct neigh_node *create_neighbor(struct orig_node *orig_node,
memcpy(neigh_node->addr, neigh, ETH_ALEN);
neigh_node->orig_node = orig_neigh_node;
neigh_node->if_incoming = if_incoming;
- kref_init(&neigh_node->refcount);
+ atomic_set(&neigh_node->refcount, 1);
spin_lock_bh(&orig_node->neigh_list_lock);
hlist_add_head_rcu(&neigh_node->list, &orig_node->neigh_list);
@@ -123,14 +113,14 @@ void orig_node_free_ref(struct kref *refcount)
list_for_each_entry_safe(neigh_node, tmp_neigh_node,
&orig_node->bond_list, bonding_list) {
list_del_rcu(&neigh_node->bonding_list);
- call_rcu(&neigh_node->rcu_bond, neigh_node_free_rcu_bond);
+ neigh_node_free_ref(neigh_node);
}
/* for all neighbors towards this originator ... */
hlist_for_each_entry_safe(neigh_node, node, node_tmp,
&orig_node->neigh_list, list) {
hlist_del_rcu(&neigh_node->list);
- call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
+ neigh_node_free_ref(neigh_node);
}
spin_unlock_bh(&orig_node->neigh_list_lock);
@@ -311,7 +301,7 @@ static bool purge_orig_neighbors(struct bat_priv *bat_priv,
hlist_del_rcu(&neigh_node->list);
bonding_candidate_del(orig_node, neigh_node);
- call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
+ neigh_node_free_ref(neigh_node);
} else {
if ((!*best_neigh_node) ||
(neigh_node->tq_avg > (*best_neigh_node)->tq_avg))
diff --git a/batman-adv/originator.h b/batman-adv/originator.h
index 360dfd1..84d96e2 100644
--- a/batman-adv/originator.h
+++ b/batman-adv/originator.h
@@ -26,13 +26,12 @@ int originator_init(struct bat_priv *bat_priv);
void originator_free(struct bat_priv *bat_priv);
void purge_orig_ref(struct bat_priv *bat_priv);
void orig_node_free_ref(struct kref *refcount);
-void neigh_node_free_rcu_bond(struct rcu_head *rcu);
struct orig_node *get_orig_node(struct bat_priv *bat_priv, uint8_t *addr);
struct neigh_node *create_neighbor(struct orig_node *orig_node,
struct orig_node *orig_neigh_node,
uint8_t *neigh,
struct batman_if *if_incoming);
-void neigh_node_free_ref(struct kref *refcount);
+void neigh_node_free_ref(struct neigh_node *neigh_node);
int orig_seq_print_text(struct seq_file *seq, void *offset);
int orig_hash_add_if(struct batman_if *batman_if, int max_if_num);
int orig_hash_del_if(struct batman_if *batman_if, int max_if_num);
diff --git a/batman-adv/routing.c b/batman-adv/routing.c
index 2861f18..091edd4 100644
--- a/batman-adv/routing.c
+++ b/batman-adv/routing.c
@@ -118,12 +118,12 @@ static void update_route(struct bat_priv *bat_priv,
orig_node->router->addr);
}
- if (neigh_node)
- kref_get(&neigh_node->refcount);
+ if (neigh_node && !atomic_inc_not_zero(&neigh_node->refcount))
+ neigh_node = NULL;
neigh_node_tmp = orig_node->router;
orig_node->router = neigh_node;
if (neigh_node_tmp)
- kref_put(&neigh_node_tmp->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node_tmp);
}
@@ -175,7 +175,11 @@ static int is_bidirectional_neigh(struct orig_node *orig_node,
if (!neigh_node)
goto unlock;
- kref_get(&neigh_node->refcount);
+ if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
+ goto unlock;
+ }
+
rcu_read_unlock();
neigh_node->last_valid = jiffies;
@@ -200,7 +204,11 @@ static int is_bidirectional_neigh(struct orig_node *orig_node,
if (!neigh_node)
goto unlock;
- kref_get(&neigh_node->refcount);
+ if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
+ goto unlock;
+ }
+
rcu_read_unlock();
}
@@ -262,7 +270,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
return ret;
}
@@ -275,8 +283,8 @@ void bonding_candidate_del(struct orig_node *orig_node,
goto out;
list_del_rcu(&neigh_node->bonding_list);
- call_rcu(&neigh_node->rcu_bond, neigh_node_free_rcu_bond);
INIT_LIST_HEAD(&neigh_node->bonding_list);
+ neigh_node_free_ref(neigh_node);
atomic_dec(&orig_node->bond_candidates);
out:
@@ -337,8 +345,10 @@ static void bonding_candidate_add(struct orig_node *orig_node,
if (!list_empty(&neigh_node->bonding_list))
goto out;
+ if (!atomic_inc_not_zero(&neigh_node->refcount))
+ goto out;
+
list_add_rcu(&neigh_node->bonding_list, &orig_node->bond_list);
- kref_get(&neigh_node->refcount);
atomic_inc(&orig_node->bond_candidates);
goto out;
@@ -382,7 +392,10 @@ static void update_orig(struct bat_priv *bat_priv,
hlist_for_each_entry_rcu(tmp_neigh_node, node,
&orig_node->neigh_list, list) {
if (compare_orig(tmp_neigh_node->addr, ethhdr->h_source) &&
- (tmp_neigh_node->if_incoming == if_incoming)) {
+ (tmp_neigh_node->if_incoming == if_incoming) &&
+ atomic_inc_not_zero(&tmp_neigh_node->refcount)) {
+ if (neigh_node)
+ neigh_node_free_ref(neigh_node);
neigh_node = tmp_neigh_node;
continue;
}
@@ -409,11 +422,15 @@ static void update_orig(struct bat_priv *bat_priv,
kref_put(&orig_tmp->refcount, orig_node_free_ref);
if (!neigh_node)
goto unlock;
+
+ if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
+ goto unlock;
+ }
} else
bat_dbg(DBG_BATMAN, bat_priv,
"Updating existing last-hop neighbor of originator\n");
- kref_get(&neigh_node->refcount);
rcu_read_unlock();
orig_node->flags = batman_packet->flags;
@@ -490,7 +507,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
}
/* checks whether the host restarted and is in the protection time.
@@ -894,7 +911,11 @@ static int recv_my_icmp_packet(struct bat_priv *bat_priv,
if (!neigh_node)
goto unlock;
- kref_get(&neigh_node->refcount);
+ if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
+ goto unlock;
+ }
+
rcu_read_unlock();
/* create a copy of the skb, if needed, to modify it. */
@@ -917,7 +938,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return ret;
@@ -958,7 +979,11 @@ static int recv_icmp_ttl_exceeded(struct bat_priv *bat_priv,
if (!neigh_node)
goto unlock;
- kref_get(&neigh_node->refcount);
+ if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
+ goto unlock;
+ }
+
rcu_read_unlock();
/* create a copy of the skb, if needed, to modify it. */
@@ -981,7 +1006,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return ret;
@@ -1054,7 +1079,11 @@ int recv_icmp_packet(struct sk_buff *skb, struct batman_if *recv_if)
if (!neigh_node)
goto unlock;
- kref_get(&neigh_node->refcount);
+ if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
+ goto unlock;
+ }
+
rcu_read_unlock();
/* create a copy of the skb, if needed, to modify it. */
@@ -1075,7 +1104,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return ret;
@@ -1108,12 +1137,11 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
/* select default router to output */
router = orig_node->router;
router_orig = orig_node->router->orig_node;
- if (!router_orig) {
+ if (!router_orig || !atomic_inc_not_zero(&router->refcount)) {
rcu_read_unlock();
return NULL;
}
-
if ((!recv_if) && (!bonding_enabled))
goto return_router;
@@ -1146,6 +1174,7 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
* is is not on the interface where the packet came
* in. */
+ neigh_node_free_ref(router);
first_candidate = NULL;
router = NULL;
@@ -1158,16 +1187,23 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
if (!first_candidate)
first_candidate = tmp_neigh_node;
/* recv_if == NULL on the first node. */
- if (tmp_neigh_node->if_incoming != recv_if) {
+ if (tmp_neigh_node->if_incoming != recv_if &&
+ atomic_inc_not_zero(&tmp_neigh_node->refcount)) {
router = tmp_neigh_node;
break;
}
}
/* use the first candidate if nothing was found. */
- if (!router)
+ if (!router && first_candidate &&
+ atomic_inc_not_zero(&first_candidate->refcount))
router = first_candidate;
+ if (!router) {
+ rcu_read_unlock();
+ return NULL;
+ }
+
/* selected should point to the next element
* after the current router */
spin_lock_bh(&primary_orig_node->neigh_list_lock);
@@ -1188,21 +1224,34 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
first_candidate = tmp_neigh_node;
/* recv_if == NULL on the first node. */
- if (tmp_neigh_node->if_incoming != recv_if)
- /* if we don't have a router yet
- * or this one is better, choose it. */
- if ((!router) ||
- (tmp_neigh_node->tq_avg > router->tq_avg)) {
- router = tmp_neigh_node;
- }
+ if (tmp_neigh_node->if_incoming == recv_if)
+ continue;
+
+ if (!atomic_inc_not_zero(&tmp_neigh_node->refcount))
+ continue;
+
+ /* if we don't have a router yet
+ * or this one is better, choose it. */
+ if ((!router) ||
+ (tmp_neigh_node->tq_avg > router->tq_avg)) {
+ /* decrement refcount of
+ * previously selected router */
+ if (router)
+ neigh_node_free_ref(router);
+
+ router = tmp_neigh_node;
+ atomic_inc_not_zero(&router->refcount);
+ }
+
+ neigh_node_free_ref(tmp_neigh_node);
}
/* use the first candidate if nothing was found. */
- if (!router)
+ if (!router && first_candidate &&
+ atomic_inc_not_zero(&first_candidate->refcount))
router = first_candidate;
}
return_router:
- kref_get(&router->refcount);
rcu_read_unlock();
return router;
}
@@ -1315,7 +1364,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return ret;
diff --git a/batman-adv/types.h b/batman-adv/types.h
index 317ede8..ee77d48 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -119,9 +119,8 @@ struct neigh_node {
struct list_head bonding_list;
unsigned long last_valid;
unsigned long real_bits[NUM_WORDS];
- struct kref refcount;
+ atomic_t refcount;
struct rcu_head rcu;
- struct rcu_head rcu_bond;
struct orig_node *orig_node;
struct batman_if *if_incoming;
};
diff --git a/batman-adv/unicast.c b/batman-adv/unicast.c
index 6a9ab61..580b547 100644
--- a/batman-adv/unicast.c
+++ b/batman-adv/unicast.c
@@ -363,7 +363,7 @@ find_router:
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
if (ret == 1)
diff --git a/batman-adv/vis.c b/batman-adv/vis.c
index 191401b..c1c3258 100644
--- a/batman-adv/vis.c
+++ b/batman-adv/vis.c
@@ -776,7 +776,11 @@ static void unicast_vis_packet(struct bat_priv *bat_priv,
if (!neigh_node)
goto unlock;
- kref_get(&neigh_node->refcount);
+ if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
+ goto unlock;
+ }
+
rcu_read_unlock();
skb = skb_clone(info->skb_packet, GFP_ATOMIC);
@@ -790,7 +794,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return;
--
1.7.2.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [B.A.T.M.A.N.] Reordered rcu refcounting, v3
2011-02-03 17:02 ` [B.A.T.M.A.N.] Reordered rcu refcounting Marek Lindner
` (3 preceding siblings ...)
2011-02-03 17:03 ` [B.A.T.M.A.N.] [PATCH 4/4] batman-adv: Correct rcu refcounting for neigh_node Marek Lindner
@ 2011-02-04 15:21 ` Linus Lüssing
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 1/7] batman-adv: Correct rcu refcounting for gw_node Linus Lüssing
` (6 subsequent siblings)
11 siblings, 0 replies; 44+ messages in thread
From: Linus Lüssing @ 2011-02-04 15:21 UTC (permalink / raw)
To: b.a.t.m.a.n
Hi everyone,
Patches [1-4] are untouched, added my stuff on top of it with some fixes:
* Made sure no rcu_read_lock() while gw_deselect() (gw_election(), gw_check_election())
* Removed orig_node null pointer check in gw_get_selected()
* Removed rcu_dereference() for orig_node
* Rebased to marec's last 4 patches
* Made extra patch for bat_priv->curr_gw rcu protection fixes
* Removed the stupid kref_get() removal which was not supposed to be part of this patch [my old 1/3]
* Removed the wrong kref_get() removal [my old 3/3]
It's all based on "batman-adv: Switch order of types.h and compat.h inclusion" (see separate patchset)
which fixes a compile error in my old [2/3] patch.
Note: rcu_dereference() in gw_node_purge() does not seem to be safe yet, there's
no rcu_read_lock(), so another thread, not through gw_node_purge(), might have just
performed a gw_deselect().
Cheers, Linus
^ permalink raw reply [flat|nested] 44+ messages in thread
* [B.A.T.M.A.N.] [PATCH 1/7] batman-adv: Correct rcu refcounting for gw_node
2011-02-03 17:02 ` [B.A.T.M.A.N.] Reordered rcu refcounting Marek Lindner
` (4 preceding siblings ...)
2011-02-04 15:21 ` [B.A.T.M.A.N.] Reordered rcu refcounting, v3 Linus Lüssing
@ 2011-02-04 15:21 ` Linus Lüssing
2011-02-10 11:01 ` Linus Lüssing
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 2/7] batman-adv: Correct rcu refcounting for softif_neigh Linus Lüssing
` (5 subsequent siblings)
11 siblings, 1 reply; 44+ messages in thread
From: Linus Lüssing @ 2011-02-04 15:21 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Marek Lindner
From: Sven Eckelmann <sven@narfation.org>
It might be possible that 2 threads access the same data in the same
rcu grace period. The first thread calls call_rcu() to decrement the
refcount and free the data while the second thread increases the
refcount to use the data. To avoid this race condition all refcount
operations have to be atomic.
Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
---
gateway_client.c | 41 ++++++++++++++++++-----------------------
types.h | 2 +-
2 files changed, 19 insertions(+), 24 deletions(-)
diff --git a/batman-adv/gateway_client.c b/batman-adv/gateway_client.c
index 429a013..517e001 100644
--- a/batman-adv/gateway_client.c
+++ b/batman-adv/gateway_client.c
@@ -28,20 +28,18 @@
#include <linux/udp.h>
#include <linux/if_vlan.h>
-static void gw_node_free_ref(struct kref *refcount)
-{
- struct gw_node *gw_node;
-
- gw_node = container_of(refcount, struct gw_node, refcount);
- kfree(gw_node);
-}
-
static void gw_node_free_rcu(struct rcu_head *rcu)
{
struct gw_node *gw_node;
gw_node = container_of(rcu, struct gw_node, rcu);
- kref_put(&gw_node->refcount, gw_node_free_ref);
+ kfree(gw_node);
+}
+
+static void gw_node_free_ref(struct gw_node *gw_node)
+{
+ if (atomic_dec_and_test(&gw_node->refcount))
+ call_rcu(&gw_node->rcu, gw_node_free_rcu);
}
void *gw_get_selected(struct bat_priv *bat_priv)
@@ -61,25 +59,26 @@ void gw_deselect(struct bat_priv *bat_priv)
bat_priv->curr_gw = NULL;
if (gw_node)
- kref_put(&gw_node->refcount, gw_node_free_ref);
+ gw_node_free_ref(gw_node);
}
-static struct gw_node *gw_select(struct bat_priv *bat_priv,
- struct gw_node *new_gw_node)
+static void gw_select(struct bat_priv *bat_priv, struct gw_node *new_gw_node)
{
struct gw_node *curr_gw_node = bat_priv->curr_gw;
- if (new_gw_node)
- kref_get(&new_gw_node->refcount);
+ if (new_gw_node && !atomic_inc_not_zero(&new_gw_node->refcount))
+ new_gw_node = NULL;
bat_priv->curr_gw = new_gw_node;
- return curr_gw_node;
+
+ if (curr_gw_node)
+ gw_node_free_ref(curr_gw_node);
}
void gw_election(struct bat_priv *bat_priv)
{
struct hlist_node *node;
- struct gw_node *gw_node, *curr_gw_tmp = NULL, *old_gw_node = NULL;
+ struct gw_node *gw_node, *curr_gw_tmp = NULL;
uint8_t max_tq = 0;
uint32_t max_gw_factor = 0, tmp_gw_factor = 0;
int down, up;
@@ -174,14 +173,10 @@ void gw_election(struct bat_priv *bat_priv)
curr_gw_tmp->orig_node->gw_flags,
curr_gw_tmp->orig_node->router->tq_avg);
- old_gw_node = gw_select(bat_priv, curr_gw_tmp);
+ gw_select(bat_priv, curr_gw_tmp);
}
rcu_read_unlock();
-
- /* the kfree() has to be outside of the rcu lock */
- if (old_gw_node)
- kref_put(&old_gw_node->refcount, gw_node_free_ref);
}
void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
@@ -242,7 +237,7 @@ static void gw_node_add(struct bat_priv *bat_priv,
memset(gw_node, 0, sizeof(struct gw_node));
INIT_HLIST_NODE(&gw_node->list);
gw_node->orig_node = orig_node;
- kref_init(&gw_node->refcount);
+ atomic_set(&gw_node->refcount, 1);
spin_lock_bh(&bat_priv->gw_list_lock);
hlist_add_head_rcu(&gw_node->list, &bat_priv->gw_list);
@@ -325,7 +320,7 @@ void gw_node_purge(struct bat_priv *bat_priv)
gw_deselect(bat_priv);
hlist_del_rcu(&gw_node->list);
- call_rcu(&gw_node->rcu, gw_node_free_rcu);
+ gw_node_free_ref(gw_node);
}
diff --git a/batman-adv/types.h b/batman-adv/types.h
index e4a0462..ca5f20a 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -100,7 +100,7 @@ struct gw_node {
struct hlist_node list;
struct orig_node *orig_node;
unsigned long deleted;
- struct kref refcount;
+ atomic_t refcount;
struct rcu_head rcu;
};
--
1.7.2.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [B.A.T.M.A.N.] [PATCH 2/7] batman-adv: Correct rcu refcounting for softif_neigh
2011-02-03 17:02 ` [B.A.T.M.A.N.] Reordered rcu refcounting Marek Lindner
` (5 preceding siblings ...)
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 1/7] batman-adv: Correct rcu refcounting for gw_node Linus Lüssing
@ 2011-02-04 15:21 ` Linus Lüssing
2011-02-10 12:45 ` Linus Lüssing
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 3/7] batman-adv: Correct rcu refcounting for batman_if Linus Lüssing
` (4 subsequent siblings)
11 siblings, 1 reply; 44+ messages in thread
From: Linus Lüssing @ 2011-02-04 15:21 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Marek Lindner
From: Sven Eckelmann <sven@narfation.org>
It might be possible that 2 threads access the same data in the same
rcu grace period. The first thread calls call_rcu() to decrement the
refcount and free the data while the second thread increases the
refcount to use the data. To avoid this race condition all refcount
operations have to be atomic.
Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
---
soft-interface.c | 35 +++++++++++++++++------------------
types.h | 2 +-
2 files changed, 18 insertions(+), 19 deletions(-)
diff --git a/batman-adv/soft-interface.c b/batman-adv/soft-interface.c
index bd088f8..bd8b539 100644
--- a/batman-adv/soft-interface.c
+++ b/batman-adv/soft-interface.c
@@ -78,20 +78,18 @@ int my_skb_head_push(struct sk_buff *skb, unsigned int len)
return 0;
}
-static void softif_neigh_free_ref(struct kref *refcount)
-{
- struct softif_neigh *softif_neigh;
-
- softif_neigh = container_of(refcount, struct softif_neigh, refcount);
- kfree(softif_neigh);
-}
-
static void softif_neigh_free_rcu(struct rcu_head *rcu)
{
struct softif_neigh *softif_neigh;
softif_neigh = container_of(rcu, struct softif_neigh, rcu);
- kref_put(&softif_neigh->refcount, softif_neigh_free_ref);
+ kfree(softif_neigh);
+}
+
+static void softif_neigh_free_ref(struct softif_neigh *softif_neigh)
+{
+ if (atomic_dec_and_test(&softif_neigh->refcount))
+ call_rcu(&softif_neigh->rcu, softif_neigh_free_rcu);
}
void softif_neigh_purge(struct bat_priv *bat_priv)
@@ -118,11 +116,10 @@ void softif_neigh_purge(struct bat_priv *bat_priv)
softif_neigh->addr, softif_neigh->vid);
softif_neigh_tmp = bat_priv->softif_neigh;
bat_priv->softif_neigh = NULL;
- kref_put(&softif_neigh_tmp->refcount,
- softif_neigh_free_ref);
+ softif_neigh_free_ref(softif_neigh_tmp);
}
- call_rcu(&softif_neigh->rcu, softif_neigh_free_rcu);
+ softif_neigh_free_ref(softif_neigh);
}
spin_unlock_bh(&bat_priv->softif_neigh_lock);
@@ -143,8 +140,11 @@ static struct softif_neigh *softif_neigh_get(struct bat_priv *bat_priv,
if (softif_neigh->vid != vid)
continue;
+ if (!atomic_inc_not_zero(&softif_neigh->refcount))
+ continue;
+
softif_neigh->last_seen = jiffies;
- goto found;
+ goto out;
}
softif_neigh = kzalloc(sizeof(struct softif_neigh), GFP_ATOMIC);
@@ -154,15 +154,14 @@ static struct softif_neigh *softif_neigh_get(struct bat_priv *bat_priv,
memcpy(softif_neigh->addr, addr, ETH_ALEN);
softif_neigh->vid = vid;
softif_neigh->last_seen = jiffies;
- kref_init(&softif_neigh->refcount);
+ /* initialize with 2 - caller decrements counter by one */
+ atomic_set(&softif_neigh->refcount, 2);
INIT_HLIST_NODE(&softif_neigh->list);
spin_lock_bh(&bat_priv->softif_neigh_lock);
hlist_add_head_rcu(&softif_neigh->list, &bat_priv->softif_neigh_list);
spin_unlock_bh(&bat_priv->softif_neigh_lock);
-found:
- kref_get(&softif_neigh->refcount);
out:
rcu_read_unlock();
return softif_neigh;
@@ -266,7 +265,7 @@ static void softif_batman_recv(struct sk_buff *skb, struct net_device *dev,
softif_neigh->addr, softif_neigh->vid);
softif_neigh_tmp = bat_priv->softif_neigh;
bat_priv->softif_neigh = softif_neigh;
- kref_put(&softif_neigh_tmp->refcount, softif_neigh_free_ref);
+ softif_neigh_free_ref(softif_neigh_tmp);
/* we need to hold the additional reference */
goto err;
}
@@ -284,7 +283,7 @@ static void softif_batman_recv(struct sk_buff *skb, struct net_device *dev,
}
out:
- kref_put(&softif_neigh->refcount, softif_neigh_free_ref);
+ softif_neigh_free_ref(softif_neigh);
err:
kfree_skb(skb);
return;
diff --git a/batman-adv/types.h b/batman-adv/types.h
index ca5f20a..8df6d9d 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -270,7 +270,7 @@ struct softif_neigh {
uint8_t addr[ETH_ALEN];
unsigned long last_seen;
short vid;
- struct kref refcount;
+ atomic_t refcount;
struct rcu_head rcu;
};
--
1.7.2.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [B.A.T.M.A.N.] [PATCH 3/7] batman-adv: Correct rcu refcounting for batman_if
2011-02-03 17:02 ` [B.A.T.M.A.N.] Reordered rcu refcounting Marek Lindner
` (6 preceding siblings ...)
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 2/7] batman-adv: Correct rcu refcounting for softif_neigh Linus Lüssing
@ 2011-02-04 15:21 ` Linus Lüssing
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 4/7] batman-adv: Correct rcu refcounting for neigh_node Linus Lüssing
` (3 subsequent siblings)
11 siblings, 0 replies; 44+ messages in thread
From: Linus Lüssing @ 2011-02-04 15:21 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Marek Lindner
From: Sven Eckelmann <sven@narfation.org>
It might be possible that 2 threads access the same data in the same
rcu grace period. The first thread calls call_rcu() to decrement the
refcount and free the data while the second thread increases the
refcount to use the data. To avoid this race condition all refcount
operations have to be atomic.
Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
---
bat_sysfs.c | 20 +++++++++-----------
hard-interface.c | 40 +++++++++++++++++++---------------------
hard-interface.h | 9 ++++-----
types.h | 2 +-
4 files changed, 33 insertions(+), 38 deletions(-)
diff --git a/batman-adv/bat_sysfs.c b/batman-adv/bat_sysfs.c
index f7b93a0..93ae20a 100644
--- a/batman-adv/bat_sysfs.c
+++ b/batman-adv/bat_sysfs.c
@@ -450,7 +450,7 @@ static ssize_t show_mesh_iface(struct kobject *kobj, struct attribute *attr,
length = sprintf(buff, "%s\n", batman_if->if_status == IF_NOT_IN_USE ?
"none" : batman_if->soft_iface->name);
- kref_put(&batman_if->refcount, hardif_free_ref);
+ hardif_free_ref(batman_if);
return length;
}
@@ -461,7 +461,7 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr,
struct net_device *net_dev = kobj_to_netdev(kobj);
struct batman_if *batman_if = get_batman_if_by_netdev(net_dev);
int status_tmp = -1;
- int ret;
+ int ret = count;
if (!batman_if)
return count;
@@ -472,7 +472,7 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr,
if (strlen(buff) >= IFNAMSIZ) {
pr_err("Invalid parameter for 'mesh_iface' setting received: "
"interface name too long '%s'\n", buff);
- kref_put(&batman_if->refcount, hardif_free_ref);
+ hardif_free_ref(batman_if);
return -EINVAL;
}
@@ -482,17 +482,14 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr,
status_tmp = IF_I_WANT_YOU;
if ((batman_if->if_status == status_tmp) || ((batman_if->soft_iface) &&
- (strncmp(batman_if->soft_iface->name, buff, IFNAMSIZ) == 0))) {
- kref_put(&batman_if->refcount, hardif_free_ref);
- return count;
- }
+ (strncmp(batman_if->soft_iface->name, buff, IFNAMSIZ) == 0)))
+ goto out;
if (status_tmp == IF_NOT_IN_USE) {
rtnl_lock();
hardif_disable_interface(batman_if);
rtnl_unlock();
- kref_put(&batman_if->refcount, hardif_free_ref);
- return count;
+ goto out;
}
/* if the interface already is in use */
@@ -503,8 +500,9 @@ static ssize_t store_mesh_iface(struct kobject *kobj, struct attribute *attr,
}
ret = hardif_enable_interface(batman_if, buff);
- kref_put(&batman_if->refcount, hardif_free_ref);
+out:
+ hardif_free_ref(batman_if);
return ret;
}
@@ -537,7 +535,7 @@ static ssize_t show_iface_status(struct kobject *kobj, struct attribute *attr,
break;
}
- kref_put(&batman_if->refcount, hardif_free_ref);
+ hardif_free_ref(batman_if);
return length;
}
diff --git a/batman-adv/hard-interface.c b/batman-adv/hard-interface.c
index e2b001a..8982485 100644
--- a/batman-adv/hard-interface.c
+++ b/batman-adv/hard-interface.c
@@ -40,13 +40,13 @@ static int batman_skb_recv(struct sk_buff *skb,
struct packet_type *ptype,
struct net_device *orig_dev);
-static void hardif_free_rcu(struct rcu_head *rcu)
+void hardif_free_rcu(struct rcu_head *rcu)
{
struct batman_if *batman_if;
batman_if = container_of(rcu, struct batman_if, rcu);
dev_put(batman_if->net_dev);
- kref_put(&batman_if->refcount, hardif_free_ref);
+ kfree(batman_if);
}
struct batman_if *get_batman_if_by_netdev(struct net_device *net_dev)
@@ -55,16 +55,14 @@ struct batman_if *get_batman_if_by_netdev(struct net_device *net_dev)
rcu_read_lock();
list_for_each_entry_rcu(batman_if, &if_list, list) {
- if (batman_if->net_dev == net_dev)
+ if (batman_if->net_dev == net_dev &&
+ atomic_inc_not_zero(&batman_if->refcount))
goto out;
}
batman_if = NULL;
out:
- if (batman_if)
- kref_get(&batman_if->refcount);
-
rcu_read_unlock();
return batman_if;
}
@@ -105,16 +103,14 @@ static struct batman_if *get_active_batman_if(struct net_device *soft_iface)
if (batman_if->soft_iface != soft_iface)
continue;
- if (batman_if->if_status == IF_ACTIVE)
+ if (batman_if->if_status == IF_ACTIVE &&
+ atomic_inc_not_zero(&batman_if->refcount))
goto out;
}
batman_if = NULL;
out:
- if (batman_if)
- kref_get(&batman_if->refcount);
-
rcu_read_unlock();
return batman_if;
}
@@ -137,14 +133,14 @@ static void set_primary_if(struct bat_priv *bat_priv,
struct batman_packet *batman_packet;
struct batman_if *old_if;
- if (batman_if)
- kref_get(&batman_if->refcount);
+ if (batman_if && !atomic_inc_not_zero(&batman_if->refcount))
+ batman_if = NULL;
old_if = bat_priv->primary_if;
bat_priv->primary_if = batman_if;
if (old_if)
- kref_put(&old_if->refcount, hardif_free_ref);
+ hardif_free_ref(old_if);
if (!bat_priv->primary_if)
return;
@@ -290,6 +286,9 @@ int hardif_enable_interface(struct batman_if *batman_if, char *iface_name)
if (batman_if->if_status != IF_NOT_IN_USE)
goto out;
+ if (!atomic_inc_not_zero(&batman_if->refcount))
+ goto out;
+
batman_if->soft_iface = dev_get_by_name(&init_net, iface_name);
if (!batman_if->soft_iface) {
@@ -328,7 +327,6 @@ int hardif_enable_interface(struct batman_if *batman_if, char *iface_name)
batman_if->batman_adv_ptype.type = __constant_htons(ETH_P_BATMAN);
batman_if->batman_adv_ptype.func = batman_skb_recv;
batman_if->batman_adv_ptype.dev = batman_if->net_dev;
- kref_get(&batman_if->refcount);
dev_add_pack(&batman_if->batman_adv_ptype);
atomic_set(&batman_if->seqno, 1);
@@ -371,6 +369,7 @@ out:
return 0;
err:
+ hardif_free_ref(batman_if);
return -ENOMEM;
}
@@ -387,7 +386,6 @@ void hardif_disable_interface(struct batman_if *batman_if)
bat_info(batman_if->soft_iface, "Removing interface: %s\n",
batman_if->net_dev->name);
dev_remove_pack(&batman_if->batman_adv_ptype);
- kref_put(&batman_if->refcount, hardif_free_ref);
bat_priv->num_ifaces--;
orig_hash_del_if(batman_if, bat_priv->num_ifaces);
@@ -399,7 +397,7 @@ void hardif_disable_interface(struct batman_if *batman_if)
set_primary_if(bat_priv, new_if);
if (new_if)
- kref_put(&new_if->refcount, hardif_free_ref);
+ hardif_free_ref(new_if);
}
kfree(batman_if->packet_buff);
@@ -416,6 +414,7 @@ void hardif_disable_interface(struct batman_if *batman_if)
softif_destroy(batman_if->soft_iface);
batman_if->soft_iface = NULL;
+ hardif_free_ref(batman_if);
}
static struct batman_if *hardif_add_interface(struct net_device *net_dev)
@@ -445,7 +444,8 @@ static struct batman_if *hardif_add_interface(struct net_device *net_dev)
batman_if->soft_iface = NULL;
batman_if->if_status = IF_NOT_IN_USE;
INIT_LIST_HEAD(&batman_if->list);
- kref_init(&batman_if->refcount);
+ /* extra reference for return */
+ atomic_set(&batman_if->refcount, 2);
check_known_mac_addr(batman_if->net_dev);
@@ -453,8 +453,6 @@ static struct batman_if *hardif_add_interface(struct net_device *net_dev)
list_add_tail_rcu(&batman_if->list, &if_list);
spin_unlock(&if_list_lock);
- /* extra reference for return */
- kref_get(&batman_if->refcount);
return batman_if;
free_if:
@@ -476,7 +474,7 @@ static void hardif_remove_interface(struct batman_if *batman_if)
batman_if->if_status = IF_TO_BE_REMOVED;
sysfs_del_hardif(&batman_if->hardif_obj);
- call_rcu(&batman_if->rcu, hardif_free_rcu);
+ hardif_free_ref(batman_if);
}
void hardif_remove_interfaces(void)
@@ -548,7 +546,7 @@ static int hard_if_event(struct notifier_block *this,
};
hardif_put:
- kref_put(&batman_if->refcount, hardif_free_ref);
+ hardif_free_ref(batman_if);
out:
return NOTIFY_DONE;
}
diff --git a/batman-adv/hard-interface.h b/batman-adv/hard-interface.h
index ad19543..e488b90 100644
--- a/batman-adv/hard-interface.h
+++ b/batman-adv/hard-interface.h
@@ -37,13 +37,12 @@ void hardif_disable_interface(struct batman_if *batman_if);
void hardif_remove_interfaces(void);
int hardif_min_mtu(struct net_device *soft_iface);
void update_min_mtu(struct net_device *soft_iface);
+void hardif_free_rcu(struct rcu_head *rcu);
-static inline void hardif_free_ref(struct kref *refcount)
+static inline void hardif_free_ref(struct batman_if *batman_if)
{
- struct batman_if *batman_if;
-
- batman_if = container_of(refcount, struct batman_if, refcount);
- kfree(batman_if);
+ if (atomic_dec_and_test(&batman_if->refcount))
+ call_rcu(&batman_if->rcu, hardif_free_rcu);
}
#endif /* _NET_BATMAN_ADV_HARD_INTERFACE_H_ */
diff --git a/batman-adv/types.h b/batman-adv/types.h
index 8df6d9d..317ede8 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -43,7 +43,7 @@ struct batman_if {
unsigned char *packet_buff;
int packet_len;
struct kobject *hardif_obj;
- struct kref refcount;
+ atomic_t refcount;
struct packet_type batman_adv_ptype;
struct net_device *soft_iface;
struct rcu_head rcu;
--
1.7.2.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [B.A.T.M.A.N.] [PATCH 4/7] batman-adv: Correct rcu refcounting for neigh_node
2011-02-03 17:02 ` [B.A.T.M.A.N.] Reordered rcu refcounting Marek Lindner
` (7 preceding siblings ...)
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 3/7] batman-adv: Correct rcu refcounting for batman_if Linus Lüssing
@ 2011-02-04 15:21 ` Linus Lüssing
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 5/7] batman-adv: Make bat_priv->curr_gw an rcu protected pointer Linus Lüssing
` (2 subsequent siblings)
11 siblings, 0 replies; 44+ messages in thread
From: Linus Lüssing @ 2011-02-04 15:21 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Marek Lindner
From: Sven Eckelmann <sven@narfation.org>
It might be possible that 2 threads access the same data in the same
rcu grace period. The first thread calls call_rcu() to decrement the
refcount and free the data while the second thread increases the
refcount to use the data. To avoid this race condition all refcount
operations have to be atomic.
Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
---
icmp_socket.c | 7 ++--
originator.c | 26 ++++---------
originator.h | 3 +-
routing.c | 111 +++++++++++++++++++++++++++++++++++++++++----------------
types.h | 3 +-
unicast.c | 2 +-
vis.c | 8 +++-
7 files changed, 101 insertions(+), 59 deletions(-)
diff --git a/batman-adv/icmp_socket.c b/batman-adv/icmp_socket.c
index fc812e1..4465a1a 100644
--- a/batman-adv/icmp_socket.c
+++ b/batman-adv/icmp_socket.c
@@ -229,10 +229,11 @@ static ssize_t bat_socket_write(struct file *file, const char __user *buff,
kref_get(&orig_node->refcount);
neigh_node = orig_node->router;
- if (!neigh_node)
+ if (!neigh_node || !atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
goto unlock;
+ }
- kref_get(&neigh_node->refcount);
rcu_read_unlock();
if (!neigh_node->if_incoming)
@@ -260,7 +261,7 @@ free_skb:
kfree_skb(skb);
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return len;
diff --git a/batman-adv/originator.c b/batman-adv/originator.c
index e8a8473..bde9778 100644
--- a/batman-adv/originator.c
+++ b/batman-adv/originator.c
@@ -56,28 +56,18 @@ err:
return 0;
}
-void neigh_node_free_ref(struct kref *refcount)
-{
- struct neigh_node *neigh_node;
-
- neigh_node = container_of(refcount, struct neigh_node, refcount);
- kfree(neigh_node);
-}
-
static void neigh_node_free_rcu(struct rcu_head *rcu)
{
struct neigh_node *neigh_node;
neigh_node = container_of(rcu, struct neigh_node, rcu);
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ kfree(neigh_node);
}
-void neigh_node_free_rcu_bond(struct rcu_head *rcu)
+void neigh_node_free_ref(struct neigh_node *neigh_node)
{
- struct neigh_node *neigh_node;
-
- neigh_node = container_of(rcu, struct neigh_node, rcu_bond);
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ if (atomic_dec_and_test(&neigh_node->refcount))
+ call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
}
struct neigh_node *create_neighbor(struct orig_node *orig_node,
@@ -101,7 +91,7 @@ struct neigh_node *create_neighbor(struct orig_node *orig_node,
memcpy(neigh_node->addr, neigh, ETH_ALEN);
neigh_node->orig_node = orig_neigh_node;
neigh_node->if_incoming = if_incoming;
- kref_init(&neigh_node->refcount);
+ atomic_set(&neigh_node->refcount, 1);
spin_lock_bh(&orig_node->neigh_list_lock);
hlist_add_head_rcu(&neigh_node->list, &orig_node->neigh_list);
@@ -123,14 +113,14 @@ void orig_node_free_ref(struct kref *refcount)
list_for_each_entry_safe(neigh_node, tmp_neigh_node,
&orig_node->bond_list, bonding_list) {
list_del_rcu(&neigh_node->bonding_list);
- call_rcu(&neigh_node->rcu_bond, neigh_node_free_rcu_bond);
+ neigh_node_free_ref(neigh_node);
}
/* for all neighbors towards this originator ... */
hlist_for_each_entry_safe(neigh_node, node, node_tmp,
&orig_node->neigh_list, list) {
hlist_del_rcu(&neigh_node->list);
- call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
+ neigh_node_free_ref(neigh_node);
}
spin_unlock_bh(&orig_node->neigh_list_lock);
@@ -311,7 +301,7 @@ static bool purge_orig_neighbors(struct bat_priv *bat_priv,
hlist_del_rcu(&neigh_node->list);
bonding_candidate_del(orig_node, neigh_node);
- call_rcu(&neigh_node->rcu, neigh_node_free_rcu);
+ neigh_node_free_ref(neigh_node);
} else {
if ((!*best_neigh_node) ||
(neigh_node->tq_avg > (*best_neigh_node)->tq_avg))
diff --git a/batman-adv/originator.h b/batman-adv/originator.h
index 360dfd1..84d96e2 100644
--- a/batman-adv/originator.h
+++ b/batman-adv/originator.h
@@ -26,13 +26,12 @@ int originator_init(struct bat_priv *bat_priv);
void originator_free(struct bat_priv *bat_priv);
void purge_orig_ref(struct bat_priv *bat_priv);
void orig_node_free_ref(struct kref *refcount);
-void neigh_node_free_rcu_bond(struct rcu_head *rcu);
struct orig_node *get_orig_node(struct bat_priv *bat_priv, uint8_t *addr);
struct neigh_node *create_neighbor(struct orig_node *orig_node,
struct orig_node *orig_neigh_node,
uint8_t *neigh,
struct batman_if *if_incoming);
-void neigh_node_free_ref(struct kref *refcount);
+void neigh_node_free_ref(struct neigh_node *neigh_node);
int orig_seq_print_text(struct seq_file *seq, void *offset);
int orig_hash_add_if(struct batman_if *batman_if, int max_if_num);
int orig_hash_del_if(struct batman_if *batman_if, int max_if_num);
diff --git a/batman-adv/routing.c b/batman-adv/routing.c
index b792df4..a2b770a 100644
--- a/batman-adv/routing.c
+++ b/batman-adv/routing.c
@@ -117,12 +117,12 @@ static void update_route(struct bat_priv *bat_priv,
orig_node->router->addr);
}
- if (neigh_node)
- kref_get(&neigh_node->refcount);
+ if (neigh_node && !atomic_inc_not_zero(&neigh_node->refcount))
+ neigh_node = NULL;
neigh_node_tmp = orig_node->router;
orig_node->router = neigh_node;
if (neigh_node_tmp)
- kref_put(&neigh_node_tmp->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node_tmp);
}
@@ -174,7 +174,11 @@ static int is_bidirectional_neigh(struct orig_node *orig_node,
if (!neigh_node)
goto unlock;
- kref_get(&neigh_node->refcount);
+ if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
+ goto unlock;
+ }
+
rcu_read_unlock();
neigh_node->last_valid = jiffies;
@@ -199,7 +203,11 @@ static int is_bidirectional_neigh(struct orig_node *orig_node,
if (!neigh_node)
goto unlock;
- kref_get(&neigh_node->refcount);
+ if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
+ goto unlock;
+ }
+
rcu_read_unlock();
}
@@ -261,7 +269,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
return ret;
}
@@ -274,8 +282,8 @@ void bonding_candidate_del(struct orig_node *orig_node,
goto out;
list_del_rcu(&neigh_node->bonding_list);
- call_rcu(&neigh_node->rcu_bond, neigh_node_free_rcu_bond);
INIT_LIST_HEAD(&neigh_node->bonding_list);
+ neigh_node_free_ref(neigh_node);
atomic_dec(&orig_node->bond_candidates);
out:
@@ -336,8 +344,10 @@ static void bonding_candidate_add(struct orig_node *orig_node,
if (!list_empty(&neigh_node->bonding_list))
goto out;
+ if (!atomic_inc_not_zero(&neigh_node->refcount))
+ goto out;
+
list_add_rcu(&neigh_node->bonding_list, &orig_node->bond_list);
- kref_get(&neigh_node->refcount);
atomic_inc(&orig_node->bond_candidates);
goto out;
@@ -381,7 +391,10 @@ static void update_orig(struct bat_priv *bat_priv,
hlist_for_each_entry_rcu(tmp_neigh_node, node,
&orig_node->neigh_list, list) {
if (compare_orig(tmp_neigh_node->addr, ethhdr->h_source) &&
- (tmp_neigh_node->if_incoming == if_incoming)) {
+ (tmp_neigh_node->if_incoming == if_incoming) &&
+ atomic_inc_not_zero(&tmp_neigh_node->refcount)) {
+ if (neigh_node)
+ neigh_node_free_ref(neigh_node);
neigh_node = tmp_neigh_node;
continue;
}
@@ -408,11 +421,15 @@ static void update_orig(struct bat_priv *bat_priv,
kref_put(&orig_tmp->refcount, orig_node_free_ref);
if (!neigh_node)
goto unlock;
+
+ if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
+ goto unlock;
+ }
} else
bat_dbg(DBG_BATMAN, bat_priv,
"Updating existing last-hop neighbor of originator\n");
- kref_get(&neigh_node->refcount);
rcu_read_unlock();
orig_node->flags = batman_packet->flags;
@@ -489,7 +506,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
}
/* checks whether the host restarted and is in the protection time.
@@ -893,7 +910,11 @@ static int recv_my_icmp_packet(struct bat_priv *bat_priv,
if (!neigh_node)
goto unlock;
- kref_get(&neigh_node->refcount);
+ if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
+ goto unlock;
+ }
+
rcu_read_unlock();
/* create a copy of the skb, if needed, to modify it. */
@@ -916,7 +937,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return ret;
@@ -957,7 +978,11 @@ static int recv_icmp_ttl_exceeded(struct bat_priv *bat_priv,
if (!neigh_node)
goto unlock;
- kref_get(&neigh_node->refcount);
+ if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
+ goto unlock;
+ }
+
rcu_read_unlock();
/* create a copy of the skb, if needed, to modify it. */
@@ -980,7 +1005,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return ret;
@@ -1053,7 +1078,11 @@ int recv_icmp_packet(struct sk_buff *skb, struct batman_if *recv_if)
if (!neigh_node)
goto unlock;
- kref_get(&neigh_node->refcount);
+ if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
+ goto unlock;
+ }
+
rcu_read_unlock();
/* create a copy of the skb, if needed, to modify it. */
@@ -1074,7 +1103,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return ret;
@@ -1107,12 +1136,11 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
/* select default router to output */
router = orig_node->router;
router_orig = orig_node->router->orig_node;
- if (!router_orig) {
+ if (!router_orig || !atomic_inc_not_zero(&router->refcount)) {
rcu_read_unlock();
return NULL;
}
-
if ((!recv_if) && (!bonding_enabled))
goto return_router;
@@ -1145,6 +1173,7 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
* is is not on the interface where the packet came
* in. */
+ neigh_node_free_ref(router);
first_candidate = NULL;
router = NULL;
@@ -1157,16 +1186,23 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
if (!first_candidate)
first_candidate = tmp_neigh_node;
/* recv_if == NULL on the first node. */
- if (tmp_neigh_node->if_incoming != recv_if) {
+ if (tmp_neigh_node->if_incoming != recv_if &&
+ atomic_inc_not_zero(&tmp_neigh_node->refcount)) {
router = tmp_neigh_node;
break;
}
}
/* use the first candidate if nothing was found. */
- if (!router)
+ if (!router && first_candidate &&
+ atomic_inc_not_zero(&first_candidate->refcount))
router = first_candidate;
+ if (!router) {
+ rcu_read_unlock();
+ return NULL;
+ }
+
/* selected should point to the next element
* after the current router */
spin_lock_bh(&primary_orig_node->neigh_list_lock);
@@ -1187,21 +1223,34 @@ struct neigh_node *find_router(struct bat_priv *bat_priv,
first_candidate = tmp_neigh_node;
/* recv_if == NULL on the first node. */
- if (tmp_neigh_node->if_incoming != recv_if)
- /* if we don't have a router yet
- * or this one is better, choose it. */
- if ((!router) ||
- (tmp_neigh_node->tq_avg > router->tq_avg)) {
- router = tmp_neigh_node;
- }
+ if (tmp_neigh_node->if_incoming == recv_if)
+ continue;
+
+ if (!atomic_inc_not_zero(&tmp_neigh_node->refcount))
+ continue;
+
+ /* if we don't have a router yet
+ * or this one is better, choose it. */
+ if ((!router) ||
+ (tmp_neigh_node->tq_avg > router->tq_avg)) {
+ /* decrement refcount of
+ * previously selected router */
+ if (router)
+ neigh_node_free_ref(router);
+
+ router = tmp_neigh_node;
+ atomic_inc_not_zero(&router->refcount);
+ }
+
+ neigh_node_free_ref(tmp_neigh_node);
}
/* use the first candidate if nothing was found. */
- if (!router)
+ if (!router && first_candidate &&
+ atomic_inc_not_zero(&first_candidate->refcount))
router = first_candidate;
}
return_router:
- kref_get(&router->refcount);
rcu_read_unlock();
return router;
}
@@ -1314,7 +1363,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return ret;
diff --git a/batman-adv/types.h b/batman-adv/types.h
index 317ede8..ee77d48 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -119,9 +119,8 @@ struct neigh_node {
struct list_head bonding_list;
unsigned long last_valid;
unsigned long real_bits[NUM_WORDS];
- struct kref refcount;
+ atomic_t refcount;
struct rcu_head rcu;
- struct rcu_head rcu_bond;
struct orig_node *orig_node;
struct batman_if *if_incoming;
};
diff --git a/batman-adv/unicast.c b/batman-adv/unicast.c
index 6a9ab61..580b547 100644
--- a/batman-adv/unicast.c
+++ b/batman-adv/unicast.c
@@ -363,7 +363,7 @@ find_router:
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
if (ret == 1)
diff --git a/batman-adv/vis.c b/batman-adv/vis.c
index 191401b..c1c3258 100644
--- a/batman-adv/vis.c
+++ b/batman-adv/vis.c
@@ -776,7 +776,11 @@ static void unicast_vis_packet(struct bat_priv *bat_priv,
if (!neigh_node)
goto unlock;
- kref_get(&neigh_node->refcount);
+ if (!atomic_inc_not_zero(&neigh_node->refcount)) {
+ neigh_node = NULL;
+ goto unlock;
+ }
+
rcu_read_unlock();
skb = skb_clone(info->skb_packet, GFP_ATOMIC);
@@ -790,7 +794,7 @@ unlock:
rcu_read_unlock();
out:
if (neigh_node)
- kref_put(&neigh_node->refcount, neigh_node_free_ref);
+ neigh_node_free_ref(neigh_node);
if (orig_node)
kref_put(&orig_node->refcount, orig_node_free_ref);
return;
--
1.7.2.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [B.A.T.M.A.N.] [PATCH 5/7] batman-adv: Make bat_priv->curr_gw an rcu protected pointer
2011-02-03 17:02 ` [B.A.T.M.A.N.] Reordered rcu refcounting Marek Lindner
` (8 preceding siblings ...)
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 4/7] batman-adv: Correct rcu refcounting for neigh_node Linus Lüssing
@ 2011-02-04 15:21 ` Linus Lüssing
2011-02-08 13:18 ` Marek Lindner
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 6/7] batman-adv: add __rcu annotations for gw_node Linus Lüssing
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 7/7] batman-adv: Increase orig_node refcount before releasing rcu read lock Linus Lüssing
11 siblings, 1 reply; 44+ messages in thread
From: Linus Lüssing @ 2011-02-04 15:21 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Linus Lüssing
The rcu protected macros rcu_dereference() and rcu_assign_pointer()
for the bat_priv->curr_gw need to be used, as well as spin/rcu locking.
Otherwise we might end up using a curr_gw pointer pointing to already
freed memory.
Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Linus Lüssing <linus.luessing@ascom.ch>
---
gateway_client.c | 87 ++++++++++++++++++++++++++++++++++++++---------------
main.c | 1 +
types.h | 1 +
3 files changed, 64 insertions(+), 25 deletions(-)
diff --git a/batman-adv/gateway_client.c b/batman-adv/gateway_client.c
index 517e001..4624515 100644
--- a/batman-adv/gateway_client.c
+++ b/batman-adv/gateway_client.c
@@ -44,19 +44,30 @@ static void gw_node_free_ref(struct gw_node *gw_node)
void *gw_get_selected(struct bat_priv *bat_priv)
{
- struct gw_node *curr_gateway_tmp = bat_priv->curr_gw;
+ struct gw_node *curr_gateway_tmp;
+ struct orig_node *orig_node;
- if (!curr_gateway_tmp)
+ rcu_read_lock();
+ curr_gateway_tmp = rcu_dereference(bat_priv->curr_gw);
+ if (!curr_gateway_tmp) {
+ rcu_read_unlock();
return NULL;
+ }
- return curr_gateway_tmp->orig_node;
+ orig_node = curr_gateway_tmp->orig_node;
+ rcu_read_unlock();
+
+ return orig_node;
}
void gw_deselect(struct bat_priv *bat_priv)
{
- struct gw_node *gw_node = bat_priv->curr_gw;
+ struct gw_node *gw_node;
- bat_priv->curr_gw = NULL;
+ spin_lock_bh(&bat_priv->curr_gw_lock);
+ gw_node = rcu_dereference(bat_priv->curr_gw);
+ rcu_assign_pointer(bat_priv->curr_gw, NULL);
+ spin_unlock_bh(&bat_priv->curr_gw_lock);
if (gw_node)
gw_node_free_ref(gw_node);
@@ -64,12 +75,15 @@ void gw_deselect(struct bat_priv *bat_priv)
static void gw_select(struct bat_priv *bat_priv, struct gw_node *new_gw_node)
{
- struct gw_node *curr_gw_node = bat_priv->curr_gw;
+ struct gw_node *curr_gw_node;
if (new_gw_node && !atomic_inc_not_zero(&new_gw_node->refcount))
new_gw_node = NULL;
- bat_priv->curr_gw = new_gw_node;
+ spin_lock_bh(&bat_priv->curr_gw_lock);
+ curr_gw_node = rcu_dereference(bat_priv->curr_gw);
+ rcu_assign_pointer(bat_priv->curr_gw, new_gw_node);
+ spin_unlock_bh(&bat_priv->curr_gw_lock);
if (curr_gw_node)
gw_node_free_ref(curr_gw_node);
@@ -78,7 +92,7 @@ static void gw_select(struct bat_priv *bat_priv, struct gw_node *new_gw_node)
void gw_election(struct bat_priv *bat_priv)
{
struct hlist_node *node;
- struct gw_node *gw_node, *curr_gw_tmp = NULL;
+ struct gw_node *gw_node, *curr_gw, *curr_gw_tmp = NULL;
uint8_t max_tq = 0;
uint32_t max_gw_factor = 0, tmp_gw_factor = 0;
int down, up;
@@ -92,19 +106,24 @@ void gw_election(struct bat_priv *bat_priv)
if (atomic_read(&bat_priv->gw_mode) != GW_MODE_CLIENT)
return;
- if (bat_priv->curr_gw)
+ rcu_read_lock();
+ curr_gw = rcu_dereference(bat_priv->curr_gw);
+ if (curr_gw) {
+ rcu_read_unlock();
return;
+ }
- rcu_read_lock();
if (hlist_empty(&bat_priv->gw_list)) {
- rcu_read_unlock();
- if (bat_priv->curr_gw) {
+ if (curr_gw) {
+ rcu_read_unlock();
bat_dbg(DBG_BATMAN, bat_priv,
"Removing selected gateway - "
"no gateway in range\n");
gw_deselect(bat_priv);
}
+ else
+ rcu_read_unlock();
return;
}
@@ -153,12 +172,12 @@ void gw_election(struct bat_priv *bat_priv)
max_gw_factor = tmp_gw_factor;
}
- if (bat_priv->curr_gw != curr_gw_tmp) {
- if ((bat_priv->curr_gw) && (!curr_gw_tmp))
+ if (curr_gw != curr_gw_tmp) {
+ if ((curr_gw) && (!curr_gw_tmp))
bat_dbg(DBG_BATMAN, bat_priv,
"Removing selected gateway - "
"no gateway in range\n");
- else if ((!bat_priv->curr_gw) && (curr_gw_tmp))
+ else if ((!curr_gw) && (curr_gw_tmp))
bat_dbg(DBG_BATMAN, bat_priv,
"Adding route to gateway %pM "
"(gw_flags: %i, tq: %i)\n",
@@ -181,11 +200,13 @@ void gw_election(struct bat_priv *bat_priv)
void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
{
- struct gw_node *curr_gateway_tmp = bat_priv->curr_gw;
+ struct gw_node *curr_gateway_tmp;
uint8_t gw_tq_avg, orig_tq_avg;
+ rcu_read_lock();
+ curr_gateway_tmp = rcu_dereference(bat_priv->curr_gw);
if (!curr_gateway_tmp)
- return;
+ goto rcu_unlock;
if (!curr_gateway_tmp->orig_node)
goto deselect;
@@ -195,12 +216,14 @@ void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
/* this node already is the gateway */
if (curr_gateway_tmp->orig_node == orig_node)
- return;
+ goto rcu_unlock;
if (!orig_node->router)
- return;
+ goto rcu_unlock;
gw_tq_avg = curr_gateway_tmp->orig_node->router->tq_avg;
+ rcu_read_unlock();
+
orig_tq_avg = orig_node->router->tq_avg;
/* the TQ value has to be better */
@@ -222,6 +245,9 @@ void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
deselect:
gw_deselect(bat_priv);
+ return;
+rcu_unlock:
+ rcu_read_unlock();
}
static void gw_node_add(struct bat_priv *bat_priv,
@@ -278,7 +304,7 @@ void gw_node_update(struct bat_priv *bat_priv,
"Gateway %pM removed from gateway list\n",
orig_node->orig);
- if (gw_node == bat_priv->curr_gw) {
+ if (gw_node == rcu_dereference(bat_priv->curr_gw)) {
rcu_read_unlock();
gw_deselect(bat_priv);
return;
@@ -316,7 +342,7 @@ void gw_node_purge(struct bat_priv *bat_priv)
atomic_read(&bat_priv->mesh_state) == MESH_ACTIVE)
continue;
- if (bat_priv->curr_gw == gw_node)
+ if (rcu_dereference(bat_priv->curr_gw) == gw_node)
gw_deselect(bat_priv);
hlist_del_rcu(&gw_node->list);
@@ -330,12 +356,16 @@ void gw_node_purge(struct bat_priv *bat_priv)
static int _write_buffer_text(struct bat_priv *bat_priv,
struct seq_file *seq, struct gw_node *gw_node)
{
- int down, up;
+ struct gw_node *curr_gw;
+ int down, up, ret;
gw_bandwidth_to_kbit(gw_node->orig_node->gw_flags, &down, &up);
- return seq_printf(seq, "%s %pM (%3i) %pM [%10s]: %3i - %i%s/%i%s\n",
- (bat_priv->curr_gw == gw_node ? "=>" : " "),
+ rcu_read_lock();
+ curr_gw = rcu_dereference(bat_priv->curr_gw);
+
+ ret = seq_printf(seq, "%s %pM (%3i) %pM [%10s]: %3i - %i%s/%i%s\n",
+ (curr_gw == gw_node ? "=>" : " "),
gw_node->orig_node->orig,
gw_node->orig_node->router->tq_avg,
gw_node->orig_node->router->addr,
@@ -345,6 +375,9 @@ static int _write_buffer_text(struct bat_priv *bat_priv,
(down > 2048 ? "MBit" : "KBit"),
(up > 2048 ? up / 1024 : up),
(up > 2048 ? "MBit" : "KBit"));
+
+ rcu_read_unlock();
+ return ret;
}
int gw_client_seq_print_text(struct seq_file *seq, void *offset)
@@ -465,8 +498,12 @@ int gw_is_target(struct bat_priv *bat_priv, struct sk_buff *skb)
if (atomic_read(&bat_priv->gw_mode) == GW_MODE_SERVER)
return -1;
- if (!bat_priv->curr_gw)
+ rcu_read_lock();
+ if (!rcu_dereference(bat_priv->curr_gw)) {
+ rcu_read_unlock();
return 0;
+ }
+ rcu_read_unlock();
return 1;
}
diff --git a/batman-adv/main.c b/batman-adv/main.c
index 658ad5a..6823868 100644
--- a/batman-adv/main.c
+++ b/batman-adv/main.c
@@ -84,6 +84,7 @@ int mesh_init(struct net_device *soft_iface)
spin_lock_init(&bat_priv->hna_lhash_lock);
spin_lock_init(&bat_priv->hna_ghash_lock);
spin_lock_init(&bat_priv->gw_list_lock);
+ spin_lock_init(&bat_priv->curr_gw_lock);
spin_lock_init(&bat_priv->vis_hash_lock);
spin_lock_init(&bat_priv->vis_list_lock);
spin_lock_init(&bat_priv->softif_neigh_lock);
diff --git a/batman-adv/types.h b/batman-adv/types.h
index ee77d48..4ae11c3 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -162,6 +162,7 @@ struct bat_priv {
spinlock_t hna_lhash_lock; /* protects hna_local_hash */
spinlock_t hna_ghash_lock; /* protects hna_global_hash */
spinlock_t gw_list_lock; /* protects gw_list */
+ spinlock_t curr_gw_lock; /* protects curr_gw updates */
spinlock_t vis_hash_lock; /* protects vis_hash */
spinlock_t vis_list_lock; /* protects vis_info::recv_list */
spinlock_t softif_neigh_lock; /* protects soft-interface neigh list */
--
1.7.2.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [B.A.T.M.A.N.] [PATCH 6/7] batman-adv: add __rcu annotations for gw_node
2011-02-03 17:02 ` [B.A.T.M.A.N.] Reordered rcu refcounting Marek Lindner
` (9 preceding siblings ...)
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 5/7] batman-adv: Make bat_priv->curr_gw an rcu protected pointer Linus Lüssing
@ 2011-02-04 15:21 ` Linus Lüssing
2011-02-13 21:10 ` Marek Lindner
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 7/7] batman-adv: Increase orig_node refcount before releasing rcu read lock Linus Lüssing
11 siblings, 1 reply; 44+ messages in thread
From: Linus Lüssing @ 2011-02-04 15:21 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Linus Lüssing
Add __rcu annotations for the rcu protected bat_priv::curr_gw pointer to
allow sparse checking.
Signed-off-by: Linus Lüssing <linus.luessing@ascom.ch>
---
compat.h | 6 ++++++
types.h | 2 +-
2 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/batman-adv/compat.h b/batman-adv/compat.h
index a76d0be..4e89049 100644
--- a/batman-adv/compat.h
+++ b/batman-adv/compat.h
@@ -270,4 +270,10 @@ int bat_seq_printf(struct seq_file *m, const char *f, ...);
#endif /* < KERNEL_VERSION(2, 6, 33) */
+#if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 36)
+
+#define __rcu
+
+#endif /* < KERNEL_VERSION(2, 6, 36) */
+
#endif /* _NET_BATMAN_ADV_COMPAT_H_ */
diff --git a/batman-adv/types.h b/batman-adv/types.h
index 4ae11c3..4dc5854 100644
--- a/batman-adv/types.h
+++ b/batman-adv/types.h
@@ -171,7 +171,7 @@ struct bat_priv {
struct delayed_work hna_work;
struct delayed_work orig_work;
struct delayed_work vis_work;
- struct gw_node *curr_gw;
+ struct gw_node __rcu *curr_gw; /* rcu protected pointer */
struct vis_info *my_vis_info;
};
--
1.7.2.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [B.A.T.M.A.N.] [PATCH 7/7] batman-adv: Increase orig_node refcount before releasing rcu read lock
2011-02-03 17:02 ` [B.A.T.M.A.N.] Reordered rcu refcounting Marek Lindner
` (10 preceding siblings ...)
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 6/7] batman-adv: add __rcu annotations for gw_node Linus Lüssing
@ 2011-02-04 15:21 ` Linus Lüssing
2011-02-04 16:33 ` [B.A.T.M.A.N.] [PATCHv2 " Linus Lüssing
2011-02-13 21:11 ` [B.A.T.M.A.N.] [PATCH " Marek Lindner
11 siblings, 2 replies; 44+ messages in thread
From: Linus Lüssing @ 2011-02-04 15:21 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Linus Lüssing
When unicast_send_skb() is increasing the orig_node's refcount another
thread might have been freeing this orig_node already. We need to
increase the refcount in the rcu read lock protected area to avoid that.
Signed-off-by: Linus Lüssing <linus.luessing@ascom.ch>
---
gateway_client.c | 3 +++
unicast.c | 1 -
2 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/batman-adv/gateway_client.c b/batman-adv/gateway_client.c
index 4624515..b3cda22 100644
--- a/batman-adv/gateway_client.c
+++ b/batman-adv/gateway_client.c
@@ -55,6 +55,9 @@ void *gw_get_selected(struct bat_priv *bat_priv)
}
orig_node = curr_gateway_tmp->orig_node;
+ if (orig_node)
+ kref_get(&orig_node->refcount);
+
rcu_read_unlock();
return orig_node;
diff --git a/batman-adv/unicast.c b/batman-adv/unicast.c
index 580b547..f4f5115 100644
--- a/batman-adv/unicast.c
+++ b/batman-adv/unicast.c
@@ -298,7 +298,6 @@ int unicast_send_skb(struct sk_buff *skb, struct bat_priv *bat_priv)
if (!orig_node)
goto trans_search;
- kref_get(&orig_node->refcount);
goto find_router;
} else {
rcu_read_lock();
--
1.7.2.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [B.A.T.M.A.N.] [PATCHv2 7/7] batman-adv: Increase orig_node refcount before releasing rcu read lock
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 7/7] batman-adv: Increase orig_node refcount before releasing rcu read lock Linus Lüssing
@ 2011-02-04 16:33 ` Linus Lüssing
2011-02-13 21:11 ` [B.A.T.M.A.N.] [PATCH " Marek Lindner
1 sibling, 0 replies; 44+ messages in thread
From: Linus Lüssing @ 2011-02-04 16:33 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Linus Lüssing
When unicast_send_skb() is increasing the orig_node's refcount another
thread might have been freeing this orig_node already. We need to
increase the refcount in the rcu read lock protected area to avoid that.
The same is true for get_orig_node().
Signed-off-by: Linus Lüssing <linus.luessing@ascom.ch>
---
gateway_client.c | 3 +++
originator.c | 4 ++--
unicast.c | 1 -
3 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/batman-adv/gateway_client.c b/batman-adv/gateway_client.c
index 4624515..b3cda22 100644
--- a/batman-adv/gateway_client.c
+++ b/batman-adv/gateway_client.c
@@ -55,6 +55,9 @@ void *gw_get_selected(struct bat_priv *bat_priv)
}
orig_node = curr_gateway_tmp->orig_node;
+ if (orig_node)
+ kref_get(&orig_node->refcount);
+
rcu_read_unlock();
return orig_node;
diff --git a/batman-adv/originator.c b/batman-adv/originator.c
index bde9778..6fb8393 100644
--- a/batman-adv/originator.c
+++ b/batman-adv/originator.c
@@ -193,12 +193,12 @@ struct orig_node *get_orig_node(struct bat_priv *bat_priv, uint8_t *addr)
orig_node = ((struct orig_node *)hash_find(bat_priv->orig_hash,
compare_orig, choose_orig,
addr));
- rcu_read_unlock();
-
if (orig_node) {
kref_get(&orig_node->refcount);
+ rcu_read_unlock();
return orig_node;
}
+ rcu_read_unlock();
bat_dbg(DBG_BATMAN, bat_priv,
"Creating new originator: %pM\n", addr);
diff --git a/batman-adv/unicast.c b/batman-adv/unicast.c
index 580b547..f4f5115 100644
--- a/batman-adv/unicast.c
+++ b/batman-adv/unicast.c
@@ -298,7 +298,6 @@ int unicast_send_skb(struct sk_buff *skb, struct bat_priv *bat_priv)
if (!orig_node)
goto trans_search;
- kref_get(&orig_node->refcount);
goto find_router;
} else {
rcu_read_lock();
--
1.7.2.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH 5/7] batman-adv: Make bat_priv->curr_gw an rcu protected pointer
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 5/7] batman-adv: Make bat_priv->curr_gw an rcu protected pointer Linus Lüssing
@ 2011-02-08 13:18 ` Marek Lindner
2011-02-10 10:42 ` Linus Lüssing
0 siblings, 1 reply; 44+ messages in thread
From: Marek Lindner @ 2011-02-08 13:18 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
On Friday 04 February 2011 16:21:34 Linus Lüssing wrote:
> + if (curr_gw) {
> + rcu_read_unlock();
> return;
> + }
>
> - rcu_read_lock();
> if (hlist_empty(&bat_priv->gw_list)) {
> - rcu_read_unlock();
>
> - if (bat_priv->curr_gw) {
> + if (curr_gw) {
> + rcu_read_unlock();
> bat_dbg(DBG_BATMAN, bat_priv,
> "Removing selected gateway - "
> "no gateway in range\n");
> gw_deselect(bat_priv);
> }
> + else
> + rcu_read_unlock();
This is a bit odd here - we return if curr_gw is not NULL and later we print a
message if curr_gw is not NULL ?
The issue existed before your patch came along.
> void gw_check_election(struct bat_priv *bat_priv, struct orig_node
> *orig_node) {
> - struct gw_node *curr_gateway_tmp = bat_priv->curr_gw;
> + struct gw_node *curr_gateway_tmp;
> uint8_t gw_tq_avg, orig_tq_avg;
>
> + rcu_read_lock();
> + curr_gateway_tmp = rcu_dereference(bat_priv->curr_gw);
> if (!curr_gateway_tmp)
> - return;
> + goto rcu_unlock;
>
> if (!curr_gateway_tmp->orig_node)
> goto deselect;
It seems if we jump to "deselect" here the rcu_lock() will never be unlocked.
> @@ -316,7 +342,7 @@ void gw_node_purge(struct bat_priv *bat_priv)
> - if (bat_priv->curr_gw == gw_node)
> + if (rcu_dereference(bat_priv->curr_gw) == gw_node)
> gw_deselect(bat_priv);
At this point we neither hold a rcu_lock() nor the newly created spinlock ...
> spinlock_t gw_list_lock; /* protects gw_list */
> + spinlock_t curr_gw_lock; /* protects curr_gw updates */
Would speak anything against re-using the gw_list_lock ?
Regards,
Marek
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH 5/7] batman-adv: Make bat_priv->curr_gw an rcu protected pointer
2011-02-08 13:18 ` Marek Lindner
@ 2011-02-10 10:42 ` Linus Lüssing
2011-02-10 14:25 ` Marek Lindner
0 siblings, 1 reply; 44+ messages in thread
From: Linus Lüssing @ 2011-02-10 10:42 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
On Tue, Feb 08, 2011 at 02:18:37PM +0100, Marek Lindner wrote:
>
> On Friday 04 February 2011 16:21:34 Linus Lüssing wrote:
> > + if (curr_gw) {
> > + rcu_read_unlock();
> > return;
> > + }
> >
> > - rcu_read_lock();
> > if (hlist_empty(&bat_priv->gw_list)) {
> > - rcu_read_unlock();
> >
> > - if (bat_priv->curr_gw) {
> > + if (curr_gw) {
> > + rcu_read_unlock();
> > bat_dbg(DBG_BATMAN, bat_priv,
> > "Removing selected gateway - "
> > "no gateway in range\n");
> > gw_deselect(bat_priv);
> > }
> > + else
> > + rcu_read_unlock();
>
> This is a bit odd here - we return if curr_gw is not NULL and later we print a
> message if curr_gw is not NULL ?
> The issue existed before your patch came along.
That's right. What do you think, is it worth an extra patch as it
is not a problem which patch 5/7 does not really try to address?
>
>
> > void gw_check_election(struct bat_priv *bat_priv, struct orig_node
> > *orig_node) {
> > - struct gw_node *curr_gateway_tmp = bat_priv->curr_gw;
> > + struct gw_node *curr_gateway_tmp;
> > uint8_t gw_tq_avg, orig_tq_avg;
> >
> > + rcu_read_lock();
> > + curr_gateway_tmp = rcu_dereference(bat_priv->curr_gw);
> > if (!curr_gateway_tmp)
> > - return;
> > + goto rcu_unlock;
> >
> > if (!curr_gateway_tmp->orig_node)
> > goto deselect;
>
> It seems if we jump to "deselect" here the rcu_lock() will never be unlocked.
True, will add that rcu_read_unlock() in the deselect jump mark.
>
>
> > @@ -316,7 +342,7 @@ void gw_node_purge(struct bat_priv *bat_priv)
> > - if (bat_priv->curr_gw == gw_node)
> > + if (rcu_dereference(bat_priv->curr_gw) == gw_node)
> > gw_deselect(bat_priv);
>
> At this point we neither hold a rcu_lock() nor the newly created spinlock ...
True again. I would prefer just adding an rcu-lock here...
>
>
> > spinlock_t gw_list_lock; /* protects gw_list */
> > + spinlock_t curr_gw_lock; /* protects curr_gw updates */
>
> Would speak anything against re-using the gw_list_lock ?
... as we are usually changing the gw_list more often than the
curr_gw, so it's not really necessary to let a gw_list change for
another node wait for a curr_gw_node reassignment to finish.
What is speaking for using the same lock for both, just having
less spinlocks in total in the code?
Does anyone know how it is usually done in the rest of the kernel,
are people tending to reduce the atomic context to the minimum
amount needed? Or is it usually a trade-off between two many locks
and and small atomic contexts?
>
> Regards,
> Marek
>
Cheers, Linus
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH 1/7] batman-adv: Correct rcu refcounting for gw_node
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 1/7] batman-adv: Correct rcu refcounting for gw_node Linus Lüssing
@ 2011-02-10 11:01 ` Linus Lüssing
0 siblings, 0 replies; 44+ messages in thread
From: Linus Lüssing @ 2011-02-10 11:01 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
So, had a second look at the way the gw_node rcu-locking
refcounting stuff is done and looks sane to me with the
rcu_dereference/assign_pointer additions. Just one thing I noticed
in gw_select() which probably is not an issue:
It's not harmful to hold a rcu_read_lock() while calling this as
we are using call_rcu() gw_node_free_ref(), right?
Do we need to increase the new_gw_node refcount? Of course we
cannot call the whole gw_select() without
any rcu locking due to the atomic_inc_not_zero(), but just
wondering if the rcu_read_unlock() should be called earlier.
So if some one can confirm that this is not an issue, then the
gw_node rcu refcount patches get my ok for being applied to the
trunk.
Cheers, Linus
On Fri, Feb 04, 2011 at 04:21:30PM +0100, Linus Lüssing wrote:
> From: Sven Eckelmann <sven@narfation.org>
>
> It might be possible that 2 threads access the same data in the same
> rcu grace period. The first thread calls call_rcu() to decrement the
> refcount and free the data while the second thread increases the
> refcount to use the data. To avoid this race condition all refcount
> operations have to be atomic.
>
> Reported-by: Sven Eckelmann <sven@narfation.org>
> Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
> ---
> gateway_client.c | 41 ++++++++++++++++++-----------------------
> types.h | 2 +-
> 2 files changed, 19 insertions(+), 24 deletions(-)
>
> diff --git a/batman-adv/gateway_client.c b/batman-adv/gateway_client.c
> index 429a013..517e001 100644
> --- a/batman-adv/gateway_client.c
> +++ b/batman-adv/gateway_client.c
> @@ -28,20 +28,18 @@
> #include <linux/udp.h>
> #include <linux/if_vlan.h>
>
> -static void gw_node_free_ref(struct kref *refcount)
> -{
> - struct gw_node *gw_node;
> -
> - gw_node = container_of(refcount, struct gw_node, refcount);
> - kfree(gw_node);
> -}
> -
> static void gw_node_free_rcu(struct rcu_head *rcu)
> {
> struct gw_node *gw_node;
>
> gw_node = container_of(rcu, struct gw_node, rcu);
> - kref_put(&gw_node->refcount, gw_node_free_ref);
> + kfree(gw_node);
> +}
> +
> +static void gw_node_free_ref(struct gw_node *gw_node)
> +{
> + if (atomic_dec_and_test(&gw_node->refcount))
> + call_rcu(&gw_node->rcu, gw_node_free_rcu);
> }
>
> void *gw_get_selected(struct bat_priv *bat_priv)
> @@ -61,25 +59,26 @@ void gw_deselect(struct bat_priv *bat_priv)
> bat_priv->curr_gw = NULL;
>
> if (gw_node)
> - kref_put(&gw_node->refcount, gw_node_free_ref);
> + gw_node_free_ref(gw_node);
> }
>
> -static struct gw_node *gw_select(struct bat_priv *bat_priv,
> - struct gw_node *new_gw_node)
> +static void gw_select(struct bat_priv *bat_priv, struct gw_node *new_gw_node)
> {
> struct gw_node *curr_gw_node = bat_priv->curr_gw;
>
> - if (new_gw_node)
> - kref_get(&new_gw_node->refcount);
> + if (new_gw_node && !atomic_inc_not_zero(&new_gw_node->refcount))
> + new_gw_node = NULL;
>
> bat_priv->curr_gw = new_gw_node;
> - return curr_gw_node;
> +
> + if (curr_gw_node)
> + gw_node_free_ref(curr_gw_node);
> }
>
> void gw_election(struct bat_priv *bat_priv)
> {
> struct hlist_node *node;
> - struct gw_node *gw_node, *curr_gw_tmp = NULL, *old_gw_node = NULL;
> + struct gw_node *gw_node, *curr_gw_tmp = NULL;
> uint8_t max_tq = 0;
> uint32_t max_gw_factor = 0, tmp_gw_factor = 0;
> int down, up;
> @@ -174,14 +173,10 @@ void gw_election(struct bat_priv *bat_priv)
> curr_gw_tmp->orig_node->gw_flags,
> curr_gw_tmp->orig_node->router->tq_avg);
>
> - old_gw_node = gw_select(bat_priv, curr_gw_tmp);
> + gw_select(bat_priv, curr_gw_tmp);
> }
>
> rcu_read_unlock();
> -
> - /* the kfree() has to be outside of the rcu lock */
> - if (old_gw_node)
> - kref_put(&old_gw_node->refcount, gw_node_free_ref);
> }
>
> void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
> @@ -242,7 +237,7 @@ static void gw_node_add(struct bat_priv *bat_priv,
> memset(gw_node, 0, sizeof(struct gw_node));
> INIT_HLIST_NODE(&gw_node->list);
> gw_node->orig_node = orig_node;
> - kref_init(&gw_node->refcount);
> + atomic_set(&gw_node->refcount, 1);
>
> spin_lock_bh(&bat_priv->gw_list_lock);
> hlist_add_head_rcu(&gw_node->list, &bat_priv->gw_list);
> @@ -325,7 +320,7 @@ void gw_node_purge(struct bat_priv *bat_priv)
> gw_deselect(bat_priv);
>
> hlist_del_rcu(&gw_node->list);
> - call_rcu(&gw_node->rcu, gw_node_free_rcu);
> + gw_node_free_ref(gw_node);
> }
>
>
> diff --git a/batman-adv/types.h b/batman-adv/types.h
> index e4a0462..ca5f20a 100644
> --- a/batman-adv/types.h
> +++ b/batman-adv/types.h
> @@ -100,7 +100,7 @@ struct gw_node {
> struct hlist_node list;
> struct orig_node *orig_node;
> unsigned long deleted;
> - struct kref refcount;
> + atomic_t refcount;
> struct rcu_head rcu;
> };
>
> --
> 1.7.2.3
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH 2/7] batman-adv: Correct rcu refcounting for softif_neigh
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 2/7] batman-adv: Correct rcu refcounting for softif_neigh Linus Lüssing
@ 2011-02-10 12:45 ` Linus Lüssing
2011-02-10 13:57 ` Marek Lindner
0 siblings, 1 reply; 44+ messages in thread
From: Linus Lüssing @ 2011-02-10 12:45 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
On Fri, Feb 04, 2011 at 04:21:31PM +0100, Linus Lüssing wrote:
> @@ -143,8 +140,11 @@ static struct softif_neigh *softif_neigh_get(struct bat_priv *bat_priv,
> if (softif_neigh->vid != vid)
> continue;
>
> + if (!atomic_inc_not_zero(&softif_neigh->refcount))
> + continue;
> +
> softif_neigh->last_seen = jiffies;
> - goto found;
> + goto out;
> }
Hmm, we could do a rcu_read_unlock() here, already, I think. Would
it be better to do so, keeping the rcu grace period as small as
possible?
>
> softif_neigh = kzalloc(sizeof(struct softif_neigh), GFP_ATOMIC);
> @@ -154,15 +154,14 @@ static struct softif_neigh *softif_neigh_get(struct bat_priv *bat_priv,
> memcpy(softif_neigh->addr, addr, ETH_ALEN);
> softif_neigh->vid = vid;
> softif_neigh->last_seen = jiffies;
> - kref_init(&softif_neigh->refcount);
> + /* initialize with 2 - caller decrements counter by one */
> + atomic_set(&softif_neigh->refcount, 2);
>
> INIT_HLIST_NODE(&softif_neigh->list);
> spin_lock_bh(&bat_priv->softif_neigh_lock);
> hlist_add_head_rcu(&softif_neigh->list, &bat_priv->softif_neigh_list);
> spin_unlock_bh(&bat_priv->softif_neigh_lock);
>
> -found:
> - kref_get(&softif_neigh->refcount);
> out:
> rcu_read_unlock();
> return softif_neigh;
The rest of the new atomic handling seems fine. Just two more
things I noticed while reading the softif_neigh specific code:
bat_priv->softif_neigh needs to be changed to a rcu-protected
pointer.
There's a race condition in softif_neigh_seq_print_text(), between
the rcu_read_unlock() and rcu_read_lock() the number of
softif_neigh's can have increased and there's no check for
accidentally writing outside of the allocated buffer.
Cheers, Linus
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH 2/7] batman-adv: Correct rcu refcounting for softif_neigh
2011-02-10 12:45 ` Linus Lüssing
@ 2011-02-10 13:57 ` Marek Lindner
2011-02-12 21:23 ` Linus Lüssing
0 siblings, 1 reply; 44+ messages in thread
From: Marek Lindner @ 2011-02-10 13:57 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
On Thursday 10 February 2011 13:45:44 Linus Lüssing wrote:
> > - goto found;
> > + goto out;
> >
> > }
>
> Hmm, we could do a rcu_read_unlock() here, already, I think. Would
> it be better to do so, keeping the rcu grace period as small as
> possible?
Can you be more specific where "here" is ? Instead of the "goto out" ?
> The rest of the new atomic handling seems fine. Just two more
> things I noticed while reading the softif_neigh specific code:
> bat_priv->softif_neigh needs to be changed to a rcu-protected
> pointer.
Agreed.
> There's a race condition in softif_neigh_seq_print_text(), between
> the rcu_read_unlock() and rcu_read_lock() the number of
> softif_neigh's can have increased and there's no check for
> accidentally writing outside of the allocated buffer.
Also correct - are you going to send a patch ?
Regards,
Marek
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH 5/7] batman-adv: Make bat_priv->curr_gw an rcu protected pointer
2011-02-10 10:42 ` Linus Lüssing
@ 2011-02-10 14:25 ` Marek Lindner
2011-02-12 21:21 ` Linus Lüssing
2011-02-12 21:21 ` [B.A.T.M.A.N.] [PATCHv2 " Linus Lüssing
0 siblings, 2 replies; 44+ messages in thread
From: Marek Lindner @ 2011-02-10 14:25 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
On Thursday 10 February 2011 11:42:50 Linus Lüssing wrote:
> > Would speak anything against re-using the gw_list_lock ?
>
> ... as we are usually changing the gw_list more often than the
> curr_gw, so it's not really necessary to let a gw_list change for
> another node wait for a curr_gw_node reassignment to finish.
Well, we only need the list lock when we are adding/deleting items from the
list which does not happen that often nor is it time critical.
> What is speaking for using the same lock for both, just having
> less spinlocks in total in the code?
Yes.
Regards,
Marek
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH 5/7] batman-adv: Make bat_priv->curr_gw an rcu protected pointer
2011-02-10 14:25 ` Marek Lindner
@ 2011-02-12 21:21 ` Linus Lüssing
2011-02-12 21:21 ` [B.A.T.M.A.N.] [PATCHv2 " Linus Lüssing
1 sibling, 0 replies; 44+ messages in thread
From: Linus Lüssing @ 2011-02-12 21:21 UTC (permalink / raw)
To: b.a.t.m.a.n
Hi Marek,
Ok, I now reused the gw_list_lock instead of introducing a new spinlock. I've left the
one issue in the beginning of gw_election() you've been mentioning untouched.
The rest should be fixed.
Cheers, Linus
^ permalink raw reply [flat|nested] 44+ messages in thread
* [B.A.T.M.A.N.] [PATCHv2 5/7] batman-adv: Make bat_priv->curr_gw an rcu protected pointer
2011-02-10 14:25 ` Marek Lindner
2011-02-12 21:21 ` Linus Lüssing
@ 2011-02-12 21:21 ` Linus Lüssing
2011-02-13 21:09 ` Marek Lindner
1 sibling, 1 reply; 44+ messages in thread
From: Linus Lüssing @ 2011-02-12 21:21 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Linus Lüssing
The rcu protected macros rcu_dereference() and rcu_assign_pointer()
for the bat_priv->curr_gw need to be used, as well as spin/rcu locking.
Otherwise we might end up using a curr_gw pointer pointing to already
freed memory.
Reported-by: Sven Eckelmann <sven@narfation.org>
Signed-off-by: Linus Lüssing <linus.luessing@ascom.ch>
---
gateway_client.c | 99 ++++++++++++++++++++++++++++++++++++++---------------
types.h | 2 +-
2 files changed, 72 insertions(+), 29 deletions(-)
diff --git a/gateway_client.c b/gateway_client.c
index 517e001..b04ee56 100644
--- a/gateway_client.c
+++ b/gateway_client.c
@@ -44,19 +44,30 @@ static void gw_node_free_ref(struct gw_node *gw_node)
void *gw_get_selected(struct bat_priv *bat_priv)
{
- struct gw_node *curr_gateway_tmp = bat_priv->curr_gw;
+ struct gw_node *curr_gateway_tmp;
+ struct orig_node *orig_node;
- if (!curr_gateway_tmp)
+ rcu_read_lock();
+ curr_gateway_tmp = rcu_dereference(bat_priv->curr_gw);
+ if (!curr_gateway_tmp) {
+ rcu_read_unlock();
return NULL;
+ }
+
+ orig_node = curr_gateway_tmp->orig_node;
+ rcu_read_unlock();
- return curr_gateway_tmp->orig_node;
+ return orig_node;
}
void gw_deselect(struct bat_priv *bat_priv)
{
- struct gw_node *gw_node = bat_priv->curr_gw;
+ struct gw_node *gw_node;
- bat_priv->curr_gw = NULL;
+ spin_lock_bh(&bat_priv->gw_list_lock);
+ gw_node = rcu_dereference(bat_priv->curr_gw);
+ rcu_assign_pointer(bat_priv->curr_gw, NULL);
+ spin_unlock_bh(&bat_priv->gw_list_lock);
if (gw_node)
gw_node_free_ref(gw_node);
@@ -64,12 +75,15 @@ void gw_deselect(struct bat_priv *bat_priv)
static void gw_select(struct bat_priv *bat_priv, struct gw_node *new_gw_node)
{
- struct gw_node *curr_gw_node = bat_priv->curr_gw;
+ struct gw_node *curr_gw_node;
if (new_gw_node && !atomic_inc_not_zero(&new_gw_node->refcount))
new_gw_node = NULL;
- bat_priv->curr_gw = new_gw_node;
+ spin_lock_bh(&bat_priv->gw_list_lock);
+ curr_gw_node = rcu_dereference(bat_priv->curr_gw);
+ rcu_assign_pointer(bat_priv->curr_gw, new_gw_node);
+ spin_unlock_bh(&bat_priv->gw_list_lock);
if (curr_gw_node)
gw_node_free_ref(curr_gw_node);
@@ -78,7 +92,7 @@ static void gw_select(struct bat_priv *bat_priv, struct gw_node *new_gw_node)
void gw_election(struct bat_priv *bat_priv)
{
struct hlist_node *node;
- struct gw_node *gw_node, *curr_gw_tmp = NULL;
+ struct gw_node *gw_node, *curr_gw, *curr_gw_tmp = NULL;
uint8_t max_tq = 0;
uint32_t max_gw_factor = 0, tmp_gw_factor = 0;
int down, up;
@@ -92,19 +106,24 @@ void gw_election(struct bat_priv *bat_priv)
if (atomic_read(&bat_priv->gw_mode) != GW_MODE_CLIENT)
return;
- if (bat_priv->curr_gw)
+ rcu_read_lock();
+ curr_gw = rcu_dereference(bat_priv->curr_gw);
+ if (curr_gw) {
+ rcu_read_unlock();
return;
+ }
- rcu_read_lock();
if (hlist_empty(&bat_priv->gw_list)) {
- rcu_read_unlock();
- if (bat_priv->curr_gw) {
+ if (curr_gw) {
+ rcu_read_unlock();
bat_dbg(DBG_BATMAN, bat_priv,
"Removing selected gateway - "
"no gateway in range\n");
gw_deselect(bat_priv);
}
+ else
+ rcu_read_unlock();
return;
}
@@ -153,12 +172,12 @@ void gw_election(struct bat_priv *bat_priv)
max_gw_factor = tmp_gw_factor;
}
- if (bat_priv->curr_gw != curr_gw_tmp) {
- if ((bat_priv->curr_gw) && (!curr_gw_tmp))
+ if (curr_gw != curr_gw_tmp) {
+ if ((curr_gw) && (!curr_gw_tmp))
bat_dbg(DBG_BATMAN, bat_priv,
"Removing selected gateway - "
"no gateway in range\n");
- else if ((!bat_priv->curr_gw) && (curr_gw_tmp))
+ else if ((!curr_gw) && (curr_gw_tmp))
bat_dbg(DBG_BATMAN, bat_priv,
"Adding route to gateway %pM "
"(gw_flags: %i, tq: %i)\n",
@@ -181,26 +200,34 @@ void gw_election(struct bat_priv *bat_priv)
void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
{
- struct gw_node *curr_gateway_tmp = bat_priv->curr_gw;
+ struct gw_node *curr_gateway_tmp;
uint8_t gw_tq_avg, orig_tq_avg;
+ rcu_read_lock();
+ curr_gateway_tmp = rcu_dereference(bat_priv->curr_gw);
if (!curr_gateway_tmp)
- return;
+ goto rcu_unlock;
- if (!curr_gateway_tmp->orig_node)
+ if (!curr_gateway_tmp->orig_node) {
+ rcu_read_unlock();
goto deselect;
+ }
- if (!curr_gateway_tmp->orig_node->router)
+ if (!curr_gateway_tmp->orig_node->router) {
+ rcu_read_unlock();
goto deselect;
+ }
/* this node already is the gateway */
if (curr_gateway_tmp->orig_node == orig_node)
- return;
+ goto rcu_unlock;
if (!orig_node->router)
- return;
+ goto rcu_unlock;
gw_tq_avg = curr_gateway_tmp->orig_node->router->tq_avg;
+ rcu_read_unlock();
+
orig_tq_avg = orig_node->router->tq_avg;
/* the TQ value has to be better */
@@ -222,6 +249,9 @@ void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
deselect:
gw_deselect(bat_priv);
+ return;
+rcu_unlock:
+ rcu_read_unlock();
}
static void gw_node_add(struct bat_priv *bat_priv,
@@ -278,7 +308,7 @@ void gw_node_update(struct bat_priv *bat_priv,
"Gateway %pM removed from gateway list\n",
orig_node->orig);
- if (gw_node == bat_priv->curr_gw) {
+ if (gw_node == rcu_dereference(bat_priv->curr_gw)) {
rcu_read_unlock();
gw_deselect(bat_priv);
return;
@@ -316,8 +346,10 @@ void gw_node_purge(struct bat_priv *bat_priv)
atomic_read(&bat_priv->mesh_state) == MESH_ACTIVE)
continue;
- if (bat_priv->curr_gw == gw_node)
- gw_deselect(bat_priv);
+ if (rcu_dereference(bat_priv->curr_gw) == gw_node) {
+ rcu_assign_pointer(bat_priv->curr_gw, NULL);
+ gw_node_free_ref(gw_node);
+ }
hlist_del_rcu(&gw_node->list);
gw_node_free_ref(gw_node);
@@ -330,12 +362,16 @@ void gw_node_purge(struct bat_priv *bat_priv)
static int _write_buffer_text(struct bat_priv *bat_priv,
struct seq_file *seq, struct gw_node *gw_node)
{
- int down, up;
+ struct gw_node *curr_gw;
+ int down, up, ret;
gw_bandwidth_to_kbit(gw_node->orig_node->gw_flags, &down, &up);
- return seq_printf(seq, "%s %pM (%3i) %pM [%10s]: %3i - %i%s/%i%s\n",
- (bat_priv->curr_gw == gw_node ? "=>" : " "),
+ rcu_read_lock();
+ curr_gw = rcu_dereference(bat_priv->curr_gw);
+
+ ret = seq_printf(seq, "%s %pM (%3i) %pM [%10s]: %3i - %i%s/%i%s\n",
+ (curr_gw == gw_node ? "=>" : " "),
gw_node->orig_node->orig,
gw_node->orig_node->router->tq_avg,
gw_node->orig_node->router->addr,
@@ -345,6 +381,9 @@ static int _write_buffer_text(struct bat_priv *bat_priv,
(down > 2048 ? "MBit" : "KBit"),
(up > 2048 ? up / 1024 : up),
(up > 2048 ? "MBit" : "KBit"));
+
+ rcu_read_unlock();
+ return ret;
}
int gw_client_seq_print_text(struct seq_file *seq, void *offset)
@@ -465,8 +504,12 @@ int gw_is_target(struct bat_priv *bat_priv, struct sk_buff *skb)
if (atomic_read(&bat_priv->gw_mode) == GW_MODE_SERVER)
return -1;
- if (!bat_priv->curr_gw)
+ rcu_read_lock();
+ if (!rcu_dereference(bat_priv->curr_gw)) {
+ rcu_read_unlock();
return 0;
+ }
+ rcu_read_unlock();
return 1;
}
diff --git a/types.h b/types.h
index ee77d48..8447330 100644
--- a/types.h
+++ b/types.h
@@ -161,7 +161,7 @@ struct bat_priv {
spinlock_t forw_bcast_list_lock; /* protects */
spinlock_t hna_lhash_lock; /* protects hna_local_hash */
spinlock_t hna_ghash_lock; /* protects hna_global_hash */
- spinlock_t gw_list_lock; /* protects gw_list */
+ spinlock_t gw_list_lock; /* protects gw_list and curr_gw */
spinlock_t vis_hash_lock; /* protects vis_hash */
spinlock_t vis_list_lock; /* protects vis_info::recv_list */
spinlock_t softif_neigh_lock; /* protects soft-interface neigh list */
--
1.7.2.3
^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH 2/7] batman-adv: Correct rcu refcounting for softif_neigh
2011-02-10 13:57 ` Marek Lindner
@ 2011-02-12 21:23 ` Linus Lüssing
0 siblings, 0 replies; 44+ messages in thread
From: Linus Lüssing @ 2011-02-12 21:23 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
> > There's a race condition in softif_neigh_seq_print_text(), between
> > the rcu_read_unlock() and rcu_read_lock() the number of
> > softif_neigh's can have increased and there's no check for
> > accidentally writing outside of the allocated buffer.
>
> Also correct - are you going to send a patch ?
Yes, will do, wait a sec.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCHv2 5/7] batman-adv: Make bat_priv->curr_gw an rcu protected pointer
2011-02-12 21:21 ` [B.A.T.M.A.N.] [PATCHv2 " Linus Lüssing
@ 2011-02-13 21:09 ` Marek Lindner
0 siblings, 0 replies; 44+ messages in thread
From: Marek Lindner @ 2011-02-13 21:09 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
On Saturday 12 February 2011 22:21:38 Linus Lüssing wrote:
> The rcu protected macros rcu_dereference() and rcu_assign_pointer()
> for the bat_priv->curr_gw need to be used, as well as spin/rcu locking.
>
> Otherwise we might end up using a curr_gw pointer pointing to already
> freed memory.
Applied in revision 1941.
Thanks,
Marek
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH 6/7] batman-adv: add __rcu annotations for gw_node
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 6/7] batman-adv: add __rcu annotations for gw_node Linus Lüssing
@ 2011-02-13 21:10 ` Marek Lindner
0 siblings, 0 replies; 44+ messages in thread
From: Marek Lindner @ 2011-02-13 21:10 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
On Friday 04 February 2011 16:21:35 Linus Lüssing wrote:
> Add __rcu annotations for the rcu protected bat_priv::curr_gw pointer to
> allow sparse checking.
Applied in revision 1942.
Thanks,
Marek
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH 7/7] batman-adv: Increase orig_node refcount before releasing rcu read lock
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 7/7] batman-adv: Increase orig_node refcount before releasing rcu read lock Linus Lüssing
2011-02-04 16:33 ` [B.A.T.M.A.N.] [PATCHv2 " Linus Lüssing
@ 2011-02-13 21:11 ` Marek Lindner
1 sibling, 0 replies; 44+ messages in thread
From: Marek Lindner @ 2011-02-13 21:11 UTC (permalink / raw)
To: The list for a Better Approach To Mobile Ad-hoc Networking
On Friday 04 February 2011 16:21:36 Linus Lüssing wrote:
> When unicast_send_skb() is increasing the orig_node's refcount another
> thread might have been freeing this orig_node already. We need to
> increase the refcount in the rcu read lock protected area to avoid that.
Applied in revision 1943.
Thanks,
Marek
^ permalink raw reply [flat|nested] 44+ messages in thread
end of thread, other threads:[~2011-02-13 21:11 UTC | newest]
Thread overview: 44+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-30 1:52 [B.A.T.M.A.N.] Reordered rcu refcounting Sven Eckelmann
2011-01-30 1:52 ` [B.A.T.M.A.N.] [PATCH 1/4] batman-adv: Correct rcu refcounting for gw_node Sven Eckelmann
2011-02-02 17:37 ` [B.A.T.M.A.N.] [PATCH] " Linus Lüssing
2011-02-02 19:49 ` Marek Lindner
2011-02-02 20:43 ` Linus Lüssing
2011-02-02 21:42 ` Sven Eckelmann
2011-02-03 0:19 ` Marek Lindner
2011-02-03 9:55 ` Linus Lüssing
2011-02-03 10:01 ` Sven Eckelmann
2011-02-03 14:43 ` [B.A.T.M.A.N.] [PATCH 1/3] " Linus Lüssing
2011-02-03 17:56 ` Sven Eckelmann
2011-02-03 14:43 ` [B.A.T.M.A.N.] [PATCH 2/3] batman-adv: add __rcu annotations " Linus Lüssing
2011-02-03 14:43 ` [B.A.T.M.A.N.] [PATCH 3/3] batman-adv: Increase orig_node refcount before releasing rcu read lock Linus Lüssing
2011-01-30 1:52 ` [B.A.T.M.A.N.] [PATCH 2/4] batman-adv: Correct rcu refcounting for softif_neigh Sven Eckelmann
2011-01-30 1:52 ` [B.A.T.M.A.N.] [PATCH 3/4] batman-adv: Correct rcu refcounting for batman_if Sven Eckelmann
2011-01-30 1:52 ` [B.A.T.M.A.N.] [PATCH 4/4] batman-adv: Correct rcu refcounting for neigh_node Sven Eckelmann
2011-01-30 15:20 ` [B.A.T.M.A.N.] [PATCHv2 " Sven Eckelmann
2011-02-03 17:02 ` [B.A.T.M.A.N.] Reordered rcu refcounting Marek Lindner
2011-02-03 17:03 ` [B.A.T.M.A.N.] [PATCH 1/4] batman-adv: Correct rcu refcounting for gw_node Marek Lindner
2011-02-03 17:03 ` [B.A.T.M.A.N.] [PATCH 2/4] batman-adv: Correct rcu refcounting for softif_neigh Marek Lindner
2011-02-03 17:03 ` [B.A.T.M.A.N.] [PATCH 3/4] batman-adv: Correct rcu refcounting for batman_if Marek Lindner
2011-02-03 17:03 ` [B.A.T.M.A.N.] [PATCH 4/4] batman-adv: Correct rcu refcounting for neigh_node Marek Lindner
2011-02-04 12:59 ` [B.A.T.M.A.N.] [PATCHv2 " Marek Lindner
2011-02-04 15:21 ` [B.A.T.M.A.N.] Reordered rcu refcounting, v3 Linus Lüssing
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 1/7] batman-adv: Correct rcu refcounting for gw_node Linus Lüssing
2011-02-10 11:01 ` Linus Lüssing
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 2/7] batman-adv: Correct rcu refcounting for softif_neigh Linus Lüssing
2011-02-10 12:45 ` Linus Lüssing
2011-02-10 13:57 ` Marek Lindner
2011-02-12 21:23 ` Linus Lüssing
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 3/7] batman-adv: Correct rcu refcounting for batman_if Linus Lüssing
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 4/7] batman-adv: Correct rcu refcounting for neigh_node Linus Lüssing
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 5/7] batman-adv: Make bat_priv->curr_gw an rcu protected pointer Linus Lüssing
2011-02-08 13:18 ` Marek Lindner
2011-02-10 10:42 ` Linus Lüssing
2011-02-10 14:25 ` Marek Lindner
2011-02-12 21:21 ` Linus Lüssing
2011-02-12 21:21 ` [B.A.T.M.A.N.] [PATCHv2 " Linus Lüssing
2011-02-13 21:09 ` Marek Lindner
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 6/7] batman-adv: add __rcu annotations for gw_node Linus Lüssing
2011-02-13 21:10 ` Marek Lindner
2011-02-04 15:21 ` [B.A.T.M.A.N.] [PATCH 7/7] batman-adv: Increase orig_node refcount before releasing rcu read lock Linus Lüssing
2011-02-04 16:33 ` [B.A.T.M.A.N.] [PATCHv2 " Linus Lüssing
2011-02-13 21:11 ` [B.A.T.M.A.N.] [PATCH " Marek Lindner
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.