Skip to content

Commit 8dbd76e

Browse files
Eric DumazetJakub Kicinski
authored andcommitted
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 d296ba6 ("soreuseport: Resolve merge conflict for v4/v6 ordering fix"), we have to add hlist_nulls_add_tail_rcu() helper. Fixes: 3b24d85 ("tcp/dccp: do not touch listener sk_refcnt under synflood") Signed-off-by: Eric Dumazet <[email protected]> Reported-by: Michal Kubecek <[email protected]> Reported-by: Firo Yang <[email protected]> Reviewed-by: Michal Kubecek <[email protected]> Link: https://lore.kernel.org/netdev/[email protected]/ Signed-off-by: Jakub Kicinski <[email protected]>
1 parent 8f9cc1e commit 8dbd76e

File tree

6 files changed

+65
-15
lines changed

6 files changed

+65
-15
lines changed

include/linux/rculist_nulls.h

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,43 @@ static inline void hlist_nulls_add_head_rcu(struct hlist_nulls_node *n,
100100
first->pprev = &n->next;
101101
}
102102

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+
103140
/**
104141
* hlist_nulls_for_each_entry_rcu - iterate over rcu list of given type
105142
* @tpos: the type * to use as a loop cursor.

include/net/inet_hashtables.h

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -103,13 +103,19 @@ struct inet_bind_hashbucket {
103103
struct hlist_head chain;
104104
};
105105

106-
/*
107-
* Sockets can be hashed in established or listening table
106+
/* Sockets can be hashed in established or listening table.
107+
* We must use different 'nulls' end-of-chain value for all hash buckets :
108+
* A socket might transition from ESTABLISH to LISTEN state without
109+
* RCU grace period. A lookup in ehash table needs to handle this case.
108110
*/
111+
#define LISTENING_NULLS_BASE (1U << 29)
109112
struct inet_listen_hashbucket {
110113
spinlock_t lock;
111114
unsigned int count;
112-
struct hlist_head head;
115+
union {
116+
struct hlist_head head;
117+
struct hlist_nulls_head nulls_head;
118+
};
113119
};
114120

115121
/* This is for listening sockets, thus all sockets which possess wildcards. */

include/net/sock.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -722,6 +722,11 @@ static inline void __sk_nulls_add_node_rcu(struct sock *sk, struct hlist_nulls_h
722722
hlist_nulls_add_head_rcu(&sk->sk_nulls_node, list);
723723
}
724724

725+
static inline void __sk_nulls_add_node_tail_rcu(struct sock *sk, struct hlist_nulls_head *list)
726+
{
727+
hlist_nulls_add_tail_rcu(&sk->sk_nulls_node, list);
728+
}
729+
725730
static inline void sk_nulls_add_node_rcu(struct sock *sk, struct hlist_nulls_head *list)
726731
{
727732
sock_hold(sk);

net/ipv4/inet_diag.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -911,11 +911,12 @@ void inet_diag_dump_icsk(struct inet_hashinfo *hashinfo, struct sk_buff *skb,
911911

912912
for (i = s_i; i < INET_LHTABLE_SIZE; i++) {
913913
struct inet_listen_hashbucket *ilb;
914+
struct hlist_nulls_node *node;
914915

915916
num = 0;
916917
ilb = &hashinfo->listening_hash[i];
917918
spin_lock(&ilb->lock);
918-
sk_for_each(sk, &ilb->head) {
919+
sk_nulls_for_each(sk, node, &ilb->nulls_head) {
919920
struct inet_sock *inet = inet_sk(sk);
920921

921922
if (!net_eq(sock_net(sk), net))

net/ipv4/inet_hashtables.c

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -516,10 +516,11 @@ static int inet_reuseport_add_sock(struct sock *sk,
516516
struct inet_listen_hashbucket *ilb)
517517
{
518518
struct inet_bind_bucket *tb = inet_csk(sk)->icsk_bind_hash;
519+
const struct hlist_nulls_node *node;
519520
struct sock *sk2;
520521
kuid_t uid = sock_i_uid(sk);
521522

522-
sk_for_each_rcu(sk2, &ilb->head) {
523+
sk_nulls_for_each_rcu(sk2, node, &ilb->nulls_head) {
523524
if (sk2 != sk &&
524525
sk2->sk_family == sk->sk_family &&
525526
ipv6_only_sock(sk2) == ipv6_only_sock(sk) &&
@@ -555,9 +556,9 @@ int __inet_hash(struct sock *sk, struct sock *osk)
555556
}
556557
if (IS_ENABLED(CONFIG_IPV6) && sk->sk_reuseport &&
557558
sk->sk_family == AF_INET6)
558-
hlist_add_tail_rcu(&sk->sk_node, &ilb->head);
559+
__sk_nulls_add_node_tail_rcu(sk, &ilb->nulls_head);
559560
else
560-
hlist_add_head_rcu(&sk->sk_node, &ilb->head);
561+
__sk_nulls_add_node_rcu(sk, &ilb->nulls_head);
561562
inet_hash2(hashinfo, sk);
562563
ilb->count++;
563564
sock_set_flag(sk, SOCK_RCU_FREE);
@@ -606,11 +607,9 @@ void inet_unhash(struct sock *sk)
606607
reuseport_detach_sock(sk);
607608
if (ilb) {
608609
inet_unhash2(hashinfo, sk);
609-
__sk_del_node_init(sk);
610-
ilb->count--;
611-
} else {
612-
__sk_nulls_del_node_init_rcu(sk);
610+
ilb->count--;
613611
}
612+
__sk_nulls_del_node_init_rcu(sk);
614613
sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);
615614
unlock:
616615
spin_unlock_bh(lock);
@@ -750,7 +749,8 @@ void inet_hashinfo_init(struct inet_hashinfo *h)
750749

751750
for (i = 0; i < INET_LHTABLE_SIZE; i++) {
752751
spin_lock_init(&h->listening_hash[i].lock);
753-
INIT_HLIST_HEAD(&h->listening_hash[i].head);
752+
INIT_HLIST_NULLS_HEAD(&h->listening_hash[i].nulls_head,
753+
i + LISTENING_NULLS_BASE);
754754
h->listening_hash[i].count = 0;
755755
}
756756

net/ipv4/tcp_ipv4.c

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2147,23 +2147,24 @@ static void *listening_get_next(struct seq_file *seq, void *cur)
21472147
struct tcp_iter_state *st = seq->private;
21482148
struct net *net = seq_file_net(seq);
21492149
struct inet_listen_hashbucket *ilb;
2150+
struct hlist_nulls_node *node;
21502151
struct sock *sk = cur;
21512152

21522153
if (!sk) {
21532154
get_head:
21542155
ilb = &tcp_hashinfo.listening_hash[st->bucket];
21552156
spin_lock(&ilb->lock);
2156-
sk = sk_head(&ilb->head);
2157+
sk = sk_nulls_head(&ilb->nulls_head);
21572158
st->offset = 0;
21582159
goto get_sk;
21592160
}
21602161
ilb = &tcp_hashinfo.listening_hash[st->bucket];
21612162
++st->num;
21622163
++st->offset;
21632164

2164-
sk = sk_next(sk);
2165+
sk = sk_nulls_next(sk);
21652166
get_sk:
2166-
sk_for_each_from(sk) {
2167+
sk_nulls_for_each_from(sk, node) {
21672168
if (!net_eq(sock_net(sk), net))
21682169
continue;
21692170
if (sk->sk_family == afinfo->family)

0 commit comments

Comments
 (0)