netdev - [PATCH net-next 1/5] neighbor: Fix locking order for gc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 11 Dec 2018 18:57:21 -0700
From:   David Ahern <dsahern@...nel.org>
To:     netdev@...r.kernel.org
Cc:     davem@...emloft.net, roopa@...ulusnetworks.com,
        David Ahern <dsahern@...il.com>
Subject: [PATCH net-next 1/5] neighbor: Fix locking order for gc_list changes

From: David Ahern <dsahern@...il.com>

Lock checker noted an inverted lock order between neigh_change_state
(neighbor lock then table lock) and neigh_periodic_work (table lock and
then neighbor lock) resulting in:

[  121.057652] ======================================================
[  121.058740] WARNING: possible circular locking dependency detected
[  121.059861] 4.20.0-rc6+ #43 Not tainted
[  121.060546] ------------------------------------------------------
[  121.061630] kworker/0:2/65 is trying to acquire lock:
[  121.062519] (____ptrval____) (&n->lock){++--}, at: neigh_periodic_work+0x237/0x324
[  121.063894]
[  121.063894] but task is already holding lock:
[  121.064920] (____ptrval____) (&tbl->lock){+.-.}, at: neigh_periodic_work+0x194/0x324
[  121.066274]
[  121.066274] which lock already depends on the new lock.
[  121.066274]
[  121.067693]
[  121.067693] the existing dependency chain (in reverse order) is:
...

Fix by renaming neigh_change_state to neigh_update_gc_list, changing
it to only manage whether an entry should be on the gc_list and taking
locks in the same order as neigh_periodic_work. Invoke at the end of
neigh_update only if diff between old or new states has the PERMANENT
flag set.

Fixes: 8cc196d6ef86 ("neighbor: gc_list changes should be protected by table lock")
Signed-off-by: David Ahern <dsahern@...il.com>
---
 net/core/neighbour.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 03fdc5ae66b0..010784123bc1 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -127,30 +127,30 @@ static void neigh_mark_dead(struct neighbour *n)
 	}
 }
 
-static void neigh_change_state(struct neighbour *n, u8 new)
+static void neigh_update_gc_list(struct neighbour *n)
 {
-	bool on_gc_list = !list_empty(&n->gc_list);
-	bool new_is_perm = new & NUD_PERMANENT;
+	bool on_gc_list, new_is_perm;
 
-	n->nud_state = new;
+	write_lock_bh(&n->tbl->lock);
+	write_lock(&n->lock);
 
 	/* remove from the gc list if new state is permanent;
 	 * add to the gc list if new state is not permanent
 	 */
+	new_is_perm = n->nud_state & NUD_PERMANENT;
+	on_gc_list = !list_empty(&n->gc_list);
+
 	if (new_is_perm && on_gc_list) {
-		write_lock_bh(&n->tbl->lock);
 		list_del_init(&n->gc_list);
-		write_unlock_bh(&n->tbl->lock);
-
 		atomic_dec(&n->tbl->gc_entries);
 	} else if (!new_is_perm && !on_gc_list) {
 		/* add entries to the tail; cleaning removes from the front */
-		write_lock_bh(&n->tbl->lock);
 		list_add_tail(&n->gc_list, &n->tbl->gc_list);
-		write_unlock_bh(&n->tbl->lock);
-
 		atomic_inc(&n->tbl->gc_entries);
 	}
+
+	write_unlock(&n->lock);
+	write_unlock_bh(&n->tbl->lock);
 }
 
 static bool neigh_del(struct neighbour *n, __u8 state, __u8 flags,
@@ -1220,7 +1220,7 @@ static int __neigh_update(struct neighbour *neigh, const u8 *lladdr,
 		neigh_del_timer(neigh);
 		if (old & NUD_CONNECTED)
 			neigh_suspect(neigh);
-		neigh_change_state(neigh, new);
+		neigh->nud_state = new;
 		err = 0;
 		notify = old & NUD_VALID;
 		if ((old & (NUD_INCOMPLETE | NUD_PROBE)) &&
@@ -1299,7 +1299,7 @@ static int __neigh_update(struct neighbour *neigh, const u8 *lladdr,
 						((new & NUD_REACHABLE) ?
 						 neigh->parms->reachable_time :
 						 0)));
-		neigh_change_state(neigh, new);
+		neigh->nud_state = new;
 		notify = 1;
 	}
 
@@ -1360,6 +1360,9 @@ static int __neigh_update(struct neighbour *neigh, const u8 *lladdr,
 		neigh_update_is_router(neigh, flags, &notify);
 	write_unlock_bh(&neigh->lock);
 
+	if ((new ^ old) & NUD_PERMANENT)
+		neigh_update_gc_list(neigh);
+
 	if (notify)
 		neigh_update_notify(neigh, nlmsg_pid);
 
-- 
2.11.0