* [PATCH net-next] net: Set tw refcount before dropping the ehash chain lock.
@ 2018-12-20 20:59 Jonathan Lemon
2018-12-20 22:47 ` Eric Dumazet
0 siblings, 1 reply; 5+ messages in thread
From: Jonathan Lemon @ 2018-12-20 20:59 UTC (permalink / raw)
To: netdev; +Cc: davem, edumazet, kernel-team, 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 <jonathan.lemon@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);
}
EXPORT_SYMBOL_GPL(inet_twsk_hashdance);
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net: Set tw refcount before dropping the ehash chain lock.
2018-12-20 20:59 [PATCH net-next] net: Set tw refcount before dropping the ehash chain lock Jonathan Lemon
@ 2018-12-20 22:47 ` Eric Dumazet
2018-12-20 23:05 ` Jonathan Lemon
2018-12-21 0:40 ` Jonathan Lemon
0 siblings, 2 replies; 5+ messages in thread
From: Eric Dumazet @ 2018-12-20 22:47 UTC (permalink / raw)
To: jonathan.lemon; +Cc: netdev, David Miller, kernel-team
On Thu, Dec 20, 2018 at 12:59 PM Jonathan Lemon
<jonathan.lemon@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.lemon@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)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net: Set tw refcount before dropping the ehash chain lock.
2018-12-20 22:47 ` Eric Dumazet
@ 2018-12-20 23:05 ` Jonathan Lemon
2018-12-20 23:10 ` Eric Dumazet
2018-12-21 0:40 ` Jonathan Lemon
1 sibling, 1 reply; 5+ messages in thread
From: Jonathan Lemon @ 2018-12-20 23:05 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, David Miller, kernel-team
On 20 Dec 2018, at 14:47, Eric Dumazet wrote:
> On Thu, Dec 20, 2018 at 12:59 PM Jonathan Lemon
> <jonathan.lemon@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.lemon@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)
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net: Set tw refcount before dropping the ehash chain lock.
2018-12-20 23:05 ` Jonathan Lemon
@ 2018-12-20 23:10 ` Eric Dumazet
0 siblings, 0 replies; 5+ messages in thread
From: Eric Dumazet @ 2018-12-20 23:10 UTC (permalink / raw)
To: jonathan.lemon; +Cc: netdev, David Miller, kernel-team
On Thu, Dec 20, 2018 at 3:05 PM Jonathan Lemon <jonathan.lemon@gmail.com> wrote:
>
> 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).
>
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 !
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net-next] net: Set tw refcount before dropping the ehash chain lock.
2018-12-20 22:47 ` Eric Dumazet
2018-12-20 23:05 ` Jonathan Lemon
@ 2018-12-21 0:40 ` Jonathan Lemon
1 sibling, 0 replies; 5+ messages in thread
From: Jonathan Lemon @ 2018-12-21 0:40 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, David Miller, kernel-team
On 20 Dec 2018, at 14:47, Eric Dumazet wrote:
> On Thu, Dec 20, 2018 at 12:59 PM Jonathan Lemon
> <jonathan.lemon@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.lemon@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.lemon@gmail.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-12-21 0:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-20 20:59 [PATCH net-next] net: Set tw refcount before dropping the ehash chain lock Jonathan Lemon
2018-12-20 22:47 ` Eric Dumazet
2018-12-20 23:05 ` Jonathan Lemon
2018-12-20 23:10 ` Eric Dumazet
2018-12-21 0:40 ` Jonathan Lemon
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.