mirrors/linux
1
0
Fork 1
mirror of https://github.com/torvalds/linux.git synced 2025-11-04 02:30:34 +02:00
Code Issues Projects Releases Packages Wiki Activity Actions

neighbor: Fix locking order for gc_list changes

Browse source 
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: 8cc196d6ef ("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>
This commit is contained in:
David Ahern 2018-12-11 18:57:21 -07:00 committed by David S. Miller
parent aeb3fecde8
commit 9c29a2f55e
1 changed files with 16 additions and 13 deletions
repo.diff.show_diff_stats Download patch file Download diff file Expand all files Collapse all files

29
net/core/neighbour.c
View file

@ -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 on_gc_list, new_is_perm;
bool new_is_perm = new & NUD_PERMANENT;
n->nud_state = new; write_lock_bh(&n->tbl->lock);
write_lock(&n->lock);
/* remove from the gc list if new state is permanent; /* remove from the gc list if new state is permanent;
* add to the gc list if new state is not permanent * add to the gc list if new state is not permanent
*/ */
if (new_is_perm && on_gc_list) { new_is_perm = n->nud_state & NUD_PERMANENT;
write_lock_bh(&n->tbl->lock); on_gc_list = !list_empty(&n->gc_list);
list_del_init(&n->gc_list);
write_unlock_bh(&n->tbl->lock);
if (new_is_perm && on_gc_list) {
list_del_init(&n->gc_list);
atomic_dec(&n->tbl->gc_entries); atomic_dec(&n->tbl->gc_entries);
} else if (!new_is_perm && !on_gc_list) { } else if (!new_is_perm && !on_gc_list) {
/* add entries to the tail; cleaning removes from the front */ /* 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); list_add_tail(&n->gc_list, &n->tbl->gc_list);
write_unlock_bh(&n->tbl->lock);
atomic_inc(&n->tbl->gc_entries); 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, 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); neigh_del_timer(neigh);
if (old & NUD_CONNECTED) if (old & NUD_CONNECTED)
neigh_suspect(neigh); neigh_suspect(neigh);
neigh_change_state(neigh, new); neigh->nud_state = new;
err = 0; err = 0;
notify = old & NUD_VALID; notify = old & NUD_VALID;
if ((old & (NUD_INCOMPLETE | NUD_PROBE)) && if ((old & (NUD_INCOMPLETE | NUD_PROBE)) &&
@ -1299,7 +1299,7 @@ static int __neigh_update(struct neighbour *neigh, const u8 *lladdr,
((new & NUD_REACHABLE) ? ((new & NUD_REACHABLE) ?
neigh->parms->reachable_time : neigh->parms->reachable_time :
0))); 0)));
neigh_change_state(neigh, new); neigh->nud_state = new;
notify = 1; notify = 1;
} }
@ -1360,6 +1360,9 @@ static int __neigh_update(struct neighbour *neigh, const u8 *lladdr,
neigh_update_is_router(neigh, flags, &notify); neigh_update_is_router(neigh, flags, &notify);
write_unlock_bh(&neigh->lock); write_unlock_bh(&neigh->lock);
if ((new ^ old) & NUD_PERMANENT)
neigh_update_gc_list(neigh);
if (notify) if (notify)
neigh_update_notify(neigh, nlmsg_pid); neigh_update_notify(neigh, nlmsg_pid);