Re: [PATCH 7/7] RDMA/zrdma: Implement device and port query operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] RDMA/zrdma: NULL pointer dereference of udata in zxdh_query_device()
- [Medium] RDMA/zrdma: missing max_send_sge and max_recv_sge in zxdh_query_device()
- [Medium] RDMA/zrdma: broken rtnl_trylock() fallback in zxdh_get_drvinfo()
- [Medium] RDMA/zrdma: flawed speed query and caching in zxdh_get_eth_netdev_speed()
- [Medium] RDMA/zrdma: zxdh_get_dev_fw_str() overwrites N/A fallback on parse failure
- [Medium] RDMA/zrdma: memory leak on probe error paths due to late ib_set_device_ops()
- [Low] RDMA/zrdma: dead zxdh_query_gid_roce() callback and misplaced build fixes
--

commit 5562e1f8054bd9cdd1fa5be99a78d65f32fb0baf
Author: Yanze Zhang <zhang.yanze@zte.com.cn>

RDMA/zrdma: Implement device and port query operations

This commit implements RDMA device and port query callbacks, including device
attributes, port attributes, GID/PKey queries, link layer reporting, firmware
version extraction, and device registration setup.

> diff --git a/drivers/infiniband/hw/zrdma/zrdma_main.h b/drivers/infiniband/hw/zrdma/zrdma_main.h
> index 060d6f627b0dd..62e52e5a23ee3 100644
> --- a/drivers/infiniband/hw/zrdma/zrdma_main.h
> +++ b/drivers/infiniband/hw/zrdma/zrdma_main.h
> @@ -208,7 +208,7 @@ static inline void zxdh_free_rsrc(struct zxdh_pci_f *rf,
>  	spin_unlock_irqrestore(&rf->rsrc_lock, flags);
>  }
>  
> -int zxdh_get_del_rdma_name(const char *sbdf, char *name);
> +int zxdh_get_del_rdma_name(const char *sbdf, char *name, size_t name_len);
>  int zxdh_ctrl_init_hw(struct zxdh_pci_f *rf);
>  int zxdh_ib_register_device(struct zxdh_device *zdev);
>  void zxdh_del_handler(struct zxdh_handler *hdl);
> diff --git a/drivers/infiniband/hw/zrdma/zrdma_verbs.c b/drivers/infiniband/hw/zrdma/zrdma_verbs.c
> index 54b016ea8f5de..a16e00e785353 100644
> --- a/drivers/infiniband/hw/zrdma/zrdma_verbs.c
> +++ b/drivers/infiniband/hw/zrdma/zrdma_verbs.c

[ ... ]

> @@ -157,7 +159,7 @@ static int zxdh_alloc_ucontext(struct ib_ucontext *uctx, struct ib_udata *udata)
>  	ucontext->zdev = zdev;
>  	ucontext->abi_ver = req.userspace_ver;
>  
> -	uresp.srq_db_bar_off = SRQ_DB_OFFSET;
> +	uresp.srq_db_bar_off = SRQ_DB_BAR_OFF & ZXDH_PAGE_OFFSET_NUM;
>  
>  	ucontext->sq_db_mmap_entry =
>  		zxdh_user_mmap_entry_insert(ucontext, SQ_DB_BAR_OFF,
> @@ -332,29 +334,327 @@ static int zxdh_mmap(struct ib_ucontext *context, struct vm_area_struct *vma)
>  	return ret;
>  }
>  
> -static void zxdh_init_roce_device(struct zxdh_device *zdev)
> +/* Fetch the driver info for @zdev->netdev, or for its first lower device
> + * when it is a bond. adj_list.lower is rtnl_lock protected, but the core
> + * may call this from a netdev notifier already holding rtnl_lock.
> + */
> +static bool zxdh_get_drvinfo(struct zxdh_device *zdev,
> +			     struct ethtool_drvinfo *info)
>  {
> -	zdev->ibdev.node_type = RDMA_NODE_IB_CA;
> -	addrconf_addr_eui48((u8 *)&zdev->ibdev.node_guid,
> -			    zdev->netdev->dev_addr);
> +	struct net_device *slave = NULL, *dev;
> +	struct list_head *iter;
> +	bool locked = false;
> +	bool ret = false;
> +
> +	memset(info, 0, sizeof(*info));
> +
> +	if ((zdev->netdev->priv_flags & IFF_BONDING) && rtnl_trylock()) {
> +		netdev_for_each_lower_dev(zdev->netdev, slave, iter)
> +			break;
> +		locked = true;
> +	}
> +
> +	dev = slave ? slave : zdev->netdev;
> +	if (dev->ethtool_ops && dev->ethtool_ops->get_drvinfo) {
> +		dev->ethtool_ops->get_drvinfo(dev, info);
> +		ret = true;
> +	}
> +
> +	if (locked)
> +		rtnl_unlock();
> +
> +	return ret;
>  }

[Severity: Medium]
What happens in zxdh_get_drvinfo() when zdev->netdev is a bonding device and
rtnl_lock is already held by the current thread or contended by another
thread?

Because rtnl_mutex is not recursive, rtnl_trylock() returns 0 even when the
caller already holds rtnl_lock. In that case, the lower device lookup is
skipped and slave remains NULL, so dev falls back to zdev->netdev (the
bonding master) instead of the lower hardware device.

Calling dev->ethtool_ops->get_drvinfo(dev, info) on the bonding master invokes
bond_ethtool_get_drvinfo(), which fills info->fw_version with BOND_ABI_VERSION
("2") and causes zxdh_get_drvinfo() to return true. When extract_version()
parses that string, it fails and outputs an empty string, resulting in
zxdh_query_device() setting props->fw_ver = 0 and zxdh_get_dev_fw_str()
overwriting "N/A" with "".

In addition, when zdev->netdev is not a bond or when rtnl_trylock() fails,
dev->ethtool_ops->get_drvinfo() is invoked without holding rtnl_lock or the
netdev ops lock.

>  
> -static int zxdh_init_rdma_device(struct zxdh_device *zdev)
> +static void extract_version(const char *input, char *output)
>  {
> +	const char *v_start, *p;
> +	int dots = 0;
> +
> +	if (!output || !input)
> +		goto fail;
> +
> +	if (strstr(input, " (")) {
> +		v_start = input;
> +		for (p = v_start; *p && *p != ' '; p++) {
> +			if (*p == '.' && ++dots == 4)
> +				break;
> +			*output++ = *p;
> +		}
> +	} else {
> +		/* Format: "E310-V25.30.103.01" - find 'V' after last '-' */
> +		const char *last_dash = strrchr(input, '-');
> +
> +		if (!last_dash)
> +			goto fail;
> +		v_start = strstr(last_dash, "V");
> +		if (!v_start)
> +			goto fail;
> +		v_start++;  /* skip 'V' */
> +
> +		for (p = v_start; *p && dots < 4; p++) {
> +			if (*p == '.' && ++dots == 4)
> +				break;
> +			*output++ = *p;
> +		}
> +	}
> +
> +fail:
> +	if (output)
> +		*output = '\0';
> +}
> +
> +static int zxdh_query_device(struct ib_device *ibdev,
> +			     struct ib_device_attr *props,
> +			     struct ib_udata *udata)
> +{
> +	struct zxdh_device *zdev = to_zdev(ibdev);
>  	struct pci_dev *pcidev = zdev->rf->pcidev;
> +	int major, sub_major, minor, sub_minor;
> +	struct ethtool_drvinfo info;
> +	char extracted_version[32];
> +
> +	if (zxdh_get_drvinfo(zdev, &info)) {
> +		extract_version(info.fw_version, extracted_version);
> +		if (sscanf(extracted_version, "%d.%d.%d.%d", &major,
> +			   &sub_major, &minor, &sub_minor) != 4) {
> +			major = 0;
> +			sub_major = 0;
> +			minor = 0;
> +			sub_minor = 0;
> +		}
> +	} else {
> +		major = sub_major = minor = sub_minor = 0;
> +	}
>  
> -	if (zdev->roce_mode)
> -		zxdh_init_roce_device(zdev);
> +	if (udata->inlen || udata->outlen)
> +		return -EINVAL;

[Severity: High]
Won't dereferencing udata without a NULL check in zxdh_query_device() cause a
NULL pointer dereference during device registration?

When zxdh_ib_register_device() calls ib_register_device(), setup_device() in
drivers/infiniband/core/device.c queries the device attributes with udata set
to NULL:

drivers/infiniband/core/device.c:setup_device() {
	...
	memset(&device->attrs, 0, sizeof(device->attrs));
	ret = device->ops.query_device(device, &device->attrs, NULL);
	...
}

Should this check for a non-NULL udata pointer first, for example
if (udata && (udata->inlen || udata->outlen))?

> +
> +	memset(props, 0, sizeof(*props));
> +	addrconf_addr_eui48((u8 *)&props->sys_image_guid,
> +			    zdev->netdev->dev_addr);
> +	props->fw_ver = ((u64)major << 48 | (u64)sub_major << 32 |
> +			 (u64)minor << 16 | sub_minor);
> +	props->device_cap_flags =
> +		IB_DEVICE_MEM_WINDOW | IB_DEVICE_MEM_MGT_EXTENSIONS |
> +		IB_DEVICE_BAD_QKEY_CNTR | IB_DEVICE_SYS_IMAGE_GUID |
> +		IB_DEVICE_RC_RNR_NAK_GEN | IB_DEVICE_N_NOTIFY_CQ;
> +	props->vendor_id = pcidev->vendor;
> +	props->vendor_part_id = pcidev->device;
> +	props->hw_ver = pcidev->revision;
> +	props->page_size_cap = SZ_4K | SZ_2M | SZ_1G;
> +	props->max_mr_size = MAX_MR_SIZE;
> +	props->max_qp = MAX_QP;
> +	props->max_qp_wr = MAX_QP_WR;
> +	props->max_cq = MAX_CQ;
> +	props->max_cqe = MAX_CQE;
> +	props->max_mr = MAX_MR;
> +	props->max_mw = MAX_MW;
> +	props->max_pd = MAX_PD;
> +	props->max_sge_rd = MAX_SGE_RD;

[Severity: Medium]
Should zxdh_query_device() also populate props->max_send_sge and
props->max_recv_sge here?

Because props is zeroed with memset() and neither field is assigned,
device->attrs.max_send_sge and device->attrs.max_recv_sge remain 0 after
setup_device() runs.

When userspace queries the device through ib_uverbs_query_device(),
copy_query_dev_fields() in drivers/infiniband/core/uverbs_cmd.c computes:

	resp->max_sge = min(attr->max_send_sge, attr->max_recv_sge);

which reports max_sge as 0 to userspace and leaves kernel ULPs seeing 0
supported SGEs in device->attrs.

[ ... ]

> +static u32 zxdh_get_eth_netdev_speed(struct zxdh_device *zdev, u32 port_num)
> +{
> +	struct ethtool_link_ksettings lksettings;
> +	u32 netdev_speed = (u32)SPEED_UNKNOWN;
> +	struct zxdh_pci_f *rf = zdev->rf;
> +	int rc;
> +
> +	if (rdma_port_get_link_layer(&zdev->ibdev, port_num) !=
> +	    IB_LINK_LAYER_ETHERNET)
> +		return -EINVAL;
> +
> +	if (zdev->netdev_speed != (u32)SPEED_UNKNOWN)
> +		return zdev->netdev_speed;
> +
> +	if (rf->drv_np_cap == ZXDH_RDMA_COMMON_FUNC_CAP &&
> +	    rf->common_func_num_max > ZXDH_FUNC_NETDEV_SPEED_GET) {
> +		rc = rf->gen_ops.zxdh_common_func(zdev->zxdh_adev->parent,
> +						  &netdev_speed,
> +						  ZXDH_FUNC_NETDEV_SPEED_GET);
> +
> +		if (rc || netdev_speed == (u32)SPEED_UNKNOWN)
> +			netdev_speed = SPEED_1000;
> +		else
> +			zdev->netdev_speed = netdev_speed;
> +	} else {
> +		if (!rtnl_trylock())
> +			return SPEED_1000;
> +
> +		rc = __ethtool_get_link_ksettings(zdev->netdev, &lksettings);
> +		rtnl_unlock();
> +
> +		if (!rc &&
> +		    lksettings.base.speed != (u32)SPEED_UNKNOWN) {
> +			netdev_speed = lksettings.base.speed;
> +			zdev->netdev_speed = netdev_speed;
> +		} else {
> +			netdev_speed = SPEED_1000;
> +		}
> +	}
> +
> +	return netdev_speed;
> +}
> +
> +static int zxdh_get_eth_speed(struct ib_device *dev, struct net_device *netdev,
> +			      u32 port_num, u16 *speed, u8 *width)
> +{
> +	struct zxdh_device *zdev = to_zdev(dev);
> +	u32 netdev_speed;
> +
> +	if (zdev)
> +		netdev_speed = zxdh_get_eth_netdev_speed(zdev, port_num);
>  	else
> -		return -EPFNOSUPPORT;
> +		netdev_speed = SPEED_1000;
> +
> +	if (netdev_speed <= SPEED_1000) {
> +		*width = IB_WIDTH_1X;
> +		*speed = IB_SPEED_SDR;
> +	} else if (netdev_speed <= SPEED_10000) {
> +		*width = IB_WIDTH_1X;
> +		*speed = IB_SPEED_FDR10;
> +	} else if (netdev_speed <= SPEED_20000) {
> +		*width = IB_WIDTH_4X;
> +		*speed = IB_SPEED_DDR;
> +	} else if (netdev_speed <= SPEED_25000) {
> +		*width = IB_WIDTH_1X;
> +		*speed = IB_SPEED_EDR;
> +	} else if (netdev_speed <= SPEED_40000) {
> +		*width = IB_WIDTH_4X;
> +		*speed = IB_SPEED_FDR10;
> +	} else if (netdev_speed <= SPEED_100000) {
> +		*width = IB_WIDTH_4X;
> +		*speed = IB_SPEED_EDR;
> +	} else {
> +		*width = IB_WIDTH_4X;
> +		*speed = IB_SPEED_HDR;
> +	}
>  
> -	zdev->ibdev.phys_port_cnt = 1;
> -	zdev->ibdev.num_comp_vectors = zdev->rf->ceqs_count;
> -	zdev->ibdev.dev.parent = &pcidev->dev;
> +	return 0;
> +}

[Severity: Medium]
Can zxdh_get_eth_netdev_speed() and zxdh_get_eth_speed() report stale or
bogus link speeds in zxdh_query_port()?

First, once zxdh_get_eth_netdev_speed() writes a speed into
zdev->netdev_speed, every subsequent call returns the cached
zdev->netdev_speed immediately. Nothing in the driver ever invalidates or
updates zdev->netdev_speed when the link speed changes, and concurrent reads
and writes to zdev->netdev_speed are unsynchronized.

Second, when querying via __ethtool_get_link_ksettings(), if rtnl_trylock()
fails because rtnl_mutex is held or contended while zdev->netdev_speed is
still SPEED_UNKNOWN, zxdh_get_eth_netdev_speed() returns a fabricated
SPEED_1000 (which maps to IB_WIDTH_1X and IB_SPEED_SDR).

Third, zxdh_get_eth_netdev_speed() returns u32, so returning -EINVAL converts
to 0xffffffea, which zxdh_get_eth_speed() treats as an unsigned speed greater
than SPEED_100000 and maps to IB_WIDTH_4X and IB_SPEED_HDR. Could
ib_get_eth_speed() be used instead, or the speed queried without permanent
caching and signed-to-unsigned conversion?

[ ... ]

> +static int zxdh_query_gid_roce(struct ib_device *ibdev, u32 port, int index,
> +			       union ib_gid *gid)
> +{
> +	int ret;
> +
> +	ret = rdma_query_gid(ibdev, port, index, gid);
> +	if (ret == -EAGAIN) {
> +		memcpy(gid, &zgid, sizeof(*gid));
> +		return 0;
> +	}
> +
> +	return ret;
> +}

[Severity: Low]
Is zxdh_query_gid_roce() ever called by the RDMA core?

For RoCE ports, the RDMA core manages the GID cache via roce_gid_mgmt, and
ib_cache_update() in drivers/infiniband/core/cache.c only calls
device->ops.query_gid via config_non_roce_gid_cache() when
!rdma_protocol_roce(device, port). Even if invoked, rdma_query_gid() only
returns 0, -EINVAL, or -ENOENT, so the ret == -EAGAIN branch cannot be
reached.

Also, the two changes earlier in this patch to the zxdh_get_del_rdma_name()
prototype in drivers/infiniband/hw/zrdma/zrdma_main.h and uresp.srq_db_bar_off
in zxdh_alloc_ucontext() appear to fix build errors introduced in earlier
commits in the series ("RDMA/zrdma: Add device probe and remove logic" and
"RDMA/zrdma: Implement context, PD, and mmap operations"). Should those fixes
be folded into the earlier commits to preserve bisectability?

[ ... ]

> +static void zxdh_get_dev_fw_str(struct ib_device *dev, char *str)
> +{
> +	struct zxdh_device *zdev = to_zdev(dev);
> +	struct ethtool_drvinfo info;
> +	char extracted_version[32];
> +
> +	strscpy(str, "N/A", IB_FW_VERSION_NAME_MAX);
> +
> +	if (!zxdh_get_drvinfo(zdev, &info))
> +		return;
> +
> +	extract_version(info.fw_version, extracted_version);
> +	strscpy(str, extracted_version, IB_FW_VERSION_NAME_MAX);
> +}

[Severity: Medium]
If extract_version() fails to match the version format in info.fw_version (for
example, when info.fw_version is empty or does not contain " (" or "-...V"),
it jumps to fail and sets extracted_version[0] = '\0'. Won't the unconditional
strscpy(str, extracted_version, IB_FW_VERSION_NAME_MAX) in
zxdh_get_dev_fw_str() overwrite the "N/A" fallback with an empty string?

Also, in zxdh_query_device(), major, sub_major, minor, and sub_minor are
parsed as signed int values with "%d.%d.%d.%d" and combined via:

	props->fw_ver = ((u64)major << 48 | (u64)sub_major << 32 |
			 (u64)minor << 16 | sub_minor);

Should extracted_version[0] be checked before overwriting str in
zxdh_get_dev_fw_str(), and should the version components in
zxdh_query_device() be parsed as unsigned 16-bit values (or masked to 0xffff)
so negative or oversized values cannot corrupt upper bits of props->fw_ver?

[ ... ]

> +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 = &pcidev->dev;
> +
> +	ib_set_device_ops(&zdev->ibdev, &zxdh_dev_ops);
> +
> +	return 0;
> +}

[Severity: Medium]
Does calling ib_set_device_ops(&zdev->ibdev, &zxdh_dev_ops) here at the end of
zxdh_init_rdma_device() leak zdev->rf, zdev->hdl, rf->qp_cnt_8k_idxs, and
rf->mem_rsrc if an earlier error path is taken in zxdh_probe()?

In drivers/infiniband/hw/zrdma/zrdma_main.c:zxdh_probe(), after allocating
zdev with ib_alloc_device(), the driver allocates zdev->rf, initializes
zdev->ah_list_lock in zxdh_fill_device_info(), allocates zdev->hdl, and
allocates rf->qp_cnt_8k_idxs and rf->mem_rsrc in zxdh_ctrl_init_hw():

drivers/infiniband/hw/zrdma/zrdma_main.c:zxdh_probe() {
	...
	hdl = kzalloc_obj(*hdl);
	if (!hdl) {
		ib_dealloc_device(&zdev->ibdev);
		return -ENOMEM;
	}
	...
	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);

	return err;
}

All of those failure paths (as well as the !zdev->roce_mode return path in
zxdh_init_rdma_device()) rely on ib_dealloc_device(&zdev->ibdev) invoking
zxdh_ib_dealloc_device() via zdev->ibdev.ops.dealloc_driver, which is Not set
until ib_set_device_ops() runs. Should ib_set_device_ops() be called
immediately after ib_alloc_device() in zxdh_probe() (or at the start of device
initialization) so dealloc_driver is always registered before any error path
calls ib_dealloc_device()?

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