Re: [PATCH 14/15] yunsilicon: add RDMA counters and sysfs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] sysfs: kobject UAF via missing .release and synchronous kfree
- [High] sysfs: global singleton g_global_cnt_interface corrupts multi-device state
- [Medium] sysfs: counters_value_read breaks partial reads and ignores offset
- [Medium] infiniband: sysfs attribute creation races with udev KOBJ_ADD
--

commit e407716ecbb78f776fbe73fcb02888c7ab5c00c8
Author: Xin Tian <tianx@yunsilicon.com>

yunsilicon: add RDMA counters and sysfs

This commit adds hardware counter reporting through the RDMA counter
interface, global counter sysfs attributes, and hca_type/hw_rev sysfs
attributes.

> diff --git a/drivers/infiniband/hw/xsc/counters.c b/drivers/infiniband/hw/xsc/counters.c
> new file mode 100644
> index 0000000000000..135df95954a75
> --- /dev/null
> +++ b/drivers/infiniband/hw/xsc/counters.c
[ ... ]
> +static ssize_t counters_value_read(struct file *file, struct kobject *kob,
> +				   const struct bin_attribute *bin_attr,
> +				   char *buf, loff_t loff, size_t size)
> +{
[ ... ]
> +	if (bin_attr->size > size || bin_attr->size == 0)
> +		return 0;

[Severity: Medium]
Does this check incorrectly reject partial or offset-bounded reads? 

Since the sysfs core bounds the requested size to max_size - loff, a read
with a non-zero offset or a user buffer smaller than bin_attr->size might
immediately fail with an early EOF here.

[ ... ]
> +	mask = xsc_get_rdma_stat_mask(xdev, be16_to_cpu(stats_rdma.ver));
> +	j = 0;
> +	for (i = 0; i < desc_size; i++) {
> +		if (!((1ULL << i) & mask))
> +			continue;
> +		tmp_value[j] = be64_to_cpu(*(u64 *)(stats + desc[i].offset));
> +		j++;
> +	}
> +
> +	memcpy(buf, tmp_value, bin_attr->size);
> +
> +	kfree(tmp_value);
> +	return bin_attr->size;
> +}

[Severity: Medium]
Could this ignore the requested offset during a read?

The loff parameter is not used in the memcpy() call, so if a read succeeded
at a non-zero offset, it would incorrectly copy from the beginning of
tmp_value instead of applying the requested offset.

[ ... ]
> +static const struct kobj_type global_cnt_ktype = {
> +	.sysfs_ops     = &global_cnt_sysfs_ops,
> +	.default_groups = global_cnt_groups,
> +};

[Severity: High]
Is this kobj_type missing the mandatory .release callback required for
kobjects? 

Without it, the kobject container memory cannot be safely managed when the
reference count reaches zero.

> +
> +static struct xsc_global_cnt_interface *g_global_cnt_interface;
> +
> +static int global_cnt_sysfs_init(struct ib_device *ib_dev, struct xsc_core_device *xdev)
> +{
[ ... ]
> +	err = kobject_init_and_add(&tmp->kobj, &global_cnt_ktype,
> +				   &ib_dev->dev.kobj, GLOBAL_COUNTERS_GROUP_NAME);
> +	if (err)
> +		goto error_return;
> +
> +	g_global_cnt_interface = tmp;
> +	tmp->xdev = xdev;
> +	return 0;

[Severity: High]
Does this blindly overwrite the global pointer g_global_cnt_interface for
every initialized physical function?

If the system contains multiple Yunsilicon network adapters, initializing the
second one would overwrite the pointer. This would lose the previous device's
state and permanently leak its memory and kobject. Furthermore, removing the
first device would cause global_cnt_sysfs_fini() to mistakenly operate on the
second device's state.

[ ... ]
> +static void global_cnt_sysfs_fini(struct xsc_core_device *xdev)
> +{
> +	if (!g_global_cnt_interface || !xdev || !xsc_core_is_pf(xdev) || xdev->pf_id != 0)
> +		return;
> +
> +	kobject_put(&g_global_cnt_interface->kobj);
> +	kfree(g_global_cnt_interface);
> +	g_global_cnt_interface = NULL;
> +}

[Severity: High]
Is it safe to synchronously free the kobject container memory here?

Since kobject_put() merely decrements the reference count, the kobject and
its sysfs representation could remain active if userspace still holds an
open file descriptor. Calling kfree() immediately afterwards destroys the
underlying memory prematurely, which might lead to a use-after-free crash
when userspace subsequently accesses the sysfs files.

> diff --git a/drivers/infiniband/hw/xsc/main.c b/drivers/infiniband/hw/xsc/main.c
> index 30927006bae2d..38c8b4f0690c2 100644
> --- a/drivers/infiniband/hw/xsc/main.c
> +++ b/drivers/infiniband/hw/xsc/main.c
[ ... ]
> @@ -745,7 +745,9 @@ static int init_one(struct xsc_core_device *xdev,
>  
>  	xsc_register_netdev_notifier(dev);
>  
> +	xsc_counters_init(&dev->ib_dev, xdev);
>  
> +	xsc_ib_sysfs_init(&dev->ib_dev, xdev);
>  
>  	return 0;

[Severity: Medium]
Does creating the sysfs attribute files here cause a race condition with
udev?

The driver registers the device earlier in init_one() by calling
ib_register_device(), which triggers a KOBJ_ADD uevent. Adding the sysfs
files after this registration means udev rules might execute before the
attributes actually exist. Should the sysfs groups be assigned to the
device's attribute groups (e.g., ib_dev->groups) prior to registration
instead?

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