tcp/dccp: fix possible race __inet_lookup_established()

Michal Kubecek and Firo Yang did a very nice analysis of crashes happening in __inet_lookup_established(). Since a TCP socket can go from TCP_ESTABLISH to TCP_LISTEN (via a close()/socket()/listen() cycle) without a RCU grace period, I should not have changed listeners linkage in their hash table. They must use the nulls protocol (Documentation/RCU/rculist_nulls.txt), so that a lookup can detect a socket in a hash list was moved in another one. Since we added code in commit d296ba60d8e2 ("soreuseport: Resolve merge conflict for v4/v6 ordering fix"), we have to add hlist_nulls_add_tail_rcu() helper.

Eric Dumazet committed 6 years ago

Commit 8dbd76e1 parent 8f9cc1e6 files · +65 −15Browse files

Unified · Split

6 files

include/linux/rculist_nulls.h +37 −0
include/net/inet_hashtables.h +9 −3
include/net/sock.h +5 −0
net/ipv4/inet_diag.c +2 −1
net/ipv4/inet_hashtables.c +8 −8
net/ipv4/tcp_ipv4.c +4 −3

include/linux/rculist_nulls.h+37−0

@@ -100,6 +100,43 @@ static inline void hlist_nulls_add_head_rcu(struct hlist_nulls_node *n, · 99 unmodified lines
100	first->pprev = &n->next;	100	first->pprev = &n->next;
101	}	101	}
102		102
		103	/**
		104	* hlist_nulls_add_tail_rcu
		105	* @n: the element to add to the hash list.
		106	* @h: the list to add to.
		107	*
		108	* Description:
		109	* Adds the specified element to the specified hlist_nulls,
		110	* while permitting racing traversals.
		111	*
		112	* The caller must take whatever precautions are necessary
		113	* (such as holding appropriate locks) to avoid racing
		114	* with another list-mutation primitive, such as hlist_nulls_add_head_rcu()
		115	* or hlist_nulls_del_rcu(), running on this same list.
		116	* However, it is perfectly legal to run concurrently with
		117	* the _rcu list-traversal primitives, such as
		118	* hlist_nulls_for_each_entry_rcu(), used to prevent memory-consistency
		119	* problems on Alpha CPUs. Regardless of the type of CPU, the
		120	* list-traversal primitive must be guarded by rcu_read_lock().
		121	*/
		122	static inline void hlist_nulls_add_tail_rcu(struct hlist_nulls_node *n,
		123	struct hlist_nulls_head *h)
		124	{
		125	struct hlist_nulls_node i, last = NULL;
		126
		127	/* Note: write side code, so rcu accessors are not needed. */
		128	for (i = h->first; !is_a_nulls(i); i = i->next)
		129	last = i;
		130
		131	if (last) {
		132	n->next = last->next;
		133	n->pprev = &last->next;
		134	rcu_assign_pointer(hlist_next_rcu(last), n);
		135	} else {
		136	hlist_nulls_add_head_rcu(n, h);
		137	}
		138	}
		139
103	/**	140	/**
104	* hlist_nulls_for_each_entry_rcu - iterate over rcu list of given type	141	* hlist_nulls_for_each_entry_rcu - iterate over rcu list of given type
105	* @tpos: the type * to use as a loop cursor.	142	* @tpos: the type * to use as a loop cursor.

include/net/inet_hashtables.h+9−3

 	struct hlist_head	chain;
 };
-/*
+/* Sockets can be hashed in established or listening table.
- * Sockets can be hashed in established or listening table
+ * We must use different 'nulls' end-of-chain value for all hash buckets :
+ * A socket might transition from ESTABLISH to LISTEN state without
+ * RCU grace period. A lookup in ehash table needs to handle this case.
  */
+#define LISTENING_NULLS_BASE (1U << 29)
 struct inet_listen_hashbucket {
 	spinlock_t		lock;
 	unsigned int		count;
-	struct hlist_head	head;
+	union {
+		struct hlist_head	head;
+		struct hlist_nulls_head	nulls_head;
+	};
 };
 /* This is for listening sockets, thus all sockets which possess wildcards. */

include/net/sock.h+5−0

 	hlist_nulls_add_head_rcu(&sk->sk_nulls_node, list);
 }
