rxrpc: Fix locking issue

There's a locking issue with the per-netns list of calls in rxrpc. The pieces of code that add and remove a call from the list use write_lock() and the calls procfile uses read_lock() to access it. However, the timer callback function may trigger a removal by trying to queue a call for processing and finding that it's already queued - at which point it has a spare refcount that it has to do something with. Unfortunately, if it puts the call and this reduces the refcount to 0, the call will be removed from the list. Unfortunately, since the _bh variants of the locking functions aren't used, this can deadlock. ================================ WARNING: inconsistent lock state 5.18.0-rc3-build4+ #10 Not tainted -------------------------------- inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage. ksoftirqd/2/25 [HC0[0]:SC1[1]:HE1:SE0] takes: ffff888107ac4038 (&rxnet->call_lock){+.?.}-{2:2}, at: rxrpc_put_call+0x103/0x14b {SOFTIRQ-ON-W} state was registered at: ... Possible unsafe locking scenario: CPU0 ---- lock(&rxnet->call_lock); <Interrupt> lock(&rxnet->call_lock); *** DEADLOCK *** 1 lock held by ksoftirqd/2/25: #0: ffff8881008ffdb0 ((&call->timer)){+.-.}-{0:0}, at: call_timer_fn+0x5/0x23d Changes ======= ver #2) - Changed to using list_next_rcu() rather than rcu_dereference() directly.

David Howells committed 4 years ago

Commit ad25f5c1 parent a0575428 files · +62 −22Browse files

Unified · Split

8 files

fs/seq_file.c +32 −0
include/linux/list.h +10 −0
include/linux/seq_file.h +4 −0
net/rxrpc/ar-internal.h +1 −1
net/rxrpc/call_accept.c +3 −3
net/rxrpc/call_object.c +9 −9
net/rxrpc/net_ns.c +1 −1
net/rxrpc/proc.c +2 −8

fs/seq_file.c+32−0

@@ -931,6 +931,38 @@ struct list_head seq_list_next(void v, struct list_head head, loff_t ppos) · 930 unmodified lines
931	931	}
932	932	EXPORT_SYMBOL(seq_list_next);
933	933
	934	+struct list_head seq_list_start_rcu(struct list_head head, loff_t pos)
	935	+{
	936	+ struct list_head *lh;
	937	+
	938	+ list_for_each_rcu(lh, head)
	939	+ if (pos-- == 0)
	940	+ return lh;
	941	+
	942	+ return NULL;
	943	+}
	944	+EXPORT_SYMBOL(seq_list_start_rcu);
	945	+
	946	+struct list_head seq_list_start_head_rcu(struct list_head head, loff_t pos)
	947	+{
	948	+ if (!pos)
	949	+ return head;
	950	+
	951	+ return seq_list_start_rcu(head, pos - 1);
	952	+}
	953	+EXPORT_SYMBOL(seq_list_start_head_rcu);
	954	+
	955	+struct list_head seq_list_next_rcu(void v, struct list_head *head,
	956	+ loff_t *ppos)
	957	+{
	958	+ struct list_head *lh;
	959	+
	960	+ lh = list_next_rcu((struct list_head *)v);
	961	+ ++*ppos;
	962	+ return lh == head ? NULL : lh;
	963	+}
	964	+EXPORT_SYMBOL(seq_list_next_rcu);
	965	+
934	966	/**
935	967	* seq_hlist_start - start an iteration of a hlist
936	968	* @head: the head of the hlist

include/linux/list.h+10−0

@@ -605,6 +605,16 @@ static inline void list_splice_tail_init(struct list_head *list, · 604 unmodified lines
605	605	#define list_for_each(pos, head) \
606	606	for (pos = (head)->next; !list_is_head(pos, (head)); pos = pos->next)
607	607
	608	+/**
	609	+ * list_for_each_rcu - Iterate over a list in an RCU-safe fashion
	610	+ * @pos: the &struct list_head to use as a loop cursor.
	611	+ * @head: the head for your list.
	612	+ */
	613	+#define list_for_each_rcu(pos, head) \
	614	+ for (pos = rcu_dereference((head)->next); \
	615	+ !list_is_head(pos, (head)); \
	616	+ pos = rcu_dereference(pos->next))
	617	+
608	618	/**
609	619	* list_for_each_continue - continue iteration over a list
610	620	* @pos: the &struct list_head to use as a loop cursor.

include/linux/seq_file.h+4−0

@@ -277,6 +277,10 @@ extern struct list_head seq_list_start_head(struct list_head head, · 276 unmodified lines
277	277	extern struct list_head seq_list_next(void v, struct list_head *head,
278	278	loff_t *ppos);
279	279
	280	+extern struct list_head seq_list_start_rcu(struct list_head head, loff_t pos);
	281	+extern struct list_head seq_list_start_head_rcu(struct list_head head, loff_t pos);
	282	+extern struct list_head seq_list_next_rcu(void v, struct list_head head, loff_t ppos);
	283	+
280	284	/*
281	285	* Helpers for iteration over hlist_head-s in seq_files
282	286	*/

net/rxrpc/ar-internal.h+1−1

