Re: [PATCH rdma-next v3 6/9] RDMA/restrack: Add error handling while adding restrack object

[Jason Gunthorpe <jgg@nvidia.com>]

On Sat, Sep 26, 2020 at 01:19:35PM +0300, Leon Romanovsky wrote:
> diff --git a/drivers/infiniband/core/cq.c b/drivers/infiniband/core/cq.c
> index 12ebacf52958..1abcb01d362f 100644
> +++ b/drivers/infiniband/core/cq.c
> @@ -267,10 +267,25 @@ struct ib_cq *__ib_alloc_cq(struct ib_device *dev, void *private, int nr_cqe,
>  		goto out_destroy_cq;
>  	}
>  
> -	rdma_restrack_add(&cq->res);
> +	ret = rdma_restrack_add(&cq->res);
> +	if (ret)
> +		goto out_poll_cq;
> +
>  	trace_cq_alloc(cq, nr_cqe, comp_vector, poll_ctx);
>  	return cq;
>  
> +out_poll_cq:
> +	switch (cq->poll_ctx) {
> +	case IB_POLL_SOFTIRQ:
> +		irq_poll_disable(&cq->iop);
> +		break;
> +	case IB_POLL_WORKQUEUE:
> +	case IB_POLL_UNBOUND_WORKQUEUE:
> +		cancel_work_sync(&cq->work);

This error unwind is *technically* in the wrong order, it is wrong in
ib_free_cq too which is an actual bug.

The cq->comp_handler should be set before calling create_cq and undone
after calling destroy_wq. We can do this right now that the
allocations have been reworked.

Otherwise there is no assurance the ib_cq_completion_workqueue() won't
be called after this cancel == use after free

Also, you need to check all the rdma_restrack_del()'s, they should
always be *before* destroying the HW object, eg ib_free_cq() has it
too late. Similarly the add should always be after the HW object is
allocated.

For instance fill_res_cq_entry() calls 

  dev->ops.fill_res_cq_entry(msg, cq) 

on an already free'd HW object with this arrangment.

These are pre-existing things so lets fix them seperately please

Jason