diff options
author | David Ahern <dsahern@gmail.com> | 2018-12-11 18:57:21 -0700 |
---|---|---|
committer | David S. Miller <davem@davemloft.net> | 2018-12-14 15:44:47 -0800 |
commit | 9c29a2f55ec05cc8b525ee3b2d75d3cd37911123 (patch) | |
tree | 39283a1e264ed4d46da436a5ec4b6c704392c5e8 /net/core | |
parent | aeb3fecde811d5392ed481d8558f5751ac542e77 (diff) | |
download | lwn-9c29a2f55ec05cc8b525ee3b2d75d3cd37911123.tar.gz lwn-9c29a2f55ec05cc8b525ee3b2d75d3cd37911123.zip |
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>
Diffstat (limited to 'net/core')
-rw-r--r-- | net/core/neighbour.c | 27 |
1 files 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 @@ out: neigh_update_is_router(neigh, flags, ¬ify); write_unlock_bh(&neigh->lock); + if ((new ^ old) & NUD_PERMANENT) + neigh_update_gc_list(neigh); + if (notify) neigh_update_notify(neigh, nlmsg_pid); |