[PATCH] RDMA/rtrs-srv: Suppress warnings passing zero to 'PTR_ERR'

[PATCH] RDMA/rtrs-srv: Suppress warnings passing zero to 'PTR_ERR'

[Jack Wang]

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, the solution is
protect move the list_add down after full initilization of rtrs_srv.
To avoid holding the srv_mutex too long, only hold it during
the list operation as suggested by Leon.

Fixes: f0751419d3a1 ("RDMA/rtrs: Only allow addition of path to an already established session")
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com>
---
 drivers/infiniband/ulp/rtrs/rtrs-srv.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
index eb17c3a08810..d071809e3ed2 100644
--- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
+++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
@@ -1347,21 +1347,18 @@ static struct rtrs_srv *get_or_create_srv(struct rtrs_srv_ctx *ctx,
 			return srv;
 		}
 	}
+	mutex_unlock(&ctx->srv_mutex);
 	/*
 	 * If this request is not the first connection request from the
 	 * client for this session then fail and return error.
 	 */
-	if (!first_conn) {
-		mutex_unlock(&ctx->srv_mutex);
+	if (!first_conn)
 		return ERR_PTR(-ENXIO);
-	}
 
 	/* need to allocate a new srv */
 	srv = kzalloc(sizeof(*srv), GFP_KERNEL);
-	if  (!srv) {
-		mutex_unlock(&ctx->srv_mutex);
+	if  (!srv)
 		return ERR_PTR(-ENOMEM);
-	}
 
 	INIT_LIST_HEAD(&srv->paths_list);
 	mutex_init(&srv->paths_mutex);
@@ -1371,8 +1368,6 @@ static struct rtrs_srv *get_or_create_srv(struct rtrs_srv_ctx *ctx,
 	srv->ctx = ctx;
 	device_initialize(&srv->dev);
 	srv->dev.release = rtrs_srv_dev_release;
-	list_add(&srv->ctx_list, &ctx->srv_list);
-	mutex_unlock(&ctx->srv_mutex);
 
 	srv->chunks = kcalloc(srv->queue_depth, sizeof(*srv->chunks),
 			      GFP_KERNEL);
@@ -1385,6 +1380,9 @@ static struct rtrs_srv *get_or_create_srv(struct rtrs_srv_ctx *ctx,
 			goto err_free_chunks;
 	}
 	refcount_set(&srv->refcount, 1);
+	mutex_lock(&ctx->srv_mutex);
+	list_add(&srv->ctx_list, &ctx->srv_list);
+	mutex_unlock(&ctx->srv_mutex);
 
 	return srv;
 
@@ -1799,11 +1797,7 @@ 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);
-	/*
-	 * "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) {
+	if (IS_ERR(srv)) {
 		err = PTR_ERR(srv);
 		goto reject_w_err;
 	}
-- 
2.25.1

Re: [PATCH] RDMA/rtrs-srv: Suppress warnings passing zero to 'PTR_ERR'

[Dan Carpenter]

On Tue, Feb 16, 2021 at 03:38:07PM +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, the solution is
> protect move the list_add down after full initilization of rtrs_srv.

In theory if Smatch had a properly up to date DB then it would print a
different warning "passing a valid pointer to PTR_ERR()".

regards,
dan carpenter

Re: [PATCH] RDMA/rtrs-srv: Suppress warnings passing zero to 'PTR_ERR'

[Jinpu Wang]

On Tue, Feb 16, 2021 at 3:49 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Tue, Feb 16, 2021 at 03:38:07PM +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, the solution is
> > protect move the list_add down after full initilization of rtrs_srv.
>
> In theory if Smatch had a properly up to date DB then it would print a
> different warning "passing a valid pointer to PTR_ERR()".
That will be easier to follow :)
>
> regards,
> dan carpenter
>
Thanks!

Re: [PATCH] RDMA/rtrs-srv: Suppress warnings passing zero to 'PTR_ERR'

[Jason Gunthorpe]

On Tue, Feb 16, 2021 at 03:38:07PM +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, the solution is
> protect move the list_add down after full initilization of rtrs_srv.
> To avoid holding the srv_mutex too long, only hold it during
> the list operation as suggested by Leon.
> 
> Fixes: f0751419d3a1 ("RDMA/rtrs: Only allow addition of path to an already established session")
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com>
> ---
>  drivers/infiniband/ulp/rtrs/rtrs-srv.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)

Applied to for-next, thanks

Jason

[PATCH] RDMA/rtrs-srv: suppress warnings passing zero to 'PTR_ERR'

[Jack Wang]

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@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com>
---
 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) {
+		err = -EBUSY;
 		goto reject_w_err;
 	}
 	mutex_lock(&srv->paths_mutex);
-- 
2.25.1

Re: [PATCH] RDMA/rtrs-srv: suppress warnings passing zero to 'PTR_ERR'

[Leon Romanovsky]

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@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com>
> ---
>  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, relying on zero in the refcount
doesn't look great.

Thanks

Re: [PATCH] RDMA/rtrs-srv: suppress warnings passing zero to 'PTR_ERR'

[Jinpu Wang]

Hi Leon,
On Tue, Feb 16, 2021 at 8:50 AM Leon Romanovsky <leon@kernel.org> 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@intel.com>
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com>
> > ---
> >  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")

Thanks for the comment.

Re: [PATCH] RDMA/rtrs-srv: suppress warnings passing zero to 'PTR_ERR'

[Leon Romanovsky]

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@kernel.org> 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@intel.com>
> > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com>
> > > ---
> > >  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

>
> Thanks for the comment.

Re: [PATCH] RDMA/rtrs-srv: suppress warnings passing zero to 'PTR_ERR'

[Jinpu Wang]

On Tue, Feb 16, 2021 at 9:17 AM Leon Romanovsky <leon@kernel.org> 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@kernel.org> 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@intel.com>
> > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > > > Signed-off-by: Jack Wang <jinpu.wang@cloud.ionos.com>
> > > > ---
> > > >  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.