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

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

[Dakshaja Uppalapati]

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(-)

diff --git a/drivers/infiniband/hw/cxgb4/cq.c b/drivers/infiniband/hw/cxgb4/cq.c
index 6c8c910..54a5b60 100644
--- a/drivers/infiniband/hw/cxgb4/cq.c
+++ b/drivers/infiniband/hw/cxgb4/cq.c
@@ -976,8 +976,7 @@ int c4iw_destroy_cq(struct ib_cq *ib_cq, struct ib_udata *udata)
 	chp = to_c4iw_cq(ib_cq);
 
 	xa_erase_irq(&chp->rhp->cqs, chp->cq.cqid);
-	refcount_dec(&chp->refcnt);
-	wait_event(chp->wait, !refcount_read(&chp->refcnt));
+	wait_event(chp->wait, refcount_dec_if_one(&chp->refcnt));
 
 	ucontext = rdma_udata_to_drv_context(udata, struct c4iw_ucontext,
 					     ibucontext);
-- 
1.8.3.1

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

[Leon Romanovsky]

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 &&

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

[Jason Gunthorpe]

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

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

[Leon Romanovsky]

On Thu, Jul 22, 2021 at 09:06:07AM -0300, Jason Gunthorpe wrote:
> 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,

We are talking about two different issues that this refcount_read patch pointed.
You are focused on wrong usage of completion, I saw useless compare of
refcount_t with 0 that can't be.

I prepared series that cleans iwpm from this type of error:
 drivers/infiniband/core/iwpm_util.c:        if (!refcount_read(&iwpm_admin.refcount)) {
 drivers/infiniband/core/iwpm_util.c:        if (!refcount_read(&iwpm_admin.refcount)) {

Thanks

> 
> Jason

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

[Jason Gunthorpe]

On Thu, Jul 22, 2021 at 03:57:39PM +0300, Leon Romanovsky wrote:

> We are talking about two different issues that this refcount_read patch pointed.
> You are focused on wrong usage of completion, I saw useless compare of
> refcount_t with 0 that can't be.

It can be zero. Anything that does refcount_dec_and_test() can make
the refcount be zero.

The issue here is that refcount_dec() cannot make the refcount zero as
it is improper use of the API.

Jason

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

[Dakshaja Uppalapati]

On Thursday, July 07/22/21, 2021 at 10:02:31 -0300, Jason Gunthorpe wrote:
> On Thu, Jul 22, 2021 at 03:57:39PM +0300, Leon Romanovsky wrote:
> 
> > We are talking about two different issues that this refcount_read patch pointed.
> > You are focused on wrong usage of completion, I saw useless compare of
> > refcount_t with 0 that can't be.
> 
> It can be zero. Anything that does refcount_dec_and_test() can make
> the refcount be zero.
> 
> The issue here is that refcount_dec() cannot make the refcount zero as
> it is improper use of the API.

Hi Jason

I didn't understood properly. Can you please tell that the current 
patch for cxgb4 is still wrong?

Thanks
Dakshaja

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

[Leon Romanovsky]

On Thu, Jul 22, 2021 at 10:02:31AM -0300, Jason Gunthorpe wrote:
> On Thu, Jul 22, 2021 at 03:57:39PM +0300, Leon Romanovsky wrote:
> 
> > We are talking about two different issues that this refcount_read patch pointed.
> > You are focused on wrong usage of completion, I saw useless compare of
> > refcount_t with 0 that can't be.
> 
> It can be zero. Anything that does refcount_dec_and_test() can make
> the refcount be zero.

It can be, but it is not the case at least for iwpm.

Thanks

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

[Jason Gunthorpe]

On Thu, Jul 22, 2021 at 05:01:30PM +0300, Leon Romanovsky wrote:
> On Thu, Jul 22, 2021 at 10:02:31AM -0300, Jason Gunthorpe wrote:
> > On Thu, Jul 22, 2021 at 03:57:39PM +0300, Leon Romanovsky wrote:
> > 
> > > We are talking about two different issues that this refcount_read patch pointed.
> > > You are focused on wrong usage of completion, I saw useless compare of
> > > refcount_t with 0 that can't be.
> > 
> > It can be zero. Anything that does refcount_dec_and_test() can make
> > the refcount be zero.
> 
> It can be, but it is not the case at least for iwpm.

That is even more broken then. Structs with non-zero refcounts in them
should not be freed.

Jason

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

[Jason Gunthorpe]

On Tue, Aug 03, 2021 at 09:29:20PM +0530, Dakshaja Uppalapati wrote:
> Hi Jason,
> 
> Gentle ping.
> 
> I think the current patch for cxgb4 is legit. I am expecting 
> this to be pulled for next rc. Please let me know if I am missing
> something here.

As I said, you need to rework this to use a proper completion pattern
not hacky like this.

Jason