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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] RDMA/irdma: integer overflow in irdma_setup_umode_cq() CQ page calculation
- [High] RDMA/irdma: unaligned CQ and shadow UMEMs bypass 4K page offset check
--

commit 79df0143a3db151761db4b97121b37dcf76bacb9
Author: David Hu <xuehaohu@google.com>

RDMA/irdma: Support UMEM attribute for CQ creation

This commit adds support for one-step CQ creation using
UVERBS_ATTR_CREATE_CQ_BUF_UMEM and IRDMA_IB_ATTR_CREATE_CQ_SHADOW_BUF_UMEM
to pin user buffers and set up hardware PBL entries directly from UMEM. It
also updates irdma_cq_free_rsrc() and irdma_resize_cq() to release UMEM and
PBLE resources when the user ring reference count reaches zero.

> diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c
> index 4b7e9c2d9cff5..02ed7ab99e48d 100644
> --- a/drivers/infiniband/hw/irdma/verbs.c
> +++ b/drivers/infiniband/hw/irdma/verbs.c

[ ... ]

> @@ -2746,14 +2768,18 @@ static int irdma_setup_umode_cq(struct irdma_device *iwdev,
>  				struct irdma_cq_init_info *info,
>  				struct uverbs_attr_bundle *attrs)
>  {
> +	struct irdma_cq_uk_init_info *ukinfo = &info->cq_uk_init_info;
>  	struct ib_udata *udata = &attrs->driver_udata;
>  	struct irdma_create_cq_req req = {};
>  	struct irdma_pci_f *rf = iwdev->rf;
> +	struct irdma_sc_dev *dev = &rf->sc_dev;
>  	struct irdma_ucontext *ucontext;
>  	struct irdma_cq_mr *cqmr_shadow;
>  	struct irdma_cq_mr *cqmr;
>  	unsigned long flags;
> +	bool cqe_64byte_ena;
>  	int err_code;
> +	u8 cqe_size;
>  
>  	iwcq->user_mode = true;
>  	ucontext = rdma_udata_to_drv_context(udata, struct irdma_ucontext,
> @@ -2767,26 +2793,87 @@ static int irdma_setup_umode_cq(struct irdma_device *iwdev,
>  	if (err_code)
>  		return err_code;
>  
> -	spin_lock_irqsave(&ucontext->cq_reg_mem_list_lock, flags);
> -	iwcq->iwpbl = irdma_get_pbl((unsigned long)req.user_cq_buf,
> -				    &ucontext->cq_reg_mem_list);
> -	spin_unlock_irqrestore(&ucontext->cq_reg_mem_list_lock, flags);
> -	if (!iwcq->iwpbl)
> -		return -EPROTO;
> +	cqe_64byte_ena = dev->hw_attrs.uk_attrs.feature_flags & IRDMA_FEATURE_64_BYTE_CQE ?
> +			 true : false;
> +	cqe_size = cqe_64byte_ena ? 64 : 32;
> +
> +	if (uverbs_attr_is_valid(attrs, UVERBS_ATTR_CREATE_CQ_BUF_UMEM)) {
> +		struct ib_umem *umem;
> +		struct irdma_mem_reg_req cq_req = {};
> +		u8 shadow_pgcnt = (rf->sc_dev.hw_attrs.uk_attrs.feature_flags &
> +				   IRDMA_FEATURE_CQ_RESIZE) ? 0 : IRDMA_SHADOW_PGCNT;
> +		u32 min_size;
> +
> +		cq_req.cq_pages = DIV_ROUND_UP(ukinfo->cq_size * cqe_size,
> +					       IRDMA_HW_PAGE_SIZE);
> +		min_size = (cq_req.cq_pages + shadow_pgcnt) << IRDMA_HW_PAGE_SHIFT;

[Severity: Critical]
Can ukinfo->cq_size * cqe_size overflow 32-bit arithmetic or truncate when
assigned to the __u16 cq_req.cq_pages field here?

Unlike irdma_create_cq() and irdma_resize_cq(), irdma_create_user_cq() and
irdma_alloc_cq() do not check attr->cqe > rf->max_cqe before setting
ukinfo->cq_size:

drivers/infiniband/hw/irdma/verbs.c:irdma_create_user_cq() {
	err_code = cq_validate_flags(attr->flags, dev->hw_attrs.uk_attrs.hw_rev);
	if (err_code)
		return err_code;

	if (udata->outlen < IRDMA_CREATE_CQ_MIN_RESP_LEN)
		return -EINVAL;

	err_code = irdma_alloc_cq(iwdev, iwcq, attr, &info);
}

Because ukinfo->cq_size is u32, cqe_size is u8 (32 or 64), and
cq_req.cq_pages in struct irdma_mem_reg_req is __u16, a large user-supplied
UVERBS_ATTR_CREATE_CQ_CQE value (such as 0x00400040 or 0x04000040 when
cqe_size is 64) causes cq_req.cq_pages to wrap or truncate to 1.

When that happens, min_size becomes 4096 bytes, ib_umem_get_cq_buf() pins a
single 4KB page, and irdma_setup_q_pbl_from_umem() accepts the 1-page UMEM
in PBLE_LEVEL_0 physical mode while __irdma_create_cq() ->
irdma_sc_cq_create() programs the unvalidated cq->cq_uk.cq_size into the
hardware CQP WQE. Could this allow hardware DMA writes beyond the single
pinned page?

> +
> +		umem = ib_umem_get_cq_buf(&iwdev->ibdev, attrs, min_size,
> +					  IB_ACCESS_LOCAL_WRITE);
> +		if (IS_ERR(umem))
> +			return PTR_ERR(umem);
> +
> +		iwcq->iwpbl = irdma_setup_q_pbl_from_umem(iwdev, umem, NULL,
> +							  IRDMA_MEMREG_TYPE_CQ, &cq_req);
> +		if (IS_ERR(iwcq->iwpbl)) {
> +			err_code = PTR_ERR(iwcq->iwpbl);
> +			iwcq->iwpbl = NULL;
> +			ib_umem_release(umem);
> +			return err_code;
> +		}
> +	} else {
> +		if (!req.user_cq_buf)
> +			return -EINVAL;
> +
> +		spin_lock_irqsave(&ucontext->cq_reg_mem_list_lock, flags);
> +		iwcq->iwpbl = irdma_get_pbl((unsigned long)req.user_cq_buf,
> +					    &ucontext->cq_reg_mem_list);
> +		spin_unlock_irqrestore(&ucontext->cq_reg_mem_list_lock, flags);
> +		if (!iwcq->iwpbl)
> +			return -EPROTO;
> +	}
>  
>  	cqmr = &iwcq->iwpbl->cq_mr;
>  
>  	if (rf->sc_dev.hw_attrs.uk_attrs.feature_flags &
>  	    IRDMA_FEATURE_CQ_RESIZE) {
> -		spin_lock_irqsave(&ucontext->cq_reg_mem_list_lock, flags);
> -		iwcq->iwpbl_shadow =
> -			irdma_get_pbl((unsigned long)req.user_shadow_area,
> -				      &ucontext->cq_reg_mem_list);
> -		spin_unlock_irqrestore(&ucontext->cq_reg_mem_list_lock, flags);
> +		if (uverbs_attr_is_valid(attrs, IRDMA_IB_ATTR_CREATE_CQ_SHADOW_BUF_UMEM)) {
> +			struct ib_umem *shadow_umem;
> +			struct irdma_mem_reg_req shadow_req = {
> +				.cq_pages = IRDMA_SHADOW_PGCNT,
> +			};
> +
> +			shadow_umem =
> +				ib_umem_get_attr(&iwdev->ibdev, attrs,
> +						 IRDMA_IB_ATTR_CREATE_CQ_SHADOW_BUF_UMEM,
> +						 IRDMA_SHADOW_AREA_SIZE << 3,
> +						 IB_ACCESS_LOCAL_WRITE);
> +			if (IS_ERR(shadow_umem))
> +				return PTR_ERR(shadow_umem);
> +
> +			iwcq->iwpbl_shadow =
> +				irdma_setup_q_pbl_from_umem(iwdev, shadow_umem, NULL,
> +							    IRDMA_MEMREG_TYPE_CQ,
> +							    &shadow_req);

[Severity: High]
What happens on RoCE if userspace passes a non-4KB-aligned buffer in
UVERBS_ATTR_CREATE_CQ_BUF_UMEM or IRDMA_IB_ATTR_CREATE_CQ_SHADOW_BUF_UMEM?

In irdma_setup_q_pbl_from_umem(), the page-offset check is skipped on RoCE,
and region->address is passed as virt to irdma_alloc_iwmr():

drivers/infiniband/hw/irdma/verbs.c:irdma_setup_q_pbl_from_umem() {
	/* iWarp: Catch page not starting on OS page boundary */
	if (!rdma_protocol_roce(&iwdev->ibdev, 1) && ib_umem_offset(region))
		return ERR_PTR(-EINVAL);

	iwmr = irdma_alloc_iwmr(region, &iwdev->ibdev, pd, region->address, reg_type);
}

Inside irdma_alloc_iwmr(), ib_umem_find_best_pgsz(region, SZ_4K, virt)
compares the virtual address alignment against the physical page offset, so
the intra-page offset cancels out and returns SZ_4K even when
region->address is not 4KB-aligned.

Later, irdma_handle_q_mem() -> irdma_setup_pbles() ->
irdma_copy_user_pgaddrs() populates the PBL using
rdma_block_iter_dma_address(&biter), which masks off the low 12 bits
(~(iwmr->page_size - 1)). Doesn't this set info->cq_base_pa (or the PBLE
entries) and info->shadow_area_pa to offset 0 of the 4KB page rather than
the user buffer's offset within the page, causing the hardware to DMA to or
from the wrong offset in that page?

[ ... ]

>  		cqmr_shadow = &iwcq->iwpbl_shadow->cq_mr;
>  		info->shadow_area_pa = cqmr_shadow->cq_pbl.addr;
>  	} else {

[ ... ]

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