netdev - Re: [PATCH net 1/2] RDS:TCP: Synchronize rds_tcp_accept_one with rds_send_xmit when resetting t

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]

Date:	Mon, 2 May 2016 09:20:05 -0700
From:	Santosh Shilimkar <santosh.shilimkar@...cle.com>
To:	Sowmini Varadhan <sowmini.varadhan@...cle.com>,
	netdev@...r.kernel.org, rds-devel@....oracle.com
Cc:	davem@...emloft.net
Subject: Re: [PATCH net 1/2] RDS:TCP: Synchronize rds_tcp_accept_one with
 rds_send_xmit when resetting t_sock

On 5/1/2016 4:10 PM, Sowmini Varadhan wrote:
> There is a race condition between rds_send_xmit -> rds_tcp_xmit
> and the code that deals with resolution of duelling syns added
> by commit 241b271952eb ("RDS-TCP: Reset tcp callbacks if re-using an
> outgoing socket in rds_tcp_accept_one()").
>
> Specifically, we may end up derefencing a null pointer in rds_send_xmit
> if we have the interleaving sequencee:
>            rds_tcp_accept_one                  rds_send_xmit
>
>                                              conn is RDS_CONN_UP, so
>     					 invoke rds_tcp_xmit
>
>                                              tc = conn->c_transport_data
>         rds_tcp_restore_callbacks
>             /* reset t_sock */
>     					 null ptr deref from tc->t_sock
>
> The race condition can be avoided without adding the overhead of
> additional locking in the xmit path: have rds_tcp_accept_one wait
> for rds_tcp_xmit threads to complete before resetting callbacks.
> The synchronization can be done in the same manner as rds_conn_shutdown().
> First set the rds_conn_state to something other than RDS_CONN_UP
> (so that new threads cannot get into rds_tcp_xmit()), then wait for
> RDS_IN_XMIT to be cleared in the conn->c_flags indicating that any
> threads in rds_tcp_xmit are done.
>
> Fixes: 241b271952eb ("RDS-TCP: Reset tcp callbacks if re-using an
> outgoing socket in rds_tcp_accept_one()")
>
> Signed-off-by: Sowmini Varadhan <sowmini.varadhan@...cle.com>
> ---
Mostly looks correct. A question below.

>  net/rds/tcp.c        |    2 +-
>  net/rds/tcp_listen.c |   40 ++++++++++++++++++++++++----------------
>  2 files changed, 25 insertions(+), 17 deletions(-)
>
> diff --git a/net/rds/tcp.c b/net/rds/tcp.c
> index 61ed2a8..9134544 100644
> --- a/net/rds/tcp.c
> +++ b/net/rds/tcp.c
> @@ -127,7 +127,7 @@ void rds_tcp_restore_callbacks(struct socket *sock,
>
>  /*
>   * This is the only path that sets tc->t_sock.  Send and receive trust that
> - * it is set.  The RDS_CONN_CONNECTED bit protects those paths from being
> + * it is set.  The RDS_CONN_UP bit protects those paths from being
>   * called while it isn't set.
>   */
>  void rds_tcp_set_callbacks(struct socket *sock, struct rds_connection *conn)
> diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
> index 0936a4a..0896187 100644
> --- a/net/rds/tcp_listen.c
> +++ b/net/rds/tcp_listen.c
> @@ -115,24 +115,32 @@ int rds_tcp_accept_one(struct socket *sock)
>  	 * rds_tcp_state_change() will do that cleanup
>  	 */
>  	rs_tcp = (struct rds_tcp_connection *)conn->c_transport_data;
> -	if (rs_tcp->t_sock &&
> -	    ntohl(inet->inet_saddr) < ntohl(inet->inet_daddr)) {
> -		struct sock *nsk = new_sock->sk;
> -
> -		nsk->sk_user_data = NULL;
> -		nsk->sk_prot->disconnect(nsk, 0);
> -		tcp_done(nsk);
> -		new_sock = NULL;
> -		ret = 0;
> -		goto out;
> -	} else if (rs_tcp->t_sock) {
> -		rds_tcp_restore_callbacks(rs_tcp->t_sock, rs_tcp);
> -		conn->c_outgoing = 0;
> -	}
> -
>  	rds_conn_transition(conn, RDS_CONN_DOWN, RDS_CONN_CONNECTING);
> +	if (rs_tcp->t_sock) {
> +		/* Need to resolve a duelling SYN between peers.
> +		 * We have an outstanding SYN to this peer, which may
> +		 * potentially have transitioned to the RDS_CONN_UP state,
> +		 * so we must quiesce any send threads before resetting
> +		 * c_transport_data.
> +		 */
> +		wait_event(conn->c_waitq,
> +			   !test_bit(RDS_IN_XMIT, &conn->c_flags));
Would it be good to check the return value of rds_conn_transition()
since if CONN is already UP above will fail and then send message
might again race and we will let message through even though passive
hasn't finished its connection.

> +		if (ntohl(inet->inet_saddr) < ntohl(inet->inet_daddr)) {
> +			struct sock *nsk = new_sock->sk;
> +
> +			nsk->sk_user_data = NULL;
> +			nsk->sk_prot->disconnect(nsk, 0);
> +			tcp_done(nsk);
> +			new_sock = NULL;
> +			ret = 0;
> +			goto out;
> +		} else if (rs_tcp->t_sock) {
> +			rds_tcp_restore_callbacks(rs_tcp->t_sock, rs_tcp);
> +			conn->c_outgoing = 0;
> +		}
> +	}
>  	rds_tcp_set_callbacks(new_sock, conn);
> -	rds_connect_complete(conn);
> +	rds_connect_complete(conn); /* marks RDS_CONN_UP */
>  	new_sock = NULL;
>  	ret = 0;
>
>