Re: [net][PATCH 3/5] rds: Accept peer connection reject messages due to incompatible version

[Yanjun Zhu <yanjun.zhu@oracle.com>]

On 2019/7/10 13:32, Santosh Shilimkar wrote:
> From: Gerd Rausch <gerd.rausch@oracle.com>
>
> Prior to
> commit d021fabf525ff ("rds: rdma: add consumer reject")
>
> function "rds_rdma_cm_event_handler_cmn" would always honor a rejected
> connection attempt by issuing a "rds_conn_drop".
>
> The commit mentioned above added a "break", eliminating
> the "fallthrough" case and made the "rds_conn_drop" rather conditional:
>
> Now it only happens if a "consumer defined" reject (i.e. "rdma_reject")
> carries an integer-value of "1" inside "private_data":
>
>    if (!conn)
>      break;
>      err = (int *)rdma_consumer_reject_data(cm_id, event, &len);
>      if (!err || (err && ((*err) == RDS_RDMA_REJ_INCOMPAT))) {
>        pr_warn("RDS/RDMA: conn <%pI6c, %pI6c> rejected, dropping connection\n",
>                &conn->c_laddr, &conn->c_faddr);
>                conn->c_proposed_version = RDS_PROTOCOL_COMPAT_VERSION;
>                rds_conn_drop(conn);
>      }
>      rdsdebug("Connection rejected: %s\n",
>               rdma_reject_msg(cm_id, event->status));
>      break;
>      /* FALLTHROUGH */
> A number of issues are worth mentioning here:
>     #1) Previous versions of the RDS code simply rejected a connection
>         by calling "rdma_reject(cm_id, NULL, 0);"
>         So the value of the payload in "private_data" will not be "1",
>         but "0".
>
>     #2) Now the code has become dependent on host byte order and sizing.
>         If one peer is big-endian, the other is little-endian,
>         or there's a difference in sizeof(int) (e.g. ILP64 vs LP64),
>         the *err check does not work as intended.
>
>     #3) There is no check for "len" to see if the data behind *err is even valid.
>         Luckily, it appears that the "rdma_reject(cm_id, NULL, 0)" will always
>         carry 148 bytes of zeroized payload.
>         But that should probably not be relied upon here.
>
>     #4) With the added "break;",
>         we might as well drop the misleading "/* FALLTHROUGH */" comment.
>
> This commit does _not_ address issue #2, as the sender would have to
> agree on a byte order as well.
>
> Here is the sequence of messages in this observed error-scenario:
>     Host-A is pre-QoS changes (excluding the commit mentioned above)
>     Host-B is post-QoS changes (including the commit mentioned above)
>
>     #1 Host-B
>        issues a connection request via function "rds_conn_path_transition"
>        connection state transitions to "RDS_CONN_CONNECTING"
>
>     #2 Host-A
>        rejects the incompatible connection request (from #1)
>        It does so by calling "rdma_reject(cm_id, NULL, 0);"
>
>     #3 Host-B
>        receives an "RDMA_CM_EVENT_REJECTED" event (from #2)
>        But since the code is changed in the way described above,
>        it won't drop the connection here, simply because "*err == 0".
>
>     #4 Host-A
>        issues a connection request
>
>     #5 Host-B
>        receives an "RDMA_CM_EVENT_CONNECT_REQUEST" event
>        and ends up calling "rds_ib_cm_handle_connect".
>        But since the state is already in "RDS_CONN_CONNECTING"
>        (as of #1) it will end up issuing a "rdma_reject" without
>        dropping the connection:
>           if (rds_conn_state(conn) == RDS_CONN_CONNECTING) {
>               /* Wait and see - our connect may still be succeeding */
>               rds_ib_stats_inc(s_ib_connect_raced);
>           }
>           goto out;
>
>     #6 Host-A
>        receives an "RDMA_CM_EVENT_REJECTED" event (from #5),
>        drops the connection and tries again (goto #4) until it gives up.
>
> Tested-by: Zhu Yanjun <yanjun.zhu@oracle.com>

Thanks

Zhu Yanjun

> Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com>
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@oracle.com>
> ---
>   net/rds/rdma_transport.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
> index 46bce83..9db455d 100644
> --- a/net/rds/rdma_transport.c
> +++ b/net/rds/rdma_transport.c
> @@ -112,7 +112,9 @@ static int rds_rdma_cm_event_handler_cmn(struct rdma_cm_id *cm_id,
>   		if (!conn)
>   			break;
>   		err = (int *)rdma_consumer_reject_data(cm_id, event, &len);
> -		if (!err || (err && ((*err) == RDS_RDMA_REJ_INCOMPAT))) {
> +		if (!err ||
> +		    (err && len >= sizeof(*err) &&
> +		     ((*err) <= RDS_RDMA_REJ_INCOMPAT))) {
>   			pr_warn("RDS/RDMA: conn <%pI6c, %pI6c> rejected, dropping connection\n",
>   				&conn->c_laddr, &conn->c_faddr);
>   			conn->c_proposed_version = RDS_PROTOCOL_COMPAT_VERSION;
> @@ -122,7 +124,6 @@ static int rds_rdma_cm_event_handler_cmn(struct rdma_cm_id *cm_id,
>   		rdsdebug("Connection rejected: %s\n",
>   			 rdma_reject_msg(cm_id, event->status));
>   		break;
> -		/* FALLTHROUGH */
>   	case RDMA_CM_EVENT_ADDR_ERROR:
>   	case RDMA_CM_EVENT_ROUTE_ERROR:
>   	case RDMA_CM_EVENT_CONNECT_ERROR: