Re: [PATCH 09/15] yunsilicon: add RDMA device support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider:
- [Critical] rdma: arbitrary physical memory mapping via unvalidated vma size
- [High] rdma: ABI breakage due to size_t in UAPI structures
- [High] rdma: out-of-bounds access due to duplicated core private structs
- [High] rdma: hardcoded physical address offset breaks PCI mapping
- [High] rdma: API violation using virt_to_phys on DMA coherent memory
- [High] rdma: double free due to auxiliary driver unregister in reboot notifier
- [High] rdma: use-after-free of netdev pointer due to missing reference and unregister handling
- [High] rdma: race condition exposing uninitialized device state to userspace
- [Medium] rdma: truncated UAPI data due to fixed struct offset accumulator
- [Medium] rdma: integer underflow causes small MTUs to report maximum MTU
- [Medium] rdma: uninitialized ib_event structure dispatched on unhandled events
- [Medium] rdma: missing udata->inlen check in xsc_ib_alloc_ucontext()
- [Low] rdma: endian-dependent firmware version formatting
--

commit 206aea3bd72ce8a9a32742a3fba3649e658d1875
Author: Xin Tian <tianx@yunsilicon.com>
Subject: yunsilicon: add RDMA device support

This commit adds basic RDMA driver support for Yunsilicon XSC devices. It
includes device registration, port queries, memory mapping, and the uverbs
ABI header.

> diff --git a/drivers/infiniband/hw/xsc/main.c b/drivers/infiniband/hw/xsc/main.c
> index 0000000000000..9d53a89beee02
> --- /dev/null
> +++ b/drivers/infiniband/hw/xsc/main.c
> @@ -40,15 +40,29 @@ static int xsc_ib_query_device(struct ib_device *ibdev,
>  {
>  	size_t udata_outlen = (udata) ? udata->outlen : 0;
[ ... ]
>  	fw_ver.data = 0;
>  	fw_ver.s.ver_major = dev->xdev->fw_version_major;
>  	fw_ver.s.ver_minor = dev->xdev->fw_version_minor;
>  	fw_ver.s.ver_patch = dev->xdev->fw_version_patch;
>  	fw_ver.s.ver_tweak = dev->xdev->fw_version_tweak;
>  	props->fw_ver = fw_ver.data;

[Severity: Low]
Does this union-based assignment expose host endianness variations to the
UAPI? Because struct bitfield and byte layout depend on host endianness, the
resulting u64 will have a different value on little-endian versus big-endian
systems. Could explicit bit shifts be used instead to ensure a consistent
firmware version value across architectures?

>  	/*response tso_caps extend param*/
>  	if (offsetofend(typeof(resp), tso_caps) <= udata_outlen) {
>  		max_tso = dev->xdev->caps.log_max_tso ? (1 << dev->xdev->caps.log_max_tso) : 0;
>  		if (max_tso) {
>  			resp.tso_caps.max_tso = max_tso;
>  			resp.tso_caps.supported_qpts |= 1 << IB_QPT_RAW_PACKET;
>  			resp.response_length += sizeof(resp.tso_caps);
>  		}
>  	}

[Severity: Medium]
Will this dynamic accumulation of lengths truncate UAPI data? The destination
buffer resp is a monolithic C structure with fixed offsets. If an
intermediate feature is disabled, its size is not added to response_length,
making the length fall behind the actual struct offset of subsequent fields.
When ib_copy_to_udata() is called with this length, will trailing features
like packet_pacing_caps be silently truncated before reaching userspace?

> @@ -292,10 +306,12 @@ int xsc_ib_alloc_ucontext(struct ib_ucontext *uctx,
>  			  struct ib_udata *udata)
>  {
>  	struct ib_device *ibdev = uctx->device;
>  	struct xsc_ib_dev *dev = to_xdev(ibdev);
>  	struct xsc_ib_alloc_ucontext_req req;
>  	struct xsc_ib_alloc_ucontext_resp resp;
>  	int err;
>  
>  	if (!dev->ib_active)
>  		return -EAGAIN;
>  
>  	err = ib_copy_from_udata(&req, udata, sizeof(req));

[Severity: Medium]
Can this copy read out-of-bounds memory or cause a fault? The copy happens
unconditionally using sizeof(req) without checking if udata->inlen is large
enough. If userspace provides a smaller buffer or passes 0 bytes, this could
read garbage data or return -EFAULT. Should it use min(udata->inlen,
sizeof(req)) instead?

> @@ -318,29 +334,16 @@ static int xsc_ib_mmap(struct ib_ucontext *ibcontext, struct vm_area_struct *vma
>  {
>  	struct xsc_ib_dev *dev = to_xdev(ibcontext->device);
>  	struct xsc_core_device *xdev = dev->xdev;
>  	unsigned long offset = vma->vm_pgoff << PAGE_SHIFT;
>  	resource_size_t reg_base;
>  	resource_size_t reg_size = vma->vm_end - vma->vm_start;

[Severity: Critical]
Does this allow mapping arbitrary physical memory? The reg_size is derived
directly from the userspace mmap syscall arguments without any upper bound
validation against the legitimate boundaries of the PCI resource. When this
unvalidated reg_size is later passed to remap_pfn_range(), could a local user
map adjacent physical memory belonging to the kernel or other hardware?

>  	if (offset == (tx_db & PAGE_MASK))
>  		reg_base = pci_resource_start(xdev->pdev, xdev->bar_num) + (tx_db & PAGE_MASK);
[ ... ]
>  	reg_base = (xsc_core_is_pf(xdev) && !is_pf_bar_compressed(xdev)) ?
>  		reg_base - 0xA0000000 : reg_base;

[Severity: High]
Does subtracting this hardcoded constant break correct memory mapping on
generic platforms? PCI BARs are dynamically mapped, and the CPU physical
address from pci_resource_start() is already correct. Modifying the generic
PCI physical address with a hardcoded, architecture-specific offset like
0xA0000000 could cause remap_pfn_range() to map unrelated RAM or trap the
system instead of the intended MMIO registers.

> @@ -417,31 +419,25 @@ static void xsc_ib_event(struct xsc_core_device *dev, void *context,
>  			 enum xsc_dev_event event, unsigned long data)
>  {
>  	struct xsc_ib_dev *ibdev = (struct xsc_ib_dev *)context;
>  	struct ib_event ibev;
>  	u8 port = 0;
>  
>  	switch (event) {
[ ... ]
>  	}
>  
>  	ibev.device	      = &ibdev->ib_dev;
>  	ibev.element.port_num = port;
>  
>  	if (ibdev->ib_active)
>  		ib_dispatch_event(&ibev);

[Severity: Medium]
Could this dispatch an uninitialized ib_event structure? The stack-allocated
ibev is not zero-initialized, and the switch statement lacks a default case.
If called with an unhandled event, ibev.event remains uninitialized, leading
to a garbage event type containing stack memory being dispatched to the core
and userspace.

> @@ -592,44 +601,31 @@ static int init_one(struct xsc_core_device *xdev,
>  		    struct xsc_ib_dev **ibdev)
>  {
>  	struct xsc_ib_dev *dev;
>  	int err;
>  
>  	dev = (struct xsc_ib_dev *)ib_alloc_device(xsc_ib_dev, ib_dev);
>  	if (!dev)
>  		return -ENOMEM;
>  
>  	dev->xdev = xdev;
>  	dev->netdev = xdev->netdev;

[Severity: High]
Does this store a pointer to the netdevice without taking a reference? The
assignment happens without dev_hold().

Looking at xsc_netdev_event() below, it explicitly ignores the
NETDEV_UNREGISTER event:

	switch (event) {
	case NETDEV_CHANGE:
	case NETDEV_UP:
	case NETDEV_DOWN:
		xsc_get_port_state(ibdev->netdev, &ev);
		xsc_ib_event(ibdev->xdev, ibdev, ev, (unsigned long)&port);
		break;
	default:
		break;
	}

If the underlying ethernet device is unbound, the netdevice is freed, leaving
dev->netdev as a dangling pointer. Subsequent calls to ib callbacks like
xsc_ib_query_port() would dereference this pointer, triggering a
use-after-free.

>  	err = ib_register_device(&dev->ib_dev, dev->ib_dev.name,
>  				 dev->xdev->device);
>  	if (err)
>  		goto err_free;
>  
>  	rdma_roce_rescan_device(&dev->ib_dev);
>  	dev->ib_active = true;
>  	*ibdev = dev;
>  
>  	xsc_register_netdev_notifier(dev);

[Severity: High]
Is there a race condition exposing the device before initialization is
complete? ib_register_device() publishes the device to userspace. However,
the netdev notifier is registered after this, and looking at xsc_ib_probe(),
init_iommu_state() is called after init_one() returns. This creates a window
where userspace can open the device and attempt operations before the IOMMU
mode is resolved or link events are tracked.

> @@ -704,22 +701,21 @@ static void init_iommu_state(struct xsc_ib_dev *dev)
>  {
>  	struct iommu_domain *domain;
>  
>  	dev->iommu_state = XSC_IB_IOMMU_MAP_DISABLE;
>  	domain = iommu_get_domain_for_dev(dev->ib_dev.dma_device);
>  	if (domain) {
>  		if (domain->type & __IOMMU_DOMAIN_DMA_API)
>  			dev->iommu_state = XSC_IB_IOMMU_MAP_NORMAL;
>  	} else {
>  		dma_addr_t dma_addr;
>  		void *tmp;
>  
>  		tmp = dma_alloc_coherent(dev->ib_dev.dma_device, PAGE_SIZE,
>  					 &dma_addr, GFP_KERNEL);
>  		if (tmp) {
>  			if (virt_to_phys(tmp) != dma_addr)

[Severity: High]
Does this violate the DMA API? Calling virt_to_phys() on memory returned by
dma_alloc_coherent() is generally unsafe because the virtual address may not
be part of the direct linear mapping (e.g., vmalloc space on ARM/MIPS).
Passing such an address to virt_to_phys() yields an incorrect physical
address and can trigger crashes if CONFIG_DEBUG_VIRTUAL is enabled.

> @@ -758,21 +757,17 @@ static struct auxiliary_driver xsc_ib_driver = {
>  	.id_table = xsc_ib_id_table,
>  };
>  
>  static int xsc_ib_reboot_event_handler(struct notifier_block *nb, unsigned long action, void *data)
>  {
>  	pr_info("xsc ib driver recv %lu event\n", action);
>  
>  	xsc_remove_rdma_driver();
>  
>  	return NOTIFY_OK;
>  }
>  
>  struct notifier_block xsc_ib_nb = {
>  	.notifier_call = xsc_ib_reboot_event_handler,
>  	.next = NULL,
>  	.priority = 2,
>  };
>  
>  void xsc_remove_rdma_driver(void)
>  {
>  	auxiliary_driver_unregister(&xsc_ib_driver);
>  }
>  
>  static int __init xsc_ib_init(void)
>  {
>  	int ret;
>  
>  	ret = auxiliary_driver_register(&xsc_ib_driver);
>  	if (ret) {
>  		pr_err("failed to register auxiliary driver\n");
>  		return ret;
>  	}
>  
>  	register_reboot_notifier(&xsc_ib_nb);
>  
>  	return 0;
>  }
>  
>  static void __exit xsc_ib_cleanup(void)
>  {
>  	unregister_reboot_notifier(&xsc_ib_nb);
>  	xsc_remove_rdma_driver();
>  }

[Severity: High]
Can this cause a double-free of the driver name? The reboot notifier
unconditionally calls xsc_remove_rdma_driver(), which invokes
auxiliary_driver_unregister(). If the module is later unloaded,
xsc_ib_cleanup() calls unregister_reboot_notifier() and then
xsc_remove_rdma_driver() a second time.

> diff --git a/drivers/infiniband/hw/xsc/xsc_ib.h b/drivers/infiniband/hw/xsc/xsc_ib.h
> index 0000000000000..a2ae382e22ce8
> --- /dev/null
> +++ b/drivers/infiniband/hw/xsc/xsc_ib.h
> @@ -379,15 +379,20 @@ static inline enum ib_mtu xsc_net_to_ib_mtu(unsigned int mtu)
>  {
>  	mtu = mtu - (IB_GRH_BYTES + IB_UDP_BYTES + IB_BTH_BYTES +
>  		     IB_EXT_XRC_BYTES + IB_EXT_ATOMICETH_BYTES +
>  		     IB_ICRC_BYTES);
>  	if (mtu >= 8192)
>  		return IB_MTU_8192;

[Severity: Medium]
Will this trigger an integer underflow if the netdevice MTU is smaller than
96 bytes? Subtracting the fixed headers without an upper bound or minimum
check means a small MTU wraps around to a massive positive number. The check
for mtu >= 8192 then evaluates to true, incorrectly returning IB_MTU_8192
instead of properly clamping the value.

> @@ -436,25 +436,20 @@ struct ib_mad_list_head {
>  	struct list_head list;
>  	struct ib_cqe cqe;
>  	struct ib_mad_queue *mad_queue;
>  };
>  
>  #define IB_MAD_SEND_REQ_MAX_SG 2
>  struct ib_mad_send_wr_private {
>  	struct ib_mad_list_head mad_list;
>  	struct list_head agent_list;
>  	struct ib_mad_agent_private *mad_agent_priv;

[Severity: High]
Does this safely duplicate internal RDMA MAD core structures? The driver
illegitimately duplicates private core structures like
ib_mad_send_wr_private and ib_mad_private. Since these are internal to the
RDMA core, they may change layout at any time.

When xsc_ib_send_mad_sg_virt_addr() unsafely casts core pointers using these
locally duplicated structures:

	mad_list = container_of(wr->wr_cqe, struct ib_mad_list_head, cqe);
	mad_send_wr = container_of(mad_list, struct ib_mad_send_wr_private,
				   mad_list);

will this lead to out-of-bounds reads or memory corruption if the core
updates the upstream layout?

> @@ -483,16 +483,12 @@ static inline void *xsc_ib_send_mad_sg_virt_addr(struct ib_device *ibdev,
>  						 const struct ib_send_wr *wr,
>  						 int sg)
>  {
>  	struct ib_mad_send_wr_private *mad_send_wr;
>  	struct ib_mad_list_head *mad_list;
>  	int iommu_state = xsc_ib_iommu_dma_map(ibdev);
>  
>  	/* direct dma mapping */
>  	if (!iommu_state)
>  		return phys_to_virt(dma_to_phys(ibdev->dma_device, wr->sg_list[sg].addr));

[Severity: High]
Does manually reverse-mapping a dma_addr_t to a virtual address using
phys_to_virt(dma_to_phys(...)) break coherency on SWIOTLB architectures or
cause crashes if the buffer is in HighMem?

> diff --git a/include/uapi/rdma/xsc-abi.h b/include/uapi/rdma/xsc-abi.h
> index 0000000000000..c4d0db30e43c4
> --- /dev/null
> +++ b/include/uapi/rdma/xsc-abi.h
> @@ -82,21 +82,18 @@ struct xsc_ib_alloc_ucontext_resp {
>  	__u32	reserved[32];
>  };
>  
>  struct xsc_ib_create_qp {
>  	__u64	buf_addr;
>  	__u64	db_addr;
>  	__u32	sq_wqe_count;
>  	__u32	rq_wqe_count;
>  	__u32	rq_wqe_shift;
>  	__u32	flags;
>  
>  	int	dmabuf_fd;
>  	size_t	dmabuf_sz;
>  };

[Severity: High]
Does using size_t in a UAPI struct break ABI compatibility? size_t changes
from 4 bytes on 32-bit architectures to 8 bytes on 64-bit architectures,
causing struct size and alignment mismatches. If a 32-bit userspace program
interacts with a 64-bit kernel, the kernel will read fields at incorrect
offsets. Should an explicitly sized type like __u64 be used instead?

> @@ -112,18 +112,16 @@ struct xsc_ib_create_srq_resp {
>  };
>  
>  struct xsc_ib_create_cq {
>  	__u64 buf_addr;
>  	__u64 db_addr;
>  	__u32	cqe_size;
>  
>  	int	dmabuf_fd;
>  	size_t	dmabuf_sz;
>  };

[Severity: High]
Does using size_t in this UAPI struct cause the same ABI breakage across
32-bit and 64-bit boundaries as mentioned above? Explicitly sized types are
required for stable UAPI boundaries.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012114352.2471317-1-tianx@yunsilicon.com?part=9