+static inline void __sk_nulls_add_node_tail_rcu(struct sock *sk, struct hlist_nulls_head *list)
+{
+	hlist_nulls_add_tail_rcu(&sk->sk_nulls_node, list);
+}
 static inline void sk_nulls_add_node_rcu(struct sock *sk, struct hlist_nulls_head *list)
 {
 	sock_hold(sk);

net/ipv4/inet_diag.c+2−1

 		for (i = s_i; i < INET_LHTABLE_SIZE; i++) {
 			struct inet_listen_hashbucket *ilb;
+			struct hlist_nulls_node *node;
 			num = 0;
 			ilb = &hashinfo->listening_hash[i];
 			spin_lock(&ilb->lock);
-			sk_for_each(sk, &ilb->head) {
+			sk_nulls_for_each(sk, node, &ilb->nulls_head) {
 				struct inet_sock *inet = inet_sk(sk);
 				if (!net_eq(sock_net(sk), net))

net/ipv4/inet_hashtables.c+8−8

 				   struct inet_listen_hashbucket *ilb)
 {
 	struct inet_bind_bucket *tb = inet_csk(sk)->icsk_bind_hash;
+	const struct hlist_nulls_node *node;
 	struct sock *sk2;
 	kuid_t uid = sock_i_uid(sk);
-	sk_for_each_rcu(sk2, &ilb->head) {
+	sk_nulls_for_each_rcu(sk2, node, &ilb->nulls_head) {
 		if (sk2 != sk &&
 		    sk2->sk_family == sk->sk_family &&
 		    ipv6_only_sock(sk2) == ipv6_only_sock(sk) &&
 	}
 	if (IS_ENABLED(CONFIG_IPV6) && sk->sk_reuseport &&
 		sk->sk_family == AF_INET6)
-		hlist_add_tail_rcu(&sk->sk_node, &ilb->head);
+		__sk_nulls_add_node_tail_rcu(sk, &ilb->nulls_head);
 	else
-		hlist_add_head_rcu(&sk->sk_node, &ilb->head);
+		__sk_nulls_add_node_rcu(sk, &ilb->nulls_head);
 	inet_hash2(hashinfo, sk);
 	ilb->count++;
 	sock_set_flag(sk, SOCK_RCU_FREE);
 		reuseport_detach_sock(sk);
 	if (ilb) {
 		inet_unhash2(hashinfo, sk);
-		 __sk_del_node_init(sk);
+		ilb->count--;
-		 ilb->count--;
-	} else {
-		__sk_nulls_del_node_init_rcu(sk);
 	}
+	__sk_nulls_del_node_init_rcu(sk);
 	sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
 unlock:
 	spin_unlock_bh(lock);
 	for (i = 0; i < INET_LHTABLE_SIZE; i++) {
 		spin_lock_init(&h->listening_hash[i].lock);
-		INIT_HLIST_HEAD(&h->listening_hash[i].head);
+		INIT_HLIST_NULLS_HEAD(&h->listening_hash[i].nulls_head,
+				      i + LISTENING_NULLS_BASE);
 		h->listening_hash[i].count = 0;
 	}

net/ipv4/tcp_ipv4.c+4−3

 	struct tcp_iter_state *st = seq->private;
 	struct net *net = seq_file_net(seq);
 	struct inet_listen_hashbucket *ilb;
+	struct hlist_nulls_node *node;
 	struct sock *sk = cur;
 	if (!sk) {
 get_head:
 		ilb = &tcp_hashinfo.listening_hash[st->bucket];
 		spin_lock(&ilb->lock);
-		sk = sk_head(&ilb->head);
+		sk = sk_nulls_head(&ilb->nulls_head);
 		st->offset = 0;
 		goto get_sk;
 	}
 	++st->num;
 	++st->offset;
-	sk = sk_next(sk);
+	sk = sk_nulls_next(sk);
 get_sk:
-	sk_for_each_from(sk) {
+	sk_nulls_for_each_from(sk, node) {
 		if (!net_eq(sock_net(sk), net))
 			continue;
 		if (sk->sk_family == afinfo->family)

tcp/dccp: fix possible race __inet_lookup_established()

Keyboard shortcuts

General

Issues and pull requests

Issues, pull requests and Inbox

Repository code