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
