* [B.A.T.M.A.N.] [PATCHv2] batman-adv: protect tt request from double deletion
@ 2015-06-19 13:50 Marek Lindner
2015-06-20 8:26 ` Sven Eckelmann
0 siblings, 1 reply; 3+ messages in thread
From: Marek Lindner @ 2015-06-19 13:50 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Marek Lindner
The list_del() call was changed to hlist_del_init() to allow
take advantage of the hlist_unhashed() check prior to deletion
in batadv_tt_req_node_new().
Signed-off-by: Marek Lindner <mareklindner@neomailbox.ch>
---
v2: removed redundant hlist_unhashed() check & reword commit message
net/batman-adv/main.c | 2 +-
net/batman-adv/translation-table.c | 28 ++++++++++++++++------------
net/batman-adv/types.h | 4 ++--
3 files changed, 19 insertions(+), 15 deletions(-)
diff --git a/net/batman-adv/main.c b/net/batman-adv/main.c
index 40750cb..d277ba7 100644
--- a/net/batman-adv/main.c
+++ b/net/batman-adv/main.c
@@ -148,7 +148,7 @@ int batadv_mesh_init(struct net_device *soft_iface)
INIT_HLIST_HEAD(&bat_priv->mcast.want_all_ipv6_list);
#endif
INIT_LIST_HEAD(&bat_priv->tt.changes_list);
- INIT_LIST_HEAD(&bat_priv->tt.req_list);
+ INIT_HLIST_HEAD(&bat_priv->tt.req_list);
INIT_LIST_HEAD(&bat_priv->tt.roam_list);
#ifdef CONFIG_BATMAN_ADV_MCAST
INIT_HLIST_HEAD(&bat_priv->mcast.mla_list);
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c
index 8ef27bb..7971427 100644
--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -2220,12 +2220,13 @@ static u32 batadv_tt_local_crc(struct batadv_priv *bat_priv,
static void batadv_tt_req_list_free(struct batadv_priv *bat_priv)
{
- struct batadv_tt_req_node *node, *safe;
+ struct batadv_tt_req_node *node;
+ struct hlist_node *safe;
spin_lock_bh(&bat_priv->tt.req_list_lock);
- list_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) {
- list_del(&node->list);
+ hlist_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) {
+ hlist_del_init(&node->list);
kfree(node);
}
@@ -2255,13 +2256,14 @@ static void batadv_tt_save_orig_buffer(struct batadv_priv *bat_priv,
static void batadv_tt_req_purge(struct batadv_priv *bat_priv)
{
- struct batadv_tt_req_node *node, *safe;
+ struct batadv_tt_req_node *node;
+ struct hlist_node *safe;
spin_lock_bh(&bat_priv->tt.req_list_lock);
- list_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) {
+ hlist_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) {
if (batadv_has_timed_out(node->issued_at,
BATADV_TT_REQUEST_TIMEOUT)) {
- list_del(&node->list);
+ hlist_del_init(&node->list);
kfree(node);
}
}
@@ -2283,7 +2285,7 @@ batadv_tt_req_node_new(struct batadv_priv *bat_priv,
struct batadv_tt_req_node *tt_req_node_tmp, *tt_req_node = NULL;
spin_lock_bh(&bat_priv->tt.req_list_lock);
- list_for_each_entry(tt_req_node_tmp, &bat_priv->tt.req_list, list) {
+ hlist_for_each_entry(tt_req_node_tmp, &bat_priv->tt.req_list, list) {
if (batadv_compare_eth(tt_req_node_tmp, orig_node) &&
!batadv_has_timed_out(tt_req_node_tmp->issued_at,
BATADV_TT_REQUEST_TIMEOUT))
@@ -2297,7 +2299,7 @@ batadv_tt_req_node_new(struct batadv_priv *bat_priv,
ether_addr_copy(tt_req_node->addr, orig_node->orig);
tt_req_node->issued_at = jiffies;
- list_add(&tt_req_node->list, &bat_priv->tt.req_list);
+ hlist_add_head(&tt_req_node->list, &bat_priv->tt.req_list);
unlock:
spin_unlock_bh(&bat_priv->tt.req_list_lock);
return tt_req_node;
@@ -2548,7 +2550,8 @@ out:
batadv_hardif_free_ref(primary_if);
if (ret && tt_req_node) {
spin_lock_bh(&bat_priv->tt.req_list_lock);
- list_del(&tt_req_node->list);
+ /* hlist_del_init() verifies tt_req_node still is in the list */
+ hlist_del_init(&tt_req_node->list);
spin_unlock_bh(&bat_priv->tt.req_list_lock);
kfree(tt_req_node);
}
@@ -2944,7 +2947,8 @@ static void batadv_handle_tt_response(struct batadv_priv *bat_priv,
struct batadv_tvlv_tt_data *tt_data,
u8 *resp_src, u16 num_entries)
{
- struct batadv_tt_req_node *node, *safe;
+ struct batadv_tt_req_node *node;
+ struct hlist_node *safe;
struct batadv_orig_node *orig_node = NULL;
struct batadv_tvlv_tt_change *tt_change;
u8 *tvlv_ptr = (u8 *)tt_data;
@@ -2982,10 +2986,10 @@ static void batadv_handle_tt_response(struct batadv_priv *bat_priv,
/* Delete the tt_req_node from pending tt_requests list */
spin_lock_bh(&bat_priv->tt.req_list_lock);
- list_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) {
+ hlist_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) {
if (!batadv_compare_eth(node->addr, resp_src))
continue;
- list_del(&node->list);
+ hlist_del_init(&node->list);
kfree(node);
}
diff --git a/net/batman-adv/types.h b/net/batman-adv/types.h
index da4c738..76fe31f 100644
--- a/net/batman-adv/types.h
+++ b/net/batman-adv/types.h
@@ -537,7 +537,7 @@ struct batadv_priv_tt {
struct list_head changes_list;
struct batadv_hashtable *local_hash;
struct batadv_hashtable *global_hash;
- struct list_head req_list;
+ struct hlist_head req_list;
struct list_head roam_list;
spinlock_t changes_list_lock; /* protects changes */
spinlock_t req_list_lock; /* protects req_list */
@@ -1006,7 +1006,7 @@ struct batadv_tt_change_node {
struct batadv_tt_req_node {
u8 addr[ETH_ALEN];
unsigned long issued_at;
- struct list_head list;
+ struct hlist_node list;
};
/**
--
2.1.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCHv2] batman-adv: protect tt request from double deletion
2015-06-19 13:50 [B.A.T.M.A.N.] [PATCHv2] batman-adv: protect tt request from double deletion Marek Lindner
@ 2015-06-20 8:26 ` Sven Eckelmann
2015-06-20 10:14 ` Marek Lindner
0 siblings, 1 reply; 3+ messages in thread
From: Sven Eckelmann @ 2015-06-20 8:26 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: Marek Lindner
[-- Attachment #1: Type: text/plain, Size: 3040 bytes --]
On Friday 19 June 2015 21:50:27 Marek Lindner wrote:
> The list_del() call was changed to hlist_del_init() to allow
> take advantage of the hlist_unhashed() check prior to deletion
> in batadv_tt_req_node_new().
>
> Signed-off-by: Marek Lindner <mareklindner@neomailbox.ch>
> ---
> v2: removed redundant hlist_unhashed() check & reword commit message
list_del_init would also work. It is not necessary to switch to hlists for
this feature. It is not possible with RCU but this is not used here.
list_del_init doesn't have a special "list_empty" or (not really existing)
"list_unhashed". It is not required because list_del_init on an initialized
list_head which is not in a list will only access this list_head and end up
with the same list_head.
The problem with the old code is unknown state of the tt_req_node in
batadv_send_tt_request. This node could either be in the list (valid state) or
already be deleted from it. The deletion state had prev and next pointer of
this node in an undefined state (either pointing to nodes in its old list,
"random" memory regions or at poisoned addresses). This can be avoided by
using list_del_init + locking for this list. All correctly initialized nodes
will then either be inside a list or have prev+next pointing to itself.
Calling again list_del_init on a node which has prev+next pointing to itself
is safe and will neither change the state of the node or other objects.
Kind regards,
Sven
[...]
> - list_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) {
> - list_del(&node->list);
> + hlist_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) {
> + hlist_del_init(&node->list);
This would only need a change to list_del_init
[...]
> spin_lock_bh(&bat_priv->tt.req_list_lock);
> - list_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) {
> + hlist_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) {
> if (batadv_has_timed_out(node->issued_at,
> BATADV_TT_REQUEST_TIMEOUT)) {
> - list_del(&node->list);
> + hlist_del_init(&node->list);
Same here
[...]
> @@ -2548,7 +2550,8 @@ out:
> batadv_hardif_free_ref(primary_if);
> if (ret && tt_req_node) {
> spin_lock_bh(&bat_priv->tt.req_list_lock);
> - list_del(&tt_req_node->list);
> + /* hlist_del_init() verifies tt_req_node still is in the list */
> + hlist_del_init(&tt_req_node->list);
> spin_unlock_bh(&bat_priv->tt.req_list_lock);
> kfree(tt_req_node);
> }
And here (and this is most likely the only reason why you do all this).
[...]
> @@ -2982,10 +2986,10 @@ static void batadv_handle_tt_response(struct
> batadv_priv *bat_priv,
>
> /* Delete the tt_req_node from pending tt_requests list */
> spin_lock_bh(&bat_priv->tt.req_list_lock);
> - list_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) {
> + hlist_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) {
> if (!batadv_compare_eth(node->addr, resp_src))
> continue;
> - list_del(&node->list);
> + hlist_del_init(&node->list);
And here
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCHv2] batman-adv: protect tt request from double deletion
2015-06-20 8:26 ` Sven Eckelmann
@ 2015-06-20 10:14 ` Marek Lindner
0 siblings, 0 replies; 3+ messages in thread
From: Marek Lindner @ 2015-06-20 10:14 UTC (permalink / raw)
To: b.a.t.m.a.n
[-- Attachment #1: Type: text/plain, Size: 790 bytes --]
On Saturday, June 20, 2015 10:26:31 Sven Eckelmann wrote:
> On Friday 19 June 2015 21:50:27 Marek Lindner wrote:
> > The list_del() call was changed to hlist_del_init() to allow
> > take advantage of the hlist_unhashed() check prior to deletion
> > in batadv_tt_req_node_new().
> >
> >
> >
> > Signed-off-by: Marek Lindner <mareklindner@neomailbox.ch>
> > ---
> > v2: removed redundant hlist_unhashed() check & reword commit message
>
> list_del_init would also work. It is not necessary to switch to hlists for
> this feature. It is not possible with RCU but this is not used here.
I am aware of that fact. Still, I'd like to convert the list to hlist. If you
prefer I can do this in 2 steps (first changing to list_del_init() and then
changing everything to hlist).
Cheers,
Marek
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-06-20 10:14 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-19 13:50 [B.A.T.M.A.N.] [PATCHv2] batman-adv: protect tt request from double deletion Marek Lindner
2015-06-20 8:26 ` Sven Eckelmann
2015-06-20 10:14 ` 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.