From: Sven Eckelmann <sven@narfation.org>
To: b.a.t.m.a.n@lists.open-mesh.org
Cc: Marek Lindner <mareklindner@neomailbox.ch>
Subject: Re: [B.A.T.M.A.N.] [PATCHv2] batman-adv: protect tt request from double deletion
Date: Sat, 20 Jun 2015 10:26:31 +0200 [thread overview]
Message-ID: <1847613.Lr5mR0jfaA@sven-edge> (raw)
In-Reply-To: <1434721827-4935-1-git-send-email-mareklindner@neomailbox.ch>
[-- 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 --]
next prev parent reply other threads:[~2015-06-20 8:26 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2015-06-20 10:14 ` Marek Lindner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1847613.Lr5mR0jfaA@sven-edge \
--to=sven@narfation.org \
--cc=b.a.t.m.a.n@lists.open-mesh.org \
--cc=mareklindner@neomailbox.ch \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.