Re: [PATCH for-rc] iw_cxgb4: Fix refcount underflow while destroying cqs.

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Jul 22, 2021 at 10:43:00AM +0300, Leon Romanovsky wrote:
> On Wed, Jul 21, 2021 at 04:51:55PM +0530, Dakshaja Uppalapati wrote:
> > Previous atomic increment decrement logic expects the atomic count
> > to be '0' after the final decrement. Replacing atomic count with
> > refcount does not allow that as refcount_dec() considers count of 1 as
> > underflow. Therefore fix the current refcount logic by decrementing
> > the refcount if one on the final deref in c4iw_destroy_cq().
> > 
> > Fixes: 7183451f846d (RDMA/cxgb4: Use refcount_t instead of atomic_t for reference counting")
> > Signed-off-by: Dakshaja Uppalapati <dakshaja@chelsio.com>
> > Reviewed-by: Potnuri Bharat Teja <bharat@chelsio.com>
> >  drivers/infiniband/hw/cxgb4/cq.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> 
> Thanks, 
> Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
> 
> We have plenty of such errors, worth to check them:
> ➜  kernel git:(rdma-next) git grep refcount_read drivers/infiniband/ | grep -v WARN_ON
> drivers/infiniband/core/device.c:	if (!refcount_read(&ib_dev->refcount))
> drivers/infiniband/core/device.c:	if (refcount_read(&device->refcount) == 0 ||
> drivers/infiniband/core/iwpm_util.c:	if (!refcount_read(&iwpm_admin.refcount)) {
> drivers/infiniband/core/iwpm_util.c:	if (!refcount_read(&iwpm_admin.refcount)) {
> drivers/infiniband/core/ucma.c:	if (refcount_read(&ctx->ref))
> drivers/infiniband/hw/cxgb4/cq.c:	wait_event(chp->wait, !refcount_read(&chp->refcnt));
> drivers/infiniband/hw/irdma/utils.c:			   refcount_read(&cqp_request->refcnt) == 1, 1000);
> drivers/infiniband/hw/mlx5/mlx5_ib.h:	wait_event(mmkey->wait, refcount_read(&mmkey->usecount) == 0);
> drivers/infiniband/hw/mlx5/mr.c:	    refcount_read(&mr->mmkey.usecount) != 0 &&

It isn't the read that is the problem, it is the naked dec.

This common pattern is just being done in an obtuse and arguably wrong
way

It is supposed to look like this:

void ib_device_put(struct ib_device *device)
{
        if (refcount_dec_and_test(&device->refcount))
                complete(&device->unreg_completion);
}

[..]

        ib_device_put(device);
        wait_for_completion(&device->unreg_completion);


Not use refcount_read() and a naked work queue

So I'd say these ones should be looked at:

drivers/infiniband/hw/cxgb4/cq.c:       refcount_dec(&chp->refcnt);
drivers/infiniband/hw/hns/hns_roce_db.c:        refcount_dec(&db->u.user_page->refcount);
drivers/infiniband/hw/irdma/cm.c:       refcount_dec(&cm_node->refcnt);
drivers/infiniband/hw/irdma/cm.c:               refcount_dec(&listener->refcnt);
drivers/infiniband/hw/irdma/cm.c:                       refcount_dec(&listener->refcnt);

Jason