Re: [PATCH for-next 2/4] RDMA/rtrs: Only allow addition of path to an already established session

[Leon Romanovsky <leon@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 <leon@kernel.org> 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 <leon@kernel.org> wrote:
> > > >
> > > > On Thu, Feb 11, 2021 at 07:55:24AM +0100, Jack Wang wrote:
> > > > > From: Md Haris Iqbal <haris.iqbal@cloud.ionos.com>
> > > > >
> > > > > 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 <haris.iqbal@cloud.ionos.com>
> > > > > Reviewed-by: Lutz Pogrell <lutz.pogrell@cloud.ionos.com>
> > > > > Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com>
> > > > > ---
> > > > >  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
> > > > >