* Re: [PATCH v2 1/4] virtio-mem: validate device-reported block size
From: Greg Kroah-Hartman @ 2026-07-17 5:03 UTC (permalink / raw)
To: David Hildenbrand (Arm)
Cc: Hari Mishal, Michael S . Tsirkin, Jason Wang, Xuan Zhuo,
Eugenio Pérez, virtualization, linux-kernel
In-Reply-To: <4dda47ba-534a-4297-a25e-0d63d9167033@kernel.org>
On Thu, Jul 16, 2026 at 05:59:05PM +0200, David Hildenbrand (Arm) wrote:
> On 7/16/26 16:18, Greg Kroah-Hartman wrote:
> > On Thu, Jul 16, 2026 at 10:55:42AM +0200, David Hildenbrand (Arm) wrote:
> >> On 7/15/26 18:41, Hari Mishal wrote:
> >>> The device_block_size read from the virtio-mem config space is used as a
> >>> divisor and also in ALIGN_DOWN() further down the code path in the
> >>> driver without further validation. A zero value leads to a division by
> >>> zero, and a non-power-of-two value corrupts the ALIGN_DOWN() bitmask
> >>> arithmetic leading to a misreporting of guest-usable ram, post crash.
> >>>
> >>> Reject both at init time instead of trusting the device.
> >>>
> >>> Signed-off-by: Hari Mishal <harimishal1@gmail.com>
> >>> ---
> >>> v2: dropped the redundant explicit zero check, since
> >>> is_power_of_2(0) already returns false.
> >>>
> >>> drivers/virtio/virtio_mem.c | 6 ++++++
> >>> 1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
> >>> index 11c441501582..0e04fec458af 100644
> >>> --- a/drivers/virtio/virtio_mem.c
> >>> +++ b/drivers/virtio/virtio_mem.c
> >>> @@ -2847,6 +2847,12 @@ static int virtio_mem_init(struct virtio_mem *vm)
> >>> &vm->plugged_size);
> >>> virtio_cread_le(vm->vdev, struct virtio_mem_config, block_size,
> >>> &vm->device_block_size);
> >>> + if (!is_power_of_2(vm->device_block_size)) {
> >>> + dev_err(&vm->vdev->dev,
> >>> + "invalid device block size: 0x%llx\n",
> >>> + (unsigned long long)vm->device_block_size);
> >>> + return -EINVAL;
> >>> + }
> >>
> >> The spec states "The device MUST set block_size to a power of two."
> >>
> >> I'm missing the point here.
> >
> > So what happens if we have a non-spec-compliant device? Shouldn't we be
> > attempting to verify this before doing something with the data?
>
> The problem I see is that there are plenty of other devices where a
> non-compliant device might cause problems.
>
> Like, we request to hotplug some memory block and our device ACKs it, but it
> simply didn't do that.
>
> Or we request to hotunplug a memory block and instead it hotplugs some random
> other memory block.
>
> Or we sense whether a memory block is plugged and the device lies to us.
>
> >
> > Or do we just always trust virtio mem devices explicitly?
>
> It's hard for me to understand where we draw the line, really.
>
> But maybe MST can clarify what we care about in virtio world where the
> hypervisor is fully in charge of the device,
That would be good to figure out, and write down somewhere, so we know
what to worry about here.
Right now, for almost all subsystems, Linux trusts the hardware
explicitly. But for some subsystems, it doesn't, or doesn't until
"something" happens (example, USB devices are untrusted until we bind a
driver to the device, and then they are trusted.)
But there are some people that want to never trust the hardware, or have
it in different states, specifically for virtual machines like this
driver operates in. The CoC model is just that, and patches from Dan
are working toward clearing this up a lot for some types of devices (PCI
and IOMMU), but I don't think they have thought about virtio devices
yet...
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v2 2/2] virtio_balloon: avoid shrinker execution during PM suspend
From: Link Lin @ 2026-07-17 3:08 UTC (permalink / raw)
To: Andrew Morton, Vlastimil Babka, Michael S . Tsirkin,
David Hildenbrand
Cc: virtualization, linux-mm, linux-kernel, prasin, rientjes, duenwen,
jasowang, xuanzhuo, Ammar Faizi, jiaqiyan, ahwilkins, Greg Thelen,
Alexander Duyck, jthoughton, stable
In-Reply-To: <20260717002311.681748-3-linkl@google.com>
On Thu, Jul 16, 2026 at 5:24 PM Link Lin <linkl@google.com> wrote:
>
> During PM freeze (e.g. S4 hibernation), virtballoon_freeze() calls
> remove_common() which resets the virtio device and deletes all virtqueues.
> However, the balloon shrinker remains registered with core MM.
>
> If memory pressure occurs during S4 hibernation image creation/saving, MM
> invokes virtio_balloon_shrinker_scan(), which attempts to reclaim free
> pages. Although return_free_pages_to_mm() only frees pages back to MM,
> reclaiming free pages under memory pressure can trigger page reporting
> which might access the deleted reporting virtqueue if it is not yet
> frozen, or interact with other parts of the driver in a teardown state.
>
> Avoid this by adding a `suspended` flag to `struct virtio_balloon`. Set
> this flag to true in virtballoon_freeze() and false in
> virtballoon_restore(). Check this flag in both shrinker callbacks (scan
> and count) and return 0 if the device is suspended, preventing any
> shrinker execution while virtqueues are deleted. Wrap the lockless reads
> in READ_ONCE() and writes in WRITE_ONCE() to prevent compiler
> optimization issues and KCSAN data race warnings.
>
> Fixes: 71019de8219b ("virtio_balloon: Add free page hinting support")
> Cc: stable@vger.kernel.org
> Acked-by: David Rientjes <rientjes@google.com>
> Signed-off-by: Link Lin <linkl@google.com>
Apologies, I missed adding a tag for the feedback that led to the
TOCTOU locking fix in this patch.
Maintainers, if this series is applied, could you please add the following?
Suggested-by: James Houghton <jthoughton@google.com>
(I will be sure to include it if a v3 is otherwise required).
^ permalink raw reply
* Re: [RFC] virtio_balloon: fix Use-After-Free in page reporting during PM freeze
From: Link Lin @ 2026-07-17 3:02 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: David Hildenbrand (Arm), Andrew Morton, Vlastimil Babka,
virtualization, linux-mm, linux-kernel, prasin, rientjes, duenwen,
jasowang, xuanzhuo, Ammar Faizi, jiaqiyan, ahwilkins, Greg Thelen,
Alexander Duyck, stable
In-Reply-To: <CALUx4KSxjpHhgkHZ5p1khLSe7+-cDHd+ZtBEJS1HytWSo5WbHQ@mail.gmail.com>
On Tue, Jul 14, 2026 at 11:05 AM Link Lin <linkl@google.com> wrote:
>
> Per the earlier feedback to keep these fixes independent, I'll format
> v2 as a 3-part patch series:
> [PATCH v2 1/3] mm/page_reporting: use system_freezable_wq
> [PATCH v2 2/3] virtio_balloon: fix shrinker UAF during PM freeze
> [PATCH v2 3/3] virtio_balloon: fix OOM notifier UAF during PM freeze
>
> I'll send that out shortly.
Hi everyone,
I have just sent out the v2 patch series in a new thread:
https://lore.kernel.org/all/20260717002311.681748-1-linkl@google.com/
Just a quick update: I previously mentioned I would include a fix for
the OOM notifier as part 3. However, upon further audit, the PM core
explicitly disables the OOM killer (via oom_killer_disable()) during
freeze_processes() before device drivers are frozen.
Therefore, the virtio_balloon_oom_notify callback cannot trigger while
the virtqueues are deleted. Since it is already safe, I dropped it
from the series.
v2 now consists solely of the page reporting workqueue migration and
the shrinker TOCTOU fix. Thanks again for the reviews!
Best,
Link
^ permalink raw reply
* Re: Re: [PATCH] vhost-vdpa: propagate set_map error to caller
From: Weimin Xiong @ 2026-07-17 2:17 UTC (permalink / raw)
To: eperezma; +Cc: virtualization, mst, jasowangio, netdev, kvm, xiongweimin
In-Reply-To: <CAJaqyWeNJwVNUihL8MAEH+-a5OsoaDGsgaPnP0BqQd6YH4ZhYg@mail.gmail.com>
From: xiongweimin <xiongweimin@kylinos.cn>
Hi Eugenio,
Thanks for your review!
You are right. My original patch has an issue with the `break` statement,
which skips the `v->in_batch = false` assignment when `set_map` fails.
The correct approach is to simply capture the return value without
breaking:
```c
case VHOST_IOTLB_BATCH_END:
if (v->in_batch && ops->set_map)
r = ops->set_map(vdpa, asid, iotlb);
v->in_batch = false;
break;
```
This way:
1. The return value of `set_map` is propagated to the caller
2. `v->in_batch` is always reset, regardless of success or failure
Updated patch attached below.
---
Subject: [PATCH] vhost-vdpa: propagate set_map error to caller
The return value of ops->set_map() is currently ignored when handling
VHOST_IOTLB_BATCH_END. If the backend fails to program the IOTLB,
the VMM incorrectly believes the operation succeeded and may continue
with stale or incorrect mappings.
Propagate the error from ops->set_map() to the caller.
Signed-off-by: xiongweimin <xiongweimin@kylinos.cn>
---
drivers/vhost/vdpa.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index bb96b1aa5..b52d8d94e 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -1298,7 +1298,7 @@ static int vhost_vdpa_process_iotlb_msg(struct vhost_dev *dev, u32 asid,
break;
case VHOST_IOTLB_BATCH_END:
if (v->in_batch && ops->set_map)
- ops->set_map(vdpa, asid, iotlb);
+ r = ops->set_map(vdpa, asid, iotlb);
v->in_batch = false;
break;
default:
--
2.39.3
^ permalink raw reply related
* (no subject)
From: Weimin Xiong @ 2026-07-17 2:13 UTC (permalink / raw)
To: eperezma; +Cc: virtualization, mst, jasowangio, netdev, kvm, xiongweimin
In-Reply-To: <CAJaqyWeNJwVNUihL8MAEH+-a5OsoaDGsgaPnP0BqQd6YH4ZhYg@mail.gmail.com>
From: xiongweimin <xiongweimin@kylinos.cn>
Hi Eugenio,
Thanks for your review!
You are right. My original patch has an issue with the `break` statement,
which skips the `v->in_batch = false` assignment when `set_map` fails.
The correct approach is to simply capture the return value without
breaking:
```c
case VHOST_IOTLB_BATCH_END:
if (v->in_batch && ops->set_map)
r = ops->set_map(vdpa, asid, iotlb);
v->in_batch = false;
break;
```
This way:
1. The return value of `set_map` is propagated to the caller
2. `v->in_batch` is always reset, regardless of success or failure
Updated patch attached below.
---
Subject: [PATCH] vhost-vdpa: propagate set_map error to caller
The return value of ops->set_map() is currently ignored when handling
VHOST_IOTLB_BATCH_END. If the backend fails to program the IOTLB,
the VMM incorrectly believes the operation succeeded and may continue
with stale or incorrect mappings.
Propagate the error from ops->set_map() to the caller.
Signed-off-by: xiongweimin <xiongweimin@kylinos.cn>
---
drivers/vhost/vdpa.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index bb96b1aa5..b52d8d94e 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -1298,7 +1298,7 @@ static int vhost_vdpa_process_iotlb_msg(struct vhost_dev *dev, u32 asid,
break;
case VHOST_IOTLB_BATCH_END:
if (v->in_batch && ops->set_map)
- ops->set_map(vdpa, asid, iotlb);
+ r = ops->set_map(vdpa, asid, iotlb);
v->in_batch = false;
break;
default:
--
2.39.3
^ permalink raw reply related
* [PATCH] vhost-vdpa: propagate set_map error to caller
From: Weimin Xiong @ 2026-07-17 2:09 UTC (permalink / raw)
To: virtualization; +Cc: xiongweimin
From: xiongweimin <xiongweimin@kylinos.cn>
The return value of ops->set_map() is currently ignored when handling
VHOST_IOTLB_BATCH_END. If the backend fails to program the IOTLB,
the VMM incorrectly believes the operation succeeded and may continue
with stale or incorrect mappings.
Propagate the error from ops->set_map() to the caller.
Signed-off-by: xiongweimin <xiongweimin@kylinos.cn>
---
drivers/vhost/vdpa.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index bb96b1aa5..b52d8d94e 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -1298,7 +1298,7 @@ static int vhost_vdpa_process_iotlb_msg(struct vhost_dev *dev, u32 asid,
break;
case VHOST_IOTLB_BATCH_END:
if (v->in_batch && ops->set_map)
- ops->set_map(vdpa, asid, iotlb);
+ r = ops->set_map(vdpa, asid, iotlb);
v->in_batch = false;
break;
default:
--
2.39.3
^ permalink raw reply related
* Re: [PATCH] vhost: reject zero-size IOTLB INVALIDATE
From: Weimin Xiong @ 2026-07-17 1:29 UTC (permalink / raw)
To: Eugenio Perez Martin
Cc: virtualization, mst, jasowangio, netdev, kvm, xiongweimin
In-Reply-To: <CAJaqyWfW9n5o+ojrEjK-m+6cwH_TSxWckjkvEXowZA8r=vDmdQ@mail.gmail.com>
From: xiongweimin <xiongweimin@kylinos.cn>
Hi Eugenio,
Thank you for your review and suggestion!
I've updated the patch to v2, moving the check to vhost_chr_write_iter
as you suggested. The existing check for VHOST_IOTLB_UPDATE is now extended
to also cover VHOST_IOTLB_INVALIDATE.
Thanks for your Acked-by!
Best regards,
Weimin Xiong
^ permalink raw reply
* [PATCH v2] vhost: reject zero-size IOTLB INVALIDATE
From: Weimin Xiong @ 2026-07-17 1:29 UTC (permalink / raw)
To: xiongwm2026
Cc: virtualization, mst, jasowangio, netdev, kvm, xiongweimin,
Eugenio Perez Martin
In-Reply-To: <20260716030236.124322-1-xiongwm2026@163.com>
From: xiongweimin <xiongweimin@kylinos.cn>
Reject VHOST_IOTLB_INVALIDATE messages with size == 0 to prevent
iova + size - 1 from underflowing to U64_MAX, which would
incorrectly delete the entire IOTLB.
Changes in v2:
- Move the check to vhost_chr_write_iter where similar check for
VHOST_IOTLB_UPDATE already exists (suggested by Eugenio Perez Martin)
- Add Acked-by from Eugenio Perez Martin
Suggested-by: Eugenio Perez Martin <eperezma@redhat.com>
Acked-by: Eugenio Perez Martin <eperezma@redhat.com>
Signed-off-by: xiongweimin <xiongweimin@kylinos.cn>
---
drivers/vhost/vhost.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 3c080c454..327e1108c 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1716,7 +1716,8 @@ ssize_t vhost_chr_write_iter(struct vhost_dev *dev,
goto done;
}
- if (msg.type == VHOST_IOTLB_UPDATE && msg.size == 0) {
+ if ((msg.type == VHOST_IOTLB_UPDATE ||
+ msg.type == VHOST_IOTLB_INVALIDATE) && !msg.size) {
ret = -EINVAL;
goto done;
}
--
2.39.3
^ permalink raw reply related
* [PATCH v2 2/2] virtio_balloon: avoid shrinker execution during PM suspend
From: Link Lin @ 2026-07-17 0:22 UTC (permalink / raw)
To: Andrew Morton, Vlastimil Babka, Michael S . Tsirkin,
David Hildenbrand
Cc: virtualization, linux-mm, linux-kernel, prasin, rientjes, duenwen,
jasowang, xuanzhuo, Ammar Faizi, jiaqiyan, ahwilkins, Greg Thelen,
Alexander Duyck, jthoughton, stable, Link Lin
In-Reply-To: <20260717002311.681748-1-linkl@google.com>
During PM freeze (e.g. S4 hibernation), virtballoon_freeze() calls
remove_common() which resets the virtio device and deletes all virtqueues.
However, the balloon shrinker remains registered with core MM.
If memory pressure occurs during S4 hibernation image creation/saving, MM
invokes virtio_balloon_shrinker_scan(), which attempts to reclaim free
pages. Although return_free_pages_to_mm() only frees pages back to MM,
reclaiming free pages under memory pressure can trigger page reporting
which might access the deleted reporting virtqueue if it is not yet
frozen, or interact with other parts of the driver in a teardown state.
Avoid this by adding a `suspended` flag to `struct virtio_balloon`. Set
this flag to true in virtballoon_freeze() and false in
virtballoon_restore(). Check this flag in both shrinker callbacks (scan
and count) and return 0 if the device is suspended, preventing any
shrinker execution while virtqueues are deleted. Wrap the lockless reads
in READ_ONCE() and writes in WRITE_ONCE() to prevent compiler
optimization issues and KCSAN data race warnings.
Fixes: 71019de8219b ("virtio_balloon: Add free page hinting support")
Cc: stable@vger.kernel.org
Acked-by: David Rientjes <rientjes@google.com>
Signed-off-by: Link Lin <linkl@google.com>
---
drivers/virtio/virtio_balloon.c | 49 +++++++++++++++++++++++++++------
1 file changed, 41 insertions(+), 8 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 088b3a0e6c..38e1166a64 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -68,6 +68,8 @@ struct virtio_balloon {
/* Prevent updating balloon when it is being canceled. */
spinlock_t stop_update_lock;
bool stop_update;
+ /* Prevent shrinker from running while device is suspended. */
+ bool suspended;
/* Bitmap to indicate if reading the related config fields are needed */
unsigned long config_read_bitmap;
@@ -471,9 +473,9 @@ static inline s64 towards_target(struct virtio_balloon *vb)
return target - vb->num_pages;
}
-/* Gives back @num_to_return blocks of free pages to mm. */
-static unsigned long return_free_pages_to_mm(struct virtio_balloon *vb,
- unsigned long num_to_return)
+/* Helper: must be called with free_page_list_lock held */
+static unsigned long __return_free_pages_to_mm(struct virtio_balloon *vb,
+ unsigned long num_to_return)
{
unsigned long num_returned = 0;
struct page *page, *next;
@@ -481,8 +483,6 @@ static unsigned long return_free_pages_to_mm(struct virtio_balloon *vb,
if (unlikely(!num_to_return))
return 0;
- spin_lock_irq(&vb->free_page_list_lock);
-
list_for_each_entry_safe(page, next, &vb->free_page_list, lru) {
list_del(&page->lru);
__free_pages(page, VIRTIO_BALLOON_HINT_BLOCK_ORDER);
@@ -490,11 +490,27 @@ static unsigned long return_free_pages_to_mm(struct virtio_balloon *vb,
break;
}
vb->num_free_page_blocks -= num_returned;
- spin_unlock_irq(&vb->free_page_list_lock);
return num_returned;
}
+/* Gives back @num_to_return blocks of free pages to mm. */
+static unsigned long return_free_pages_to_mm(struct virtio_balloon *vb,
+ unsigned long num_to_return)
+{
+ unsigned long ret;
+
+ spin_lock_irq(&vb->free_page_list_lock);
+ if (vb->suspended) {
+ spin_unlock_irq(&vb->free_page_list_lock);
+ return 0;
+ }
+ ret = __return_free_pages_to_mm(vb, num_to_return);
+ spin_unlock_irq(&vb->free_page_list_lock);
+
+ return ret;
+}
+
static void virtio_balloon_queue_free_page_work(struct virtio_balloon *vb)
{
if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
@@ -871,6 +887,9 @@ static unsigned long virtio_balloon_shrinker_scan(struct shrinker *shrinker,
{
struct virtio_balloon *vb = shrinker->private_data;
+ if (READ_ONCE(vb->suspended))
+ return 0;
+
return shrink_free_pages(vb, sc->nr_to_scan);
}
@@ -879,6 +898,9 @@ static unsigned long virtio_balloon_shrinker_count(struct shrinker *shrinker,
{
struct virtio_balloon *vb = shrinker->private_data;
+ if (READ_ONCE(vb->suspended))
+ return 0;
+
return vb->num_free_page_blocks * VIRTIO_BALLOON_HINT_BLOCK_PAGES;
}
@@ -1089,8 +1111,11 @@ static void remove_common(struct virtio_balloon *vb)
update_balloon_size(vb);
/* There might be free pages that are being reported: release them. */
- if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
- return_free_pages_to_mm(vb, ULONG_MAX);
+ if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
+ spin_lock_irq(&vb->free_page_list_lock);
+ __return_free_pages_to_mm(vb, ULONG_MAX);
+ spin_unlock_irq(&vb->free_page_list_lock);
+ }
/* Now we reset the device so we can clean up the queues. */
virtio_reset_device(vb->vdev);
@@ -1133,6 +1158,10 @@ static int virtballoon_freeze(struct virtio_device *vdev)
* The workqueue is already frozen by the PM core before this
* function is called.
*/
+ spin_lock_irq(&vb->free_page_list_lock);
+ WRITE_ONCE(vb->suspended, true);
+ spin_unlock_irq(&vb->free_page_list_lock);
+
remove_common(vb);
return 0;
}
@@ -1148,6 +1177,10 @@ static int virtballoon_restore(struct virtio_device *vdev)
virtio_device_ready(vdev);
+ spin_lock_irq(&vb->free_page_list_lock);
+ WRITE_ONCE(vb->suspended, false);
+ spin_unlock_irq(&vb->free_page_list_lock);
+
if (towards_target(vb))
virtballoon_changed(vdev);
update_balloon_size(vb);
--
2.55.0.229.g6434b31f56-goog
^ permalink raw reply related
* [PATCH v2 1/2] mm/page_reporting: use system_freezable_wq to fix UAF during suspend
From: Link Lin @ 2026-07-17 0:22 UTC (permalink / raw)
To: Andrew Morton, Vlastimil Babka, Michael S . Tsirkin,
David Hildenbrand
Cc: virtualization, linux-mm, linux-kernel, prasin, rientjes, duenwen,
jasowang, xuanzhuo, Ammar Faizi, jiaqiyan, ahwilkins, Greg Thelen,
Alexander Duyck, jthoughton, stable, Link Lin, David Hildenbrand
In-Reply-To: <20260717002311.681748-1-linkl@google.com>
During PM freeze (e.g. S3 suspend or S4 hibernation), device drivers like
virtio_balloon reset their underlying virtio devices and delete their
virtqueues via vdev->config->del_vqs().
However, page reporting work (page_reporting_process) was scheduled on
the global system_wq. Because system_wq lacks the WQ_FREEZABLE flag, the
PM freezer skips it, leaving page_reporting_process active during
suspend. If pages are freed into the buddy allocator while suspending,
page reporting invokes virtballoon_free_page_report() on deleted
virtqueues:
[ 196.795226] general protection fault, probably for non-canonical address 0xaa1436fe70dae6df: 0000 [#1] SMP NOPTI
[ 196.825967] Workqueue: events page_reporting_process
[ 196.831038] RIP: 0010:virtqueue_add_split+0x233/0x4c0 [virtio_ring]
[ 196.927073] virtballoon_free_page_report+0x3a/0xe0 [virtio_balloon]
[ 196.946943] page_reporting_process+0x370/0x4f0
Fix this by switching page reporting work to system_freezable_wq. This
ensures that the PM freezer pauses page_reporting_process before device
drivers destroy their reporting virtqueues.
This aligns with the driver's existing design. The comment in
virtballoon_freeze() states:
/*
* The workqueue is already frozen by the PM core before this
* function is called.
*/
Suggested-by: David Hildenbrand <david@redhat.com>
Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: David Rientjes <rientjes@google.com>
Fixes: 924a663f75e2 ("virtio-balloon: Reporting free page reservations")
Cc: stable@vger.kernel.org
Signed-off-by: Link Lin <linkl@google.com>
---
mm/page_reporting.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/mm/page_reporting.c b/mm/page_reporting.c
index 7418f2e500..4dc6f4b852 100644
--- a/mm/page_reporting.c
+++ b/mm/page_reporting.c
@@ -80,7 +80,8 @@ __page_reporting_request(struct page_reporting_dev_info *prdev)
* now we are limiting this to running no more than once every
* couple of seconds.
*/
- schedule_delayed_work(&prdev->work, PAGE_REPORTING_DELAY);
+ queue_delayed_work(system_freezable_wq, &prdev->work,
+ PAGE_REPORTING_DELAY);
}
/* notify prdev of free page reporting request */
@@ -343,7 +344,8 @@ static void page_reporting_process(struct work_struct *work)
*/
state = atomic_cmpxchg(&prdev->state, state, PAGE_REPORTING_IDLE);
if (state == PAGE_REPORTING_REQUESTED)
- schedule_delayed_work(&prdev->work, PAGE_REPORTING_DELAY);
+ queue_delayed_work(system_freezable_wq, &prdev->work,
+ PAGE_REPORTING_DELAY);
}
static DEFINE_MUTEX(page_reporting_mutex);
--
2.55.0.229.g6434b31f56-goog
^ permalink raw reply related
* [PATCH v2 0/2] virtio_balloon: fix Use-After-Free bugs during PM freeze
From: Link Lin @ 2026-07-17 0:22 UTC (permalink / raw)
To: Andrew Morton, Vlastimil Babka, Michael S . Tsirkin,
David Hildenbrand
Cc: virtualization, linux-mm, linux-kernel, prasin, rientjes, duenwen,
jasowang, xuanzhuo, Ammar Faizi, jiaqiyan, ahwilkins, Greg Thelen,
Alexander Duyck, jthoughton, stable, Link Lin
This patch series addresses a class of Use-After-Free vulnerabilities
in the virtio_balloon driver that can occur during system power management
freeze (e.g., suspend or hibernation).
The core issue is a lifecycle mismatch: when the system enters freeze,
virtballoon_freeze() resets the virtio device and deletes its virtqueues.
However, several asynchronous or event-driven components of the driver and
related subsystems (Free Page Reporting and Shrinker) remain registered
and active. If they trigger during the freeze/suspend process, they
attempt to access the now-deleted virtqueues, causing kernel crashes
(General Protection Faults or UAF).
To fix this:
1. Migrate Free Page Reporting to use the system_freezable_wq, ensuring
its worker is frozen before driver freeze callbacks run. (Suggested
by David Hildenbrand).
2. Avoid shrinker execution while the device is suspended by adding a
suspended flag to struct virtio_balloon. Wrap lockless reads of this
flag in READ_ONCE() and writes in WRITE_ONCE() to prevent data races and
KCSAN warnings.
Note: The OOM notifier (virtio_balloon_oom_notify) is also registered, but
since the PM core disables the OOM killer (via oom_killer_disable()) during
freeze_processes() before device drivers are frozen, it cannot trigger
while the device is frozen. Thus, it does not require a fix.
Testing:
I have verified these fixes using Google’s virtualization infrastructure by
running continuous suspend/resume iterations (40+ cycles) while
churning memory using stress-ng (stress-ng --vm 4 --vm-bytes 60% --timeout 1)
to constantly create free pages for the buddy allocator. We also set the
page_reporting_order parameter to 0 to make the page reporting worker
highly sensitive, forcing it to pick up any 4K free pages. This
confirmed that the UAF crashes are no longer reproducible.
RFC: https://lore.kernel.org/r/20260709224330.946683-1-linkl@google.com
---
RFC -> v2:
- Switched page reporting fix from unregister/re-register in driver to
using system_freezable_wq in mm/page_reporting.c, which avoids complex
restore rollback logic for that component.
- Switched shrinker fix from unregister/re-register to using a simple
suspended boolean flag to avoid complex rollback logic. Wrap its reads
and writes with READ_ONCE()/WRITE_ONCE() to prevent KCSAN data race
warnings.
- Dropped OOM notifier fix since it's already protected by the PM core's
oom_killer_disable().
Link Lin (2):
mm/page_reporting: use system_freezable_wq to fix UAF during suspend
virtio_balloon: avoid shrinker execution during PM suspend
drivers/virtio/virtio_balloon.c | 49 +++++++++++++++++++++++++++------
mm/page_reporting.c | 6 ++--
2 files changed, 45 insertions(+), 10 deletions(-)
--
2.55.0.229.g6434b31f56-goog
^ permalink raw reply
* Re: [PATCH v4 4/5] vhost: synchronize with RCU readers when freeing workers
From: Andrey Drobyshev @ 2026-07-16 18:01 UTC (permalink / raw)
To: Stefano Garzarella
Cc: linux-kernel, kvm, virtualization, netdev, mst, stefanha,
dongli.zhang, maciej.szmigiero, bchaney, mark.kanda, ptikhomirov,
den
In-Reply-To: <alj_YKTitwqMjAez@sgarzare-redhat>
On 7/16/26 7:13 PM, Stefano Garzarella wrote:
> On Thu, Jul 16, 2026 at 06:39:48PM +0300, Andrey Drobyshev wrote:
>> On 7/16/26 11:57 AM, Stefano Garzarella wrote:
>>> On Tue, Jul 14, 2026 at 06:16:37PM +0300, Andrey Drobyshev wrote:
>>>> vhost_vq_work_queue() only holds the RCU read lock while it dereferences
>>>> vq->worker and queues work on it. vhost_workers_free() however clears
>>>> the vq->worker pointers and immediately frees the workers, without
>>>> waiting for a grace period. A caller that fetched the worker right
>>>> before the pointer was cleared can therefore still be queueing work on
>>>> it while it is freed. And even when the queueing itself wins the race,
>>>> the work is never run, so its VHOST_WORK_QUEUED bit stays set and all
>>>> future attempts to queue it are silently skipped.
>>>>
>>>> None of the current callers can actually hit this: net and scsi stop
>>>> their virtqueues before the workers are freed, and vsock unhashes the
>>>> device and does synchronize_rcu() of its own in vhost_vsock_dev_release()
>>>> before the workers go away. But the upcoming VHOST_RESET_OWNER support
>>>> in vhost-vsock keeps the device hashed while its workers are freed, so
>>>> the lockless send/cancel paths become able to race with the teardown.
>>>>
>>>> Close this the way vhost_worker_killed() already does: clear the
>>>> vq->worker pointers, wait for a grace period, run whatever the last
>>>> readers may have queued, and only then free the workers. The
>>>> synchronize_rcu() is skipped if the device has no workers, so cleanup of
>>>> devices which never got an owner stays cheap.
>>>>
>>>
>>> Do we need a Fixes tag for this?
>>>
>>
>> I'm guessing it should be:
>>
>> Fixes: 228a27cf78af ("vhost: Allow worker switching while work is queueing")
>>
>>> Thanks for pointing out that the issue wasn't occurring, but I think we
>>> should add it because it's a sneaky problem we discovered by chance.
>>> IMO the code should already have `synchronize_rcu()` after
>>> `rcu_assign_pointer()` loop.
>>>
>>> @Michael, what do you think?
>>>
>>>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>>>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>>>> ---
>>>> drivers/vhost/vhost.c | 15 +++++++++++++++
>>>> 1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>>>> index 4c525b3e16ea..0d1414d40f4e 100644
>>>> --- a/drivers/vhost/vhost.c
>>>> +++ b/drivers/vhost/vhost.c
>>>> @@ -729,6 +729,21 @@ static void vhost_workers_free(struct vhost_dev *dev)
>>>>
>>>> for (i = 0; i < dev->nvqs; i++)
>>>> rcu_assign_pointer(dev->vqs[i]->worker, NULL);
>>>> +
>>>> + /*
>>>> + * vhost_vq_work_queue() reads vq->worker under rcu_read_lock(), so a
>>>> + * caller that fetched a worker before we cleared the pointers above
>>>> + * may still be about to queue work on it. Wait for those RCU readers
>>>> + * to finish before freeing the worker, then run whatever they queued
>>>> + * so nothing is left with VHOST_WORK_QUEUED set. Mirrors
>>>> + * vhost_worker_killed().
>>>> + */
>>>> + if (!xa_empty(&dev->worker_xa)) {
>>>> + synchronize_rcu();
>>>> + xa_for_each(&dev->worker_xa, i, worker)
>>>> + vhost_run_work_list(worker);
>>>> + }
>>>> +
>>>
>>> Following sashiko review [1], I tried to undersand why we need this, but
>>> TBH I'm really confused. That said, this seems wrong also because it
>>> will work only with vhost_tasks, and not with kthreads.
>>>
>>> IIUC vhost_worker_killed() will be called anyway when calling
>>> vhost_worker_destroy(). For vhost_tasks, it will call
>>> vhost_task_do_stop() that calls vhost_task_stop(). This sets
>>> VHOST_TASK_FLAGS_STOP and wait the worker on vtsk->exited before freeing
>>> stuff. The worker breaks the loop and calls vtsk->handle_sigkill() that
>>> is exactly vhost_worker_killed() you mentioned we are mirroring here.
>>>
>>
>> Hmm, are we sure it's the case for our codepath? Looking at the
>> vhost_task loop function:
>>
>>> static int vhost_task_fn(void *data)
>>> {
>>> for (;;) {
>>> if (signal_pending(current)) {
>>> if (get_signal(&ksig))
>>> break;
>>> }
>>> ...
>>> if (test_bit(VHOST_TASK_FLAGS_STOP, &vtsk->flags)) {
>>> __set_current_state(TASK_RUNNING);
>>> break;
>>> }
>>> did_work = vtsk->fn(vtsk->data);
>>> ...
>>> }
>>>
>>> ...
>>>
>>> if (!test_bit(VHOST_TASK_FLAGS_STOP, &vtsk->flags)) {
>>> set_bit(VHOST_TASK_FLAGS_KILLED, &vtsk->flags);
>>> vtsk->handle_sigkill(vtsk->data);
>>> }
>>> ...
>>> }
>>
>> AFAICT, we exit the loop in 2 cases: signal delivery or STOP bit
>> setting. Like you said, STOP is set by vhost_task_stop. E.g. for our
>> RESET_OWNER case:
>>
>> vhost_vsock_reset_owner()
>> vhost_dev_reset_owner()
>> vhost_dev_cleanup()
>> vhost_workers_free()
>> vhost_worker_destroy()
>> vhost_task_stop() // for vhost_task_ops backend
>> set_bit(VHOST_TASK_FLAGS_STOP)
>>
>> So, first of all, actual work by .fn() callback is done after the exit
>> checks, therefore we skip it - no chance to drain there.
>>
>> Secondly, the handle_sigkill() callback is deliberately NOT called in
>> the STOP case and only called on fatal signal delivery. And for
>> vhost_task backend the .handle_sigkill() callback is exactly
>> vhost_worker_killed().
>>
>> So my understanding is: if we only call synchronize_rcu() here and leave
>> this path undrained, then whatever work which was put by send_pkt() for
>> the worker currently being freed - will be lost. Please correct me if
>> I'm wrong.
>
> Yep, your right. But what will be the issue of loosing them?
>
> IIUC we are not loosing any data, just avoiding some works that will be
> handled later when/if will set a new owner.
>
But will it actually be handled?
vhost_transport_send_pkt() // called on every packet send
virtio_vsock_skb_queue_tail(&send_pkt_queue, skb) // add skb to list
vhost_vq_work_queue(&send_pkt_work) // try to arm the work
vhost_worker_queue()
if (!test_and_set_bit(VHOST_WORK_QUEUED, &work->flags)) {
llist_add(&worker->work_list)
}
So send_pkt_queue is a list of skbs, it lives on the vhost_vsock device
state, and survives RESET_OWNER. In that sense you're probably right
that we aren't loosing any data.
There's also send_pkt_work object, also living on the vhost_vsock device
state. So we're accumulating skbs, and then send_pkg_work gets put in
the worker task list - but only if it's NOT already armed in there, i.e.
QUEUED bit is unset. And the bit gets cleared by the workload callback
- for vhost_task backend it's vhost_run_work_list().
The most important thing is WHERE this piece of work is being put. That
is worker->work_list - this list does not survive RESET_OWNER, as we
free the worker in vhost_workers_free().
Now imagine we have RESET_OWNER racing with send_pkt. In
vhost_workers_free() we acquire ptr to a worker but not NULL'ify it yet.
Then on the send_pkt path we arm the send_pkt_work, set the QUEUED bit,
and place it on the work_list of a DYING worker. Then the worker gets
freed. Now we have send_pkt_work (a singleton struct) with QUEUED set
in its flags, and with no worker to walk through this piece of work and
clear this flag. As a result - send_pkt_work can't be placed in the
list of any other worker, because it doesn't pass the "if
(!test_and_set_bit(QUEUED)" check. Thus no new packets can be
processed, and the connection is stalled.
Does this make sense?
>>
>> That said, I agree that vhost_run_work_list() will only work with
>> vhost_task backend, not with kthreads backend. If we do
>> vhost_worker_flush() instead - I guess it'll keep the drain here, yet
>> become backend-agnostic. I.e.:
>>
>>> + if (!xa_empty(&dev->worker_xa)) {
>>> + synchronize_rcu();
>>> + xa_for_each(&dev->worker_xa, i, worker)
>>> + vhost_worker_flush(worker);
>>> + }
>>
>> With the last 2 lines being equivalent to just calling
>> vhost_dev_flush(dev). And once we become backend-agnostic here, I'm
>> guessing the warning reported by Sashiko should be dealt with as well.
>
> I'd avoid `if !xa_empty(&dev->worker_xa)` at all, and call
> synchronize_rcu() in any case.
>
Agreed.
> About vhost_dev_flush(), we are calling it in several places, and maybe
> we should re-check them. E.g. we call in vhost_vsock_flush(), but it's
> also called by vhost_dev_stop(), maybe we can avoid to call
> vhost_vsock_flush() if we call vhost_dev_stop().
>
> I'm not sure we really need another one here, but if you think some
> other works can be queued between the vhost_dev_stop() and the
> synchronize_rcu() we are adding here, then okay, it may have sense.
>
Note that in our particular case we're gonna do:
vhost_workers_free()
vhost_dev_flush() // the flush we're planning to add
xa_for_each(&dev->worker_xa, i, worker)
vhost_worker_destroy(dev, worker)
xa_destroy(&dev->worker_xa)
So we walk through the XArray, destroy workers in it one by one, then
destroy the XArray itself. Then the next time we call
vhost_dev_flush(), e.g. from vhost_dev_stop() or wherever else, it tries
iterating over the XArray which no longer exists - which is gonna be a
no-op.
Now, we can reach vhost_workers_free() via (at least) 2 paths:
RESET_OWNER and device release path. On the former the flush is needed
as I illustrated above. On the latter it's indeed redundant but is
cheap as it's a no-op.
Andrey
> Thanks,
> Stefano
>
^ permalink raw reply
* Re: [PATCH v2 00/21] gpio: Improvements around device-id arrays
From: Andy Shevchenko @ 2026-07-16 17:45 UTC (permalink / raw)
To: Uwe Kleine-König (The Capable Hub)
Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio, linux-kernel,
Lixu Zhang, Sakari Ailus, Hans de Goede, Andy Shevchenko, Ray Jui,
Broadcom internal kernel review list, Florian Fainelli,
Scott Branden, Eugeniy Paltsev, Frank Li, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Imre Kaloz, Conor Dooley,
Daire McNamara, Daniel Palmer, Romain Perier, Robert Jarzmik, imx,
linux-arm-kernel, linux-riscv, Hoan Tran, Alan Borzeszkowski,
Mika Westerberg, Enrico Weigelt, metux IT consult, Viresh Kumar,
virtualization, Yinbo Zhu, Thierry Reding, Jonathan Hunter,
linux-tegra, Geert Uytterhoeven, Joel Stanley, Andrew Jeffery,
Alban Bedel, James Cowgill, Matt Redfearn, Neil Jones,
Nikolaos Pasaloukos, Doug Berger, Keerthy, Vladimir Zapolskiy,
Piotr Wojtaszczyk, Sven Peter, Janne Grunau, Neal Gompa,
Mathieu Dubois-Briand, André Draszik, Bamvor Jian Zhang,
Marek Behún, Matthias Brugger, AngeloGioacchino Del Regno,
Avi Fishman, Tomer Maimon, Tali Perry, Patrick Venture,
Nancy Yuen, Benjamin Fair, Santosh Shilimkar, Kevin Hilman,
Orson Zhai, Baolin Wang, Chunyan Zhang, Magnus Damm,
Manivannan Sadhasivam, Heiko Stuebner, Ludovic Desroches,
Paul Walmsley, Samuel Holland, Michael Walle, Maxime Coquelin,
Alexandre Torgue, Nobuhiro Iwamatsu, Shubhrajyoti Datta,
Srinivas Neeli, Michal Simek, linux-aspeed, asahi, linux-mediatek,
openbmc, linux-omap, linux-renesas-soc, linux-unisoc,
linux-rockchip, linux-stm32, Michael Buesch,
William Breathitt Gray, Robert Richter, linux-acpi
In-Reply-To: <alkYWcuMsfE5jkLR@ashevche-desk.local>
On Thu, Jul 16, 2026 at 08:44:14PM +0300, Andy Shevchenko wrote:
> On Wed, Jul 15, 2026 at 06:55:05PM +0200, Uwe Kleine-König (The Capable Hub) wrote:
> > Hello,
> >
> > as with v1 the original motivation for this series are the patches that
> > convert the arrays to use named initializers, see
> > https://lore.kernel.org/all/cover.1780048925.git.u.kleine-koenig@baylibre.com/
> > for the idea behind it. Then by not closing the eyes for small issues
> > spotted while working on that, this series grew a bit.
> >
> > Changes since v1 (available at
> > https://lore.kernel.org/linux-kernel/cover.1784013063.git.u.kleine-koenig@baylibre.com):
>
> Sorry, but at first glance it seems this version is messed up.
> I'm not going to look into it deeply, waiting for *carefully*
> checked v3...
And with the above being said, my tags against v2 are not valid
(withdrawn).
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2 00/21] gpio: Improvements around device-id arrays
From: Andy Shevchenko @ 2026-07-16 17:43 UTC (permalink / raw)
To: Uwe Kleine-König (The Capable Hub)
Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio, linux-kernel,
Lixu Zhang, Sakari Ailus, Hans de Goede, Andy Shevchenko, Ray Jui,
Broadcom internal kernel review list, Florian Fainelli,
Scott Branden, Eugeniy Paltsev, Frank Li, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, Imre Kaloz, Conor Dooley,
Daire McNamara, Daniel Palmer, Romain Perier, Robert Jarzmik, imx,
linux-arm-kernel, linux-riscv, Hoan Tran, Alan Borzeszkowski,
Mika Westerberg, Enrico Weigelt, metux IT consult, Viresh Kumar,
virtualization, Yinbo Zhu, Thierry Reding, Jonathan Hunter,
linux-tegra, Geert Uytterhoeven, Joel Stanley, Andrew Jeffery,
Alban Bedel, James Cowgill, Matt Redfearn, Neil Jones,
Nikolaos Pasaloukos, Doug Berger, Keerthy, Vladimir Zapolskiy,
Piotr Wojtaszczyk, Sven Peter, Janne Grunau, Neal Gompa,
Mathieu Dubois-Briand, André Draszik, Bamvor Jian Zhang,
Marek Behún, Matthias Brugger, AngeloGioacchino Del Regno,
Avi Fishman, Tomer Maimon, Tali Perry, Patrick Venture,
Nancy Yuen, Benjamin Fair, Santosh Shilimkar, Kevin Hilman,
Orson Zhai, Baolin Wang, Chunyan Zhang, Magnus Damm,
Manivannan Sadhasivam, Heiko Stuebner, Ludovic Desroches,
Paul Walmsley, Samuel Holland, Michael Walle, Maxime Coquelin,
Alexandre Torgue, Nobuhiro Iwamatsu, Shubhrajyoti Datta,
Srinivas Neeli, Michal Simek, linux-aspeed, asahi, linux-mediatek,
openbmc, linux-omap, linux-renesas-soc, linux-unisoc,
linux-rockchip, linux-stm32, Michael Buesch,
William Breathitt Gray, Robert Richter, linux-acpi
In-Reply-To: <cover.1784133987.git.u.kleine-koenig@baylibre.com>
On Wed, Jul 15, 2026 at 06:55:05PM +0200, Uwe Kleine-König (The Capable Hub) wrote:
> Hello,
>
> as with v1 the original motivation for this series are the patches that
> convert the arrays to use named initializers, see
> https://lore.kernel.org/all/cover.1780048925.git.u.kleine-koenig@baylibre.com/
> for the idea behind it. Then by not closing the eyes for small issues
> spotted while working on that, this series grew a bit.
>
> Changes since v1 (available at
> https://lore.kernel.org/linux-kernel/cover.1784013063.git.u.kleine-koenig@baylibre.com):
Sorry, but at first glance it seems this version is messed up.
I'm not going to look into it deeply, waiting for *carefully*
checked v3...
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 2/4] virtio_input: validate device-reported multitouch slot count
From: Dmitry Torokhov @ 2026-07-16 17:33 UTC (permalink / raw)
To: Hari Mishal
Cc: Michael S. Tsirkin, Amit Shah, Arnd Bergmann, Greg Kroah-Hartman,
Gerd Hoffmann, Jason Wang, David Hildenbrand, Henrik Rydberg,
Xuan Zhuo, Eugenio Pérez, virtualization, linux-kernel,
linux-input
In-Reply-To: <alkTnRb9qhgcMGGi@google.com>
[ Just realized that CC was dropped in the email I was replying to, so
restoring and resending... ]
On Thu, Jul 16, 2026 at 10:28:36AM -0700, Dmitry Torokhov wrote:
> On Thu, Jul 16, 2026 at 12:34:57PM +0200, Hari Mishal wrote:
> > > What is the failure mode if we keep the ABS_MT_SLOT capability? Does the
> > > kernel crash? And if this can cause crash then we should fix
> > > input_mt_init_slots() to reject requests for 0 slots with -EINVAL.
> > >
> >
> > No, it doesn't crash. I think every place in the input core that touches
> > dev->mt guards against it being NULL: input_handle_abs_event() and
> > the mt_slots check in input.c, and evdev's EVIOCGMTSLOTS ioctl
> > handler all explicitly check for NULL and degrade cleanly instead of
> > dereferencing. From my understanding, the worst case is what the
> > original commit message already covers: the device advertises
> > multitouch support it can't back.
>
> So what? I still do not see the problem. Let's say I have a device that
> properly supports multitouch and has slots, but then never sends any
> events because firmware is buggy. How would that affect anything?
>
> If there is no crash that I would leave the driver alone.
>
> And we need to remember that we are dealing with a hypervisor here that
> normally had higher level of trust than the VM. If it messes up we do
> not have to clean up after it. This scenario is different from user
> attaching a malicious USB device to their system and getting owned.
>
> Thanks.
>
--
Dmitry
^ permalink raw reply
* [PATCH 6.12] vsock/virtio: fix zerocopy completion for multi-skb sends
From: Alexander Martyniuk @ 2026-07-16 16:35 UTC (permalink / raw)
To: stable, Greg Kroah-Hartman
Cc: Alexander Martyniuk, lvc-project, Michael S. Tsirkin, Jason Wang,
Xuan Zhuo, Eugenio Pérez, Stefan Hajnoczi,
Stefano Garzarella, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Simon Horman, Arseniy Krasnov, virtualization, kvm,
netdev, linux-kernel, Maher Azzouzi
From: Stefano Garzarella <sgarzare@redhat.com>
commit ae38d9179190a956e2a87a69ef1dd6f451b51c4d upstream.
When a large message is fragmented into multiple skbs, the zerocopy
uarg is only allocated and attached to the last skb in the loop.
Non-final skbs carry pinned user pages with no completion tracking,
so the kernel has no way to notify userspace when those pages are safe
to reuse. If the loop breaks early the uarg is never allocated at all,
leaking pinned pages with no completion notification.
Fix this by following the approach used by TCP: allocate the zerocopy
uarg (if not provided by the caller) before the send loop and attach
it to every skb via skb_zcopy_set(), which takes a reference per skb.
Each skb's completion properly decrements the refcount, and the
notification only fires after the last skb is freed.
On failure, if no data was sent, the uarg is cleanly aborted via
net_zcopy_put_abort().
This issue was initially discovered by sashiko while reviewing commit
1cb36e252211 ("vsock/virtio: fix MSG_ZEROCOPY pinned-pages accounting")
but was pre-existing.
Fixes: 581512a6dc93 ("vsock/virtio: MSG_ZEROCOPY flag support")
Closes: https://sashiko.dev/#/patchset/20260420132051.217589-1-sgarzare%40redhat.com
Reported-by: Maher Azzouzi <maherazz04@gmail.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Arseniy Krasnov <avkrasnov@salutedevices.com>
Link: https://patch.msgid.link/20260514092948.268720-1-sgarzare@redhat.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Alexander Martyniuk <alexevgmart@gmail.com>
---
Backport fix for CVE-2026-53365
net/vmw_vsock/virtio_transport_common.c | 78 +++++++++++--------------
1 file changed, 34 insertions(+), 44 deletions(-)
diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
index 95170c7be758..aeb205e84bd3 100644
--- a/net/vmw_vsock/virtio_transport_common.c
+++ b/net/vmw_vsock/virtio_transport_common.c
@@ -72,35 +72,6 @@ static bool virtio_transport_can_zcopy(const struct virtio_transport *t_ops,
return true;
}
-static int virtio_transport_init_zcopy_skb(struct vsock_sock *vsk,
- struct sk_buff *skb,
- struct msghdr *msg,
- bool zerocopy)
-{
- struct ubuf_info *uarg;
-
- if (msg->msg_ubuf) {
- uarg = msg->msg_ubuf;
- net_zcopy_get(uarg);
- } else {
- struct iov_iter *iter = &msg->msg_iter;
- struct ubuf_info_msgzc *uarg_zc;
-
- uarg = msg_zerocopy_realloc(sk_vsock(vsk),
- iter->count,
- NULL);
- if (!uarg)
- return -1;
-
- uarg_zc = uarg_to_msgzc(uarg);
- uarg_zc->zerocopy = zerocopy ? 1 : 0;
- }
-
- skb_zcopy_init(skb, uarg);
-
- return 0;
-}
-
static int virtio_transport_fill_skb(struct sk_buff *skb,
struct virtio_vsock_pkt_info *info,
size_t len,
@@ -321,8 +292,10 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
u32 src_cid, src_port, dst_cid, dst_port;
const struct virtio_transport *t_ops;
struct virtio_vsock_sock *vvs;
+ struct ubuf_info *uarg = NULL;
u32 pkt_len = info->pkt_len;
bool can_zcopy = false;
+ bool have_uref = false;
u32 rest_len;
int ret;
@@ -364,6 +337,25 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
if (can_zcopy)
max_skb_len = min_t(u32, VIRTIO_VSOCK_MAX_PKT_BUF_SIZE,
(MAX_SKB_FRAGS * PAGE_SIZE));
+
+ if (info->msg->msg_flags & MSG_ZEROCOPY &&
+ info->op == VIRTIO_VSOCK_OP_RW) {
+ uarg = info->msg->msg_ubuf;
+
+ if (!uarg) {
+ uarg = msg_zerocopy_realloc(sk_vsock(vsk),
+ pkt_len, NULL);
+ if (!uarg) {
+ virtio_transport_put_credit(vvs, pkt_len);
+ return -ENOMEM;
+ }
+
+ if (!can_zcopy)
+ uarg_to_msgzc(uarg)->zerocopy = 0;
+
+ have_uref = true;
+ }
+ }
}
rest_len = pkt_len;
@@ -382,21 +374,7 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
break;
}
- /* We process buffer part by part, allocating skb on
- * each iteration. If this is last skb for this buffer
- * and MSG_ZEROCOPY mode is in use - we must allocate
- * completion for the current syscall.
- */
- if (info->msg && info->msg->msg_flags & MSG_ZEROCOPY &&
- skb_len == rest_len && info->op == VIRTIO_VSOCK_OP_RW) {
- if (virtio_transport_init_zcopy_skb(vsk, skb,
- info->msg,
- can_zcopy)) {
- kfree_skb(skb);
- ret = -ENOMEM;
- break;
- }
- }
+ skb_zcopy_set(skb, uarg, NULL);
virtio_transport_inc_tx_pkt(vvs, skb);
@@ -420,6 +398,18 @@ static int virtio_transport_send_pkt_info(struct vsock_sock *vsk,
virtio_transport_put_credit(vvs, rest_len);
+ /* msg_zerocopy_realloc() initializes the ubuf_info refcnt to 1.
+ * skb_zcopy_set() increases it for each skb, so we can drop that
+ * initial reference to keep it balanced.
+ */
+ if (have_uref) {
+ if (rest_len == pkt_len)
+ /* No data sent, abort the notification. */
+ net_zcopy_put_abort(uarg, true);
+ else
+ net_zcopy_put(uarg);
+ }
+
/* Return number of bytes, if any data has been sent. */
if (rest_len != pkt_len)
ret = pkt_len - rest_len;
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v4 4/5] vhost: synchronize with RCU readers when freeing workers
From: Stefano Garzarella @ 2026-07-16 16:13 UTC (permalink / raw)
To: Andrey Drobyshev
Cc: linux-kernel, kvm, virtualization, netdev, mst, stefanha,
dongli.zhang, maciej.szmigiero, bchaney, mark.kanda, ptikhomirov,
den
In-Reply-To: <2f680236-f4c1-418b-8401-4dea1230caf0@virtuozzo.com>
On Thu, Jul 16, 2026 at 06:39:48PM +0300, Andrey Drobyshev wrote:
>On 7/16/26 11:57 AM, Stefano Garzarella wrote:
>> On Tue, Jul 14, 2026 at 06:16:37PM +0300, Andrey Drobyshev wrote:
>>> vhost_vq_work_queue() only holds the RCU read lock while it dereferences
>>> vq->worker and queues work on it. vhost_workers_free() however clears
>>> the vq->worker pointers and immediately frees the workers, without
>>> waiting for a grace period. A caller that fetched the worker right
>>> before the pointer was cleared can therefore still be queueing work on
>>> it while it is freed. And even when the queueing itself wins the race,
>>> the work is never run, so its VHOST_WORK_QUEUED bit stays set and all
>>> future attempts to queue it are silently skipped.
>>>
>>> None of the current callers can actually hit this: net and scsi stop
>>> their virtqueues before the workers are freed, and vsock unhashes the
>>> device and does synchronize_rcu() of its own in vhost_vsock_dev_release()
>>> before the workers go away. But the upcoming VHOST_RESET_OWNER support
>>> in vhost-vsock keeps the device hashed while its workers are freed, so
>>> the lockless send/cancel paths become able to race with the teardown.
>>>
>>> Close this the way vhost_worker_killed() already does: clear the
>>> vq->worker pointers, wait for a grace period, run whatever the last
>>> readers may have queued, and only then free the workers. The
>>> synchronize_rcu() is skipped if the device has no workers, so cleanup of
>>> devices which never got an owner stays cheap.
>>>
>>
>> Do we need a Fixes tag for this?
>>
>
>I'm guessing it should be:
>
>Fixes: 228a27cf78af ("vhost: Allow worker switching while work is queueing")
>
>> Thanks for pointing out that the issue wasn't occurring, but I think we
>> should add it because it's a sneaky problem we discovered by chance.
>> IMO the code should already have `synchronize_rcu()` after
>> `rcu_assign_pointer()` loop.
>>
>> @Michael, what do you think?
>>
>>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>>> ---
>>> drivers/vhost/vhost.c | 15 +++++++++++++++
>>> 1 file changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>>> index 4c525b3e16ea..0d1414d40f4e 100644
>>> --- a/drivers/vhost/vhost.c
>>> +++ b/drivers/vhost/vhost.c
>>> @@ -729,6 +729,21 @@ static void vhost_workers_free(struct vhost_dev *dev)
>>>
>>> for (i = 0; i < dev->nvqs; i++)
>>> rcu_assign_pointer(dev->vqs[i]->worker, NULL);
>>> +
>>> + /*
>>> + * vhost_vq_work_queue() reads vq->worker under rcu_read_lock(), so a
>>> + * caller that fetched a worker before we cleared the pointers above
>>> + * may still be about to queue work on it. Wait for those RCU readers
>>> + * to finish before freeing the worker, then run whatever they queued
>>> + * so nothing is left with VHOST_WORK_QUEUED set. Mirrors
>>> + * vhost_worker_killed().
>>> + */
>>> + if (!xa_empty(&dev->worker_xa)) {
>>> + synchronize_rcu();
>>> + xa_for_each(&dev->worker_xa, i, worker)
>>> + vhost_run_work_list(worker);
>>> + }
>>> +
>>
>> Following sashiko review [1], I tried to undersand why we need this, but
>> TBH I'm really confused. That said, this seems wrong also because it
>> will work only with vhost_tasks, and not with kthreads.
>>
>> IIUC vhost_worker_killed() will be called anyway when calling
>> vhost_worker_destroy(). For vhost_tasks, it will call
>> vhost_task_do_stop() that calls vhost_task_stop(). This sets
>> VHOST_TASK_FLAGS_STOP and wait the worker on vtsk->exited before freeing
>> stuff. The worker breaks the loop and calls vtsk->handle_sigkill() that
>> is exactly vhost_worker_killed() you mentioned we are mirroring here.
>>
>
>Hmm, are we sure it's the case for our codepath? Looking at the
>vhost_task loop function:
>
>> static int vhost_task_fn(void *data)
>> {
>> for (;;) {
>> if (signal_pending(current)) {
>> if (get_signal(&ksig))
>> break;
>> }
>> ...
>> if (test_bit(VHOST_TASK_FLAGS_STOP, &vtsk->flags)) {
>> __set_current_state(TASK_RUNNING);
>> break;
>> }
>> did_work = vtsk->fn(vtsk->data);
>> ...
>> }
>>
>> ...
>>
>> if (!test_bit(VHOST_TASK_FLAGS_STOP, &vtsk->flags)) {
>> set_bit(VHOST_TASK_FLAGS_KILLED, &vtsk->flags);
>> vtsk->handle_sigkill(vtsk->data);
>> }
>> ...
>> }
>
>AFAICT, we exit the loop in 2 cases: signal delivery or STOP bit
>setting. Like you said, STOP is set by vhost_task_stop. E.g. for our
>RESET_OWNER case:
>
>vhost_vsock_reset_owner()
> vhost_dev_reset_owner()
> vhost_dev_cleanup()
> vhost_workers_free()
> vhost_worker_destroy()
> vhost_task_stop() // for vhost_task_ops backend
> set_bit(VHOST_TASK_FLAGS_STOP)
>
>So, first of all, actual work by .fn() callback is done after the exit
>checks, therefore we skip it - no chance to drain there.
>
>Secondly, the handle_sigkill() callback is deliberately NOT called in
>the STOP case and only called on fatal signal delivery. And for
>vhost_task backend the .handle_sigkill() callback is exactly
>vhost_worker_killed().
>
>So my understanding is: if we only call synchronize_rcu() here and leave
>this path undrained, then whatever work which was put by send_pkt() for
>the worker currently being freed - will be lost. Please correct me if
>I'm wrong.
Yep, your right. But what will be the issue of loosing them?
IIUC we are not loosing any data, just avoiding some works that will be
handled later when/if will set a new owner.
>
>That said, I agree that vhost_run_work_list() will only work with
>vhost_task backend, not with kthreads backend. If we do
>vhost_worker_flush() instead - I guess it'll keep the drain here, yet
>become backend-agnostic. I.e.:
>
>> + if (!xa_empty(&dev->worker_xa)) {
>> + synchronize_rcu();
>> + xa_for_each(&dev->worker_xa, i, worker)
>> + vhost_worker_flush(worker);
>> + }
>
>With the last 2 lines being equivalent to just calling
>vhost_dev_flush(dev). And once we become backend-agnostic here, I'm
>guessing the warning reported by Sashiko should be dealt with as well.
I'd avoid `if !xa_empty(&dev->worker_xa)` at all, and call
synchronize_rcu() in any case.
About vhost_dev_flush(), we are calling it in several places, and maybe
we should re-check them. E.g. we call in vhost_vsock_flush(), but it's
also called by vhost_dev_stop(), maybe we can avoid to call
vhost_vsock_flush() if we call vhost_dev_stop().
I'm not sure we really need another one here, but if you think some
other works can be queued between the vhost_dev_stop() and the
synchronize_rcu() we are adding here, then okay, it may have sense.
Thanks,
Stefano
^ permalink raw reply
* Re: [PATCH v2 1/4] virtio-mem: validate device-reported block size
From: David Hildenbrand (Arm) @ 2026-07-16 15:59 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Hari Mishal, Michael S . Tsirkin, Jason Wang, Xuan Zhuo,
Eugenio Pérez, virtualization, linux-kernel
In-Reply-To: <2026071635-relive-flogging-2a81@gregkh>
On 7/16/26 16:18, Greg Kroah-Hartman wrote:
> On Thu, Jul 16, 2026 at 10:55:42AM +0200, David Hildenbrand (Arm) wrote:
>> On 7/15/26 18:41, Hari Mishal wrote:
>>> The device_block_size read from the virtio-mem config space is used as a
>>> divisor and also in ALIGN_DOWN() further down the code path in the
>>> driver without further validation. A zero value leads to a division by
>>> zero, and a non-power-of-two value corrupts the ALIGN_DOWN() bitmask
>>> arithmetic leading to a misreporting of guest-usable ram, post crash.
>>>
>>> Reject both at init time instead of trusting the device.
>>>
>>> Signed-off-by: Hari Mishal <harimishal1@gmail.com>
>>> ---
>>> v2: dropped the redundant explicit zero check, since
>>> is_power_of_2(0) already returns false.
>>>
>>> drivers/virtio/virtio_mem.c | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
>>> index 11c441501582..0e04fec458af 100644
>>> --- a/drivers/virtio/virtio_mem.c
>>> +++ b/drivers/virtio/virtio_mem.c
>>> @@ -2847,6 +2847,12 @@ static int virtio_mem_init(struct virtio_mem *vm)
>>> &vm->plugged_size);
>>> virtio_cread_le(vm->vdev, struct virtio_mem_config, block_size,
>>> &vm->device_block_size);
>>> + if (!is_power_of_2(vm->device_block_size)) {
>>> + dev_err(&vm->vdev->dev,
>>> + "invalid device block size: 0x%llx\n",
>>> + (unsigned long long)vm->device_block_size);
>>> + return -EINVAL;
>>> + }
>>
>> The spec states "The device MUST set block_size to a power of two."
>>
>> I'm missing the point here.
>
> So what happens if we have a non-spec-compliant device? Shouldn't we be
> attempting to verify this before doing something with the data?
The problem I see is that there are plenty of other devices where a
non-compliant device might cause problems.
Like, we request to hotplug some memory block and our device ACKs it, but it
simply didn't do that.
Or we request to hotunplug a memory block and instead it hotplugs some random
other memory block.
Or we sense whether a memory block is plugged and the device lies to us.
>
> Or do we just always trust virtio mem devices explicitly?
It's hard for me to understand where we draw the line, really.
But maybe MST can clarify what we care about in virtio world where the
hypervisor is fully in charge of the device,
--
Cheers,
David
^ permalink raw reply
* Re: [PATCH v4 4/5] vhost: synchronize with RCU readers when freeing workers
From: Andrey Drobyshev @ 2026-07-16 15:39 UTC (permalink / raw)
To: Stefano Garzarella
Cc: linux-kernel, kvm, virtualization, netdev, mst, stefanha,
dongli.zhang, maciej.szmigiero, bchaney, mark.kanda, ptikhomirov,
den
In-Reply-To: <alibLtcrC7o5r4Dh@sgarzare-redhat>
On 7/16/26 11:57 AM, Stefano Garzarella wrote:
> On Tue, Jul 14, 2026 at 06:16:37PM +0300, Andrey Drobyshev wrote:
>> vhost_vq_work_queue() only holds the RCU read lock while it dereferences
>> vq->worker and queues work on it. vhost_workers_free() however clears
>> the vq->worker pointers and immediately frees the workers, without
>> waiting for a grace period. A caller that fetched the worker right
>> before the pointer was cleared can therefore still be queueing work on
>> it while it is freed. And even when the queueing itself wins the race,
>> the work is never run, so its VHOST_WORK_QUEUED bit stays set and all
>> future attempts to queue it are silently skipped.
>>
>> None of the current callers can actually hit this: net and scsi stop
>> their virtqueues before the workers are freed, and vsock unhashes the
>> device and does synchronize_rcu() of its own in vhost_vsock_dev_release()
>> before the workers go away. But the upcoming VHOST_RESET_OWNER support
>> in vhost-vsock keeps the device hashed while its workers are freed, so
>> the lockless send/cancel paths become able to race with the teardown.
>>
>> Close this the way vhost_worker_killed() already does: clear the
>> vq->worker pointers, wait for a grace period, run whatever the last
>> readers may have queued, and only then free the workers. The
>> synchronize_rcu() is skipped if the device has no workers, so cleanup of
>> devices which never got an owner stays cheap.
>>
>
> Do we need a Fixes tag for this?
>
I'm guessing it should be:
Fixes: 228a27cf78af ("vhost: Allow worker switching while work is queueing")
> Thanks for pointing out that the issue wasn't occurring, but I think we
> should add it because it's a sneaky problem we discovered by chance.
> IMO the code should already have `synchronize_rcu()` after
> `rcu_assign_pointer()` loop.
>
> @Michael, what do you think?
>
>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> ---
>> drivers/vhost/vhost.c | 15 +++++++++++++++
>> 1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>> index 4c525b3e16ea..0d1414d40f4e 100644
>> --- a/drivers/vhost/vhost.c
>> +++ b/drivers/vhost/vhost.c
>> @@ -729,6 +729,21 @@ static void vhost_workers_free(struct vhost_dev *dev)
>>
>> for (i = 0; i < dev->nvqs; i++)
>> rcu_assign_pointer(dev->vqs[i]->worker, NULL);
>> +
>> + /*
>> + * vhost_vq_work_queue() reads vq->worker under rcu_read_lock(), so a
>> + * caller that fetched a worker before we cleared the pointers above
>> + * may still be about to queue work on it. Wait for those RCU readers
>> + * to finish before freeing the worker, then run whatever they queued
>> + * so nothing is left with VHOST_WORK_QUEUED set. Mirrors
>> + * vhost_worker_killed().
>> + */
>> + if (!xa_empty(&dev->worker_xa)) {
>> + synchronize_rcu();
>> + xa_for_each(&dev->worker_xa, i, worker)
>> + vhost_run_work_list(worker);
>> + }
>> +
>
> Following sashiko review [1], I tried to undersand why we need this, but
> TBH I'm really confused. That said, this seems wrong also because it
> will work only with vhost_tasks, and not with kthreads.
>
> IIUC vhost_worker_killed() will be called anyway when calling
> vhost_worker_destroy(). For vhost_tasks, it will call
> vhost_task_do_stop() that calls vhost_task_stop(). This sets
> VHOST_TASK_FLAGS_STOP and wait the worker on vtsk->exited before freeing
> stuff. The worker breaks the loop and calls vtsk->handle_sigkill() that
> is exactly vhost_worker_killed() you mentioned we are mirroring here.
>
Hmm, are we sure it's the case for our codepath? Looking at the
vhost_task loop function:
> static int vhost_task_fn(void *data)
> {
> for (;;) {
> if (signal_pending(current)) {
> if (get_signal(&ksig))
> break;
> }
> ...
> if (test_bit(VHOST_TASK_FLAGS_STOP, &vtsk->flags)) {
> __set_current_state(TASK_RUNNING);
> break;
> }
> did_work = vtsk->fn(vtsk->data);
> ...
> }
>
> ...
>
> if (!test_bit(VHOST_TASK_FLAGS_STOP, &vtsk->flags)) {
> set_bit(VHOST_TASK_FLAGS_KILLED, &vtsk->flags);
> vtsk->handle_sigkill(vtsk->data);
> }
> ...
> }
AFAICT, we exit the loop in 2 cases: signal delivery or STOP bit
setting. Like you said, STOP is set by vhost_task_stop. E.g. for our
RESET_OWNER case:
vhost_vsock_reset_owner()
vhost_dev_reset_owner()
vhost_dev_cleanup()
vhost_workers_free()
vhost_worker_destroy()
vhost_task_stop() // for vhost_task_ops backend
set_bit(VHOST_TASK_FLAGS_STOP)
So, first of all, actual work by .fn() callback is done after the exit
checks, therefore we skip it - no chance to drain there.
Secondly, the handle_sigkill() callback is deliberately NOT called in
the STOP case and only called on fatal signal delivery. And for
vhost_task backend the .handle_sigkill() callback is exactly
vhost_worker_killed().
So my understanding is: if we only call synchronize_rcu() here and leave
this path undrained, then whatever work which was put by send_pkt() for
the worker currently being freed - will be lost. Please correct me if
I'm wrong.
That said, I agree that vhost_run_work_list() will only work with
vhost_task backend, not with kthreads backend. If we do
vhost_worker_flush() instead - I guess it'll keep the drain here, yet
become backend-agnostic. I.e.:
> + if (!xa_empty(&dev->worker_xa)) {
> + synchronize_rcu();
> + xa_for_each(&dev->worker_xa, i, worker)
> + vhost_worker_flush(worker);
> + }
With the last 2 lines being equivalent to just calling
vhost_dev_flush(dev). And once we become backend-agnostic here, I'm
guessing the warning reported by Sashiko should be dealt with as well.
WDYT?
Andrey
> So, why we need this?
>
> Should be enough to call synchronize_rcu() in any case after the
> rcu_assign_pointer() loop?
>
> Thanks,
> Stefano
>
> [1]
> https://sashiko.dev/#/patchset/20260714151638.143019-1-andrey.drobyshev@virtuozzo.com?part=4
>
^ permalink raw reply
* Re: [PATCH v2 1/4] virtio-mem: validate device-reported block size
From: Greg Kroah-Hartman @ 2026-07-16 14:18 UTC (permalink / raw)
To: David Hildenbrand (Arm)
Cc: Hari Mishal, Michael S . Tsirkin, Jason Wang, Xuan Zhuo,
Eugenio Pérez, virtualization, linux-kernel
In-Reply-To: <b2176313-b444-4f66-a71c-647b92cc16a8@kernel.org>
On Thu, Jul 16, 2026 at 10:55:42AM +0200, David Hildenbrand (Arm) wrote:
> On 7/15/26 18:41, Hari Mishal wrote:
> > The device_block_size read from the virtio-mem config space is used as a
> > divisor and also in ALIGN_DOWN() further down the code path in the
> > driver without further validation. A zero value leads to a division by
> > zero, and a non-power-of-two value corrupts the ALIGN_DOWN() bitmask
> > arithmetic leading to a misreporting of guest-usable ram, post crash.
> >
> > Reject both at init time instead of trusting the device.
> >
> > Signed-off-by: Hari Mishal <harimishal1@gmail.com>
> > ---
> > v2: dropped the redundant explicit zero check, since
> > is_power_of_2(0) already returns false.
> >
> > drivers/virtio/virtio_mem.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
> > index 11c441501582..0e04fec458af 100644
> > --- a/drivers/virtio/virtio_mem.c
> > +++ b/drivers/virtio/virtio_mem.c
> > @@ -2847,6 +2847,12 @@ static int virtio_mem_init(struct virtio_mem *vm)
> > &vm->plugged_size);
> > virtio_cread_le(vm->vdev, struct virtio_mem_config, block_size,
> > &vm->device_block_size);
> > + if (!is_power_of_2(vm->device_block_size)) {
> > + dev_err(&vm->vdev->dev,
> > + "invalid device block size: 0x%llx\n",
> > + (unsigned long long)vm->device_block_size);
> > + return -EINVAL;
> > + }
>
> The spec states "The device MUST set block_size to a power of two."
>
> I'm missing the point here.
So what happens if we have a non-spec-compliant device? Shouldn't we be
attempting to verify this before doing something with the data?
Or do we just always trust virtio mem devices explicitly?
thanks,
greg k-h
^ permalink raw reply
* [PATCH] vdpa: Remove redundant dev_err()
From: Pan Chuang @ 2026-07-16 14:13 UTC (permalink / raw)
To: reviewer:MARVELL OCTEON ENDPOINT VIRTIO DATA PATH ACCELE...,commit_signer:2/8=25%,authored:2/8=25%,added_lines:97/119=82%,added_lines:17/119=14%,removed_lines:16/25=64%,removed_lines:3/25=12%,
reviewer:MARVELL OCTEON ENDPOINT VIRTIO DATA PATH ACCELE...,commit_signer:2/8=25%,authored:2/8=25%,added_lines:97/119=82%,added_lines:17/119=14%,removed_lines:16/25=64%,removed_lines:3/25=12%,
Michael S. Tsirkin, Jason Wang, Xuan Zhuo, Eugenio Pérez,
Pan Chuang, Miaoqian Lin, Kees Cook, open list:VIRTIO CORE,
open list
Since commit 55b48e23f5c4 ("genirq/devres: Add error handling in
devm_request_*_irq()"), devm_request_irq() automatically logs
detailed error messages on failure. Remove the now-redundant
driver-specific dev_err() calls.
Signed-off-by: Pan Chuang <panchuang@vivo.com>
---
drivers/vdpa/octeon_ep/octep_vdpa_main.c | 4 +---
drivers/vdpa/virtio_pci/vp_vdpa.c | 12 +++---------
2 files changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/vdpa/octeon_ep/octep_vdpa_main.c b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
index 5b35993750f5..6e32925bc57a 100644
--- a/drivers/vdpa/octeon_ep/octep_vdpa_main.c
+++ b/drivers/vdpa/octeon_ep/octep_vdpa_main.c
@@ -170,10 +170,8 @@ static int octep_request_irqs(struct octep_hw *oct_hw, irqreturn_t (*irq_handler
irq = pci_irq_vector(pdev, idx);
ret = devm_request_irq(&pdev->dev, irq, irq_handler, 0, dev_name(&pdev->dev),
oct_hw);
- if (ret) {
- dev_err(&pdev->dev, "Failed to register interrupt handler\n");
+ if (ret)
goto free_irqs;
- }
oct_hw->irqs[idx] = irq;
}
oct_hw->requested_irqs = nb_irqs;
diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c
index 51ffc245a038..f2eb654b1665 100644
--- a/drivers/vdpa/virtio_pci/vp_vdpa.c
+++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
@@ -189,11 +189,8 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
vp_vdpa_vq_handler,
0, vp_vdpa->vring[i].msix_name,
&vp_vdpa->vring[i]);
- if (ret) {
- dev_err(&pdev->dev,
- "vp_vdpa: fail to request irq for vq %d\n", i);
+ if (ret)
goto err;
- }
vp_modern_queue_vector(mdev, i, msix_vec);
vp_vdpa->vring[i].irq = irq;
msix_vec++;
@@ -204,11 +201,8 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
irq = pci_irq_vector(pdev, msix_vec);
ret = devm_request_irq(&pdev->dev, irq, vp_vdpa_config_handler, 0,
vp_vdpa->msix_name, vp_vdpa);
- if (ret) {
- dev_err(&pdev->dev,
- "vp_vdpa: fail to request irq for config: %d\n", ret);
- goto err;
- }
+ if (ret)
+ goto err;
vp_modern_config_vector(mdev, msix_vec);
vp_vdpa->config_irq = irq;
--
2.34.1
^ permalink raw reply related
* [PATCH v3] virtio_ring: fix infinite loop in virtnet_poll_cleantx when device is broken
From: Jinqian Yang @ 2026-07-16 11:59 UTC (permalink / raw)
To: mst, jasowang, xuanzhuo, eperezma, andrew+netdev, davem, edumazet,
kuba, pabeni
Cc: netdev, virtualization, linux-kernel, liuyonglong, wangzhou1,
linuxarm, Jinqian Yang
virtnet_poll_cleantx() contains a do-while loop that cleans up
transmitted TX buffers and calls virtqueue_enable_cb_delayed() to check
whether more buffers need processing. When the virtio backend stops
responding during guest reboot, used->idx is never updated, so
virtqueue_enable_cb_delayed() always returns false and the loop never
terminates. Then it will block reboot process, and the guest will hang.
The problem occurs during guest reboot under network traffic:
1. kernel_restart() -> device_shutdown() traverses the device list
2. virtio_dev_shutdown() calls virtio_break_device() which sets
vq->broken = true
3. virtio_dev_shutdown() then calls virtio_synchronize_cbs() to wait
for in-flight callbacks to complete
4. A virtio interrupt fires, softirq is deferred to ksoftirqd which
calls net_rx_action() -> virtnet_poll() -> virtnet_poll_cleantx()
5. virtnet_poll_cleantx() enters the do-while loop and never exits
because the QEMU backend has stopped updating used->idx, despite
vq->broken having been set to true in step 2.
Since the loop runs inside ksoftirqd (a SCHED_OTHER kthread), it is
visible to the scheduler and does not trigger a hard lockup. However,
the kthread never leaves the loop, so RCU detects it as a CPU stall
and reports it periodically. Meanwhile, the reboot process remains
blocked in device_shutdown() because virtio_dev_shutdown() cannot
complete its synchronization step, and the guest hangs permanently.
This can be reproduced on a guest with a virtio-net device: run iperf3
traffic in the guest, then trigger reboot. The reboot occasionally hangs
permanently with RCU stall on ksoftirqd.
Observed on ARM64 KVM guest:
CPU#1 RCU stall (ksoftirqd/1), repeated periodically:
virtqueue_enable_cb_delayed_split <- virtnet_poll <- __napi_poll <-
net_rx_action <- handle_softirqs <- run_ksoftirqd <-
smpboot_thread_fn <- kthread
Fix by adding a vq->broken check in virtqueue_enable_cb_delayed(), so
that the loop exits immediately when the device is broken, allowing
the device shutdown to proceed.
Signed-off-by: Jinqian Yang <yangjinqian1@huawei.com>
---
Changes in v2:
- Moved vq->broken check to virtqueue_enable_cb_delayed().
Changes in v3:
- Updated the patch subject prefix.
v1: https://lore.kernel.org/lkml/20260713132025.703147-1-yangjinqian1@huawei.com/
v2: https://lore.kernel.org/lkml/20260716035201.3736582-1-yangjinqian1@huawei.com/
---
drivers/virtio/virtio_ring.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index b438dc2ce1b8..5c169fbb418a 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -3233,6 +3233,14 @@ bool virtqueue_enable_cb_delayed(struct virtqueue *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
+ /*
+ * When the device is broken there is no point in polling used->idx,
+ * the backend will never update it. Return true to let callers
+ * exit their cleanup loops instead of spinning forever.
+ */
+ if (unlikely(vq->broken))
+ return true;
+
if (vq->event_triggered)
data_race(vq->event_triggered = false);
--
2.33.0
^ permalink raw reply related
* Re: [PATCH] vhost-vdpa: reject zero-size unmap
From: Eugenio Perez Martin @ 2026-07-16 10:04 UTC (permalink / raw)
To: Weimin Xiong; +Cc: virtualization, mst, jasowangio, netdev, kvm, xiongweimin
In-Reply-To: <20260716030238.124368-1-xiongwm2026@163.com>
On Thu, Jul 16, 2026 at 5:02 AM Weimin Xiong <xiongwm2026@163.com> wrote:
>
> From: xiongweimin <xiongweimin@kylinos.cn>
>
> Reject unmap requests with size == 0 to prevent iova + size - 1
> from underflowing to U64_MAX, which would incorrectly unmap the
> entire IOTLB range.
>
> This fix also covers the error rollback path in vhost_vdpa_va_map:
> when the first VMA lookup fails, map_iova equals iova, resulting
> in a zero-size unmap that would otherwise clear the whole IOTLB.
>
> Signed-off-by: xiongweimin <xiongweimin@kylinos.cn>
Acked-by: Eugenio Pérez <eperezma@redhat.com>
Thanks!
> ---
> drivers/vhost/vdpa.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index bb96b1aa5..f49bf1cfb 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -1035,6 +1035,9 @@ static void vhost_vdpa_unmap(struct vhost_vdpa *v,
> const struct vdpa_config_ops *ops = vdpa->config;
> u32 asid = iotlb_to_asid(iotlb);
>
> + if (!size)
> + return;
> +
> vhost_vdpa_iotlb_unmap(v, iotlb, iova, iova + size - 1, asid);
>
> if (ops->set_map) {
> --
> 2.39.3
>
^ permalink raw reply
* Re: [PATCH] vhost-vdpa: propagate set_map error to caller
From: Eugenio Perez Martin @ 2026-07-16 10:01 UTC (permalink / raw)
To: Weimin Xiong; +Cc: virtualization, mst, jasowangio, netdev, kvm, xiongweimin
In-Reply-To: <20260716030242.124455-1-xiongwm2026@163.com>
On Thu, Jul 16, 2026 at 5:02 AM Weimin Xiong <xiongwm2026@163.com> wrote:
>
> From: xiongweimin <xiongweimin@kylinos.cn>
>
> The return value of ops->set_map() is currently ignored when handling
> VHOST_IOTLB_BATCH_END. If the backend fails to program the IOTLB,
> the VMM incorrectly believes the operation succeeded and may continue
> with stale or incorrect mappings.
>
> Save and propagate the error from ops->set_map() in BATCH_END.
>
> Signed-off-by: xiongweimin <xiongweimin@kylinos.cn>
> ---
> drivers/vhost/vdpa.c | 6 +++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index bb96b1aa5..ffbb10a92 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -1297,8 +1297,10 @@ static int vhost_vdpa_process_iotlb_msg(struct vhost_dev *dev, u32 asid,
> v->in_batch = true;
> break;
> case VHOST_IOTLB_BATCH_END:
> - if (v->in_batch && ops->set_map)
> - ops->set_map(vdpa, asid, iotlb);
> + if (v->in_batch && ops->set_map) {
> + r = ops->set_map(vdpa, asid, iotlb);
> + break;
> + }
> v->in_batch = false;
Maybe this needs to be discussed, but v->in_batch should be set to
false even in failure case.
> break;
> default:
> --
> 2.39.3
>
^ permalink raw reply
* Re: [PATCH] vhost: reject zero-size IOTLB INVALIDATE
From: Eugenio Perez Martin @ 2026-07-16 9:41 UTC (permalink / raw)
To: Weimin Xiong; +Cc: virtualization, mst, jasowangio, netdev, kvm, xiongweimin
In-Reply-To: <20260716030236.124322-1-xiongwm2026@163.com>
On Thu, Jul 16, 2026 at 5:02 AM Weimin Xiong <xiongwm2026@163.com> wrote:
>
> From: xiongweimin <xiongweimin@kylinos.cn>
>
> Reject VHOST_IOTLB_INVALIDATE messages with size == 0 to prevent
> iova + size - 1 from underflowing to U64_MAX, which would
> incorrectly delete the entire IOTLB.
>
> Signed-off-by: xiongweimin <xiongweimin@kylinos.cn>
> ---
> drivers/vhost/vhost.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 3c080c454e374cabd7321416ed92c5f7d3135254..xxxxxxxxxx 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -1656,6 +1656,10 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev, u32 asid,
> if (!dev->iotlb) {
> ret = -EFAULT;
> break;
> + }
> + if (!msg->size) {
> + ret = -EINVAL;
> + break;
> }
I think the issue is real, but how about adding the condition to the
caller vhost_chr_write_iter? It is already the
if (msg.type == VHOST_IOTLB_UPDATE && msg.size == 0) {
ret = -EINVAL;
goto done;
}
So it should be somthing in the line of:
if ((msg.type == VHOST_IOTLB_UPDATE || msg.type ==
VHOST_IOTLB_INVALIDATE) && msg.size == 0) {
ret = -EINVAL;
goto done;
}
With that, please add my acked-by.
> vhost_vq_meta_reset(dev);
> vhost_iotlb_del_range(dev->iotlb, msg->iova,
> --
> 2.39.3
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox