net: Set tw refcount before dropping the ehash chain lock.
Subsystems (3)
PATCH 1/1 net: Set tw refcount before dropping the ehash chain lock.
This protects against callers like inet_diag_dump_icsk(), which may walk the chain on another cpu and change the refcount before the tw structure is ready.
--- net/ipv4/inet_timewait_sock.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
net/ipv4/inet_timewait_sock.c +2 -2
@@ -125,8 +125,6 @@ void inet_twsk_hashdance(struct inet_timewait_sock *tw, struct sock *sk,if (__sk_nulls_del_node_init_rcu (sk))sock_prot_inuse_add (sock_net (sk), sk->sk_prot,-1 ); -spin_unlock (lock); -/* tw_refcnt is set to 3 because we have : * - one reference for bhash chain. * - one reference for ehash chain.
@@ -137,6 +135,8 @@ void inet_twsk_hashdance(struct inet_timewait_sock *tw, struct sock *sk,* so we are not allowed to use tw anymore. */ refcount_set (&tw->tw_refcnt,3 ); + +spin_unlock (lock); }EXPORT_SYMBOL_GPL (inet_twsk_hashdance);
Signature · 1 line
2.17.1
Quoted 23 lines · Jonathan Lemon
This protects against callers like
inet_diag_dump_icsk(), which may walk the chain on another cpu and change the refcount before the tw structure is ready.Signed-off-by Jonathan Lemon <[email protected]>--- net/ipv4/inet_timewait_sock.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)net/ipv4/inet_timewait_sock.c +0
-2@@ -125,8 +125,6 @@ void inet_twsk_hashdance(struct inet_timewait_sock *tw, struct sock *sk,if (__sk_nulls_del_node_init_rcu (sk))sock_prot_inuse_add (sock_net (sk), sk->sk_prot,-1 ); -spin_unlock (lock); -/* tw_refcnt is set to 3 because we have : * - one reference for bhash chain. * - one reference for ehash chain.
net/ipv4/inet_timewait_sock.c void inet_twsk_hashdance(struct inet_timewait_sock *tw, struct sock *sk, · line 139
Quoted 5 lines
net/ipv4/inet_timewait_sock.c +2
-0@@ -137,6 +135,8 @@ void inet_twsk_hashdance(struct inet_timewait_sock *tw, struct sock *sk,* so we are not allowed to use tw anymore. */ refcount_set (&tw->tw_refcnt,3 ); + +spin_unlock (lock);
Hi Jonathan
Nice catch, but this patch is not correct.
We need to make inet_diag_dump_icsk() more robust, otherwise we would have to change other points in the stack (not only for TIMEWAIT sockets), and that is a bit too risky
in term of locking dependencies.
Please try the following fix instead :
tcp_get_info()")
net/ipv4/inet_diag.c +0 -0
100644
net/ipv4/inet_diag.c +0 -0
@@ -998,7 +998,9 @@ void inet_diag_dump_icsk(struct inet_hashinfo
*hashinfo, struct sk_buff *skb,
if (!inet_diag_bc_sk(bc, sk))
goto next_normal;- sock_hold(sk);
+ if (!refcount_inc_not_zero(&sk->sk_refcnt))
+ goto next_normal;
+
num_arr[accum] = num;
sk_arr[accum] = sk;
if (++accum == SKARR_SZ)Quoted 64 lines · Eric Dumazet
Quoted 24 lines · Jonathan Lemon
This protects against callers like
inet_diag_dump_icsk(), which may walk the chain on another cpu and change the refcount before the tw structure is ready.Signed-off-by Jonathan Lemon <[email protected]>--- net/ipv4/inet_timewait_sock.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)net/ipv4/inet_timewait_sock.c +0
-2@@ -125,8 +125,6 @@ void inet_twsk_hashdance(struct inet_timewait_sock *tw, struct sock *sk, if (__sk_nulls_del_node_init_rcu(sk)) sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1); -spin_unlock (lock ); -/* tw_refcnt is set to 3 because we have : * - one reference for bhash chain. * - one reference for ehash chain.
net/ipv4/inet_timewait_sock.c void inet_twsk_hashdance(struct · line 140
Quoted 6 lines
net/ipv4/inet_timewait_sock.c +2
-0@@ -137,6 +135,8 @@ void inet_twsk_hashdance(structinet_timewait_sock *tw, struct sock *sk, * so we are not allowed to use tw anymore. */ refcount_set (&tw->tw_refcnt,3 ); + +spin_unlock (lock);Hi Jonathan
Nice catch, but this patch is not correct.
We need to make
inet_diag_dump_icsk()more robust, otherwise we would have to change other points in the stack (not only for TIMEWAIT sockets), and that is a bit too risky
in term of locking dependencies.Please try the following fix instead :
Fixes 67db3e4bfbc9 ("tcp: no longer hold ehash lock while calling
tcp_get_info()")net/ipv4/inet_diag.c +0
-0100644
net/ipv4/inet_diag.c +3
-1@@ -998,7 +998,9 @@ void inet_diag_dump_icsk(struct inet_hashinfo *hashinfo, struct sk_buff *skb, if (!inet_diag_bc_sk(bc, sk)) goto next_normal; -sock_hold (sk); +if (!refcount_inc_not_zero (&sk->sk_refcnt)) + goto next_normal; + num_arr[accum] = num; sk_arr[accum] = sk;
Quoted 1 line
if (++accum == SKARR_SZ)
This works too; I was considering doing it this way, but my initial approach was a simple fix for ec94c2696f0b (forgot to add Fixes: tag).
Signature · 1 line
Jonathan
Quoted 6 lines · Jonathan Lemon
This works too; I was considering doing it this way, but my initial approach
was a simple fix forec94c2696f0b(forgot to add Fixes: tag).
What about other kinds of sockets ? SYN_RECV and ESTABLISHED ones ?
Your patch focused on the immediate problem you had, but the problem was generic.
Anyway, thanks a lot for your help !
Jonathan Lemon
Acked-byQuoted 64 lines · Eric Dumazet
Quoted 24 lines · Jonathan Lemon
This protects against callers like
inet_diag_dump_icsk(), which may walk the chain on another cpu and change the refcount before the tw structure is ready.Signed-off-by Jonathan Lemon <[email protected]>--- net/ipv4/inet_timewait_sock.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)net/ipv4/inet_timewait_sock.c +0
-2@@ -125,8 +125,6 @@ void inet_twsk_hashdance(struct inet_timewait_sock *tw, struct sock *sk, if (__sk_nulls_del_node_init_rcu(sk)) sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1); -spin_unlock (lock ); -/* tw_refcnt is set to 3 because we have : * - one reference for bhash chain. * - one reference for ehash chain.
net/ipv4/inet_timewait_sock.c void inet_twsk_hashdance(struct · line 140
Quoted 6 lines
net/ipv4/inet_timewait_sock.c +2
-0@@ -137,6 +135,8 @@ void inet_twsk_hashdance(structinet_timewait_sock *tw, struct sock *sk, * so we are not allowed to use tw anymore. */ refcount_set (&tw->tw_refcnt,3 ); + +spin_unlock (lock);Hi Jonathan
Nice catch, but this patch is not correct.
We need to make
inet_diag_dump_icsk()more robust, otherwise we would have to change other points in the stack (not only for TIMEWAIT sockets), and that is a bit too risky
in term of locking dependencies.Please try the following fix instead :
Fixes 67db3e4bfbc9 ("tcp: no longer hold ehash lock while calling
tcp_get_info()")net/ipv4/inet_diag.c +0
-0100644
net/ipv4/inet_diag.c +3
-1@@ -998,7 +998,9 @@ void inet_diag_dump_icsk(struct inet_hashinfo *hashinfo, struct sk_buff *skb, if (!inet_diag_bc_sk(bc, sk)) goto next_normal; -sock_hold (sk); +if (!refcount_inc_not_zero (&sk->sk_refcnt)) + goto next_normal; + num_arr[accum] = num; sk_arr[accum] = sk;
Quoted 1 line
if (++accum == SKARR_SZ)