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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8E67B159-BFC9-4590-8184-B3922B5227D8@gmail.com>
Date:   Thu, 20 Dec 2018 15:05:43 -0800
From:   "Jonathan Lemon" <jonathan.lemon@...il.com>
To:     "Eric Dumazet" <edumazet@...gle.com>
Cc:     netdev <netdev@...r.kernel.org>,
        "David Miller" <davem@...emloft.net>,
        kernel-team <kernel-team@...com>
Subject: Re: [PATCH net-next] net: Set tw refcount before dropping the ehash
 chain lock.



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

> On Thu, Dec 20, 2018 at 12:59 PM Jonathan Lemon
> <jonathan.lemon@...il.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.lemon@...il.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)


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).

-- 
Jonathan