neighbor: Fix locking order for gc_list changes (9c29a2f5) · Commits · 戴 / test

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: ("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>

net/core/neighbour.c

+15 −12

Original line number	Diff line number	Diff line
		@@ -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);

Admin message