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=-16.6 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 EC041C433DB for ; Thu, 11 Feb 2021 09:55:21 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id AD3DD64E8A for ; Thu, 11 Feb 2021 09:55:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229668AbhBKJzO (ORCPT ); Thu, 11 Feb 2021 04:55:14 -0500 Received: from mail.kernel.org ([198.145.29.99]:46756 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230047AbhBKJwY (ORCPT ); Thu, 11 Feb 2021 04:52:24 -0500 Received: by mail.kernel.org (Postfix) with ESMTPSA id 94F4E64D73; Thu, 11 Feb 2021 09:51:37 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1613037098; bh=Sx1D559/xLLZ2d6xzJA656DBrA8/7JV2X0bmEKWQR2w=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=lkGEFV7dTLt/5pU1kZuoASCIejfmU+YOFkYEJeAEAdt9wPH1EkmW5LGv37owkDswn Ac17bpPuTfNBXN6ObbRRsyhsWoXT7iy70pQnS1GQUMGG1U016Q0EgArWdKDtRYlmce Ygj1HOygTANE+zjLwQVqNo+mYJOsq04zdAiVaSI7EoClOqBHF9RSRkCeFBIeJqFaQn rrRCfJvsYIsxoZtdyzFriiTLY6dIxzPeMROedRCBkMKMFTPVloGRcj3hegomqGmWaj fLf2Nz5FZkXpQlG4ofPqFtAJad/1z9sQVRb9+EM9YcpLq8m6FaFD1IxGZxaYjYRqG+ e+ndrq6n9bl6g== Date: Thu, 11 Feb 2021 11:51:34 +0200 From: Leon Romanovsky To: Jinpu Wang Cc: linux-rdma@vger.kernel.org, Bart Van Assche , Doug Ledford , Jason Gunthorpe , Danil Kipnis , Md Haris Iqbal , Lutz Pogrell Subject: Re: [PATCH for-next 2/4] RDMA/rtrs: Only allow addition of path to an already established session Message-ID: <20210211095134.GF1275163@unreal> References: <20210211065526.7510-1-jinpu.wang@cloud.ionos.com> <20210211065526.7510-3-jinpu.wang@cloud.ionos.com> <20210211084300.GB1275163@unreal> <20210211093658.GE1275163@unreal> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-rdma@vger.kernel.org On Thu, Feb 11, 2021 at 10:48:48AM +0100, Jinpu Wang wrote: > On Thu, Feb 11, 2021 at 10:37 AM Leon Romanovsky wrote: > > > > On Thu, Feb 11, 2021 at 10:23:54AM +0100, Jinpu Wang wrote: > > > On Thu, Feb 11, 2021 at 9:43 AM Leon Romanovsky wrote: > > > > > > > > On Thu, Feb 11, 2021 at 07:55:24AM +0100, Jack Wang wrote: > > > > > From: Md Haris Iqbal > > > > > > > > > > While adding a path from the client side to an already established > > > > > session, it was possible to provide the destination IP to a different > > > > > server. This is dangerous. > > > > > > > > > > This commit adds an extra member to the rtrs_msg_conn_req structure, named > > > > > first_conn; which is supposed to notify if the connection request is the > > > > > first for that session or not. > > > > > > > > > > On the server side, if a session does not exist but the first_conn > > > > > received inside the rtrs_msg_conn_req structure is 1, the connection > > > > > request is failed. This signifies that the connection request is for an > > > > > already existing session, and since the server did not find one, it is an > > > > > wrong connection request. > > > > > > > > > > Fixes: 6a98d71daea1 ("RDMA/rtrs: client: main functionality") > > > > > Fixes: 9cb837480424 ("RDMA/rtrs: server: main functionality") > > > > > Signed-off-by: Md Haris Iqbal > > > > > Reviewed-by: Lutz Pogrell > > > > > Signed-off-by: Jack Wang > > > > > --- > > > > > drivers/infiniband/ulp/rtrs/rtrs-clt.c | 5 +++++ > > > > > drivers/infiniband/ulp/rtrs/rtrs-clt.h | 1 + > > > > > drivers/infiniband/ulp/rtrs/rtrs-pri.h | 4 +++- > > > > > drivers/infiniband/ulp/rtrs/rtrs-srv.c | 21 ++++++++++++++++----- > > > > > 4 files changed, 25 insertions(+), 6 deletions(-) > > > > <...> > > > > > > > > > > > > mutex_lock(&ctx->srv_mutex); > > > > > list_for_each_entry(srv, &ctx->srv_list, ctx_list) { > > > > > @@ -1346,12 +1348,20 @@ static struct rtrs_srv *get_or_create_srv(struct rtrs_srv_ctx *ctx, > > > > > return srv; > > > > > } > > > > > } > > > > > + /* > > > > > + * If this request is not the first connection request from the > > > > > + * client for this session then fail and return error. > > > > > + */ > > > > > + if (!first_conn) { > > > > > + err = -ENXIO; > > > > > + goto err; > > > > > + } > > > > > > > > Are you sure that this check not racy? > > > I can't see how a function parameter check can be racy, can you elaborate? > > > > get_or_create_srv() itself is protected with mutex_lock, but it can be called > > in parallel by rtrs_rdma_connect(), this is why I asked. > > I think again, still can't see how it could be racy. No problem, thanks. > Thanks! > > > > Thanks > > > > > > > > > > Thanks > > > Thanks for the review.! > > > > > > > > > > > > > > /* need to allocate a new srv */ > > > > > srv = kzalloc(sizeof(*srv), GFP_KERNEL); > > > > > if (!srv) { > > > > > mutex_unlock(&ctx->srv_mutex); > > > > > - return NULL; > > > > > + goto err; > > > > > } > > > > > > > > > > INIT_LIST_HEAD(&srv->paths_list); > > > > > @@ -1386,7 +1396,8 @@ static struct rtrs_srv *get_or_create_srv(struct rtrs_srv_ctx *ctx, > > > > > > > > > > err_free_srv: > > > > > kfree(srv); > > > > > - return NULL; > > > > > +err: > > > > > + return ERR_PTR(err); > > > > > } > > > > > > > > > > static void put_srv(struct rtrs_srv *srv) > > > > > @@ -1787,12 +1798,12 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id, > > > > > goto reject_w_econnreset; > > > > > } > > > > > recon_cnt = le16_to_cpu(msg->recon_cnt); > > > > > - srv = get_or_create_srv(ctx, &msg->paths_uuid); > > > > > + srv = get_or_create_srv(ctx, &msg->paths_uuid, msg->first_conn); > > > > > /* > > > > > * "refcount == 0" happens if a previous thread calls get_or_create_srv > > > > > * allocate srv, but chunks of srv are not allocated yet. > > > > > */ > > > > > - if (!srv || refcount_read(&srv->refcount) == 0) { > > > > > + if (IS_ERR(srv) || refcount_read(&srv->refcount) == 0) { > > > > > err = -ENOMEM; > > > > > goto reject_w_err; > > > > > } > > > > > -- > > > > > 2.25.1 > > > > >