Re: [PATCH rdma v7 5/7] RDMA/zrdma: Add device probe and remove logic

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] RDMA/zrdma: uninitialized ceqs_count and hw.device in zxdh_probe()
- [High] RDMA/zrdma: un-cleared pointer in zxdh_initialize_hw_rsrc() causes double free
- [Low] RDMA/zrdma: unpopulated zxdh_rdma_list and unused zxdh_handlers list
--

commit 63daa22995c515a9faef7a5a084166c75b7006ce
Author: Yanze Zhang <zhang.yanze@zte.com.cn>

RDMA/zrdma: Add device probe and remove logic

This commit implements the auxiliary bus probe and remove functions for the
zrdma driver, including device initialization, hardware resource setup,
handler list management, and IB device registration and deallocation.

> diff --git a/drivers/infiniband/hw/zrdma/zrdma_main.c b/drivers/infiniband/hw/zrdma/zrdma_main.c
> index af1d37be63362..14c36531cc453 100644
> --- a/drivers/infiniband/hw/zrdma/zrdma_main.c
> +++ b/drivers/infiniband/hw/zrdma/zrdma_main.c

[ ... ]

> +LIST_HEAD(zxdh_handlers);
> +DEFINE_SPINLOCK(zxdh_handler_lock);
>  
> -MODULE_DEVICE_TABLE(auxiliary, zxdh_auxiliary_id_table);
> +static struct mutex rdma_name_lock;
> +static struct list_head zxdh_rdma_list;
> +
> +static void zxdh_add_handler(struct zxdh_handler *hdl)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&zxdh_handler_lock, flags);
> +	list_add(&hdl->list, &zxdh_handlers);
> +	spin_unlock_irqrestore(&zxdh_handler_lock, flags);
> +}
> +
> +void zxdh_del_handler(struct zxdh_handler *hdl)
> +{
> +	unsigned long flags;
> +
> +	if (!hdl)
> +		return;
> +
> +	spin_lock_irqsave(&zxdh_handler_lock, flags);
> +	list_del(&hdl->list);
> +	spin_unlock_irqrestore(&zxdh_handler_lock, flags);
> +}
> +
> +int zxdh_get_del_rdma_name(const char *sbdf, char *name, size_t name_len)
> +{
> +	struct zxdh_rdma_node *pos, *tmp;
> +	size_t len, sbdf_len;
> +
> +	if (!sbdf || !name || !name_len)
> +		return -EINVAL;
> +
> +	if (name_len > IB_DEVICE_NAME_MAX) {
> +		pr_err("zrdma: name buffer too large\n");
> +		return -EINVAL;
> +	}
> +
> +	sbdf_len = strlen(sbdf);
> +
> +	mutex_lock(&rdma_name_lock);
> +	list_for_each_entry_safe(pos, tmp, &zxdh_rdma_list, list) {
> +		len = strlen(pos->sbdf);
> +		if (sbdf_len == len &&
> +		    strncasecmp(sbdf, pos->sbdf, len) == 0) {
> +			snprintf(name, name_len, "%s", pos->name);
> +			list_del(&pos->list);
> +			kfree(pos);
> +			mutex_unlock(&rdma_name_lock);
> +			return 0;
> +		}
> +	}
> +	mutex_unlock(&rdma_name_lock);
> +	return -ENOENT;
> +}

[Severity: Low]
Are zxdh_rdma_list and zxdh_handlers used anywhere in the driver?

zxdh_rdma_list is static to zrdma_main.c and initialized in
zrdma_module_init(), but nothing in the driver or patch series ever inserts
a struct zxdh_rdma_node into it. As a result, zxdh_get_del_rdma_name() can
only return -EINVAL or -ENOENT, and the list traversal here and in
zrdma_module_exit() cannot match any entry.

In addition, zxdh_get_del_rdma_name() rejects caller buffers when
name_len > IB_DEVICE_NAME_MAX, and zrdma_module_init() initializes
rdma_name_lock dynamically without calling mutex_destroy(&rdma_name_lock) if
auxiliary_driver_register() fails.

Similarly, zxdh_probe() allocates struct zxdh_handler and adds it to
zxdh_handlers via zxdh_add_handler(), and zxdh_remove() removes and frees
it, but nothing in the series ever reads or iterates zxdh_handlers.

Could these unused lists and helpers be dropped until they have users?

[ ... ]

> +static void zxdh_fill_device_info(struct zxdh_device *zdev,
> +				  struct zxdh_rdma_core_dev *zdev_info)
> +{
> +	struct zxdh_pci_f *rf = zdev->rf;
> +
> +	rf->ftype = ZXDH_FUNC_TYPE(zdev_info->vport_id);
> +	rf->pf_id = ZXDH_PF_ID(zdev_info->vport_id);
> +	rf->sc_dev.ep_id = ZXDH_EP_ID(zdev_info->vport_id);
> +	rf->ep_id = rf->sc_dev.ep_id;
> +	rf->sc_dev.driver_load = true;
> +
> +	rf->zdev_info = zdev_info;
> +	rf->pcidev = zdev_info->pdev;
> +
> +	rf->msix_count = zdev_info->msix_count;
> +	rf->msix_entries = zdev_info->msix_entries;
> +	if (rf->msix_count)
> +		rf->sc_dev.max_ceqs = (rf->msix_count - 1);
> +	else
> +		rf->sc_dev.max_ceqs = 0;

[ ... ]

> +static int zxdh_probe(struct auxiliary_device *aux_dev,
> +		      const struct auxiliary_device_id *id)
> +{

[ ... ]

> +	rf = zdev->rf;
> +	err = zxdh_ctrl_init_hw(rf);
> +	if (err)
> +		goto err_dealloc;
> +
> +	err = zxdh_ib_register_device(zdev);
> +	if (err)
> +		goto err_dealloc;

[Severity: High]
Where is zdev->rf->ceqs_count initialized before zxdh_ib_register_device() is
called?

In zxdh_probe(), zdev->rf is allocated with kzalloc_obj(*zdev->rf), and
zxdh_fill_device_info() sets rf->msix_count and rf->sc_dev.max_ceqs, but
neither zxdh_fill_device_info() nor zxdh_ctrl_init_hw() sets rf->ceqs_count.

When zxdh_probe() calls zxdh_ib_register_device(zdev),
zxdh_init_rdma_device() sets:

drivers/infiniband/hw/zrdma/zrdma_verbs.c:zxdh_init_rdma_device() {
	zdev->ibdev.phys_port_cnt = 1;
	zdev->ibdev.num_comp_vectors = zdev->rf->ceqs_count;
	zdev->ibdev.dev.parent = &zdev->rf->pcidev->dev;

	return 0;
}

With zdev->rf->ceqs_count left at 0 (even at the end of the patch series),
ibdev.num_comp_vectors is registered as 0, which causes uverbs CQ creation to
reject all completion vectors with -EINVAL and causes ib_cq_pool_get() to
divide by zero (% dev->num_comp_vectors).

Additionally, at this point in the series (prior to commit "RDMA/zrdma:
Implement device and port query operations"), zdev->rf->hw.device is still
NULL and mandatory ib_device_ops callbacks (get_port_immutable and
query_device) are not yet installed when zxdh_ib_register_device() passes
zdev->rf->hw.device to ib_register_device().

> diff --git a/drivers/infiniband/hw/zrdma/zrdma_verbs.c b/drivers/infiniband/hw/zrdma/zrdma_verbs.c
> index 90ff9a8264918..d47dfafa717ad 100644
> --- a/drivers/infiniband/hw/zrdma/zrdma_verbs.c
> +++ b/drivers/infiniband/hw/zrdma/zrdma_verbs.c
> @@ -12,8 +12,37 @@ const struct ib_device_ops zxdh_dev_ops = {
>  	.owner = THIS_MODULE,
>  	.driver_id = RDMA_DRIVER_ZRDMA,
>  	.uverbs_abi_ver = ZXDH_ABI_VER,
> +	.dealloc_driver = zxdh_ib_dealloc_device,
>  };
>  
> +/* Release everything the probe path may have allocated. The hook is
> + * called from ib_dealloc_device(), so it must tolerate a partially
> + * initialised device.
> + */
> +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;
> +	}

[Severity: High]
Can this double-free zdev->rf->qp_cnt_8k_idxs if zxdh_ctrl_init_hw() fails?

In zxdh_probe()->zxdh_ctrl_init_hw()->zxdh_initialize_hw_rsrc(), if the
first vzalloc() succeeds for rf->qp_cnt_8k_idxs and the second vzalloc()
fails for rf->mem_rsrc, zxdh_initialize_hw_rsrc() frees rf->qp_cnt_8k_idxs
without clearing the pointer:

drivers/infiniband/hw/zrdma/zrdma_hw.c:zxdh_initialize_hw_rsrc() {
	...
	rf->qp_cnt_8k_idxs = vzalloc(rf->max_8k_idx * sizeof(u32));
	if (!rf->qp_cnt_8k_idxs) {
		ret = -ENOMEM;
		goto mem_8k_qp_cnt_vmalloc_fail;
	}

	rsrc_size = zxdh_calc_mem_rsrc_size(rf);
	rf->mem_rsrc = vzalloc(rsrc_size);
	if (!rf->mem_rsrc) {
		ret = -ENOMEM;
		goto mem_rsrc_vmalloc_fail;
	}
	...
mem_rsrc_vmalloc_fail:
	vfree(rf->qp_cnt_8k_idxs);
mem_8k_qp_cnt_vmalloc_fail:
	return ret;
}

zxdh_probe() then jumps to err_dealloc and calls ib_dealloc_device(), which
invokes zxdh_ib_dealloc_device() and calls kvfree(zdev->rf->qp_cnt_8k_idxs) a
second time on the dangling pointer.

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