Unbinding a virtio balloon device:
echo virtio0 > /sys/bus/virtio/drivers/virtio_balloon/unbind
triggers a NULL pointer dereference. The dmesg says:
BUG: kernel NULL pointer dereference, address: 0000000000000008
[...]
RIP: 0010:__list_del_entry_valid_or_report+0x5/0xf0
Call Trace:
<TASK>
vp_del_vqs+0x121/0x230
remove_common+0x135/0x150
virtballoon_remove+0xee/0x100
virtio_dev_remove+0x3b/0x80
device_release_driver_internal+0x187/0x2c0
unbind_store+0xb9/0xe0
kernfs_fop_write_iter.llvm.11660790530567441834+0xf6/0x180
vfs_write+0x2a9/0x3b0
ksys_write+0x5c/0xd0
do_syscall_64+0x54/0x230
entry_SYSCALL_64_after_hwframe+0x29/0x31
[...]
</TASK>
The virtio_balloon device registers 5 queues (inflate, deflate, stats,
free_page, reporting) but only the first two are unconditional. The
stats, free_page and reporting queues are each conditional on their
respective feature bits. When any of these features are absent, the
corresponding vqs_info entry has name == NULL, creating holes in the
array.
The root cause is an indexing mismatch introduced when vq info storage
was changed to be passed as an argument. vp_find_vqs_msix() and
vp_find_vqs_intx() store the info pointer at vp_dev->vqs[i], where 'i'
is the caller's sparse array index. However, the virtqueue itself gets
vq->index assigned from queue_idx, a dense index that skips NULL
entries. When holes exist, 'i' and queue_idx diverge. Later,
vp_del_vqs() looks up info via vp_dev->vqs[vq->index] using the dense
index into the sparsely-populated array, and hits NULL.
Fix this by storing info at vp_dev->vqs[queue_idx] instead of
vp_dev->vqs[i], so the store index matches the lookup index
(vq->index). Apply the fix to both the MSIX and INTX paths.
Cc: Yichun Zhang <[email protected]>
Cc: Jiri Pirko <[email protected]>
Cc: [email protected] # v6.11+
Tested-by: Yuka <[email protected]>
Fixes: 89a1c435aec2 ("virtio_pci: pass vq info as an argument to vp_setup_vq()")
Signed-off-by: Ammar Faizi <[email protected]>
---
drivers/virtio/virtio_pci_common.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/virtio/virtio_pci_common.c
b/drivers/virtio/virtio_pci_common.c
index da97b6a988de..164f480b18a6 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -414,28 +414,29 @@ static int vp_find_vqs_msix(struct virtio_device *vdev,
unsigned int nvqs,
err = vp_request_msix_vectors(vdev, nvectors, per_vq_vectors, desc);
if (err)
goto error_find;
vp_dev->per_vq_vectors = per_vq_vectors;
allocated_vectors = vp_dev->msix_used_vectors;
for (i = 0; i < nvqs; ++i) {
vqi = &vqs_info[i];
if (!vqi->name) {
vqs[i] = NULL;
continue;
}
- vqs[i] = vp_find_one_vq_msix(vdev, queue_idx++, vqi->callback,
+ vqs[i] = vp_find_one_vq_msix(vdev, queue_idx, vqi->callback,
vqi->name, vqi->ctx, false,
&allocated_vectors, vector_policy,
- &vp_dev->vqs[i]);
+ &vp_dev->vqs[queue_idx]);
+ queue_idx++;
if (IS_ERR(vqs[i])) {
err = PTR_ERR(vqs[i]);
goto error_find;
}
}
if (!avq_num)
return 0;
sprintf(avq->name, "avq.%u", avq->vq_index);
vq = vp_find_one_vq_msix(vdev, avq->vq_index, vp_modern_avq_done,
avq->name, false, true, &allocated_vectors,
vector_policy, &vp_dev->admin_vq.info);
@@ -476,27 +477,28 @@ static int vp_find_vqs_intx(struct virtio_device *vdev,
unsigned int nvqs,
if (err)
goto out_del_vqs;
vp_dev->intx_enabled = 1;
vp_dev->per_vq_vectors = false;
for (i = 0; i < nvqs; ++i) {
struct virtqueue_info *vqi = &vqs_info[i];
if (!vqi->name) {
vqs[i] = NULL;
continue;
}
- vqs[i] = vp_setup_vq(vdev, queue_idx++, vqi->callback,
+ vqs[i] = vp_setup_vq(vdev, queue_idx, vqi->callback,
vqi->name, vqi->ctx,
- VIRTIO_MSI_NO_VECTOR, &vp_dev->vqs[i]);
+ VIRTIO_MSI_NO_VECTOR,
&vp_dev->vqs[queue_idx]);
+ queue_idx++;
if (IS_ERR(vqs[i])) {
err = PTR_ERR(vqs[i]);
goto out_del_vqs;
}
}
if (!avq_num)
return 0;
sprintf(avq->name, "avq.%u", avq->vq_index);
vq = vp_setup_vq(vdev, queue_idx++, vp_modern_avq_done, avq->name,
false, VIRTIO_MSI_NO_VECTOR,
&vp_dev->admin_vq.info);
--
Ammar Faizi
