Re: [PATCH net-next] net: Set tw refcount before dropping the ehash chain lock.

[Jonathan Lemon]

On 20 Dec 2018, at 14:47, Eric Dumazet wrote:

On Thu, Dec 20, 2018 at 12:59 PM Jonathan Lemon
<jonathan.le...@gmail.com> wrote:

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 <jonathan.le...@gmail.com>
---
 net/ipv4/inet_timewait_sock.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/inet_timewait_sock.c b/net/ipv4/inet_timewait_sock.c

index 88c5069b5d20..128cfcada5e6 100644
--- a/net/ipv4/inet_timewait_sock.c
+++ b/net/ipv4/inet_timewait_sock.c

@@ -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);

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

diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c

index 4e5bc4b2f14e6786ceb7d63e5902f8fc17819dfa..1a4e9ff02762ed757545da13de1ee352f38c867b

100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -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)

Acked-by: Jonathan Lemon <jonathan.le...@gmail.com>