netdev - Re: [PATCH net-next 1/3] net/rds: Use a single TCP socket for both send and receive.

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:	Wed, 30 Sep 2015 08:50:02 -0700
From:	santosh shilimkar <santosh.shilimkar@...cle.com>
To:	Sowmini Varadhan <sowmini.varadhan@...cle.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:	davem@...emloft.net, rds-devel@....oracle.com,
	ajaykumar.hotchandani@...cle.com, igor.maximov@...cle.com
Subject: Re: [PATCH net-next 1/3] net/rds: Use a single TCP socket for both
 send and receive.

minor nit though not a strict rule. Just to be consistent based on
what we are following.

- core RDS patches "RDS:"
- RDS IB patches "RDS: IB:" or "RDS/IB:"
- RDS IW patches "RDS: IW:" or
- RDS TCP can use "RDS: TCP" or "RDS/TCP:"

$subject
s/net/rds:/RDS:

On 9/30/2015 6:45 AM, Sowmini Varadhan wrote:
> Commit f711a6ae062c ("net/rds: RDS-TCP: Always create a new rds_sock
> for an incoming connection.") modified rds-tcp so that an incoming SYN
> would ignore an existing "client" TCP connection which had the local
> port set to the transient port.  The motivation for ignoring the existing
> "client" connection in f711a6ae was to avoid race conditions and an
> endless duel of reconnect attempts triggered by a restart/abort of one
> of the nodes in the TCP connection.
>
> However, having separate sockets for active and passive sides
> is avoidable, and the simpler model of a single TCP socket for
> both send and receives of all RDS connections associated with
> that tcp socket makes for easier observability. We avoid the race
> conditions from f711a6ae by attempting reconnects in rds_conn_shutdown
> if, and only if, the (new) c_outgoing bit is set for RDS_TRANS_TCP.
> The c_outgoing bit is initialized in __rds_conn_create().
>
> A side-effect of re-using the client rds_connection for an incoming
> SYN is the potential of encountering duelling SYNs, i.e., we
> have an outgoing RDS_CONN_CONNECTING socket when we get the incoming
> SYN. The logic to arbitrate this criss-crossing SYN exchange in
> rds_tcp_accept_one() has been modified to emulate the BGP state
> machine: the smaller IP address should back off from the connection attempt.
>
> Signed-off-by: Sowmini Varadhan <sowmini.varadhan@...cle.com>
> ---
>   net/rds/connection.c |   22 ++++++----------------
>   net/rds/rds.h        |    4 +++-
>   net/rds/tcp_listen.c |   19 +++++++------------
>   3 files changed, 16 insertions(+), 29 deletions(-)
>

[...]

> diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
> index 444d78d..ee70d13 100644
> --- a/net/rds/tcp_listen.c
> +++ b/net/rds/tcp_listen.c
> @@ -110,28 +110,23 @@ int rds_tcp_accept_one(struct socket *sock)
>   		goto out;
>   	}
>   	/* An incoming SYN request came in, and TCP just accepted it.
> -	 * We always create a new conn for listen side of TCP, and do not
> -	 * add it to the c_hash_list.
>   	 *
>   	 * If the client reboots, this conn will need to be cleaned up.
>   	 * rds_tcp_state_change() will do that cleanup
>   	 */
>   	rs_tcp = (struct rds_tcp_connection *)conn->c_transport_data;
> -	WARN_ON(!rs_tcp || rs_tcp->t_sock);
> +	if (rs_tcp->t_sock && inet->inet_saddr < inet->inet_daddr) {
> +		struct sock *nsk = new_sock->sk;
>
Any reason you dropped the WARN_ON. Note that till we got commit
74e98eb0 (" RDS: verify the underlying transport exists before creating
a connection") merged, we had an issue. That guards it now.

Am curious about WARN_ON() and hence the question.

Rest of the patch looks fine to me.
Acked-by: Santosh Shilimkar <santosh.shilimkar@...cle.com>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html