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