@@ -60,7 +60,7 @@ struct rxrpc_net { · 59 unmodified lines
60	60	struct proc_dir_entry proc_net; / Subdir in /proc/net */
61	61	u32 epoch; /* Local epoch for detecting local-end reset */
62	62	struct list_head calls; /* List of calls active in this namespace */
63		− rwlock_t call_lock; /* Lock for ->calls */
	63	+ spinlock_t call_lock; /* Lock for ->calls */
64	64	atomic_t nr_calls; /* Count of allocated calls */
65	65
66	66	atomic_t nr_conns;

net/rxrpc/call_accept.c+3−3

@@ -140,9 +140,9 @@ static int rxrpc_service_prealloc_one(struct rxrpc_sock *rx, · 139 unmodified lines
140	140	write_unlock(&rx->call_lock);
141	141
142	142	rxnet = call->rxnet;
143		− write_lock(&rxnet->call_lock);
144		− list_add_tail(&call->link, &rxnet->calls);
145		− write_unlock(&rxnet->call_lock);
	143	+ spin_lock_bh(&rxnet->call_lock);
	144	+ list_add_tail_rcu(&call->link, &rxnet->calls);
	145	+ spin_unlock_bh(&rxnet->call_lock);
146	146
147	147	b->call_backlog[call_head] = call;
148	148	smp_store_release(&b->call_backlog_head, (call_head + 1) & (size - 1));

net/rxrpc/call_object.c+9−9

@@ -337,9 +337,9 @@ struct rxrpc_call rxrpc_new_client_call(struct rxrpc_sock rx, · 336 unmodified lines
337	337	write_unlock(&rx->call_lock);
338	338
339	339	rxnet = call->rxnet;
340		− write_lock(&rxnet->call_lock);
341		− list_add_tail(&call->link, &rxnet->calls);
342		− write_unlock(&rxnet->call_lock);
	340	+ spin_lock_bh(&rxnet->call_lock);
	341	+ list_add_tail_rcu(&call->link, &rxnet->calls);
	342	+ spin_unlock_bh(&rxnet->call_lock);
343	343
344	344	/* From this point on, the call is protected by its own lock. */
345	345	release_sock(&rx->sk);
@@ -633,9 +633,9 @@ void rxrpc_put_call(struct rxrpc_call *call, enum rxrpc_call_trace op) · 287 unmodified lines
633	633	ASSERTCMP(call->state, ==, RXRPC_CALL_COMPLETE);
634	634
635	635	if (!list_empty(&call->link)) {
636		− write_lock(&rxnet->call_lock);
	636	+ spin_lock_bh(&rxnet->call_lock);
637	637	list_del_init(&call->link);
638		− write_unlock(&rxnet->call_lock);
	638	+ spin_unlock_bh(&rxnet->call_lock);
639	639	}
640	640
641	641	rxrpc_cleanup_call(call);
@@ -707,7 +707,7 @@ void rxrpc_destroy_all_calls(struct rxrpc_net *rxnet) · 65 unmodified lines
707	707	_enter("");
708	708
709	709	if (!list_empty(&rxnet->calls)) {
710		− write_lock(&rxnet->call_lock);
	710	+ spin_lock_bh(&rxnet->call_lock);
711	711
712	712	while (!list_empty(&rxnet->calls)) {
713	713	call = list_entry(rxnet->calls.next,
@@ -722,12 +722,12 @@ void rxrpc_destroy_all_calls(struct rxrpc_net *rxnet) · 8 unmodified lines
722	722	rxrpc_call_states[call->state],
723	723	call->flags, call->events);
724	724
725		− write_unlock(&rxnet->call_lock);
	725	+ spin_unlock_bh(&rxnet->call_lock);
726	726	cond_resched();
727		− write_lock(&rxnet->call_lock);
	727	+ spin_lock_bh(&rxnet->call_lock);
728	728	}
729	729
730		− write_unlock(&rxnet->call_lock);
	730	+ spin_unlock_bh(&rxnet->call_lock);
731	731	}
732	732
733	733	atomic_dec(&rxnet->nr_calls);

net/rxrpc/net_ns.c+1−1

@@ -50,7 +50,7 @@ static __net_init int rxrpc_init_net(struct net *net) · 49 unmodified lines
50	50	rxnet->epoch \|= RXRPC_RANDOM_EPOCH;
51	51
52	52	INIT_LIST_HEAD(&rxnet->calls);
53		− rwlock_init(&rxnet->call_lock);
	53	+ spin_lock_init(&rxnet->call_lock);
54	54	atomic_set(&rxnet->nr_calls, 1);
55	55
56	56	atomic_set(&rxnet->nr_conns, 1);

net/rxrpc/proc.c+2−8

@@ -26,29 +26,23 @@ static const char *const rxrpc_conn_states[RXRPC_CONN__NR_STATES] = { · 25 unmodified lines
26	26	*/
27	27	static void rxrpc_call_seq_start(struct seq_file seq, loff_t *_pos)
28	28	__acquires(rcu)
29		− __acquires(rxnet->call_lock)
30	29	{
31	30	struct rxrpc_net *rxnet = rxrpc_net(seq_file_net(seq));
32	31
33	32	rcu_read_lock();
34		− read_lock(&rxnet->call_lock);
35		− return seq_list_start_head(&rxnet->calls, *_pos);
	33	+ return seq_list_start_head_rcu(&rxnet->calls, *_pos);
36	34	}
37	35
38	36	static void rxrpc_call_seq_next(struct seq_file seq, void v, loff_t pos)
39	37	{
40	38	struct rxrpc_net *rxnet = rxrpc_net(seq_file_net(seq));
41	39
42		− return seq_list_next(v, &rxnet->calls, pos);
	40	+ return seq_list_next_rcu(v, &rxnet->calls, pos);
43	41	}
44	42
45	43	static void rxrpc_call_seq_stop(struct seq_file seq, void v)
46		− __releases(rxnet->call_lock)
47	44	__releases(rcu)
48	45	{
49		− struct rxrpc_net *rxnet = rxrpc_net(seq_file_net(seq));
50		−
51		− read_unlock(&rxnet->call_lock);
52	46	rcu_read_unlock();
53	47	}
54	48

rxrpc: Fix locking issue

Keyboard shortcuts

General

Issues and pull requests

Issues, pull requests and Inbox

Repository code