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 49901C43381 for ; Wed, 6 Mar 2019 08:36:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 0F9B22064A for ; Wed, 6 Mar 2019 08:36:53 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=oracle.com header.i=@oracle.com header.b="Odpa2WmZ" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729221AbfCFIgv (ORCPT ); Wed, 6 Mar 2019 03:36:51 -0500 Received: from userp2130.oracle.com ([156.151.31.86]:32834 "EHLO userp2130.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725747AbfCFIgv (ORCPT ); Wed, 6 Mar 2019 03:36:51 -0500 Received: from pps.filterd (userp2130.oracle.com [127.0.0.1]) by userp2130.oracle.com (8.16.0.27/8.16.0.27) with SMTP id x268YIpU113721; Wed, 6 Mar 2019 08:36:48 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=8R69Gka3HbEJmeCto1NVRVAI5K22ccOAs+gaHSKFwnE=; b=Odpa2WmZOxj3ZMWzwegjbpG9FaPQ9lkOcyhqtT7H+1oLsc6Rxnl73y1Rqt4Lz7ooi61M lWtBGTioNipnKrGnSuys2hV/lqmgS2o9vhTIqW6X8glCJHDRthOb0/qO1CnT1yOi21wj gXOJLJpcOHDfpKyypEv+UwciV8q7dt5MJJVc3QlslVLT1Ej2zOjq5pdkPK6qNFaFZsWG Jj18/tsaudfEXfiguBrAl9Kf/jxgWAlum28v5RaAqA3yVajgv0eY88nRPUT0NJ23exU/ NGfk+W6VKtwBOjtCSFY4WX+9lX70UFu9AKy2Z+IedFNpNJl//HPiGFZjSkP5MKcLSbuX tA== Received: from userv0022.oracle.com (userv0022.oracle.com [156.151.31.74]) by userp2130.oracle.com with ESMTP id 2qyh8uab68-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 06 Mar 2019 08:36:48 +0000 Received: from aserv0122.oracle.com (aserv0122.oracle.com [141.146.126.236]) by userv0022.oracle.com (8.14.4/8.14.4) with ESMTP id x268ahsd012753 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 6 Mar 2019 08:36:43 GMT Received: from abhmp0013.oracle.com (abhmp0013.oracle.com [141.146.116.19]) by aserv0122.oracle.com (8.14.4/8.14.4) with ESMTP id x268agQf028552; Wed, 6 Mar 2019 08:36:42 GMT Received: from [10.182.69.170] (/10.182.69.170) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Wed, 06 Mar 2019 00:36:42 -0800 Subject: Re: [PATCH] net/rds: Accept peer connection reject messages due to incompatible version To: Gerd Rausch , Santosh Shilimkar , netdev@vger.kernel.org Cc: David Miller References: <20190306070409.26840-1-gerd.rausch@oracle.com> From: Yanjun Zhu Organization: Oracle Corporation Message-ID: <41644dfc-9323-6440-c9bf-0c4980132743@oracle.com> Date: Wed, 6 Mar 2019 16:41:00 +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-Transfer-Encoding: 7bit Content-Language: en-US X-Proofpoint-Virus-Version: vendor=nai engine=5900 definitions=9186 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-1903060059 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On 2019/3/6 15:04, 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 This is the internal bug. It should be removed. Zhu Yanjun > > 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: