Re: [PATCH 6/7] RDMA/zrdma: Implement context, PD, and mmap operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] RDMA/zrdma: resource leak on probe failure due to late ib_set_device_ops()
- [Medium] RDMA/zrdma: truncated SRQ doorbell mmap range in zxdh_alloc_ucontext()
--

commit 42854f9bc8b33facfd22e1942728befbae8346de
Author: Yanze Zhang <zhang.yanze@zte.com.cn>

RDMA/zrdma: Implement context, PD, and mmap operations

This commit implements protection domain allocation and deallocation, user
context management, and doorbell memory mapping for the zrdma driver. It also
adds device teardown and initialization logic for RDMA device registration.

> diff --git a/drivers/infiniband/hw/zrdma/zrdma_verbs.c b/drivers/infiniband/hw/zrdma/zrdma_verbs.c
> index 6db536c94e01a..54b016ea8f5de 100644
> --- a/drivers/infiniband/hw/zrdma/zrdma_verbs.c
> +++ b/drivers/infiniband/hw/zrdma/zrdma_verbs.c
> @@ -7,24 +7,370 @@
>  #include "zrdma_main.h"
>  #include "zrdma_verbs.h"
>  #include "zrdma_ctrl.h"
> +#include <rdma/ib_mad.h>
> +#include <rdma/ib_cache.h>
> +#include <rdma/ib_user_verbs.h>
> +#include <linux/etherdevice.h>
>  
> -static const struct ib_device_ops zxdh_dev_ops = {
> -	.owner = THIS_MODULE,
> -	.driver_id = RDMA_DRIVER_ZRDMA,
> -	.uverbs_abi_ver = ZXDH_ABI_VER,
> -};

[ ... ]

> +static void zxdh_ib_dealloc_device(struct ib_device *ibdev)
> +{
> +	struct zxdh_device *zdev = to_zdev(ibdev);
> +
> +	if (!zdev)
> +		return;
> +
> +	if (zdev->rf) {
> +		kvfree(zdev->rf->qp_cnt_8k_idxs);
> +		kvfree(zdev->rf->mem_rsrc);
> +		kfree(zdev->rf);
> +		zdev->rf = NULL;
> +		/* rf == NULL => zxdh_probe early failure path; ah_list_lock
> +		 * not initialised, must not be destroyed.
> +		 */
> +		mutex_destroy(&zdev->ah_list_lock);
> +	}
> +	kfree(zdev->hdl);
> +	zdev->hdl = NULL;
> +}

[ ... ]

