Re: [net-next,v23,05/14] virtio: Expose object create and destroy API

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 (1) · Low: 1

- [Low] virtio_admin_obj_create()/virtio_admin_obj_destroy()
  (drivers/virtio/virtio_admin_commands.c:98 and :142) forward a caller-…

Pre-existing issues:
- [Medium] Both new helpers kfree() the command payload (and let the
  stack-resident 'struct virtio_admin_cmd cmd' go out of scope)…

--

virtio: Expose object create and destroy API

This adds two exported helpers, virtio_admin_obj_create() and
virtio_admin_obj_destroy(), which build the resource object create/destroy
admin commands generically instead of only for dev parts objects.  The new
prototypes and kernel-doc go into include/linux/virtio_admin.h.

[ ... ]

+	obj_create_data = data;
+	obj_create_data->hdr.type = cpu_to_le16(obj_type);
+	obj_create_data->hdr.id = cpu_to_le32(obj_id);
+	memcpy(obj_create_data->resource_obj_specific_data, obj_specific_data,
+	       obj_specific_data_size);
+	sg_init_one(&data_sg, data, data_size);
+
+	cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_RESOURCE_OBJ_CREATE);
+	cmd.group_type = cpu_to_le16(group_type);
+	cmd.group_member_id = cpu_to_le64(group_member_id);
+	cmd.data_sg = &data_sg;
+
+	err = vdev->config->admin_cmd_exec(vdev, &cmd);
+	kfree(data);

[Severity: Medium]

This isn't a bug introduced by this patch, but is the unconditional
kfree(data) here safe for every return value of admin_cmd_exec()?

There is one path in virtqueue_exec_admin_cmd() where the descriptors have
already been published and the function returns without waiting and without
reclaiming them:

drivers/virtio/virtio_pci_modern.c:virtqueue_exec_admin_cmd() {
	...
	ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
	if (ret < 0) {
		...
		goto unlock_err;
	}
	if (!virtqueue_kick(vq))
		goto unlock_err;
	mutex_unlock(&admin_vq->lock);

	wait_for_completion(&cmd->completion);

	return cmd->ret;

unlock_err:
	mutex_unlock(&admin_vq->lock);
	return -EIO;
}

On that -EIO return the buffer is still referenced by the virtqueue, and
virtio_admin_obj_create() then frees data and lets the stack-resident
struct virtio_admin_cmd cmd go out of scope.  The device-writable
va_status allocation in vp_modern_admin_cmd_exec() is freed on the same
error path:

drivers/virtio/virtio_pci_modern.c:vp_modern_admin_cmd_exec() {
	...
err_cmd_exec:
	kfree(va_hdr);
err_alloc:
	kfree(va_status);
	return ret;
}

In practice vp_notify() always returns true, so virtqueue_kick() can only
fail through the vq->broken check inside virtqueue_notify(), which returns
before notifying the device, and virtqueue_get_buf() returns NULL on a
broken vq, so vp_modern_avq_work() will not complete into the recycled
stack frame.  The same pattern exists at the baseline for
virtio_admin_cap_set(), virtio_pci_admin_mode_set() and
virtio_pci_admin_obj_create(); this patch adds two more callers of it.

Would the reclaim (a wait, or virtqueue_detach_unused_buf()) belong in
virtqueue_exec_admin_cmd() rather than in each caller?

+
+	return err;
+}
+EXPORT_SYMBOL_GPL(virtio_admin_obj_create);
+
+int virtio_admin_obj_destroy(struct virtio_device *vdev,
+			     u16 obj_type,
+			     u32 obj_id,
+			     u16 group_type,
+			     u64 group_member_id)
+{

[ ... ]

+	data->type = cpu_to_le16(obj_type);
+	data->id = cpu_to_le32(obj_id);
+	sg_init_one(&data_sg, data, sizeof(*data));
+	cmd.opcode = cpu_to_le16(VIRTIO_ADMIN_CMD_RESOURCE_OBJ_DESTROY);
+	cmd.group_type = cpu_to_le16(group_type);
+	cmd.group_member_id = cpu_to_le64(group_member_id);
+	cmd.data_sg = &data_sg;
+
+	err = vdev->config->admin_cmd_exec(vdev, &cmd);
+	kfree(data);

The kfree(data) note above applies here in virtio_admin_obj_destroy() as
well.

[Severity: Low]

This isn't a bug, but could the kernel-doc for virtio_admin_obj_create()
and virtio_admin_obj_destroy() say who owns the obj_id space?

The pre-existing dev parts helper keeps the id space in the core:

drivers/virtio/virtio_pci_modern.c:virtio_pci_admin_obj_create() {
	...
	id = ida_alloc_range(&avq->dev_parts_ida, 0,
			     avq->max_dev_parts_objects - 1, GFP_KERNEL);
	...
}

and it still rejects any obj_type other than VIRTIO_RESOURCE_OBJ_DEV_PARTS,
so nothing in tree can currently desync that IDA through the new generic
API.  The generic helpers, though, forward obj_type/obj_id/group_type/
group_member_id straight to the device with no statement that the caller
owns the id space for a given (group_type, group_member_id, obj_type)
tuple, and no equivalent of the "caller must serialize" note carried by the
dev parts helpers.

Would adding that contract to the kernel-doc make the expectations for new
callers clearer?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831161109.2999926-1-shshitrit%40nvidia.com