Re: [PATCH rdma-next v3 3/6] RDMA/irdma: Support UMEM attribute for QP creation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] RDMA/irdma: invalid sg_page() on dmabuf QP umem in irdma_copy_user_pgaddrs()
--

commit 200a925805870f2d575192eafe96be3cef694b7d
Author: David Hu <xuehaohu@google.com>

RDMA/irdma: Support UMEM attribute for QP creation

This commit adds support for the 1-step queue buffer creation model for QPs
via UVERBS_ATTR_CREATE_QP_BUF_UMEM and ib_umem_get_attr(), while retaining
backward compatibility with the legacy 2-step MR registration model.

> diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c
> index b2ffdc8e3d677..ab25445f38282 100644
> --- a/drivers/infiniband/hw/irdma/verbs.c
> +++ b/drivers/infiniband/hw/irdma/verbs.c
[ ... ]
> @@ -713,6 +708,52 @@ static int irdma_setup_umode_qp(struct irdma_ucontext *ucontext,
>  		ukinfo->rq_size = ukinfo->rq_depth >> ukinfo->rq_shift;
>  	}
>  
> +	if (attrs && uverbs_attr_is_valid(attrs, UVERBS_ATTR_CREATE_QP_BUF_UMEM)) {
> +		struct irdma_mem_reg_req qp_req = {};
> +		struct ib_umem *umem;
> +		u32 total;
> +
> +		/* Legacy context does not provide depth info */
> +		if (!ucontext->use_raw_attrs)
> +			return -EOPNOTSUPP;
> +
> +		qp_req.sq_pages = DIV_ROUND_UP(ukinfo->sq_depth * IRDMA_QP_WQE_MIN_SIZE,
> +					       IRDMA_HW_PAGE_SIZE);
> +		/* In case srq is used, this will be a single page */
> +		qp_req.rq_pages = DIV_ROUND_UP(ukinfo->rq_depth * IRDMA_QP_WQE_MIN_SIZE,
> +					       IRDMA_HW_PAGE_SIZE);
> +		total = qp_req.sq_pages + qp_req.rq_pages + IRDMA_SHADOW_PGCNT;
> +
> +		umem = ib_umem_get_attr(&iwdev->ibdev, attrs,
> +					UVERBS_ATTR_CREATE_QP_BUF_UMEM,
> +					total << IRDMA_HW_PAGE_SHIFT,
> +					IB_ACCESS_LOCAL_WRITE);
> +		if (IS_ERR(umem))
> +			return PTR_ERR(umem);
> +
> +		iwqp->iwpbl = irdma_setup_q_pbl_from_umem(iwdev, umem,
> +							  iwqp->ibqp.pd,
> +							  IRDMA_MEMREG_TYPE_QP,
> +							  &qp_req);
[ ... ]
> @@ -3532,6 +3574,64 @@ static void irdma_free_iwmr(struct irdma_mr *iwmr)
>  	kfree(iwmr);
>  }
>  
> +static struct irdma_pbl *irdma_setup_q_pbl_from_umem(struct irdma_device *iwdev,
> +						     struct ib_umem *region,
> +						     struct ib_pd *pd,
> +						     enum irdma_memreg_type reg_type,
> +						     struct irdma_mem_reg_req *req)
> +{
> +	struct irdma_pbl *iwpbl;
> +	struct irdma_mr *iwmr;
> +	u32 total;
> +	u8 lvl;
> +	int err;
> +
> +	/* iWarp: Catch page not starting on OS page boundary */
> +	if (!rdma_protocol_roce(&iwdev->ibdev, 1) && ib_umem_offset(region))
> +		return ERR_PTR(-EINVAL);

[Severity: Critical]
Can passing an IB_UVERBS_BUFFER_TYPE_DMABUF descriptor via
UVERBS_ATTR_CREATE_QP_BUF_UMEM cause a NULL pointer dereference or write to
the wrong page during iWARP connection establishment?

In irdma_setup_umode_qp(), ib_umem_get_attr() resolves the buffer descriptor
through __ib_umem_get_desc_dir(), which accepts both IB_UVERBS_BUFFER_TYPE_VA
and IB_UVERBS_BUFFER_TYPE_DMABUF. Previously, IRDMA_MEMREG_TYPE_QP MRs could
only be registered via irdma_reg_user_mr() -> ib_umem_get_va(), whereas
irdma_reg_user_mr_dmabuf() hardcoded IRDMA_MEMREG_TYPE_MEM.

When irdma_setup_q_pbl_from_umem() calls irdma_handle_q_mem() ->
irdma_setup_pbles() -> irdma_copy_user_pgaddrs(), the driver extracts the
first SQ page from the scatterlist:

drivers/infiniband/hw/irdma/verbs.c:irdma_copy_user_pgaddrs() {
	if (iwmr->type == IRDMA_MEMREG_TYPE_QP)
		iwpbl->qp_mr.sq_page = sg_page(region->sgt_append.sgt.sgl);
}

and irdma_setup_virt_qp() assigns iwqp->page = qpmr->sq_page, while
iwqp->sc_qp.qp_uk.sq_base remains NULL for a user-mode QP.

On an iWARP device, irdma_accept() and irdma_cm_event_connected() map
iwqp->page to write the LSMM or RTT WQE:

drivers/infiniband/hw/irdma/cm.c:irdma_accept() {
	if (iwqp->page)
		iwqp->sc_qp.qp_uk.sq_base = kmap_local_page(iwqp->page);

	cm_node->lsmm_size = accept.size + conn_param->private_data_len;
	irdma_sc_send_lsmm(&iwqp->sc_qp, iwqp->ietf_mem.va, cm_node->lsmm_size,
			   ibmr->lkey);
}

drivers/infiniband/hw/irdma/cm.c:irdma_cm_event_connected() {
	if (iwqp->page)
		iwqp->sc_qp.qp_uk.sq_base = kmap_local_page(iwqp->page);
	irdma_sc_send_rtt(&iwqp->sc_qp, read0);
	if (iwqp->page)
		kunmap_local(iwqp->sc_qp.qp_uk.sq_base);
}

With a dmabuf-backed ib_umem, calling sg_page() on the scatterlist breaks in
two ways:

1. When CONFIG_DMABUF_DEBUG is enabled (where dma_buf_wrap_sg_table() clears
   page_link) or when the exporter does not populate struct page pointers
   (such as uverbs_dmabuf or P2P/MMIO exporters), sg_page() returns NULL.
   This leaves iwqp->page NULL, so irdma_accept() and
   irdma_cm_event_connected() skip kmap_local_page() and pass a NULL
   qp_uk->sq_base into irdma_sc_send_lsmm() / irdma_sc_send_rtt(), which
   immediately dereferences qp_uk->sq_base->elem.

2. Even when sg_page() is populated by a system-RAM exporter, if userspace
   passes a non-zero page-aligned offset within a coalesced multi-page SG
   entry, ib_umem_dmabuf_map_pages() advances sg_dma_address(sg) and
   decrements sg_dma_len(sg) in place without updating sg->page_link:

   drivers/infiniband/core/umem_dmabuf.c:ib_umem_dmabuf_map_pages() {
		if (cur <= start && start < cur + sg_dma_len(sg)) {
			unsigned long offset = start - cur;

			umem_dmabuf->first_sg = sg;
			umem_dmabuf->first_sg_offset = offset;
			sg_dma_address(sg) += offset;
			sg_dma_len(sg) -= offset;
			cur += offset;
		}
   }

   As a result, sg_page(region->sgt_append.sgt.sgl) points to the start of
   that SG segment rather than the SQ page at the requested offset, causing
   kmap_local_page(iwqp->page) to map and overwrite the wrong page.

Should irdma_setup_q_pbl_from_umem() (or irdma_setup_umode_qp()) reject
region->is_dmabuf, at least when !rdma_protocol_roce(&iwdev->ibdev, 1) (or
unconditionally if dmabuf queue rings are not supported)?

> +
> +	iwmr = irdma_alloc_iwmr(region, &iwdev->ibdev, pd, region->address, reg_type);
> +	if (IS_ERR(iwmr))
> +		return ERR_CAST(iwmr);
[ ... ]

ওয়াও! (Wait, let me make sure there is no extra text at the end! Oh! Why did those characters appear? Let me remove that!)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016202909.1289496-1-dhu@x6u.co?part=3