> +static int zxdh_alloc_ucontext(struct ib_ucontext *uctx, struct ib_udata *udata)
> +{
> +	struct zxdh_ucontext *ucontext = to_ucontext(uctx);
> +	struct zxdh_alloc_ucontext_resp uresp = {};
> +	struct ib_device *ibdev = uctx->device;
> +	struct zxdh_device *zdev = to_zdev(ibdev);
> +	struct zxdh_alloc_ucontext_req req = {};

[ ... ]

> +	uresp.srq_db_bar_off = SRQ_DB_OFFSET;

[ ... ]

> +	if (ucontext->abi_ver == ZXDH_CONTEXT_VER_V1 ||
> +	    ucontext->abi_ver == ZXDH_CONTEXT_VER_V2) {
> +		ucontext->srq_db_mmap_entry =
> +			zxdh_mp_mmap_entry_insert(ucontext, SRQ_DB_BAR_OFF,
> +						  PAGE_SIZE,
> +						  ZXDH_MMAP_IO_NC_BY_SIZE,
> +						  &uresp.srq_db_mmap_key);
> +	} else {
> +		ucontext->srq_db_mmap_entry =
> +			zxdh_mp_mmap_entry_insert(ucontext, SRQ_DB_BAR_OFF,
> +						  SRQ_DB_MMAP_SIZE,
> +						  ZXDH_MMAP_IO_NC_BY_SIZE,
> +						  &uresp.srq_db_mmap_key);
> +	}
> +	if (!ucontext->srq_db_mmap_entry) {
> +		rdma_user_mmap_entry_remove(ucontext->sq_db_mmap_entry);
> +		rdma_user_mmap_entry_remove(ucontext->cq_db_mmap_entry);
> +		pr_err("zrdma: srq_db_mmap_entry is NULL!\n");
> +		return -ENOMEM;
> +	}
> +
> +	uresp.srq_db_mmap_size = SRQ_DB_MMAP_SIZE;

[ ... ]

> +static int zxdh_mmap(struct ib_ucontext *context, struct vm_area_struct *vma)
> +{
> +	struct rdma_user_mmap_entry *rdma_entry;
> +	struct zxdh_user_mmap_entry *entry;
> +	struct zxdh_ucontext *ucontext;
> +	u64 pfn;
> +	int ret;
> +
> +	ucontext = to_ucontext(context);
> +
> +	rdma_entry = rdma_user_mmap_entry_get(&ucontext->ibucontext, vma);
> +	if (!rdma_entry) {
> +		pr_err("zrdma: pgoff[0x%lx] does not have valid entry\n",
> +		       vma->vm_pgoff);
> +		return -EINVAL;
> +	}
> +
> +	entry = to_zxdh_mmap_entry(rdma_entry);
> +
> +	pfn = (entry->bar_offset +
> +	       pci_resource_start(ucontext->zdev->rf->pcidev, 0)) >>
> +	      PAGE_SHIFT;
> +
> +	switch (entry->mmap_flag) {
> +	case ZXDH_MMAP_IO_NC:
> +		ret = rdma_user_mmap_io(context, vma, pfn, PAGE_SIZE,
> +					pgprot_noncached(vma->vm_page_prot),
> +					rdma_entry);
> +		break;
> +	case ZXDH_MMAP_IO_NC_BY_SIZE:
> +		ret = rdma_user_mmap_io(context, vma, pfn,
> +					vma->vm_end - vma->vm_start,
> +					pgprot_noncached(vma->vm_page_prot),
> +					rdma_entry);
> +		break;

[Severity: Medium]
In drivers/infiniband/hw/zrdma/zrdma_verbs.h, SRQ_DB_BAR_OFF is defined as
0x62000 and SRQ_DB_MMAP_SIZE is 0x10000:

#define ZXDH_PAGE_OFFSET_NUM (PAGE_SIZE - 1)
...
#define SRQ_DB_MMAP_SIZE 0x10000
#define SRQ_DB_BAR_OFF 0x62000

On kernels with 16KB or 64KB pages, SRQ_DB_BAR_OFF has an intra-page offset of
0x2000. When zxdh_alloc_ucontext() registers ucontext->srq_db_mmap_entry with
length SRQ_DB_MMAP_SIZE (0x10000) for ZXDH_CONTEXT_VER_V3, and zxdh_mmap()
computes:

	pfn = (entry->bar_offset +
	       pci_resource_start(ucontext->zdev->rf->pcidev, 0)) >>
	      PAGE_SHIFT;

pfn is rounded down to BAR0 offset 0x60000. Mapping 0x10000 bytes from that
pfn only covers BAR0 [0x60000, 0x70000). When userspace adds srq_db_bar_off
(0x2000) to the mapped base address, won't the upper 0x2000 bytes of the SRQ
doorbell region at [0x70000, 0x72000) remain unmapped?

Also, when ucontext->abi_ver is ZXDH_CONTEXT_VER_V1 or ZXDH_CONTEXT_VER_V2,
zxdh_alloc_ucontext() inserts ucontext->srq_db_mmap_entry with length
PAGE_SIZE, but still sets uresp.srq_db_mmap_size = SRQ_DB_MMAP_SIZE. Should
uresp.srq_db_mmap_size match the registered entry size for V1 and V2 so
rdma_user_mmap_entry_get() does not reject a mapping of uresp.srq_db_mmap_size
bytes?

[ ... ]

>  static int zxdh_init_rdma_device(struct zxdh_device *zdev)
>  {
> +	struct pci_dev *pcidev = zdev->rf->pcidev;
> +
> +	if (zdev->roce_mode)
> +		zxdh_init_roce_device(zdev);
> +	else
> +		return -EPFNOSUPPORT;
> +
>  	zdev->ibdev.phys_port_cnt = 1;
>  	zdev->ibdev.num_comp_vectors = zdev->rf->ceqs_count;
> -	zdev->ibdev.dev.parent = &zdev->rf->pcidev->dev;
> -
> -	ib_set_device_ops(&zdev->ibdev, &zxdh_dev_ops);
> +	zdev->ibdev.dev.parent = &pcidev->dev;
>  
>  	return 0;
>  }
>  
> +static const struct ib_device_ops zxdh_dev_ops = {
> +	.owner = THIS_MODULE,
> +	.driver_id = RDMA_DRIVER_ZRDMA,
> +	.uverbs_abi_ver = ZXDH_ABI_VER,
> +	.alloc_pd = zxdh_alloc_pd,
> +	.alloc_ucontext = zxdh_alloc_ucontext,
> +	.dealloc_driver = zxdh_ib_dealloc_device,
> +	.dealloc_pd = zxdh_dealloc_pd,
> +	.dealloc_ucontext = zxdh_dealloc_ucontext,
> +	.mmap = zxdh_mmap,
> +	.mmap_free = zxdh_mmap_free,
> +
> +	INIT_RDMA_OBJ_SIZE(ib_pd, zxdh_pd, ibpd),
> +	INIT_RDMA_OBJ_SIZE(ib_ucontext, zxdh_ucontext, ibucontext),
> +};
> +
>  int zxdh_ib_register_device(struct zxdh_device *zdev)
>  {
>  	int ret;
> @@ -33,6 +379,8 @@ int zxdh_ib_register_device(struct zxdh_device *zdev)
>  	if (ret)
>  		return ret;
>  
> +	ib_set_device_ops(&zdev->ibdev, &zxdh_dev_ops);

[Severity: High]
Does calling ib_set_device_ops() here in zxdh_ib_register_device() leak
zdev->rf, zdev->hdl, zdev->rf->qp_cnt_8k_idxs, and zdev->rf->mem_rsrc if an
earlier failure occurs during zxdh_probe()?

In drivers/infiniband/hw/zrdma/zrdma_main.c, zxdh_probe() relies on
ib_dealloc_device(&zdev->ibdev) to invoke zxdh_ib_dealloc_device() on its
error paths:

	hdl = kzalloc_obj(*hdl);
	if (!hdl) {
		ib_dealloc_device(&zdev->ibdev);
		return -ENOMEM;
	}
...
	rf = zdev->rf;
	err = zxdh_ctrl_init_hw(rf);
	if (err)
		goto err_ctrl_init;

	err = zxdh_ib_register_device(zdev);
	if (err)
		goto err_ib_reg;
...
err_ib_reg:
err_ctrl_init:
	ib_dealloc_device(&zdev->ibdev);

If kzalloc_obj(*hdl), zxdh_ctrl_init_hw(), or zxdh_init_rdma_device() fails
(or even in follow-up commit 5562e1f8054b where ib_set_device_ops() is at the
end of zxdh_init_rdma_device()), zdev->ibdev.ops.dealloc_driver is still NULL
when ib_dealloc_device() runs, so zxdh_ib_dealloc_device() is never called.

Should ib_set_device_ops() be called right after ib_alloc_device() so that
zxdh_ib_dealloc_device() is always invoked on probe failure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024071539.2182864-1-zhang.yanze@zte.com.cn?part=6