neighbor: Fix locking order for gc_list changes

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@gmail.com>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 03fdc5ae66b07e6a0493c8d3df3cb8d46a206fed..010784123bc1eb2d4b002d1d2dac065e46e8e035 100644 (file)
--- 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 @@ out:
                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);