From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.1 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 14354C43381 for ; Wed, 6 Mar 2019 17:53:30 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id C049020661 for ; Wed, 6 Mar 2019 17:53:29 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=oracle.com header.i=@oracle.com header.b="nfkbuiVR" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730427AbfCFRx2 (ORCPT ); Wed, 6 Mar 2019 12:53:28 -0500 Received: from userp2120.oracle.com ([156.151.31.85]:59710 "EHLO userp2120.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727199AbfCFRx2 (ORCPT ); Wed, 6 Mar 2019 12:53:28 -0500 Received: from pps.filterd (userp2120.oracle.com [127.0.0.1]) by userp2120.oracle.com (8.16.0.27/8.16.0.27) with SMTP id x26HnNod016913; Wed, 6 Mar 2019 17:53:23 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=subject : to : cc : references : from : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=corp-2018-07-02; bh=Of5skUdi6mRzFNj0nggOMdm/QLIOQwx0L7vBIAiR7R0=; b=nfkbuiVRvhghWG1yes9OVe0ZPcUoIK/Qxl1w1/tq+r1TVSIdfntjMktdXDif44ga5gxu 3n2mf8jAXV++bFQI3xQjwt+Y2fZV0bHTbnfrV4kuqvx+VrtjbBdMvWa94ZUJqe4ZpZbs emw9adhaZGbiG5wKGdLo1fmm+msjvfqAybERI/1X23PQAzNICV3aGlfBtNzxBDHd9+Tv YAFxjKFO6kbOEj1RJgvo2KDT8zCLYy1L8dHFqPCm/RSP35yXjtH8FMEJPCU7esLTTbFx 3VNL1Rmzyy5TFIqyz8MGTy9ksWUlE/mU/+gmOob/KUD6mkFmsWtQhlJMvk6O6QXPuPEA Kg== Received: from aserv0021.oracle.com (aserv0021.oracle.com [141.146.126.233]) by userp2120.oracle.com with ESMTP id 2qyjfrn7f7-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 06 Mar 2019 17:53:23 +0000 Received: from userv0121.oracle.com (userv0121.oracle.com [156.151.31.72]) by aserv0021.oracle.com (8.14.4/8.14.4) with ESMTP id x26HrLGa013695 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 6 Mar 2019 17:53:22 GMT Received: from abhmp0022.oracle.com (abhmp0022.oracle.com [141.146.116.28]) by userv0121.oracle.com (8.14.4/8.13.8) with ESMTP id x26HrLaA011865; Wed, 6 Mar 2019 17:53:21 GMT Received: from [10.209.243.127] (/10.209.243.127) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Wed, 06 Mar 2019 09:53:21 -0800 Subject: Re: [PATCH] net/rds: Accept peer connection reject messages due to incompatible version To: Gerd Rausch , netdev@vger.kernel.org, Yanjun Zhu Cc: David Miller References: <20190306070409.26840-1-gerd.rausch@oracle.com> From: Santosh Shilimkar Organization: Oracle Corporation Message-ID: <10a0428b-a6da-7f66-c3a6-1e74fac34da5@oracle.com> Date: Wed, 6 Mar 2019 09:55:01 -0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.5.1 MIME-Version: 1.0 In-Reply-To: <20190306070409.26840-1-gerd.rausch@oracle.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Proofpoint-Virus-Version: vendor=nai engine=5900 definitions=9187 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 mlxscore=0 impostorscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1810050000 definitions=main-1903060124 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 3/5/2019 11:04 PM, Gerd Rausch wrote: > 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. > > Orabug: 29444532 > > Signed-off-by: Gerd Rausch > --- > net/rds/rdma_transport.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c > index 46bce8389066..f628e7fda66d 100644 > --- a/net/rds/rdma_transport.c > +++ b/net/rds/rdma_transport.c > @@ -112,7 +112,7 @@ 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 +122,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: > Very similar test diff [1] I sent Yanjun to test yesterday... Thanks for checking. Will submit a cleaned up fix Gerd. Thansk for checking From 82c638f11137e29220051c704b63e3d7ed7d44d0 Mon Sep 17 00:00:00 2001 From: Santosh Shilimkar Date: Tue, 5 Mar 2019 18:19:52 -0800 Subject: [PATCH] TEST debug patch Signed-off-by: Santosh Shilimkar --- net/rds/rdma_transport.c | 16 ++++++++++++++++ 1 files changed, 16 insertions(+), 0 deletions(-) diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c index 46bce83..c7b1fff 100644 --- a/net/rds/rdma_transport.c +++ b/net/rds/rdma_transport.c @@ -109,8 +109,10 @@ static int rds_rdma_cm_event_handler_cmn(struct rdma_cm_id *cm_id, break; case RDMA_CM_EVENT_REJECTED: + err = (int *)event->param.conn.private_data; if (!conn) break; +#if 0 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", @@ -119,6 +121,20 @@ static int rds_rdma_cm_event_handler_cmn(struct rdma_cm_id *cm_id, conn->c_tos = 0; rds_conn_drop(conn); } +#endif + if (event->status == RDS_REJ_CONSUMER_DEFINED && + (*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; + conn->c_tos = 0; + rds_conn_drop(conn); + } else { + pr_debug("RDS/RDMA: conn <%pI6c, %pI6c> rejected, dropping connection\n", + &conn->c_laddr, &conn->c_faddr); + rds_conn_drop(conn); + } rdsdebug("Connection rejected: %s\n", rdma_reject_msg(cm_id, event->status)); break; -- 1.7.1