Re: [PATCH rdma-next v3 4/6] RDMA/irdma: Split CQ setup to umode and kmode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] RDMA/irdma: wrong CQ ID freed in irdma_cq_free_rsrc() on create error
--

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

RDMA/irdma: Split CQ setup to umode and kmode

This commit separates user-mode and kernel-mode CQ creation by adding the
irdma_create_user_cq() callback and refactoring common setup logic into
shared helper functions.

> diff --git a/drivers/infiniband/hw/irdma/verbs.c b/drivers/infiniband/hw/irdma/verbs.c
> index ab25445f38282..4b7e9c2d9cff5 100644
> --- a/drivers/infiniband/hw/irdma/verbs.c
> +++ b/drivers/infiniband/hw/irdma/verbs.c
> @@ -2624,41 +2624,198 @@ static inline int cq_validate_flags(u32 flags, u8 hw_rev)
>  	return flags & ~IB_UVERBS_CQ_FLAGS_TIMESTAMP_COMPLETION ? -EOPNOTSUPP : 0;
>  }
>  
> -/**
> - * irdma_create_cq - create cq
> - * @ibcq: CQ allocated
> - * @attr: attributes for cq
> - * @attrs: uverbs attribute bundle
> - */
> -static int irdma_create_cq(struct ib_cq *ibcq,
> -			   const struct ib_cq_init_attr *attr,
> -			   struct uverbs_attr_bundle *attrs)
> +static int __irdma_create_cq(struct irdma_device *iwdev,
> +			     struct irdma_cq *iwcq,
> +			     struct irdma_cq_init_info *info,
> +			     struct ib_udata *udata)
>  {
[ ... ]
> +	info->shadow_read_threshold = min(info->cq_uk_init_info.cq_size / 2,
> +					  (u32)IRDMA_MAX_CQ_READ_THRESH);
> +
> +	if (irdma_sc_cq_init(cq, info)) {
> +		ibdev_dbg(&iwdev->ibdev, "VERBS: init cq fail\n");
> +		err_code = -EPROTO;
> +		goto cq_free_rsrc;
> +	}

[ ... ]

> @@ -2674,169 +2831,99 @@ static int irdma_create_cq(struct ib_cq *ibcq,
>  	iwcq->cq_num = cq_num;
>  	iwcq->iwpbl = NULL;
>  	iwcq->iwpbl_shadow = NULL;
> -	info.dev = dev;
> +	info->dev = dev;
>  	ukinfo->cq_size = max(entries, 4);
>  	ukinfo->cq_id = cq_num;
[ ... ]
> +static int irdma_create_user_cq(struct ib_cq *ibcq,
> +				const struct ib_cq_init_attr *attr,
> +				struct uverbs_attr_bundle *attrs)
> +{
[ ... ]
> +	err_code = irdma_alloc_cq(iwdev, iwcq, attr, &info);
> +	if (err_code)
> +		return err_code;
>  
> -		if (entries * cqe_size == IRDMA_HW_PAGE_SIZE)
> -			entries += 2;
> +	err_code = irdma_setup_umode_cq(iwdev, iwcq, &info, attrs);
> +	if (err_code)
> +		goto cq_free_rsrc;
>  
> -		ukinfo->cq_size = entries;
> +	return __irdma_create_cq(iwdev, iwcq, &info, udata);
> +cq_free_rsrc:
> +	irdma_cq_free_rsrc(rf, iwcq);
>  
> +	return err_code;
> +}
>  
> +/**
> + * irdma_create_cq - create kmode cq
> + * @ibcq: CQ allocated
> + * @attr: attributes for cq
> + * @attrs: uverbs attribute bundle
> + */
> +static int irdma_create_cq(struct ib_cq *ibcq,
> +			   const struct ib_cq_init_attr *attr,
> +			   struct uverbs_attr_bundle *attrs)
> +{
[ ... ]
> +	err_code = irdma_alloc_cq(iwdev, iwcq, attr, &info);
> +	if (err_code)
> +		return err_code;
>  
> +	err_code = irdma_setup_kmode_cq(iwdev, iwcq, &info, attr);
>  	if (err_code)
>  		goto cq_free_rsrc;
>  
> +	return __irdma_create_cq(iwdev, iwcq, &info, NULL);
>  cq_free_rsrc:
>  	irdma_cq_free_rsrc(rf, iwcq);

[Severity: High]
This problem wasn't introduced by this patch, but if irdma_setup_umode_cq()
in irdma_create_user_cq(), irdma_setup_kmode_cq() in irdma_create_cq(), or
the early PBLE bounds check in irdma_sc_cq_init() fails, does
irdma_cq_free_rsrc() free the wrong CQ ID from rf->allocated_cqs?

In irdma_alloc_cq(), the allocated cq_num is saved in iwcq->cq_num and
ukinfo->cq_id:

	iwcq->cq_num = cq_num;
	...
	ukinfo->cq_id = cq_num;

However, cq->cq_uk.cq_id is only populated later inside __irdma_create_cq()
when irdma_sc_cq_init() calls irdma_uk_cq_init(&cq->cq_uk,
&info->cq_uk_init_info).

Before that happens, cq->cq_uk.cq_id is still 0, while irdma_cq_free_rsrc()
frees cq->cq_uk.cq_id:

drivers/infiniband/hw/irdma/verbs.c:irdma_cq_free_rsrc() {
	...
	irdma_free_rsrc(rf, rf->allocated_cqs, cq->cq_uk.cq_id);
}

When one of these pre-init error paths jumps to cq_free_rsrc, wouldn't this
leak iwcq->cq_num and clear reserved bit 0 (IRDMA_RSVD_CQ_ID_CQP) in
rf->allocated_cqs instead?

[ ... ]

>
> -- 
> 2.51.2
>

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