On Tue, Feb 16, 2021 at 9:17 AM Leon Romanovsky <leon@xxxxxxxxxx> wrote:
>
> On Tue, Feb 16, 2021 at 09:02:03AM +0100, Jinpu Wang wrote:
> > Hi Leon,
> > On Tue, Feb 16, 2021 at 8:50 AM Leon Romanovsky <leon@xxxxxxxxxx> wrote:
> > >
> > > On Tue, Feb 16, 2021 at 08:39:58AM +0100, Jack Wang wrote:
> > > > smatch warnings:
> > > > drivers/infiniband/ulp/rtrs/rtrs-srv.c:1805 rtrs_rdma_connect() warn: passing zero to 'PTR_ERR'
> > > >
> > > > Smatch seems confused by the refcount_read condition, so just
> > > > treat it seperately.
> > > >
> > > > Fixes: f0751419d3a1 ("RDMA/rtrs: Only allow addition of path to an already established session")
> > > > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > > > Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > > > Signed-off-by: Jack Wang <jinpu.wang@xxxxxxxxxxxxxxx>
> > > > ---
> > > > drivers/infiniband/ulp/rtrs/rtrs-srv.c | 8 ++++++--
> > > > 1 file changed, 6 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > > index eb17c3a08810..2f6d665bea90 100644
> > > > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > > @@ -1799,12 +1799,16 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
> > > > }
> > > > recon_cnt = le16_to_cpu(msg->recon_cnt);
> > > > srv = get_or_create_srv(ctx, &msg->paths_uuid, msg->first_conn);
> > > > + if (IS_ERR(srv)) {
> > > > + err = PTR_ERR(srv);
> > > > + goto reject_w_err;
> > > > + }
> > > > /*
> > > > * "refcount == 0" happens if a previous thread calls get_or_create_srv
> > > > * allocate srv, but chunks of srv are not allocated yet.
> > > > */
> > > > - if (IS_ERR(srv) || refcount_read(&srv->refcount) == 0) {
> > > > - err = PTR_ERR(srv);
> > > > + if (refcount_read(&srv->refcount) == 0) {
> > >
> > > I would say that "list_add(&srv->ctx_list, &ctx->srv_list);" line in the
> > > get_or_create_srv() is performed too early,
> > Moving list_add down to the end was the initial code, but we noticed
> > the memory allocation could take quite
> > some time when system under memory pressure, hence we changed it in
> > d715ff8acbd5 ("RDMA/rtrs-srv: Don't guard the whole __alloc_srv with
> > srv_mutex")
>
> You don't need to hold lock during allocation, only during search in the list.
Thanks, I will try and test.
>
> Thanks
>
> >
> > Thanks for the comment.
