ACK: [SRU][Bionic][Cosmic][PATCH 1/1] tcp: do not release socket ownership in tcp_close()

[Connor Kuehl]

On 5/28/19 5:22 PM, Ioanna Alifieraki wrote:
> From: Eric Dumazet <edumazet at google.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1830813
> 
> syzkaller was able to hit the WARN_ON(sock_owned_by_user(sk));
> in tcp_close()
> 
> While a socket is being closed, it is very possible other
> threads find it in rtnetlink dump.
> 
> tcp_get_info() will acquire the socket lock for a short amount
> of time (slow = lock_sock_fast(sk)/unlock_sock_fast(sk, slow);),
> enough to trigger the warning.
> 
> Fixes: 67db3e4bfbc9 ("tcp: no longer hold ehash lock while calling tcp_get_info()")
> Signed-off-by: Eric Dumazet <edumazet at google.com>
> Reported-by: syzbot <syzkaller at googlegroups.com>
> Signed-off-by: David S. Miller <davem at davemloft.net>
> (cherry picked from commit 8873c064d1de579ea23412a6d3eee972593f142b)
> Signed-off-by: Ioanna Alifieraki <ioanna-maria.alifieraki at canonical.com>

Acked-by: Connor Kuehl <connor.kuehl at canonical.com>

> ---
>  include/net/sock.h |  1 +
>  net/core/sock.c    |  2 +-
>  net/ipv4/tcp.c     | 11 +++--------
>  3 files changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 7a7b14e9628a..5fcdd5b7b47c 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1461,6 +1461,7 @@ static inline void lock_sock(struct sock *sk)
>  	lock_sock_nested(sk, 0);
>  }
>  
> +void __release_sock(struct sock *sk);
>  void release_sock(struct sock *sk);
>  
>  /* BH context may only use the following locking interface. */
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 1039cb1fdcff..050dba776f75 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2242,7 +2242,7 @@ static void __lock_sock(struct sock *sk)
>  	finish_wait(&sk->sk_lock.wq, &wait);
>  }
>  
> -static void __release_sock(struct sock *sk)
> +void __release_sock(struct sock *sk)
>  	__releases(&sk->sk_lock.slock)
>  	__acquires(&sk->sk_lock.slock)
>  {
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index ba6255d934a4..8a9ea72c82fa 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -2242,16 +2242,10 @@ void tcp_close(struct sock *sk, long timeout)
>  	sock_hold(sk);
>  	sock_orphan(sk);
>  
> -	/* It is the last release_sock in its life. It will remove backlog. */
> -	release_sock(sk);
> -
> -
> -	/* Now socket is owned by kernel and we acquire BH lock
> -	 *  to finish close. No need to check for user refs.
> -	 */
>  	local_bh_disable();
>  	bh_lock_sock(sk);
> -	WARN_ON(sock_owned_by_user(sk));
> +	/* remove backlog if any, without releasing ownership. */
> +	__release_sock(sk);
>  
>  	percpu_counter_inc(sk->sk_prot->orphan_count);
>  
> @@ -2320,6 +2314,7 @@ void tcp_close(struct sock *sk, long timeout)
>  out:
>  	bh_unlock_sock(sk);
>  	local_bh_enable();
> +	release_sock(sk);
>  	sock_put(sk);
>  }
>  EXPORT_SYMBOL(tcp_close);
> 


-- 
Connor
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pEpkey.asc
Type: application/pgp-keys
Size: 5950 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20190530/8fe68365/attachment-0001.key>