Re: [for-next v1 3/5] RDMA/ionic: segregate rq related fields from ionic_qp into a new ionic_rq struct

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] Incomplete refactoring and introduction of dead code in `struct ionic_rq`.

Pre-existing issues:
- [High] Double free and use-after-free of rdma_user_mmap_entry structures on the error path in `ionic_create_qp`.
--

commit df35ccd280a5e88e710952bc7435abdf49666d70
Author: Amit Kumar <a.k@amd.com>

RDMA/ionic: segregate rq related fields from ionic_qp into a new ionic_rq struct

This commit refactors the ionic_qp structure by segregating receive queue
related fields into a newly created ionic_rq struct. This is done to prepare
for upcoming SRQ support.

> diff --git a/drivers/infiniband/hw/ionic/ionic_controlpath.c b/drivers/infiniband/hw/ionic/ionic_controlpath.c
> --- a/drivers/infiniband/hw/ionic/ionic_controlpath.c
> +++ b/drivers/infiniband/hw/ionic/ionic_controlpath.c
[ ... ]
> @@ -2501,19 +2498,19 @@ int ionic_create_qp(struct ib_qp *ibqp, struct ib_qp_init_attr *attr,
>  	if (qp->has_rq) {
>  		cq = to_ionic_vcq_cq(attr->recv_cq, qp->udma_idx);
>  
> -		attr->cap.max_recv_wr = qp->rq.mask;
> +		attr->cap.max_recv_wr = qp->rq.q.mask;
>  		attr->cap.max_recv_sge =
> -			ionic_v1_recv_wqe_max_sge(qp->rq.stride_log2,
> -						  qp->rq_spec,
> -						  qp->rq_cmb & IONIC_CMB_EXPDB);
> -		qp->rq_cqid = cq->cqid;
> +			ionic_v1_recv_wqe_max_sge(qp->rq.q.stride_log2,
> +						  qp->rq.spec,
> +						  qp->rq.cmb & IONIC_CMB_EXPDB);
> +		qp->rq.cqid = cq->cqid;
>  	}
>  
>  	return 0;
>  
>  err_resp:
> -	if (udata && (qp->rq_cmb & IONIC_CMB_ENABLE))
> -		rdma_user_mmap_entry_remove(qp->mmap_rq_cmb);
> +	if (udata && (qp->rq.cmb & IONIC_CMB_ENABLE))
> +		rdma_user_mmap_entry_remove(qp->rq.mmap_cmb);
>  err_mmap_rq:
>  	if (udata && (qp->sq_cmb & IONIC_CMB_ENABLE))
>  		rdma_user_mmap_entry_remove(qp->mmap_sq_cmb);
> @@ -2521,7 +2518,8 @@ int ionic_create_qp(struct ib_qp *ibqp, struct ib_qp_init_attr *attr,
>  	ionic_destroy_qp_cmd(dev, qp->qpid);
>  err_cmd:
>  	ionic_pgtbl_unbuf(dev, &rq_buf);
> -	ionic_qp_rq_destroy(dev, ctx, qp);
> +	if (qp->has_rq)
> +		ionic_rq_destroy(dev, ctx, &qp->rq);
>  err_rq:
>  	ionic_pgtbl_unbuf(dev, &sq_buf);
>  	ionic_qp_sq_destroy(dev, ctx, qp);

[Severity: High]
This is a pre-existing issue, but can this sequence trigger a use-after-free
or double free of the mmap entries?

In ionic_create_qp(), if ib_respond_udata() fails, the unwind paths
at err_resp and err_mmap_rq manually call rdma_user_mmap_entry_remove() on
both qp->rq.mmap_cmb and qp->mmap_sq_cmb.

Because the pointers are not set to NULL, execution falls through to
err_cmd and err_rq, which invoke ionic_rq_destroy() and
ionic_qp_sq_destroy(). These downstream functions then call
ionic_rq_destroy_cmb() and ionic_qp_sq_destroy_cmb(), executing
rdma_user_mmap_entry_remove() unconditionally on the exact same
pointers again.

> diff --git a/drivers/infiniband/hw/ionic/ionic_ibdev.h b/drivers/infiniband/hw/ionic/ionic_ibdev.h
> --- a/drivers/infiniband/hw/ionic/ionic_ibdev.h
> +++ b/drivers/infiniband/hw/ionic/ionic_ibdev.h
[ ... ]
> @@ -241,6 +241,30 @@ struct ionic_rq_meta {
>  	u64			wrid;
>  };
>  
> +struct ionic_rq {
> +	struct ionic_queue		q;
> +	u32				qid;
> +	u32				cqid;
> +	struct list_head		cq_flush;
> +	spinlock_t			lock; /* for posting and polling */
> +
> +	phys_addr_t			cmb_addr;
> +	void		__iomem		*cmb_ptr;
> +	struct rdma_user_mmap_entry	*mmap_cmb;
> +
> +	struct ionic_rq_meta		*meta;
> +	struct ionic_rq_meta		*meta_head;
> +	struct ib_umem			*umem;
> +
> +	int				spec;
> +	int				cmb_order;
> +	u32				cmb_pgid;
> +	u16				cmb_prod;
> +	u16				old_prod;
> +	u8				cmb;
> +	bool				flush;
> +};
> +

[Severity: Low]
Does this new structure introduce unused fields? The fields cq_flush,
cmb_ptr, and cmb_prod appear to be unreferenced dead code.

Additionally, it seems the actively used list head cq_flush_rq was left
behind in struct ionic_qp instead of being migrated here along with the
rest of the queue state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908090802.152142-1-abhijit.gangurde@amd.com?part=3