| Message ID | 20210216143807.65923-1-jinpu.wang@cloud.ionos.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | RDMA/rtrs-srv: Suppress warnings passing zero to 'PTR_ERR' | expand |
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
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!
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
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; }
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(-)