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

[netdev-bot+sashiko@kernel.org]

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.

> diff --git a/drivers/virtio/virtio_admin_commands.c b/drivers/virtio/virtio_admin_commands.c
> index 60d92875da233..24b06075d944f 100644
> --- a/drivers/virtio/virtio_admin_commands.c
> +++ b/drivers/virtio/virtio_admin_commands.c
> @@ -94,3 +94,80 @@ int virtio_admin_cap_set(struct virtio_device *vdev,
>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(virtio_admin_cap_set);
> +
> +int virtio_admin_obj_create(struct virtio_device *vdev,
> +			    u16 obj_type,
> +			    u32 obj_id,
> +			    u16 group_type,
> +			    u64 group_member_id,
> +			    const void *obj_specific_data,
> +			    size_t obj_specific_data_size)
> +{

[ ... ]

> +	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.

> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(virtio_admin_obj_destroy);
> diff --git a/include/linux/virtio_admin.h b/include/linux/virtio_admin.h
> index 1177c4a20e53c..ebd4106835d68 100644
> --- a/include/linux/virtio_admin.h
> +++ b/include/linux/virtio_admin.h
> @@ -86,4 +86,48 @@ int virtio_admin_cap_set(struct virtio_device *vdev,
>  			 const void *caps,
>  			 size_t cap_size);
>  
> +/**
> + * virtio_admin_obj_create - Create an object on a virtio device
> + * @vdev: the virtio device
> + * @obj_type: type of object to create
> + * @obj_id: ID for the new object
> + * @group_type: administrative group type for the operation
> + * @group_member_id: member identifier within the administrative group
> + * @obj_specific_data: object-specific data for creation
> + * @obj_specific_data_size: size of the object-specific data in bytes
> + *
> + * Creates a new object on the virtio device with the specified type and ID.
> + * The object may require object-specific data for proper initialization.

[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