* [PATCH RFC v3 00/19] mm/virtio: skip redundant zeroing of host-zeroed reported pages
From: Michael S. Tsirkin @ 2026-04-21 22:01 UTC (permalink / raw)
To: linux-kernel
Cc: Andrew Morton, David Hildenbrand, Vlastimil Babka,
Brendan Jackman, Michal Hocko, Suren Baghdasaryan, Jason Wang,
Andrea Arcangeli, Gregory Price, linux-mm, virtualization
When a guest reports free pages to the hypervisor via virtio-balloon's
free page reporting, the host typically zeros those pages when reclaiming
their backing memory (e.g., via MADV_DONTNEED on anonymous mappings).
When the guest later reallocates those pages, the kernel zeros them
again, redundantly.
Further, on architectures with aliasing caches, upstream with init_on_alloc
double-zeros user pages: once via kernel_init_pages() in
post_alloc_hook, and again via clear_user_highpage() at the
callsite (because user_alloc_needs_zeroing() returns true).
This series eliminates that double-zeroing by moving the zeroing
into the post_alloc_hook + propagating the "host
already zeroed this page" information through the buddy allocator.
For the reporting part, I am working on virtio spec now, so sending this
out for early feedback. In particular:
- is the mm zeroing rework acceptable?
- is sysfs testing hook for flushing acceptable?
- first 10 patches, including the fix for init_on_alloc double zeroing
are independently mergeable mm rework -
are they deemed a desirable rework, and should I post them
separately for inclusion?
Thanks in advance.
Still an RFC as virtio bits need work, but I would very much like
to get a general agreement on mm bits first, so we don't add
a spec for something we can't then use.
-------
Performance with THP enabled on a 2GB VM, 1 vCPU, allocating
256MB of anonymous pages:
metric baseline optimized delta
task-clock 175 +- 10 ms 40 +- 9 ms -77%
cache-misses 924K +- 323K 287K +- 93K -69%
instructions 15.3M +- 634K 13.5M +- 337K -12%
With hugetlb surplus pages:
metric baseline optimized delta
task-clock 169 +- 9 ms 49 +- 19 ms -71%
cache-misses 1.24M +- 222K 316K +- 114K -75%
instructions 17.3M +- 1.23M 15.0M +- 604K -13%
Notes:
- The virtio_balloon module parameter (13/19) is a testing hack.
A proper virtio feature flag is needed before merging.
- Patch 14/19 adds a sysfs flush trigger for deterministic testing
(avoids waiting for the 2-second reporting delay).
- When host_zeroes_pages is set, callers skip folio_zero_user() for
pages known to be zeroed by the host. This is safe on all
architectures because the hypervisor invalidates guest cache lines
when reclaiming page backing (MADV_DONTNEED).
Two flags track known-zero pages:
PG_zeroed (aliased to PG_private) marks buddy allocator pages that
are known to contain all zeros -- either because the host zeroed
them during page reporting, or because they were freed via the
balloon deflate path. It lives on free-list pages and is consumed
by post_alloc_hook() on allocation.
HPG_zeroed (stored in hugetlb folio->private bits) serves the same
purpose for hugetlb pool pages, which are kept in a pool and may
be zeroed long after buddy allocation, so PG_zeroed (consumed at
allocation time) cannot track their state.
- PG_zeroed is aliased to PG_private. It is excluded from
PAGE_FLAGS_CHECK_AT_PREP because it must survive on free-list pages
until post_alloc_hook() consumes and clears it. Is this acceptable,
or should a different bit be used?
- On architectures with aliasing caches, upstream with init_on_alloc
double-zeros user pages: once via kernel_init_pages() in
post_alloc_hook, and again via clear_user_highpage() at the
callsite (because user_alloc_needs_zeroing() returns true).
Our patches eliminate this by zeroing once via folio_zero_user()
in post_alloc_hook. Not yet performance-tested on aliasing
hardware.
PG_zeroed lifecycle:
Sets PG_zeroed:
- page_reporting_drain: on reported pages when host zeroes them
- __free_pages_ok / __free_frozen_pages: when FPI_ZEROED is set
(balloon deflate path)
- buddy merge: on merged page if both buddies were zeroed
- expand(): propagate to split-off buddy sub-pages
Clears PG_zeroed:
- buddy merge: clear both pages before merge, then conditionally
re-set on merged head if both were zeroed
- post_alloc_hook: clear on head page after consuming the hint
HPG_zeroed lifecycle (hugetlb pool pages, stored in folio->private):
Sets HPG_zeroed:
- alloc_surplus_hugetlb_folio: after buddy allocation with
__GFP_ZERO, mark pool page as known-zero
Clears HPG_zeroed:
- free_huge_folio: page was mapped to userspace, no longer
known-zero when it returns to the pool
- alloc_hugetlb_folio / alloc_hugetlb_folio_reserve: clear
after reporting to caller via bool *zeroed output (consumed)
- The optimization is most effective with THP, where entire 2MB
pages are allocated directly from reported order-9+ buddy pages.
Without THP, only ~21% of order-0 allocations come from reported
pages due to low-order fragmentation.
- Persistent hugetlb pool pages are not covered: when freed by
userspace they return to the hugetlb free pool, not the buddy
allocator, so they are never reported to the host. Surplus
hugetlb pages are allocated from buddy and do benefit.
Test program:
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/mman.h>
#ifndef MADV_POPULATE_WRITE
#define MADV_POPULATE_WRITE 23
#endif
#ifndef MAP_HUGETLB
#define MAP_HUGETLB 0x40000
#endif
int main(int argc, char **argv)
{
unsigned long size;
int flags = MAP_PRIVATE | MAP_ANONYMOUS;
void *p;
int r;
if (argc < 2) {
fprintf(stderr, "usage: %s <size_mb> [huge]\n", argv[0]);
return 1;
}
size = atol(argv[1]) * 1024UL * 1024;
if (argc >= 3 && strcmp(argv[2], "huge") == 0)
flags |= MAP_HUGETLB;
p = mmap(NULL, size, PROT_READ | PROT_WRITE, flags, -1, 0);
if (p == MAP_FAILED) {
perror("mmap");
return 1;
}
r = madvise(p, size, MADV_POPULATE_WRITE);
if (r) {
perror("madvise");
return 1;
}
munmap(p, size);
return 0;
}
Test script (bench.sh):
#!/bin/bash
# Usage: bench.sh <size_mb> <mode> <iterations> [huge]
# mode 0 = baseline, mode 1 = skip zeroing
SZ=${1:-256}; MODE=${2:-0}; ITER=${3:-10}; HUGE=${4:-}
FLUSH=/sys/module/page_reporting/parameters/flush
PERF_DATA=/tmp/perf-$MODE.csv
rmmod virtio_balloon 2>/dev/null
insmod virtio_balloon.ko host_zeroes_pages=$MODE
echo 512 > $FLUSH
[ "$HUGE" = "huge" ] && echo $((SZ/2)) > /proc/sys/vm/nr_overcommit_hugepages
rm -f $PERF_DATA
echo "=== sz=${SZ}MB mode=$MODE iter=$ITER $HUGE ==="
for i in $(seq 1 $ITER); do
echo 3 > /proc/sys/vm/drop_caches
echo 512 > $FLUSH
perf stat -e task-clock,instructions,cache-misses \
-x, -o $PERF_DATA --append -- ./alloc_once $SZ $HUGE
done
[ "$HUGE" = "huge" ] && echo 0 > /proc/sys/vm/nr_overcommit_hugepages
rmmod virtio_balloon
awk -F, '/^#/||/^$/{next}{v=$1+0;e=$3;gsub(/ /,"",e);s[e]+=v;ss[e]+=v*v;n[e]++}
END{for(e in s){a=s[e]/n[e];d=sqrt(ss[e]/n[e]-a*a);printf " %-16s %10.0f +- %8.0f (n=%d)\n",e,a,d,n[e]}}' $PERF_DATA
Compile and run:
gcc -static -O2 -o alloc_once alloc_once.c
bash bench.sh 256 0 10 # baseline (regular pages)
bash bench.sh 256 1 10 # optimized (regular pages)
bash bench.sh 256 0 10 huge # baseline (hugetlb surplus)
bash bench.sh 256 1 10 huge # optimized (hugetlb surplus)
Changes since v2 (address review by Gregory Price and David Hildenbrand):
- v2 used pghint_t / vma_alloc_folio_hints API. v3 switches to
threading user_addr through the page allocator and using __GFP_ZERO,
so post_alloc_hook() can use folio_zero_user() for cache-friendly
zeroing when the user fault address is known.
- Exclude __PG_ZEROED from PAGE_FLAGS_CHECK_AT_PREP macro definition
instead of runtime masking in __free_one_page.
- Drop redundant page_poisoning_enabled() check from mm core free
path -- already guarded at feature negotiation time in
virtio_balloon_validate. The balloon driver keeps its own
page_poisoning_enabled_static() check as defense in depth.
- Split free_frozen_pages_zeroed and put_page_zeroed into separate
patches. David Hildenbrand indicated he intends to rework balloon
pages to be frozen (no refcount), at which point put_page_zeroed
(16/19) can be dropped and the balloon can call
free_frozen_pages_zeroed directly.
- Use HPG_zeroed flag (in hugetlb folio->private) for hugetlb pool
pages instead of PG_zeroed, since pool pages are zeroed long after
buddy allocation and PG_zeroed is consumed at allocation time.
- syzbot CI found a PF_NO_COMPOUND BUG in the v2 pghint_t approach
where __ClearPageZeroed was called on compound hugetlb pages in
free_huge_folio. The v3 HPG_zeroed approach avoids this.
- Remove redundant arch vma_alloc_zeroed_movable_folio overrides
on x86, s390, m68k, and alpha (10/19). Suggested by David
Hildenbrand.
- Updated benchmarking script to compute per-run avg +- stddev
via awk on CSV output.
Changes v1->v2:
- Replaced __GFP_PREZEROED with PG_zeroed page flag (aliased PG_private)
- Added pghint_t type and vma_alloc_folio_hints() API
- Track PG_zeroed across buddy merges and splits
- Added post_alloc_hook integration (single consume/clear point)
- Added hugetlb support (pool pages + memfd)
- Added page_reporting flush parameter for deterministic testing
- Added free_frozen_pages_hint/put_page_hint for balloon deflate path
- Added try_to_claim_block PG_zeroed preservation
- Updated perf numbers with per-iteration flush methodology
Written with assistance from Claude (claude-opus-4-6).
Reviewed by cursor-agent (GPT-5.4-xhigh).
Everything manually read, patchset split and commit logs edited manually.
Michael S. Tsirkin (19):
mm: thread user_addr through page allocator for cache-friendly zeroing
mm: add folio_zero_user stub for configs without THP/HUGETLBFS
mm: page_alloc: move prep_compound_page before post_alloc_hook
mm: use folio_zero_user for user pages in post_alloc_hook
mm: use __GFP_ZERO in vma_alloc_zeroed_movable_folio
mm: use __GFP_ZERO in alloc_anon_folio
mm: use __GFP_ZERO in vma_alloc_anon_folio_pmd
mm: hugetlb: use __GFP_ZERO and skip zeroing for zeroed pages
mm: memfd: skip zeroing for zeroed hugetlb pool pages
mm: remove arch vma_alloc_zeroed_movable_folio overrides
mm: page_alloc: propagate PageReported flag across buddy splits
mm: page_reporting: skip redundant zeroing of host-zeroed reported
pages
virtio_balloon: a hack to enable host-zeroed page optimization
mm: page_reporting: add flush parameter with page budget
mm: add free_frozen_pages_zeroed
mm: add put_page_zeroed and folio_put_zeroed
mm: page_alloc: clear PG_zeroed on buddy merge if not both zero
mm: page_alloc: preserve PG_zeroed in page_del_and_expand
virtio_balloon: mark deflated pages as zeroed
arch/alpha/include/asm/page.h | 3 -
arch/m68k/include/asm/page_no.h | 3 -
arch/s390/include/asm/page.h | 3 -
arch/x86/include/asm/page.h | 3 -
drivers/virtio/virtio_balloon.c | 12 ++-
fs/hugetlbfs/inode.c | 10 ++-
include/linux/gfp.h | 26 ++++--
include/linux/highmem.h | 9 +-
include/linux/hugetlb.h | 14 ++-
include/linux/mm.h | 43 +++++++++
include/linux/page-flags.h | 12 ++-
include/linux/page_reporting.h | 3 +
mm/compaction.c | 5 +-
mm/filemap.c | 3 +-
mm/huge_memory.c | 12 +--
mm/hugetlb.c | 101 +++++++++++++++-------
mm/internal.h | 8 +-
mm/khugepaged.c | 2 +-
mm/memfd.c | 17 ++--
mm/memory.c | 15 +---
mm/mempolicy.c | 39 ++++++---
mm/migrate.c | 2 +-
mm/page_alloc.c | 149 ++++++++++++++++++++++++--------
mm/page_frag_cache.c | 4 +-
mm/page_reporting.c | 56 +++++++++++-
mm/page_reporting.h | 12 +++
mm/shmem.c | 2 +-
mm/slub.c | 4 +-
mm/swap.c | 18 +++-
mm/swap_state.c | 2 +-
30 files changed, 433 insertions(+), 159 deletions(-)
--
MST
^ permalink raw reply
* Re: [RFC PATCH v1] virtio_pci: only store successfully populated virtio_pci_vq_info
From: Michael S. Tsirkin @ 2026-04-21 21:51 UTC (permalink / raw)
To: Link Lin
Cc: jasowang, xuanzhuo, eperezma, jiaqiyan, rientjes, weixugc,
virtualization, linux-kernel
In-Reply-To: <CALUx4KTPcRCGvyvJfHSU_t61UJOuTBaAGxJ6nhUuaghE0_ZfUg@mail.gmail.com>
On Tue, Apr 21, 2026 at 02:47:32PM -0700, Link Lin wrote:
> Hi everyone,
>
> Friendly ping. Apologies if you are getting this the second time - my
> last ping wasn't in plain text mode and got rejected by some mailing
> lists.
>
> Please let me know if anyone has had a chance to look at this RFC
> patch, or if any changes are needed.
>
> Thanks,
> Link
>
> On Tue, Apr 21, 2026 at 2:24 PM Link Lin <linkl@google.com> wrote:
> >
> > Hi everyone,
> >
> > Friendly ping on this RFC patch. Please let me know if anyone has had a chance to look at this, or if any changes are needed.
> >
> > Thanks,
> > Link
> >
> > On Tue, Apr 7, 2026 at 2:25 PM Link Lin <linkl@google.com> wrote:
> >>
> >> In environments where free page reporting is disabled, a kernel
> >> panic is triggered when tearing down the virtio_balloon module:
> >>
> >> [12261.808190] Call trace:
> >> [12261.808471] __list_del_entry_valid_or_report+0x18/0xe0
> >> [12261.809064] vp_del_vqs+0x12c/0x270
> >> [12261.809462] remove_common+0x80/0x98 [virtio_balloon]
> >> [12261.810034] virtballoon_remove+0xfc/0x158 [virtio_balloon]
> >> [12261.810663] virtio_dev_remove+0x68/0xf8
> >> [12261.811108] device_release_driver_internal+0x17c/0x278
> >> [12261.811701] driver_detach+0xd4/0x138
> >> [12261.812117] bus_remove_driver+0x90/0xd0
> >> [12261.812562] driver_unregister+0x40/0x70
> >> [12261.813006] unregister_virtio_driver+0x20/0x38
> >> [12261.813518] cleanup_module+0x20/0x7a8 [virtio_balloon]
> >> [12261.814109] __arm64_sys_delete_module+0x278/0x3d0
> >> [12261.814654] invoke_syscall+0x5c/0x120
> >> [12261.815086] el0_svc_common+0x90/0xf8
> >> [12261.815506] do_el0_svc+0x2c/0x48
> >> [12261.815883] el0_svc+0x3c/0xa8
> >> [12261.816235] el0t_64_sync_handler+0x8c/0x108
> >> [12261.816724] el0t_64_sync+0x198/0x1a0
> >>
> >> The issue originates in vp_find_vqs_intx(). It kzalloc_objs() based
> >> on the nvqs count provided by the caller, virtio_balloon::init_vqs().
> >> However, it is not always the case that all nvqs number of
> >> virtio_pci_vq_info objects will be properly populated.
> >>
> >> For example, when VIRTIO_BALLOON_F_FREE_PAGE_HINT is absent, the
> >> VIRTIO_BALLOON_VQ_FREE_PAGE-th item in the vp_dev->vqs array is
> >> actually never populated, and is still a zeroe-initialized
> >> virtio_pci_vq_info object, which is eventually going to trigger
> >> a __list_del_entry_valid_or_report() crash.
> >>
> >> Tested by applying this patch to a guest VM kernel with the
> >> VIRTIO_BALLOON_F_REPORTING feature enabled and the
> >> VIRTIO_BALLOON_F_FREE_PAGE_HINT feature disabled.
> >> Without this patch, unloading the virtio_balloon module triggers a panic.
> >> With this patch, no panic is observed.
> >>
> >> The fix is to use queue_idx to handle the case that vp_find_vqs_intx()
> >> skips vp_setup_vq() when caller provided null vqs_info[i].name, when
> >> the caller doesn't populate all nvqs number of virtqueue_info objects.
> >> Invariantly queue_idx is the correct index to store a successfully
> >> created and populated virtio_pci_vq_info object. As a result, now
> >> a virtio_pci_device object only stores queue_idx number of valid
> >> virtio_pci_vq_info objects in its vqs array when the for-loop over
> >> nvqs finishes (of course, without goto out_del_vqs).
> >>
> >> vp_find_vqs_msix() has similar issue, so fix it in the same way.
> >>
> >> This patch is marked as RFC because we are uncertain if any virtio-pci
> >> code implicitly requires virtio_pci_device's vqs array to always
> >> contain nvqs number of virtio_pci_vq_info objects, and to store
> >> zero-initialized virtio_pci_vq_info objects. We have not observed
> >> any issues in our testing, but insights or alternatives are welcome!
> >>
> >> Signed-off-by: Link Lin <linkl@google.com>
> >> Co-developed-by: Jiaqi Yan <jiaqiyan@google.com>
> >> Signed-off-by: Jiaqi Yan <jiaqiyan@google.com>
> >> ---
> >> 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..9b32301529e5 100644
> >> --- a/drivers/virtio/virtio_pci_common.c
> >> +++ b/drivers/virtio/virtio_pci_common.c
> >> @@ -423,14 +423,15 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
> >> 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]);
> >> if (IS_ERR(vqs[i])) {
> >> err = PTR_ERR(vqs[i]);
> >> goto error_find;
> >> }
> >> + ++queue_idx;
> >> }
> >>
> >> if (!avq_num)
> >> @@ -485,13 +486,14 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned int nvqs,
> >> 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]);
> >> if (IS_ERR(vqs[i])) {
> >> err = PTR_ERR(vqs[i]);
> >> goto out_del_vqs;
> >> }
> >> + ++queue_idx;
> >> }
> >>
> >> if (!avq_num)
> >> --
> >> 2.53.0.1213.gd9a14994de-goog
I have this in my tree:
https://lore.kernel.org/all/20260315141808.547081-1-ammarfaizi2@openresty.com/
same?
^ permalink raw reply
* Re: [RFC PATCH v1] virtio_pci: only store successfully populated virtio_pci_vq_info
From: Michael S. Tsirkin @ 2026-04-21 21:50 UTC (permalink / raw)
To: Link Lin
Cc: jasowang, xuanzhuo, eperezma, jiaqiyan, rientjes, weixugc,
virtualization, linux-kernel
In-Reply-To: <CALUx4KQKNYZm5AZzQXNqLRdGAT0nQOpmn_Lz6WHie73w1d9JQA@mail.gmail.com>
On Tue, Apr 21, 2026 at 02:24:16PM -0700, Link Lin wrote:
> Hi everyone,
>
> Friendly ping on this RFC patch. Please let me know if anyone has had a chance
> to look at this, or if any changes are needed.
>
> Thanks,
> Link
>
> On Tue, Apr 7, 2026 at 2:25 PM Link Lin <linkl@google.com> wrote:
>
> In environments where free page reporting is disabled, a kernel
> panic is triggered when tearing down the virtio_balloon module:
>
> [12261.808190] Call trace:
> [12261.808471] __list_del_entry_valid_or_report+0x18/0xe0
> [12261.809064] vp_del_vqs+0x12c/0x270
> [12261.809462] remove_common+0x80/0x98 [virtio_balloon]
> [12261.810034] virtballoon_remove+0xfc/0x158 [virtio_balloon]
> [12261.810663] virtio_dev_remove+0x68/0xf8
> [12261.811108] device_release_driver_internal+0x17c/0x278
> [12261.811701] driver_detach+0xd4/0x138
> [12261.812117] bus_remove_driver+0x90/0xd0
> [12261.812562] driver_unregister+0x40/0x70
> [12261.813006] unregister_virtio_driver+0x20/0x38
> [12261.813518] cleanup_module+0x20/0x7a8 [virtio_balloon]
> [12261.814109] __arm64_sys_delete_module+0x278/0x3d0
> [12261.814654] invoke_syscall+0x5c/0x120
> [12261.815086] el0_svc_common+0x90/0xf8
> [12261.815506] do_el0_svc+0x2c/0x48
> [12261.815883] el0_svc+0x3c/0xa8
> [12261.816235] el0t_64_sync_handler+0x8c/0x108
> [12261.816724] el0t_64_sync+0x198/0x1a0
>
> The issue originates in vp_find_vqs_intx(). It kzalloc_objs() based
> on the nvqs count provided by the caller, virtio_balloon::init_vqs().
> However, it is not always the case that all nvqs number of
> virtio_pci_vq_info objects will be properly populated.
>
> For example, when VIRTIO_BALLOON_F_FREE_PAGE_HINT is absent, the
> VIRTIO_BALLOON_VQ_FREE_PAGE-th item in the vp_dev->vqs array is
> actually never populated, and is still a zeroe-initialized
> virtio_pci_vq_info object, which is eventually going to trigger
> a __list_del_entry_valid_or_report() crash.
>
> Tested by applying this patch to a guest VM kernel with the
> VIRTIO_BALLOON_F_REPORTING feature enabled and the
> VIRTIO_BALLOON_F_FREE_PAGE_HINT feature disabled.
> Without this patch, unloading the virtio_balloon module triggers a panic.
> With this patch, no panic is observed.
>
> The fix is to use queue_idx to handle the case that vp_find_vqs_intx()
> skips vp_setup_vq() when caller provided null vqs_info[i].name, when
> the caller doesn't populate all nvqs number of virtqueue_info objects.
> Invariantly queue_idx is the correct index to store a successfully
> created and populated virtio_pci_vq_info object. As a result, now
> a virtio_pci_device object only stores queue_idx number of valid
> virtio_pci_vq_info objects in its vqs array when the for-loop over
> nvqs finishes (of course, without goto out_del_vqs).
>
> vp_find_vqs_msix() has similar issue, so fix it in the same way.
>
> This patch is marked as RFC because we are uncertain if any virtio-pci
> code implicitly requires virtio_pci_device's vqs array to always
> contain nvqs number of virtio_pci_vq_info objects, and to store
> zero-initialized virtio_pci_vq_info objects. We have not observed
> any issues in our testing, but insights or alternatives are welcome!
>
> Signed-off-by: Link Lin <linkl@google.com>
> Co-developed-by: Jiaqi Yan <jiaqiyan@google.com>
> Signed-off-by: Jiaqi Yan <jiaqiyan@google.com>
> ---
> 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..9b32301529e5 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -423,14 +423,15 @@ static int vp_find_vqs_msix(struct virtio_device
> *vdev, unsigned int nvqs,
> 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]);
> if (IS_ERR(vqs[i])) {
> err = PTR_ERR(vqs[i]);
> goto error_find;
> }
> + ++queue_idx;
> }
>
> if (!avq_num)
> @@ -485,13 +486,14 @@ static int vp_find_vqs_intx(struct virtio_device
> *vdev, unsigned int nvqs,
> 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]);
> if (IS_ERR(vqs[i])) {
> err = PTR_ERR(vqs[i]);
> goto out_del_vqs;
> }
> + ++queue_idx;
> }
>
> if (!avq_num)
> --
> 2.53.0.1213.gd9a14994de-goog
>
I have this in my tree:
https://lore.kernel.org/all/20260315141808.547081-1-ammarfaizi2@openresty.com/
same?
--
MST
^ permalink raw reply
* Re: [RFC PATCH v1] virtio_pci: only store successfully populated virtio_pci_vq_info
From: Link Lin @ 2026-04-21 21:47 UTC (permalink / raw)
To: mst, jasowang, xuanzhuo
Cc: eperezma, jiaqiyan, rientjes, weixugc, virtualization,
linux-kernel
In-Reply-To: <CALUx4KQKNYZm5AZzQXNqLRdGAT0nQOpmn_Lz6WHie73w1d9JQA@mail.gmail.com>
Hi everyone,
Friendly ping. Apologies if you are getting this the second time - my
last ping wasn't in plain text mode and got rejected by some mailing
lists.
Please let me know if anyone has had a chance to look at this RFC
patch, or if any changes are needed.
Thanks,
Link
On Tue, Apr 21, 2026 at 2:24 PM Link Lin <linkl@google.com> wrote:
>
> Hi everyone,
>
> Friendly ping on this RFC patch. Please let me know if anyone has had a chance to look at this, or if any changes are needed.
>
> Thanks,
> Link
>
> On Tue, Apr 7, 2026 at 2:25 PM Link Lin <linkl@google.com> wrote:
>>
>> In environments where free page reporting is disabled, a kernel
>> panic is triggered when tearing down the virtio_balloon module:
>>
>> [12261.808190] Call trace:
>> [12261.808471] __list_del_entry_valid_or_report+0x18/0xe0
>> [12261.809064] vp_del_vqs+0x12c/0x270
>> [12261.809462] remove_common+0x80/0x98 [virtio_balloon]
>> [12261.810034] virtballoon_remove+0xfc/0x158 [virtio_balloon]
>> [12261.810663] virtio_dev_remove+0x68/0xf8
>> [12261.811108] device_release_driver_internal+0x17c/0x278
>> [12261.811701] driver_detach+0xd4/0x138
>> [12261.812117] bus_remove_driver+0x90/0xd0
>> [12261.812562] driver_unregister+0x40/0x70
>> [12261.813006] unregister_virtio_driver+0x20/0x38
>> [12261.813518] cleanup_module+0x20/0x7a8 [virtio_balloon]
>> [12261.814109] __arm64_sys_delete_module+0x278/0x3d0
>> [12261.814654] invoke_syscall+0x5c/0x120
>> [12261.815086] el0_svc_common+0x90/0xf8
>> [12261.815506] do_el0_svc+0x2c/0x48
>> [12261.815883] el0_svc+0x3c/0xa8
>> [12261.816235] el0t_64_sync_handler+0x8c/0x108
>> [12261.816724] el0t_64_sync+0x198/0x1a0
>>
>> The issue originates in vp_find_vqs_intx(). It kzalloc_objs() based
>> on the nvqs count provided by the caller, virtio_balloon::init_vqs().
>> However, it is not always the case that all nvqs number of
>> virtio_pci_vq_info objects will be properly populated.
>>
>> For example, when VIRTIO_BALLOON_F_FREE_PAGE_HINT is absent, the
>> VIRTIO_BALLOON_VQ_FREE_PAGE-th item in the vp_dev->vqs array is
>> actually never populated, and is still a zeroe-initialized
>> virtio_pci_vq_info object, which is eventually going to trigger
>> a __list_del_entry_valid_or_report() crash.
>>
>> Tested by applying this patch to a guest VM kernel with the
>> VIRTIO_BALLOON_F_REPORTING feature enabled and the
>> VIRTIO_BALLOON_F_FREE_PAGE_HINT feature disabled.
>> Without this patch, unloading the virtio_balloon module triggers a panic.
>> With this patch, no panic is observed.
>>
>> The fix is to use queue_idx to handle the case that vp_find_vqs_intx()
>> skips vp_setup_vq() when caller provided null vqs_info[i].name, when
>> the caller doesn't populate all nvqs number of virtqueue_info objects.
>> Invariantly queue_idx is the correct index to store a successfully
>> created and populated virtio_pci_vq_info object. As a result, now
>> a virtio_pci_device object only stores queue_idx number of valid
>> virtio_pci_vq_info objects in its vqs array when the for-loop over
>> nvqs finishes (of course, without goto out_del_vqs).
>>
>> vp_find_vqs_msix() has similar issue, so fix it in the same way.
>>
>> This patch is marked as RFC because we are uncertain if any virtio-pci
>> code implicitly requires virtio_pci_device's vqs array to always
>> contain nvqs number of virtio_pci_vq_info objects, and to store
>> zero-initialized virtio_pci_vq_info objects. We have not observed
>> any issues in our testing, but insights or alternatives are welcome!
>>
>> Signed-off-by: Link Lin <linkl@google.com>
>> Co-developed-by: Jiaqi Yan <jiaqiyan@google.com>
>> Signed-off-by: Jiaqi Yan <jiaqiyan@google.com>
>> ---
>> 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..9b32301529e5 100644
>> --- a/drivers/virtio/virtio_pci_common.c
>> +++ b/drivers/virtio/virtio_pci_common.c
>> @@ -423,14 +423,15 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
>> 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]);
>> if (IS_ERR(vqs[i])) {
>> err = PTR_ERR(vqs[i]);
>> goto error_find;
>> }
>> + ++queue_idx;
>> }
>>
>> if (!avq_num)
>> @@ -485,13 +486,14 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned int nvqs,
>> 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]);
>> if (IS_ERR(vqs[i])) {
>> err = PTR_ERR(vqs[i]);
>> goto out_del_vqs;
>> }
>> + ++queue_idx;
>> }
>>
>> if (!avq_num)
>> --
>> 2.53.0.1213.gd9a14994de-goog
^ permalink raw reply
* Re: [PATCH 1/2] vfio/pci: Set up barmap in vfio_pci_core_enable()
From: Alex Williamson @ 2026-04-21 19:31 UTC (permalink / raw)
To: Matt Evans
Cc: Kevin Tian, Jason Gunthorpe, Ankit Agrawal, Alistair Popple,
Leon Romanovsky, Kees Cook, Shameer Kolothum, Yishai Hadas,
Alexey Kardashevskiy, Eric Auger, Peter Xu, Vivek Kasireddy,
Zhi Wang, kvm, linux-kernel, virtualization, alex
In-Reply-To: <20260421174143.3883579-2-mattev@meta.com>
On Tue, 21 Apr 2026 10:41:40 -0700
Matt Evans <mattev@meta.com> wrote:
> The previous lazy-setup of the barmaps provided opportunities to
> forget to do it (for example, DMABUF export). Since all users will
> ultimately require BAR resources to have been requested, request them
> in vfio_pci_core_enable.
>
> Existing calls to vfio_pci_core_setup_barmap() are now benign, but
> remain because some callers use it to validate a BAR index. This
> fixes at least the case where DMABUF export could succeed before
> resources were requested.
>
> This keeps resource request and ioremap() done at the same time, but
> in future the ioremap() could be done on-demand (not all VFIO users
> need it).
>
> Fixes: 7f5764e179c6 ("vfio: use vfio_pci_core_setup_barmap to map bar in mmap")
> Fixes: 0d77ed3589ac0 ("vfio/pci: Pull BAR mapping setup from read-write path")
> Signed-off-by: Matt Evans <mattev@meta.com>
> ---
> drivers/vfio/pci/vfio_pci_core.c | 63 +++++++++++++++++++++++++++-----
> drivers/vfio/pci/vfio_pci_rdwr.c | 25 +++----------
> 2 files changed, 60 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 3f8d093aacf8..4a314213f3ae 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -482,6 +482,55 @@ static int vfio_pci_core_runtime_resume(struct device *dev)
> }
> #endif /* CONFIG_PM */
>
> +static void __vfio_pci_core_unmap_bars(struct vfio_pci_core_device *vdev)
> +{
> + struct pci_dev *pdev = vdev->pdev;
> + int i;
> +
> + for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> + int bar = i + PCI_STD_RESOURCES;
> +
> + if (!vdev->barmap[i])
> + continue;
> + pci_iounmap(pdev, vdev->barmap[bar]);
> + pci_release_selected_regions(pdev, 1 << bar);
> + vdev->barmap[bar] = NULL;
> + }
> +}
> +
> +static int __vfio_pci_core_map_bars(struct vfio_pci_core_device *vdev)
> +{
> + struct pci_dev *pdev = vdev->pdev;
> + int ret = 0;
> + int i;
> +
> + /* Eager-request BAR resources (and iomap) */
> + for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> + int bar = i + PCI_STD_RESOURCES;
> + void __iomem *io;
> +
> + if (pci_resource_len(pdev, i) == 0)
> + continue;
> +
> + ret = pci_request_selected_regions(pdev, 1 << bar, "vfio");
> + if (ret)
> + goto err_free_barmap;
> +
> + io = pci_iomap(pdev, bar, 0);
> + if (!io) {
> + pci_release_selected_regions(pdev, 1 << bar);
> + ret = -ENOMEM;
> + goto err_free_barmap;
> + }
> + vdev->barmap[bar] = io;
> + }
> + return 0;
> +
> +err_free_barmap:
> + __vfio_pci_core_unmap_bars(vdev);
> + return ret;
> +}
> +
> /*
> * The pci-driver core runtime PM routines always save the device state
> * before going into suspended state. If the device is going into low power
> @@ -568,6 +617,9 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
> if (!vfio_vga_disabled() && vfio_pci_is_vga(pdev))
> vdev->has_vga = true;
>
> + ret = __vfio_pci_core_map_bars(vdev);
> + if (ret)
> + goto out_free_zdev;
Beyond simply changing to a preemptive mapping scheme, this hard
failure makes me concerned about regressing userspace. With the
current lazy scheme, we only claim the BAR if the user accesses it and
the failure only occurs in the access path. That means we could have
BARs that are never mapped. With a hard failure here, with might be
uncovering some latent issues, and if those latent issues are with BARs
that we never cared to map previously, we're causing trouble for no
gain.
I'd suggest setup should be a void function that generates pci_warn()
on conflict or iomap error, but doesn't block the device. Each usage
path (including mmap that gets removed in the next path) still needs to
validate the mapping is present for the given BAR and fail the call
path otherwise.
There's also a subtle range check added in the virtio driver in the
next patch, is that fixing a bug or paranoia? Do we need a helper that
does a range test and barmap test? Thanks,
Alex
>
> return 0;
>
> @@ -591,7 +643,7 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
> struct pci_dev *pdev = vdev->pdev;
> struct vfio_pci_dummy_resource *dummy_res, *tmp;
> struct vfio_pci_ioeventfd *ioeventfd, *ioeventfd_tmp;
> - int i, bar;
> + int i;
>
> /* For needs_reset */
> lockdep_assert_held(&vdev->vdev.dev_set->lock);
> @@ -646,14 +698,7 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
>
> vfio_config_free(vdev);
>
> - for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> - bar = i + PCI_STD_RESOURCES;
> - if (!vdev->barmap[bar])
> - continue;
> - pci_iounmap(pdev, vdev->barmap[bar]);
> - pci_release_selected_regions(pdev, 1 << bar);
> - vdev->barmap[bar] = NULL;
> - }
> + __vfio_pci_core_unmap_bars(vdev);
>
> list_for_each_entry_safe(dummy_res, tmp,
> &vdev->dummy_resources_list, res_next) {
> diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
> index 4251ee03e146..d1386a31a3a3 100644
> --- a/drivers/vfio/pci/vfio_pci_rdwr.c
> +++ b/drivers/vfio/pci/vfio_pci_rdwr.c
> @@ -200,26 +200,13 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_do_io_rw);
>
> int vfio_pci_core_setup_barmap(struct vfio_pci_core_device *vdev, int bar)
> {
> - struct pci_dev *pdev = vdev->pdev;
> - int ret;
> - void __iomem *io;
> -
> - if (vdev->barmap[bar])
> - return 0;
> -
> - ret = pci_request_selected_regions(pdev, 1 << bar, "vfio");
> - if (ret)
> - return ret;
> -
> - io = pci_iomap(pdev, bar, 0);
> - if (!io) {
> - pci_release_selected_regions(pdev, 1 << bar);
> - return -ENOMEM;
> - }
> -
> - vdev->barmap[bar] = io;
> + /*
> + * The barmap is now always set up in vfio_pci_core_enable().
> + * Some legacy callers use this function to check if the BAR
> + * is legitimate, so maintain that:
> + */
>
> - return 0;
> + return vdev->barmap[bar] ? 0 : -EBUSY;
> }
> EXPORT_SYMBOL_GPL(vfio_pci_core_setup_barmap);
>
^ permalink raw reply
* RE: [PATCH net v3] hv_sock: Report EOF instead of -EIO for FIN
From: Dexuan Cui @ 2026-04-21 17:57 UTC (permalink / raw)
To: Dexuan Cui, KY Srinivasan, Haiyang Zhang, wei.liu@kernel.org,
Long Li, sgarzare@redhat.com, davem@davemloft.net,
edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
horms@kernel.org, niuxuewei.nxw@antgroup.com,
linux-hyperv@vger.kernel.org, virtualization@lists.linux.dev,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: stable@vger.kernel.org, Ben Hillis, Mitchell Levy
In-Reply-To: <20260421025950.1099495-1-decui@microsoft.com>
> From: Dexuan Cui
> Sent: Monday, April 20, 2026 8:00 PM
Please ignore the email, as I just posted an incremental patch here:
https://lore.kernel.org/linux-hyperv/20260421174931.1152238-1-decui@microsoft.com/T/#u
See the link for more context:
https://lore.kernel.org/linux-hyperv/177672238581.1802062.15838493180057695674.git-patchwork-notify@kernel.org/T/#t
^ permalink raw reply
* RE: [EXTERNAL] Re: [PATCH net v2] hv_sock: Report EOF instead of -EIO for FIN
From: Dexuan Cui @ 2026-04-21 17:54 UTC (permalink / raw)
To: Jakub Kicinski, Stefano Garzarella
Cc: patchwork-bot+netdevbpf@kernel.org, KY Srinivasan, Haiyang Zhang,
wei.liu@kernel.org, Long Li, davem@davemloft.net,
edumazet@google.com, pabeni@redhat.com, horms@kernel.org,
niuxuewei.nxw@antgroup.com, linux-hyperv@vger.kernel.org,
virtualization@lists.linux.dev, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, stable@vger.kernel.org, Ben Hillis,
levymitchell0@gmail.com
In-Reply-To: <20260421071839.30217a60@kernel.org>
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Tuesday, April 21, 2026 7:19 AM
> ...
> > Anyway, let's wait for Jakub's or other net maintainers' suggestions.
>
> Yes, you have to post an incremental fix
Thanks for the quick replies! I posted an incremental fix:
https://lore.kernel.org/linux-hyperv/20260421174931.1152238-1-decui@microsoft.com/T/#u
^ permalink raw reply
* [PATCH net] hv_sock: Return -EIO for malformed/short packets
From: Dexuan Cui @ 2026-04-21 17:49 UTC (permalink / raw)
To: kys, haiyangz, wei.liu, decui, longli, sgarzare, davem, edumazet,
kuba, pabeni, horms, niuxuewei.nxw, linux-hyperv, virtualization,
netdev, linux-kernel
Cc: stable
Commit f63152958994 fixes a regression, however it fails to report an
error for malformed/short packets -- normally we should never see such
packets, but let's report an error for them just in case.
Fixes: f63152958994 ("hv_sock: Report EOF instead of -EIO for FIN")
Cc: stable@vger.kernel.org
Signed-off-by: Dexuan Cui <decui@microsoft.com>
---
Commit f63152958994 is currently only in net.git's master branch.
net/vmw_vsock/hyperv_transport.c | 29 +++++++++++++++++++----------
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
index 76e78c83fdbc..8faaa14bccda 100644
--- a/net/vmw_vsock/hyperv_transport.c
+++ b/net/vmw_vsock/hyperv_transport.c
@@ -704,18 +704,27 @@ static s64 hvs_stream_has_data(struct vsock_sock *vsk)
if (hvs->recv_desc) {
/* Here hvs->recv_data_len is 0, so hvs->recv_desc must
* be NULL unless it points to the 0-byte-payload FIN
- * packet: see hvs_update_recv_data().
+ * packet or a malformed/short packet: see
+ * hvs_update_recv_data().
*
- * Here all the payload has been dequeued, but
- * hvs_channel_readable_payload() still returns 1,
- * because the VMBus ringbuffer's read_index is not
- * updated for the FIN packet: hvs_stream_dequeue() ->
- * hv_pkt_iter_next() updates the cached priv_read_index
- * but has no opportunity to update the read_index in
- * hv_pkt_iter_close() as hvs_stream_has_data() returns
- * 0 for the FIN packet, so it won't get dequeued.
+ * If hvs->recv_desc points to the FIN packet, here all
+ * the payload has been dequeued and the peer_shutdown
+ * flag is set, but hvs_channel_readable_payload() still
+ * returns 1, because the VMBus ringbuffer's read_index
+ * is not updated for the FIN packet:
+ * hvs_stream_dequeue() -> hv_pkt_iter_next() updates
+ * the cached priv_read_index but has no opportunity to
+ * update the read_index in hv_pkt_iter_close() as
+ * hvs_stream_has_data() returns 0 for the FIN packet,
+ * so it won't get dequeued.
+ *
+ * In case hvs->recv_desc points to a malformed/short
+ * packet, return -EIO.
*/
- return 0;
+ if (hvs->vsk->peer_shutdown & SEND_SHUTDOWN)
+ return 0;
+ else
+ return -EIO;
}
hvs->recv_desc = hv_pkt_iter_first(hvs->chan);
--
2.49.0
^ permalink raw reply related
* Re: [PATCH 1/2] vfio/pci: Set up VFIO barmap before creating a DMABUF
From: Matt Evans @ 2026-04-21 17:46 UTC (permalink / raw)
To: Tian, Kevin, Alex Williamson
Cc: Ankit Agrawal, Jason Gunthorpe, Yishai Hadas, Shameer Kolothum,
Alistair Popple, Leon Romanovsky, Kasireddy, Vivek, Kees Cook,
Zhi Wang, Peter Xu, Alexey Kardashevskiy, Eric Auger,
kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
virtualization@lists.linux.dev
In-Reply-To: <3fd01aa0-955d-44e3-aa4a-1918f9c42bb0@meta.com>
Hi,
On 17/04/2026 20:11, Matt Evans wrote:
> Hi Kevin, Alex,
>
> On 17/04/2026 06:16, Tian, Kevin wrote:
>>
>>> From: Alex Williamson <alex@shazbot.org>
>>> Sent: Friday, April 17, 2026 6:44 AM
>>>
>>> On Wed, 15 Apr 2026 11:14:22 -0700
>>> Matt Evans <mattev@meta.com> wrote:
>>>
>>>> A DMABUF exports access to BAR resources which need to be requested
>>>> before the DMABUF is handed out. Usually the resources are requested
>>>> when setting up the barmap when the VFIO device fd is mmap()ed, but
>>>> there's no guarantee that's done before a DMABUF is created.
>>>>
>>>> Set up the barmap (and so request resources) in the DMABUF-creation
>>>> path.
>>>>
>>>> Fixes: 5d74781ebc86c ("vfio/pci: Add dma-buf export support for MMIO
>>> regions")
>>>> Signed-off-by: Matt Evans <mattev@meta.com>
>>>> ---
>>>> drivers/vfio/pci/vfio_pci_dmabuf.c | 9 +++++++++
>>>> 1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c
>>> b/drivers/vfio/pci/vfio_pci_dmabuf.c
>>>> index 4ccaf3531e02..fefe7cf4256b 100644
>>>> --- a/drivers/vfio/pci/vfio_pci_dmabuf.c
>>>> +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c
>>>> @@ -272,6 +272,15 @@ int vfio_pci_core_feature_dma_buf(struct
>>> vfio_pci_core_device *vdev, u32 flags,
>>>> goto err_free_priv;
>>>> }
>>>>
>>>> + /*
>>>> + * See comment in vfio_pci_core_mmap(); ensure PCI regions
>>>> + * were requested before returning DMABUFs that reference
>>>> + * them. Barmap setup does this:
>>>> + */
>>>> + ret = vfio_pci_core_setup_barmap(vdev, get_dma_buf.region_index);
>>>> + if (ret)
>>>> + goto err_free_phys;
>>>> +
>>>> priv->vdev = vdev;
>>>> priv->nr_ranges = get_dma_buf.nr_ranges;
>>>> priv->size = length;
>>>
>>> Wouldn't this get a lot easier if we just setup all the barmaps in
>>> vfio_pci_core_enable(), conditional on pci_resource_len() just like we
>>> use to filter in REGION_INFO?
>>>
>>> I don't recall if there's some reason we've avoid this so far, maybe
>>> others can shout it out if they do.
>>
>> I don't remember too. probably just because it's not a wide requirement
>> then was made in this on-demand approach...
>>
>>>
>>> We already tear them all down in vfio_pci_core_disable(). It would be
>>> a small patch to add that, which we would mark as Fixes:, then a small
>>> follow-up on top of that that removes any then redundant or unnecessary
>>> callers (all of them). Thoughts? Thanks,
>>>
>>
>> Agree. then the next patch fixing the racing conditions is also not
>> required.
>
> Oh, if we can do it earlier it's all much cleaner, agreed. I'd naively
> thought there was a reason it was done lazily. One benefit is truly
> huge TB-scale BARs don't get ioremap()ed until actually used by
> userspace. I can't think of a good usage example that relies on open
> performance.
>
> If you say fine, then great: Ignore these, and I'll post a new pair of
> patches doing that (...with a cover letter this time).
New series posted:
https://lore.kernel.org/kvm/20260421174143.3883579-1-mattev@meta.com/
(Titles/semantics very different, so didn't send as a v2 of this.)
Matt
^ permalink raw reply
* [PATCH 2/2] vfio/pci: Remove vfio_pci_core_setup_barmap()
From: Matt Evans @ 2026-04-21 17:41 UTC (permalink / raw)
To: Alex Williamson, Kevin Tian, Jason Gunthorpe, Ankit Agrawal,
Alistair Popple, Leon Romanovsky, Kees Cook, Shameer Kolothum,
Yishai Hadas
Cc: Alexey Kardashevskiy, Eric Auger, Peter Xu, Vivek Kasireddy,
Zhi Wang, kvm, linux-kernel, virtualization
In-Reply-To: <20260421174143.3883579-1-mattev@meta.com>
Since "vfio/pci: Set up barmap in vfio_pci_core_enable()", the BAR
mapping and resource acquisition are done at setup time, and most uses
of vfio_pci_core_setup_barmap() are now unnecessary. Remove the
callers, and the function.
Some callers used it to also test if a mapping is present before a
direct access, and these places now do a simple test of
vdev->barmap[].
Signed-off-by: Matt Evans <mattev@meta.com>
---
drivers/vfio/pci/nvgrace-gpu/main.c | 11 +++++------
drivers/vfio/pci/vfio_pci_core.c | 8 --------
drivers/vfio/pci/vfio_pci_rdwr.c | 22 ++++------------------
drivers/vfio/pci/virtio/legacy_io.c | 5 ++---
4 files changed, 11 insertions(+), 35 deletions(-)
diff --git a/drivers/vfio/pci/nvgrace-gpu/main.c b/drivers/vfio/pci/nvgrace-gpu/main.c
index fa056b69f899..3712f6b54d46 100644
--- a/drivers/vfio/pci/nvgrace-gpu/main.c
+++ b/drivers/vfio/pci/nvgrace-gpu/main.c
@@ -184,14 +184,13 @@ static int nvgrace_gpu_open_device(struct vfio_device *core_vdev)
/*
* GPU readiness is checked by reading the BAR0 registers.
- *
- * ioremap BAR0 to ensure that the BAR0 mapping is present before
- * register reads on first fault before establishing any GPU
- * memory mapping.
+ * BARs should have been mapped at device enable, but it's
+ * good to assert they're present before the access:
*/
- ret = vfio_pci_core_setup_barmap(vdev, 0);
- if (ret)
+ if (!vdev->barmap[0]) {
+ ret = -EINVAL;
goto error_exit;
+ }
if (nvdev->resmem.memlength) {
ret = nvgrace_gpu_vfio_pci_register_pfn_range(core_vdev, &nvdev->resmem);
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 4a314213f3ae..228d92ce61d1 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -1805,14 +1805,6 @@ int vfio_pci_core_mmap(struct vfio_device *core_vdev, struct vm_area_struct *vma
if (req_start + req_len > phys_len)
return -EINVAL;
- /*
- * Even though we don't make use of the barmap for the mmap,
- * we need to request the region and the barmap tracks that.
- */
- ret = vfio_pci_core_setup_barmap(vdev, index);
- if (ret)
- return ret;
-
vma->vm_private_data = vdev;
vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
index d1386a31a3a3..21e19ed48bf5 100644
--- a/drivers/vfio/pci/vfio_pci_rdwr.c
+++ b/drivers/vfio/pci/vfio_pci_rdwr.c
@@ -198,18 +198,6 @@ ssize_t vfio_pci_core_do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem,
}
EXPORT_SYMBOL_GPL(vfio_pci_core_do_io_rw);
-int vfio_pci_core_setup_barmap(struct vfio_pci_core_device *vdev, int bar)
-{
- /*
- * The barmap is now always set up in vfio_pci_core_enable().
- * Some legacy callers use this function to check if the BAR
- * is legitimate, so maintain that:
- */
-
- return vdev->barmap[bar] ? 0 : -EBUSY;
-}
-EXPORT_SYMBOL_GPL(vfio_pci_core_setup_barmap);
-
ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
size_t count, loff_t *ppos, bool iswrite)
{
@@ -261,9 +249,8 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
*/
max_width = VFIO_PCI_IO_WIDTH_4;
} else {
- int ret = vfio_pci_core_setup_barmap(vdev, bar);
- if (ret) {
- done = ret;
+ if (!vdev->barmap[bar]) {
+ done = -EINVAL;
goto out;
}
@@ -439,9 +426,8 @@ int vfio_pci_ioeventfd(struct vfio_pci_core_device *vdev, loff_t offset,
if (count == 8)
return -EINVAL;
- ret = vfio_pci_core_setup_barmap(vdev, bar);
- if (ret)
- return ret;
+ if (!vdev->barmap[bar])
+ return -EINVAL;
mutex_lock(&vdev->ioeventfds_lock);
diff --git a/drivers/vfio/pci/virtio/legacy_io.c b/drivers/vfio/pci/virtio/legacy_io.c
index 1ed349a55629..67f93b96c099 100644
--- a/drivers/vfio/pci/virtio/legacy_io.c
+++ b/drivers/vfio/pci/virtio/legacy_io.c
@@ -305,9 +305,8 @@ static int virtiovf_set_notify_addr(struct virtiovf_pci_core_device *virtvdev)
* Setup the BAR where the 'notify' exists to be used by vfio as well
* This will let us mmap it only once and use it when needed.
*/
- ret = vfio_pci_core_setup_barmap(core_device,
- virtvdev->notify_bar);
- if (ret)
+ if (virtvdev->notify_bar >= PCI_STD_NUM_BARS ||
+ !core_device->barmap[virtvdev->notify_bar])
return ret;
virtvdev->notify_addr = core_device->barmap[virtvdev->notify_bar] +
--
2.47.3
^ permalink raw reply related
* [PATCH 0/2] vfio/pci: Request resources and map BARs at enable time
From: Matt Evans @ 2026-04-21 17:41 UTC (permalink / raw)
To: Alex Williamson, Kevin Tian, Jason Gunthorpe, Ankit Agrawal,
Alistair Popple, Leon Romanovsky, Kees Cook, Shameer Kolothum,
Yishai Hadas
Cc: Alexey Kardashevskiy, Eric Auger, Peter Xu, Vivek Kasireddy,
Zhi Wang, kvm, linux-kernel, virtualization
Hi,
This is a replacement for these patches posted to set up the
VFIO barmap in the DMABUF export path, and add serialisation:
https://lore.kernel.org/kvm/20260415181423.1008458-1-mattev@meta.com
Those patches were motivated by the DMABUF export originally missing a
call to vfio_pci_core_setup_barmap() and so possibly exporting a range
for which resources weren't yet requested.
Responses in the thread indicated there wasn't a strong historical
reason to require the bar-mapping to be performed on-demand at BAR
reference time. It's much simpler to move this to
vfio_pci_core_enable(), and that then avoids having to deal with
concurrent lazy requests.
Now VFIO drivers requiring the BAR resources (mmap(), DMABUF export,
etc.) don't need to do anything else, and there's nothing to forget in
future code.
The first patch sets up the barmap (requesting PCI resources and
ioremap() of the BARs) in vfio_pci_core_enable(). It can be
considered a bugfix for the VFIO DMABUF path.
The second patch removes calls to vfio_pci_core_setup_barmap(), which
were left untouched but harmless (just a stub) in the first patch.
However, vfio_pci_core_setup_barmap() did two things, requesting
resources and ioremap()ing them. Some callers relied on it to perform
the ioremap() just before an in-kernel access to the BAR (e.g. the BAR
R/W routines, nvgrace-gpu). These calls are replaced by a cheap check
that the required BAR has indeed been mapped, keeping the same
behaviour.
In future we could reconsider performing all the ioremap()s at startup
time, since a lot of VFIO users will never access BARs in-kernel and
won't need them. Mapping huge BARs will cost some time & memory, and
if necessary this could be reduced by doing the ioremap() portion on
demand.
Matt Evans (2):
vfio/pci: Set up barmap in vfio_pci_core_enable()
vfio/pci: Remove vfio_pci_core_setup_barmap()
drivers/vfio/pci/nvgrace-gpu/main.c | 11 ++---
drivers/vfio/pci/vfio_pci_core.c | 71 ++++++++++++++++++++++-------
drivers/vfio/pci/vfio_pci_rdwr.c | 35 ++------------
drivers/vfio/pci/virtio/legacy_io.c | 5 +-
4 files changed, 65 insertions(+), 57 deletions(-)
--
2.47.3
^ permalink raw reply
* [PATCH 1/2] vfio/pci: Set up barmap in vfio_pci_core_enable()
From: Matt Evans @ 2026-04-21 17:41 UTC (permalink / raw)
To: Alex Williamson, Kevin Tian, Jason Gunthorpe, Ankit Agrawal,
Alistair Popple, Leon Romanovsky, Kees Cook, Shameer Kolothum,
Yishai Hadas
Cc: Alexey Kardashevskiy, Eric Auger, Peter Xu, Vivek Kasireddy,
Zhi Wang, kvm, linux-kernel, virtualization
In-Reply-To: <20260421174143.3883579-1-mattev@meta.com>
The previous lazy-setup of the barmaps provided opportunities to
forget to do it (for example, DMABUF export). Since all users will
ultimately require BAR resources to have been requested, request them
in vfio_pci_core_enable.
Existing calls to vfio_pci_core_setup_barmap() are now benign, but
remain because some callers use it to validate a BAR index. This
fixes at least the case where DMABUF export could succeed before
resources were requested.
This keeps resource request and ioremap() done at the same time, but
in future the ioremap() could be done on-demand (not all VFIO users
need it).
Fixes: 7f5764e179c6 ("vfio: use vfio_pci_core_setup_barmap to map bar in mmap")
Fixes: 0d77ed3589ac0 ("vfio/pci: Pull BAR mapping setup from read-write path")
Signed-off-by: Matt Evans <mattev@meta.com>
---
drivers/vfio/pci/vfio_pci_core.c | 63 +++++++++++++++++++++++++++-----
drivers/vfio/pci/vfio_pci_rdwr.c | 25 +++----------
2 files changed, 60 insertions(+), 28 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 3f8d093aacf8..4a314213f3ae 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -482,6 +482,55 @@ static int vfio_pci_core_runtime_resume(struct device *dev)
}
#endif /* CONFIG_PM */
+static void __vfio_pci_core_unmap_bars(struct vfio_pci_core_device *vdev)
+{
+ struct pci_dev *pdev = vdev->pdev;
+ int i;
+
+ for (i = 0; i < PCI_STD_NUM_BARS; i++) {
+ int bar = i + PCI_STD_RESOURCES;
+
+ if (!vdev->barmap[i])
+ continue;
+ pci_iounmap(pdev, vdev->barmap[bar]);
+ pci_release_selected_regions(pdev, 1 << bar);
+ vdev->barmap[bar] = NULL;
+ }
+}
+
+static int __vfio_pci_core_map_bars(struct vfio_pci_core_device *vdev)
+{
+ struct pci_dev *pdev = vdev->pdev;
+ int ret = 0;
+ int i;
+
+ /* Eager-request BAR resources (and iomap) */
+ for (i = 0; i < PCI_STD_NUM_BARS; i++) {
+ int bar = i + PCI_STD_RESOURCES;
+ void __iomem *io;
+
+ if (pci_resource_len(pdev, i) == 0)
+ continue;
+
+ ret = pci_request_selected_regions(pdev, 1 << bar, "vfio");
+ if (ret)
+ goto err_free_barmap;
+
+ io = pci_iomap(pdev, bar, 0);
+ if (!io) {
+ pci_release_selected_regions(pdev, 1 << bar);
+ ret = -ENOMEM;
+ goto err_free_barmap;
+ }
+ vdev->barmap[bar] = io;
+ }
+ return 0;
+
+err_free_barmap:
+ __vfio_pci_core_unmap_bars(vdev);
+ return ret;
+}
+
/*
* The pci-driver core runtime PM routines always save the device state
* before going into suspended state. If the device is going into low power
@@ -568,6 +617,9 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
if (!vfio_vga_disabled() && vfio_pci_is_vga(pdev))
vdev->has_vga = true;
+ ret = __vfio_pci_core_map_bars(vdev);
+ if (ret)
+ goto out_free_zdev;
return 0;
@@ -591,7 +643,7 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
struct pci_dev *pdev = vdev->pdev;
struct vfio_pci_dummy_resource *dummy_res, *tmp;
struct vfio_pci_ioeventfd *ioeventfd, *ioeventfd_tmp;
- int i, bar;
+ int i;
/* For needs_reset */
lockdep_assert_held(&vdev->vdev.dev_set->lock);
@@ -646,14 +698,7 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
vfio_config_free(vdev);
- for (i = 0; i < PCI_STD_NUM_BARS; i++) {
- bar = i + PCI_STD_RESOURCES;
- if (!vdev->barmap[bar])
- continue;
- pci_iounmap(pdev, vdev->barmap[bar]);
- pci_release_selected_regions(pdev, 1 << bar);
- vdev->barmap[bar] = NULL;
- }
+ __vfio_pci_core_unmap_bars(vdev);
list_for_each_entry_safe(dummy_res, tmp,
&vdev->dummy_resources_list, res_next) {
diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
index 4251ee03e146..d1386a31a3a3 100644
--- a/drivers/vfio/pci/vfio_pci_rdwr.c
+++ b/drivers/vfio/pci/vfio_pci_rdwr.c
@@ -200,26 +200,13 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_do_io_rw);
int vfio_pci_core_setup_barmap(struct vfio_pci_core_device *vdev, int bar)
{
- struct pci_dev *pdev = vdev->pdev;
- int ret;
- void __iomem *io;
-
- if (vdev->barmap[bar])
- return 0;
-
- ret = pci_request_selected_regions(pdev, 1 << bar, "vfio");
- if (ret)
- return ret;
-
- io = pci_iomap(pdev, bar, 0);
- if (!io) {
- pci_release_selected_regions(pdev, 1 << bar);
- return -ENOMEM;
- }
-
- vdev->barmap[bar] = io;
+ /*
+ * The barmap is now always set up in vfio_pci_core_enable().
+ * Some legacy callers use this function to check if the BAR
+ * is legitimate, so maintain that:
+ */
- return 0;
+ return vdev->barmap[bar] ? 0 : -EBUSY;
}
EXPORT_SYMBOL_GPL(vfio_pci_core_setup_barmap);
--
2.47.3
^ permalink raw reply related
* Re: [PATCH 2/2] powerpc: Run typos -w
From: Segher Boessenkool @ 2026-04-21 17:04 UTC (permalink / raw)
To: Link Mauve
Cc: linuxppc-dev, Madhavan Srinivasan, Michael Ellerman,
Nicholas Piggin, Christophe Leroy (CS GROUP), Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Herbert Xu, David S. Miller,
Juergen Gross, Ajay Kaher, Alexey Makhalov,
Broadcom internal kernel review list, Geoff Levand,
Mahesh J Salgaonkar, Oliver O'Halloran, Anatolij Gustschin,
Breno Leitão, Nayna Jain, Paulo Flabiano Smorigo,
Eric Biggers, Jason A. Donenfeld, Ard Biesheuvel, Thorsten Blum,
Thomas Huth, Jason Gunthorpe, David Hildenbrand, Alistair Popple,
Ritesh Harjani (IBM), Donet Tom, Andrew Morton,
Björn Töpel, Will Deacon, Lorenzo Stoakes (Oracle),
Paul Moore, Nam Cao, Alexander Gordeev, Sourabh Jain,
Hari Bathini, Srikar Dronamraju, Shrikanth Hegde, Jiri Bohac,
Mike Rapoport (Microsoft), Jiri Slaby (SUSE), Greg Kroah-Hartman,
Andy Shevchenko, Ilpo Järvinen, Kees Cook, Stephen Rothwell,
Xichao Zhao, Gautam Menghani, Peter Zijlstra, K Prateek Nayak,
Guangshuo Li, Li Chen, Aboorva Devarajan, Petr Mladek, Feng Tang,
Nysal Jan K.A., Aditya Gupta, Sayali Patil, Rohan McLure,
Pasha Tatashin, Yeoreum Yun, Kevin Brodsky,
Matthew Wilcox (Oracle), Andrew Donnellan, Vishal Moola (Oracle),
Thomas Weißschuh, Athira Rajeev, Kajol Jain, Thomas Gleixner,
Chen Ni, Haren Myneni, Jonathan Greental, Ingo Molnar,
Yury Norov (NVIDIA), Gaurav Batra, Nilay Shroff, Vivian Wang,
Adrian Barnaś, Rafael J. Wysocki (Intel), Thierry Reding,
Yury Norov, Mukesh Kumar Chaurasiya (IBM), Ruben Wauters,
linux-kernel, devicetree, linux-crypto, kvm, virtualization, x86
In-Reply-To: <20260421121420.26079-3-linkmauve@linkmauve.fr>
Hi!
On Tue, Apr 21, 2026 at 02:14:14PM +0200, Link Mauve wrote:
> diff --git a/arch/powerpc/boot/dts/fsl/ppa8548.dts b/arch/powerpc/boot/dts/fsl/ppa8548.dts
> index f39838d93994..32558104b3a9 100644
> --- a/arch/powerpc/boot/dts/fsl/ppa8548.dts
> +++ b/arch/powerpc/boot/dts/fsl/ppa8548.dts
> @@ -95,7 +95,7 @@ i2c@3100 {
>
> /*
> * Only ethernet controller @25000 and @26000 are used.
> - * Use alias enet2 and enet3 for the remainig controllers,
> + * Use alias enet2 and enet3 for the remaining controllers,
Aliases.
> diff --git a/arch/powerpc/boot/dts/mpc8308_p1m.dts b/arch/powerpc/boot/dts/mpc8308_p1m.dts
> index 41f917f97dab..48a98449ecbb 100644
> --- a/arch/powerpc/boot/dts/mpc8308_p1m.dts
> +++ b/arch/powerpc/boot/dts/mpc8308_p1m.dts
> @@ -90,14 +90,14 @@ can@1,0 {
> compatible = "nxp,sja1000";
> reg = <0x1 0x0 0x80>;
> interrupts = <18 0x8>;
> - interrups-parent = <&ipic>;
> + interrupts-parent = <&ipic>;
> };
interrupt-parent .
All the names of properties have a meaning! Just as you cannot change
a function name in C without changing all calls to it as well, you
really should never change a property name (if you want stuff to keep
on working, that is ;-) ).
In this case, the property was never actually used (because of the
typo). Maybe it wasn't needed? If you make changes to a DTS, post it
*separately* from the rest of this series, and test it *thoroughly*.
Just a "does it boot" test is certainly not enough.
It could well be that fixing the typo (so that the property name becomes
"interrupt-parent") makes the kernel no longer boot on the systems
affected, or less obvious problems can show up.
It will need to be tested and evaluated by whoever maintains the DTSes
in question, really :-/ And you cannot test it works for one DTS and
then conclude it will work everywhere, heh.
Segher
^ permalink raw reply
* [PATCH v3 2/2] Bluetooth: virtio_bt: validate rx pkt_type header length
From: Michael Bommarito @ 2026-04-21 17:08 UTC (permalink / raw)
To: Marcel Holtmann, Luiz Augusto von Dentz, linux-bluetooth
Cc: linux-kernel, Soenke Huster, Michael S . Tsirkin, virtualization
In-Reply-To: <20260421170845.3469513-1-michael.bommarito@gmail.com>
virtbt_rx_handle() reads the leading pkt_type byte from the RX skb
and forwards the remainder to hci_recv_frame() for every
event/ACL/SCO/ISO type, without checking that the remaining payload
is at least the fixed HCI header for that type.
After the preceding patch bounds the backend-supplied used.len to
[1, VIRTBT_RX_BUF_SIZE], a one-byte completion still reaches
hci_recv_frame() with skb->len already pulled to 0. If the byte
happened to be HCI_ACLDATA_PKT, the ACL-vs-ISO classification
fast-path in hci_dev_classify_pkt_type() dereferences
hci_acl_hdr(skb)->handle whenever the HCI device has an active
CIS_LINK, BIS_LINK, or PA_LINK connection, reading two bytes of
uninitialized RX-buffer data. The same hazard exists for every
packet type the driver accepts because none of the switch cases in
virtbt_rx_handle() check skb->len against the per-type minimum HCI
header size before handing the frame to the core.
After stripping pkt_type, require skb->len to cover the fixed
header size for the selected type (event 2, ACL 4, SCO 3, ISO 4)
before calling hci_recv_frame(); drop ratelimited otherwise.
Unknown pkt_type values still take the original kfree_skb() default
path.
Use bt_dev_err_ratelimited() because both the length and pkt_type
values come from an untrusted backend that can otherwise flood the
kernel log.
Fixes: 160fbcf3bfb9 ("Bluetooth: virtio_bt: Use skb_put to set length")
Cc: stable@vger.kernel.org
Cc: Soenke Huster <soenke.huster@eknoes.de>
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
Assisted-by: Claude:claude-opus-4-7
---
Changes in v3:
- new patch, split out of the v2 commit per Luiz's request on
the v2 thread so the per-pkt-type header-length check can be
reviewed on its own
Changes in v2:
- in virtbt_rx_handle(), require skb->len to cover the fixed HCI
header size for the selected pkt_type (event 2, ACL 4, SCO 3,
ISO 4) before calling hci_recv_frame(); this prevents a one-byte
HCI_ACLDATA_PKT completion from reaching
hci_dev_classify_pkt_type() and dereferencing hci_acl_hdr(skb)
over uninitialized RX buffer data when CIS/BIS/PA connections
are present
- switch the error log to bt_dev_err_ratelimited() because the
length and pkt_type values come from an untrusted backend that
can otherwise flood the kernel log
drivers/bluetooth/virtio_bt.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/drivers/bluetooth/virtio_bt.c b/drivers/bluetooth/virtio_bt.c
index 2c5c39356a1c..140ab55c9fc5 100644
--- a/drivers/bluetooth/virtio_bt.c
+++ b/drivers/bluetooth/virtio_bt.c
@@ -198,6 +198,7 @@ static int virtbt_shutdown_generic(struct hci_dev *hdev)
static void virtbt_rx_handle(struct virtio_bluetooth *vbt, struct sk_buff *skb)
{
+ size_t min_hdr;
__u8 pkt_type;
pkt_type = *((__u8 *) skb->data);
@@ -205,16 +206,32 @@ static void virtbt_rx_handle(struct virtio_bluetooth *vbt, struct sk_buff *skb)
switch (pkt_type) {
case HCI_EVENT_PKT:
+ min_hdr = sizeof(struct hci_event_hdr);
+ break;
case HCI_ACLDATA_PKT:
+ min_hdr = sizeof(struct hci_acl_hdr);
+ break;
case HCI_SCODATA_PKT:
+ min_hdr = sizeof(struct hci_sco_hdr);
+ break;
case HCI_ISODATA_PKT:
- hci_skb_pkt_type(skb) = pkt_type;
- hci_recv_frame(vbt->hdev, skb);
+ min_hdr = sizeof(struct hci_iso_hdr);
break;
default:
kfree_skb(skb);
- break;
+ return;
}
+
+ if (skb->len < min_hdr) {
+ bt_dev_err_ratelimited(vbt->hdev,
+ "rx pkt_type 0x%02x payload %u < hdr %zu\n",
+ pkt_type, skb->len, min_hdr);
+ kfree_skb(skb);
+ return;
+ }
+
+ hci_skb_pkt_type(skb) = pkt_type;
+ hci_recv_frame(vbt->hdev, skb);
}
static void virtbt_rx_work(struct work_struct *work)
--
2.53.0
^ permalink raw reply related
* [PATCH v3 1/2] Bluetooth: virtio_bt: clamp rx length before skb_put
From: Michael Bommarito @ 2026-04-21 17:08 UTC (permalink / raw)
To: Marcel Holtmann, Luiz Augusto von Dentz, linux-bluetooth
Cc: linux-kernel, Soenke Huster, Michael S . Tsirkin, virtualization
In-Reply-To: <20260421170845.3469513-1-michael.bommarito@gmail.com>
virtbt_rx_work() calls skb_put(skb, len) where len comes directly
from virtqueue_get_buf() with no validation against the buffer we
posted to the device. The RX skb is allocated in virtbt_add_inbuf()
and exposed to virtio as exactly 1000 bytes via sg_init_one().
Checking len against skb_tailroom(skb) is not sufficient because
alloc_skb() can leave more tailroom than the 1000 bytes actually
handed to the device. A malicious or buggy backend can therefore
report used.len between 1001 and skb_tailroom(skb), causing skb_put()
to include uninitialized kernel heap bytes that were never written by
the device.
The same path also accepts len == 0, in which case skb_put(skb, 0)
leaves the skb empty but virtbt_rx_handle() still reads the pkt_type
byte from skb->data, consuming uninitialized memory.
Define VIRTBT_RX_BUF_SIZE once and reuse it in alloc_skb() and
sg_init_one(), and gate virtbt_rx_work() on that same constant so
the bound checked matches the buffer actually exposed to the device.
Reject used.len == 0 in the same gate so an empty completion can
no longer reach virtbt_rx_handle().
Use bt_dev_err_ratelimited() because the length value comes from an
untrusted backend that can otherwise flood the kernel log.
Same class of bug as commit c04db81cd028 ("net/9p: Fix buffer
overflow in USB transport layer"), which hardened the USB 9p
transport against unchecked device-reported length.
Fixes: 160fbcf3bfb9 ("Bluetooth: virtio_bt: Use skb_put to set length")
Cc: stable@vger.kernel.org
Cc: Soenke Huster <soenke.huster@eknoes.de>
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
Assisted-by: Claude:claude-opus-4-7
---
Changes in v3:
- split off the per-pkt-type header-length check into its own
patch (2/2) per Luiz's request; this patch now only covers the
used.len vs buffer-size hazard and the len == 0 path
Changes in v2:
- validate used.len against VIRTBT_RX_BUF_SIZE (the 1000 bytes
actually exposed to the device via sg_init_one()) rather than
skb_tailroom(), which can exceed 1000 due to slab padding
- reject used.len == 0 before virtbt_rx_handle() reads the
pkt_type byte, so zero-length completions can no longer pull an
empty skb into hci_recv_frame()
- switch the error log to bt_dev_err_ratelimited() because the
length value comes from an untrusted backend that can otherwise
flood the kernel log
drivers/bluetooth/virtio_bt.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/bluetooth/virtio_bt.c b/drivers/bluetooth/virtio_bt.c
index 76d61af8a275..2c5c39356a1c 100644
--- a/drivers/bluetooth/virtio_bt.c
+++ b/drivers/bluetooth/virtio_bt.c
@@ -12,6 +12,7 @@
#include <net/bluetooth/hci_core.h>
#define VERSION "0.1"
+#define VIRTBT_RX_BUF_SIZE 1000
enum {
VIRTBT_VQ_TX,
@@ -33,11 +34,11 @@ static int virtbt_add_inbuf(struct virtio_bluetooth *vbt)
struct sk_buff *skb;
int err;
- skb = alloc_skb(1000, GFP_KERNEL);
+ skb = alloc_skb(VIRTBT_RX_BUF_SIZE, GFP_KERNEL);
if (!skb)
return -ENOMEM;
- sg_init_one(sg, skb->data, 1000);
+ sg_init_one(sg, skb->data, VIRTBT_RX_BUF_SIZE);
err = virtqueue_add_inbuf(vq, sg, 1, skb, GFP_KERNEL);
if (err < 0) {
@@ -227,8 +228,15 @@ static void virtbt_rx_work(struct work_struct *work)
if (!skb)
return;
- skb_put(skb, len);
- virtbt_rx_handle(vbt, skb);
+ if (!len || len > VIRTBT_RX_BUF_SIZE) {
+ bt_dev_err_ratelimited(vbt->hdev,
+ "rx reply len %u outside [1, %u]\n",
+ len, VIRTBT_RX_BUF_SIZE);
+ kfree_skb(skb);
+ } else {
+ skb_put(skb, len);
+ virtbt_rx_handle(vbt, skb);
+ }
if (virtbt_add_inbuf(vbt) < 0)
return;
--
2.53.0
^ permalink raw reply related
* [PATCH v3 0/2] Bluetooth: virtio_bt: harden rx against untrusted backend
From: Michael Bommarito @ 2026-04-21 17:08 UTC (permalink / raw)
To: Marcel Holtmann, Luiz Augusto von Dentz, linux-bluetooth
Cc: linux-kernel, Soenke Huster, Michael S . Tsirkin, virtualization
In-Reply-To: <CABBYNZKqx4ureNbRGEghGjGWcmMQSoiY4oUCECme_Vgn8RvYYw@mail.gmail.com>
Respin of the virtio_bt rx hardening patch, split per Luiz's
request on the v2 thread:
https://lore.kernel.org/linux-bluetooth/20260421151659.3326690-1-michael.bommarito@gmail.com/
v2 bundled two independent hazards in one commit and the commit
message got convoluted trying to explain them together. This v3
keeps each hazard in its own patch so the rationale for each is
self-contained.
Patch 1/2 keeps the original v1/v2 concern: virtbt_rx_work() calls
skb_put() with a used.len the device controls, with no validation
against the 1000-byte buffer we actually exposed to virtio via
sg_init_one(). Checking against skb_tailroom() is not enough
because alloc_skb() can leave more tailroom than 1000 bytes due to
slab padding. len == 0 is also accepted, which lets an empty skb
reach virtbt_rx_handle() where the pkt_type byte is then read from
uninitialized memory. 1/2 defines VIRTBT_RX_BUF_SIZE, reuses it in
alloc_skb() and sg_init_one(), and gates virtbt_rx_work() on
[1, VIRTBT_RX_BUF_SIZE] with bt_dev_err_ratelimited() on the drop
path.
Patch 2/2 closes a residual short-payload hazard found during v2
pre-send review. After 1/2 restricts used.len to [1, 1000], a
one-byte completion still reaches hci_recv_frame() with skb->len
pulled to 0. If that byte was HCI_ACLDATA_PKT, the ACL-vs-ISO
classification fast-path in hci_dev_classify_pkt_type()
dereferences hci_acl_hdr(skb)->handle whenever the HCI device has
an active CIS_LINK, BIS_LINK, or PA_LINK connection, reading two
bytes of uninitialized RX-buffer data. The same hazard shape
exists for every packet type because none of the switch cases in
virtbt_rx_handle() checked skb->len against the per-type minimum
HCI header. 2/2 requires skb->len to cover the fixed header size
for the selected type (event 2, ACL 4, SCO 3, ISO 4) before
calling hci_recv_frame(); drop ratelimited otherwise. Unknown
pkt_type values still take the original kfree_skb() default path.
Both patches carry the same Fixes: 160fbcf3bfb9 and Cc: stable@
as the v2 commit, because both hazards have existed since the
driver switched to skb_put() for used.len handling.
Fresh runtime repro of the original skb_over_panic on an unpatched
tree and a clean reject log with 1/2 applied are attached in the
evidence bundle referenced from the v2 thread; no runtime change
between v2 and v3 beyond the split (the final tree after 1/2+2/2
is byte-identical to the v2 commit).
Prior public versions of this patch on linux-bluetooth:
v1: <20260418000138.1848813-1-michael.bommarito@gmail.com>
v2: <20260421151659.3326690-1-michael.bommarito@gmail.com>
Michael Bommarito (2):
Bluetooth: virtio_bt: clamp rx length before skb_put
Bluetooth: virtio_bt: validate rx pkt_type header length
drivers/bluetooth/virtio_bt.c | 39 ++++++++++++++++++++++++++++-------
1 file changed, 32 insertions(+), 7 deletions(-)
--
2.53.0
^ permalink raw reply
* Re: [PATCH RFC v2 00/18] mm/virtio: skip redundant zeroing of host-zeroed reported pages
From: Michael S. Tsirkin @ 2026-04-21 16:59 UTC (permalink / raw)
To: Gregory Price
Cc: David Hildenbrand (Arm), linux-kernel, Andrew Morton,
Vlastimil Babka, Brendan Jackman, Michal Hocko,
Suren Baghdasaryan, Jason Wang, Andrea Arcangeli, linux-mm,
virtualization
In-Reply-To: <aeeq9MUrqL6f47Y-@gourry-fedora-PF4VCD3F>
On Tue, Apr 21, 2026 at 12:51:00PM -0400, Gregory Price wrote:
> On Tue, Apr 21, 2026 at 09:06:00AM -0400, Michael S. Tsirkin wrote:
> > On Mon, Apr 20, 2026 at 10:38:19PM -0400, Gregory Price wrote:
> > >
> > > Can we leave folio_zero_user() callers the same, but add a PG_zeroed
> > > check in folio_zero_user() that skips the zeroing (but not the cache
> > > flush) and clear the PG_zeroed bit?
> > >
> > > Is this feasible?
> >
> > I do not see how - this would require leaking the page flag out of the
> > buddy allocator.
> >
>
> Right, but you're leaking that bit of information out one way or another
> - whether it's a page-flag or something else (pghint_t) you have the
> same lifecycle problems (when does it become invalidated? how long can
> it be trusted for?).
>
> I suppose at least with (pghint_t) the data (in theory) falls out of
> scope and doesn't live with the page - but guaranteed it just ends up
> polluting more and more interfaces.
>
> I'm seeing why David's suggest to plumb __GFP_ZERO correctly makes
> sense, it's really the only feasible approach here that doesn't generate
> a staleness problem with whatever information you try to leak out.
>
> ~Gregory
OK, v3 with that incoming.
^ permalink raw reply
* Re: [PATCH RFC v2 00/18] mm/virtio: skip redundant zeroing of host-zeroed reported pages
From: Gregory Price @ 2026-04-21 16:51 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: David Hildenbrand (Arm), linux-kernel, Andrew Morton,
Vlastimil Babka, Brendan Jackman, Michal Hocko,
Suren Baghdasaryan, Jason Wang, Andrea Arcangeli, linux-mm,
virtualization
In-Reply-To: <20260421090341-mutt-send-email-mst@kernel.org>
On Tue, Apr 21, 2026 at 09:06:00AM -0400, Michael S. Tsirkin wrote:
> On Mon, Apr 20, 2026 at 10:38:19PM -0400, Gregory Price wrote:
> >
> > Can we leave folio_zero_user() callers the same, but add a PG_zeroed
> > check in folio_zero_user() that skips the zeroing (but not the cache
> > flush) and clear the PG_zeroed bit?
> >
> > Is this feasible?
>
> I do not see how - this would require leaking the page flag out of the
> buddy allocator.
>
Right, but you're leaking that bit of information out one way or another
- whether it's a page-flag or something else (pghint_t) you have the
same lifecycle problems (when does it become invalidated? how long can
it be trusted for?).
I suppose at least with (pghint_t) the data (in theory) falls out of
scope and doesn't live with the page - but guaranteed it just ends up
polluting more and more interfaces.
I'm seeing why David's suggest to plumb __GFP_ZERO correctly makes
sense, it's really the only feasible approach here that doesn't generate
a staleness problem with whatever information you try to leak out.
~Gregory
^ permalink raw reply
* Re: [PATCH] Bluetooth: virtio_bt: clamp rx length before skb_put
From: Luiz Augusto von Dentz @ 2026-04-21 15:50 UTC (permalink / raw)
To: Michael Bommarito
Cc: Marcel Holtmann, linux-bluetooth, linux-kernel, Soenke Huster,
Michael S . Tsirkin, virtualization
In-Reply-To: <CAJJ9bXz0nwWsFg-B0nn77bn4dh=ob-F11a6azK+VV8sbLOBmLw@mail.gmail.com>
Hi Michael,
On Tue, Apr 21, 2026 at 11:19 AM Michael Bommarito
<michael.bommarito@gmail.com> wrote:
>
> On Mon, Apr 20, 2026 at 3:17 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> > All seem like valid comments to me, first one is odd to me thought,
> > never would have though that skb_tailroom wouldn't be enough to check
> > if using `skb_put` is safe.
>
> Thanks, those were definitely gnarly. Went back in with
> cscope/libclang and found another spot to guard too.
>
> Let me know if you want me to re-organize/separate these into a
> patchset instead of one bigger patch.
Yeah, let's have it split into different patches, otherwise the commit
message gets convoluted trying to explain the what and why of each
change.
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [PATCH] Bluetooth: virtio_bt: clamp rx length before skb_put
From: Michael Bommarito @ 2026-04-21 15:19 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Marcel Holtmann, linux-bluetooth, linux-kernel, Soenke Huster,
Michael S . Tsirkin, virtualization
In-Reply-To: <CABBYNZKP-Rwumw0c7FHEsL5e6XpFZCBA-O2avNujtYgRHeVZhA@mail.gmail.com>
On Mon, Apr 20, 2026 at 3:17 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> All seem like valid comments to me, first one is odd to me thought,
> never would have though that skb_tailroom wouldn't be enough to check
> if using `skb_put` is safe.
Thanks, those were definitely gnarly. Went back in with
cscope/libclang and found another spot to guard too.
Let me know if you want me to re-organize/separate these into a
patchset instead of one bigger patch.
^ permalink raw reply
* [PATCH v2] Bluetooth: virtio_bt: clamp rx length before skb_put
From: Michael Bommarito @ 2026-04-21 15:16 UTC (permalink / raw)
To: Marcel Holtmann, Luiz Augusto von Dentz, linux-bluetooth
Cc: linux-kernel, Soenke Huster, Michael S . Tsirkin, virtualization
In-Reply-To: <CABBYNZKP-Rwumw0c7FHEsL5e6XpFZCBA-O2avNujtYgRHeVZhA@mail.gmail.com>
virtbt_rx_work() calls skb_put(skb, len) where len comes directly
from virtqueue_get_buf() with no validation against the buffer we
posted to the device. The RX skb is allocated in virtbt_add_inbuf()
and exposed to virtio as exactly 1000 bytes via sg_init_one().
Checking len against skb_tailroom(skb) is not sufficient because
alloc_skb() can leave more tailroom than the 1000 bytes actually
handed to the device. A malicious or buggy backend can therefore
report used.len between 1001 and skb_tailroom(skb), causing skb_put()
to include uninitialized kernel heap bytes that were never written by
the device.
The same path also accepts len == 0, in which case skb_put(skb, 0)
leaves the skb empty but virtbt_rx_handle() still reads the pkt_type
byte from skb->data, consuming uninitialized memory.
A single-byte completion also reaches hci_recv_frame() with skb->len
already pulled to 0. If the byte happened to be HCI_ACLDATA_PKT, the
ACL-vs-ISO classification fast-path in hci_dev_classify_pkt_type()
dereferences hci_acl_hdr(skb)->handle whenever the HCI device has an
active CIS_LINK, BIS_LINK, or PA_LINK connection, reading two bytes
of uninitialized RX-buffer data. The same hazard exists for every
packet type the driver accepts because none of the switch cases in
virtbt_rx_handle() check skb->len against the per-type minimum HCI
header size before handing the frame to the core.
Close all three paths:
- define VIRTBT_RX_BUF_SIZE once, reuse it in alloc_skb() and
sg_init_one() and as the upper bound on used.len, so the gate
matches the buffer actually exposed to the device
- reject used.len == 0 before virtbt_rx_handle() reads pkt_type
- after stripping pkt_type, require skb->len to cover the fixed
header size for the selected type (event/ACL/SCO/ISO) before
calling hci_recv_frame(); drop ratelimited otherwise
Use bt_dev_err_ratelimited() because the length and packet-type
values come from an untrusted backend that can otherwise flood the
kernel log.
Same class of bug as commit c04db81cd028 ("net/9p: Fix buffer
overflow in USB transport layer"), which hardened the USB 9p
transport against unchecked device-reported length.
Fixes: 160fbcf3bfb9 ("Bluetooth: virtio_bt: Use skb_put to set length")
Cc: stable@vger.kernel.org
Cc: Soenke Huster <soenke.huster@eknoes.de>
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
Assisted-by: Claude:claude-opus-4-7
---
Changes in v2:
- validate used.len against VIRTBT_RX_BUF_SIZE (the 1000 bytes
actually exposed to the device via sg_init_one()) rather than
skb_tailroom(), which can exceed 1000 due to slab padding
- reject used.len == 0 before virtbt_rx_handle() reads the pkt_type
byte, so zero-length completions can no longer pull an empty skb
into hci_recv_frame()
- in virtbt_rx_handle(), require skb->len to cover the fixed HCI
header size for the selected pkt_type (event 2, ACL 4, SCO 3,
ISO 4) before calling hci_recv_frame(); this prevents a one-byte
HCI_ACLDATA_PKT completion from reaching
hci_dev_classify_pkt_type() and dereferencing hci_acl_hdr(skb)
over uninitialized RX buffer data when CIS/BIS/PA connections
are present
- switch the error log to bt_dev_err_ratelimited() because the
length and pkt_type values come from an untrusted backend that
can otherwise flood the kernel log
drivers/bluetooth/virtio_bt.c | 39 ++++++++++++++++++++++++++++-------
1 file changed, 32 insertions(+), 7 deletions(-)
diff --git a/drivers/bluetooth/virtio_bt.c b/drivers/bluetooth/virtio_bt.c
index 76d61af8a275..140ab55c9fc5 100644
--- a/drivers/bluetooth/virtio_bt.c
+++ b/drivers/bluetooth/virtio_bt.c
@@ -12,6 +12,7 @@
#include <net/bluetooth/hci_core.h>
#define VERSION "0.1"
+#define VIRTBT_RX_BUF_SIZE 1000
enum {
VIRTBT_VQ_TX,
@@ -33,11 +34,11 @@ static int virtbt_add_inbuf(struct virtio_bluetooth *vbt)
struct sk_buff *skb;
int err;
- skb = alloc_skb(1000, GFP_KERNEL);
+ skb = alloc_skb(VIRTBT_RX_BUF_SIZE, GFP_KERNEL);
if (!skb)
return -ENOMEM;
- sg_init_one(sg, skb->data, 1000);
+ sg_init_one(sg, skb->data, VIRTBT_RX_BUF_SIZE);
err = virtqueue_add_inbuf(vq, sg, 1, skb, GFP_KERNEL);
if (err < 0) {
@@ -197,6 +198,7 @@ static int virtbt_shutdown_generic(struct hci_dev *hdev)
static void virtbt_rx_handle(struct virtio_bluetooth *vbt, struct sk_buff *skb)
{
+ size_t min_hdr;
__u8 pkt_type;
pkt_type = *((__u8 *) skb->data);
@@ -204,16 +206,32 @@ static void virtbt_rx_handle(struct virtio_bluetooth *vbt, struct sk_buff *skb)
switch (pkt_type) {
case HCI_EVENT_PKT:
+ min_hdr = sizeof(struct hci_event_hdr);
+ break;
case HCI_ACLDATA_PKT:
+ min_hdr = sizeof(struct hci_acl_hdr);
+ break;
case HCI_SCODATA_PKT:
+ min_hdr = sizeof(struct hci_sco_hdr);
+ break;
case HCI_ISODATA_PKT:
- hci_skb_pkt_type(skb) = pkt_type;
- hci_recv_frame(vbt->hdev, skb);
+ min_hdr = sizeof(struct hci_iso_hdr);
break;
default:
kfree_skb(skb);
- break;
+ return;
+ }
+
+ if (skb->len < min_hdr) {
+ bt_dev_err_ratelimited(vbt->hdev,
+ "rx pkt_type 0x%02x payload %u < hdr %zu\n",
+ pkt_type, skb->len, min_hdr);
+ kfree_skb(skb);
+ return;
}
+
+ hci_skb_pkt_type(skb) = pkt_type;
+ hci_recv_frame(vbt->hdev, skb);
}
static void virtbt_rx_work(struct work_struct *work)
@@ -227,8 +245,15 @@ static void virtbt_rx_work(struct work_struct *work)
if (!skb)
return;
- skb_put(skb, len);
- virtbt_rx_handle(vbt, skb);
+ if (!len || len > VIRTBT_RX_BUF_SIZE) {
+ bt_dev_err_ratelimited(vbt->hdev,
+ "rx reply len %u outside [1, %u]\n",
+ len, VIRTBT_RX_BUF_SIZE);
+ kfree_skb(skb);
+ } else {
+ skb_put(skb, len);
+ virtbt_rx_handle(vbt, skb);
+ }
if (virtbt_add_inbuf(vbt) < 0)
return;
--
2.53.0
^ permalink raw reply related
* Re: [PATCH] virtio_console: add timeout to __send_to_port() spin loop
From: Peng Yang @ 2026-04-21 14:23 UTC (permalink / raw)
To: Arnd Bergmann, Amit Shah, Greg Kroah-Hartman
Cc: kernel, virtualization, linux-kernel, kernel
In-Reply-To: <9e5be98b-3035-4e61-8325-52ed8fcf65fa@app.fastmail.com>
On 4/21/2026 8:14 PM, Arnd Bergmann wrote:
> On Tue, Apr 21, 2026, at 11:18, Peng Yang wrote:
>> On 4/21/2026 3:38 PM, Arnd Bergmann wrote:
>>>
>>> Which host implementation do you use? The way the virtio_console
>>> driver works really assumes that virtqueue_kick() consumes the
>>> buffer synchronously. Even though that is not how virtio is
>>> specified, this does tend to work. ;-)
>>>
>> We are using crosvm as the host VMM with its virtio-console backend,
>> running on Android. The trigger is Android host reboot/shutdown: when
>> Android initiates a reboot, the crosvm process exits and tears down
>> the virtio-console backend. At that point, the TX virtqueue is no
>> longer being drained by the host and will never be consumed again.
>
> I see, so the normal behavior is likely just fine, but the error
> handling is what goes wrong. Maybe there is a way for the guest
> to detect the device being turn down already so it does not
> actually have to wait any more?
>
Yes, exactly. Normal operation is fine; the problem is purely
in the error/teardown path.
We investigated both the virtqueue_is_broken() path and the
virtio config status register path. Neither works in our
scenario, for the reasons explained below.
>> The crash dump from the actual failure confirms the exact deadlock
>> scenario:
>>
>> Core 3 holds outvq_lock and spins forever in virtqueue_get_buf waiting
>> for the host to consume the buffer:
>>
>> virtqueue_get_buf
>> __send_to_port
>> put_chars
>> hvc_push
>> hvc_write
>> n_tty_write
>> <- writev() syscall
>
> This current loop here is
>
> while (!virtqueue_get_buf(out_vq, &len)
> && !virtqueue_is_broken(out_vq))
> cpu_relax();
>
> which looks like the virtqueue_is_broken() check is meant to
> catch this exact case. Do you know why this does not break
> out of the loop after crosvm tears down the virtio-console
> device?
>
virtqueue_is_broken() only reads the guest-side vq->broken
flag, which is set either by virtio_break_device() or by a
failed virtqueue_notify() kick. Neither happens here:
- When the host VMM exits, it does so as a pure userspace
process termination. No PCI interrupt or notification is
sent to the guest, so virtio_break_device() is never
called from the guest side.
- __send_to_port() runs with IRQs disabled and outvq_lock
held. Even if the host were to send a config change
interrupt, it cannot be delivered in this context, so the
async chain virtio_config_changed() -> config_intr()
-> virtio_break_device() is completely blocked.
As a result, vq->broken remains false forever and the loop
never exits.
>> Core 0 has a watchdog bark ISR fire and attempts printk, holds the
>> console lock, but spins on _raw_spin_lock_irqsave waiting to acquire
>> outvq_lock:
>>
>> queued_spin_lock_slowpath
>> _raw_spin_lock_irqsave
>> __send_to_port
>> put_chars
>> hvc_console_print
>> console_flush_all
>> console_unlock
>> vprintk_emit
>> <- printk (watchdog bark handler)
>
> My first thought here was that __send_to_port() should perhaps
> release the lock during the while() loop, which should avoid
> blocking the other threads on the spin_lock_irqsave() but
> would not avoid blocking on the loop.
>
Releasing the lock during the spin would unblock other CPUs
waiting on outvq_lock (e.g. the watchdog bark handler trying
to printk). However it does not fix the root issue — the CPU
holding the lock would still spin forever. It also introduces
a TOCTOU race: another thread could modify the port or queue
state between the unlock and re-lock. The timeout avoids both
problems by bounding the spin duration without releasing the
lock.
>> The 200 ms timeout is intended as a minimal, targeted workaround to prevent
>> the watchdog bite in our specific scenario. We are open to suggestions on a
>> better long-term approach.
>
> Not sure how to do it, but I think finding a way to call
> virtio_break_device() at the point the host device goes away is
> the best solution here. Ideally there would just be a notification
> from the host, but since __send_to_port() may be called with
> interrupts disabled and may be running on the only CPU, that
> would still be unreliable.
>
> Maybe there is a way for virtio_console to read a status
> register in the virtio config that tells it whether the
> host has turned it off? I was thinking vdev->config->get_status(vdev)
> but that seems to only get updated by the guest.
>
> Arnd
We checked this. In our host VMM implementation,
VIRTIO_CONFIG_S_NEEDS_RESET is never set on the teardown
path — the device simply stops responding without updating
any status register. So polling vp_modern_get_status()
inside the spin loop would not help here.
We agree that the ideal long-term fix is for the host to
trigger virtio_break_device() via a clean PCI hot-unplug
sequence, but that is not possible in a crash or forced
reboot scenario.
The 200ms value is chosen to be well above normal host
response time (microseconds) to avoid false positives, while
remaining well below the watchdog bark-to-bite window
(3 seconds) to ensure all CPUs can exit the loop and complete
the bark handler before a bite occurs.
^ permalink raw reply
* Re: [PATCH RFC v2 00/18] mm/virtio: skip redundant zeroing of host-zeroed reported pages
From: David Hildenbrand (Arm) @ 2026-04-21 14:18 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Gregory Price, linux-kernel, Andrew Morton, Vlastimil Babka,
Brendan Jackman, Michal Hocko, Suren Baghdasaryan, Jason Wang,
Andrea Arcangeli, linux-mm, virtualization
In-Reply-To: <20260421101409-mutt-send-email-mst@kernel.org>
On 4/21/26 16:15, Michael S. Tsirkin wrote:
> On Tue, Apr 21, 2026 at 12:04:49PM +0200, David Hildenbrand (Arm) wrote:
>> On 4/21/26 04:38, Gregory Price wrote:
>>>
>>> Why does it need to propogate?
>>>
>>> Can we leave folio_zero_user() callers the same, but add a PG_zeroed
>>> check in folio_zero_user() that skips the zeroing (but not the cache
>>> flush) and clear the PG_zeroed bit?
>>
>> folio_zero_user() is just an abomination, really.
>
> We can't completely replace it with GFP_ZERO though e.g. because hugetlbfs
> has its own pool and needs to zero that.
Right, hugetlb will have to keep using it for now.
--
Cheers,
David
^ permalink raw reply
* Re: [EXTERNAL] Re: [PATCH net v2] hv_sock: Report EOF instead of -EIO for FIN
From: Jakub Kicinski @ 2026-04-21 14:18 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Dexuan Cui, patchwork-bot+netdevbpf@kernel.org, KY Srinivasan,
Haiyang Zhang, wei.liu@kernel.org, Long Li, davem@davemloft.net,
edumazet@google.com, pabeni@redhat.com, horms@kernel.org,
niuxuewei.nxw@antgroup.com, linux-hyperv@vger.kernel.org,
virtualization@lists.linux.dev, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, stable@vger.kernel.org, Ben Hillis,
levymitchell0@gmail.com
In-Reply-To: <CAGxU2F6DVcLDLg3dT5DsDmsaOuhOcD+4VSG5dqXcFRwsN1NZ+A@mail.gmail.com>
On Tue, 21 Apr 2026 09:28:19 +0200 Stefano Garzarella wrote:
> > I'm sorry -- I just posted v3
> > https://lore.kernel.org/linux-hyperv/20260421025950.1099495-1-decui@microsoft.com/T/#u
> > and then I realized that the v2 had been merged into the main branch :-(
> >
> > Should I post a new delta patch(with a Fixes tag against the v2) based on the main branch?
>
> Ehm, I'm not sure about the process but if it's merged in net tree,
> maybe we need a follow up patch.
>
> Anyway, let's wait for Jakub's or other net maintainers' suggestions.
Yes, you have to post an incremental fix
^ permalink raw reply
* Re: [PATCH RFC v2 00/18] mm/virtio: skip redundant zeroing of host-zeroed reported pages
From: Michael S. Tsirkin @ 2026-04-21 14:15 UTC (permalink / raw)
To: David Hildenbrand (Arm)
Cc: Gregory Price, linux-kernel, Andrew Morton, Vlastimil Babka,
Brendan Jackman, Michal Hocko, Suren Baghdasaryan, Jason Wang,
Andrea Arcangeli, linux-mm, virtualization
In-Reply-To: <0bb2d565-4b3e-4b0b-8e81-57898d8a2a21@kernel.org>
On Tue, Apr 21, 2026 at 12:04:49PM +0200, David Hildenbrand (Arm) wrote:
> On 4/21/26 04:38, Gregory Price wrote:
> > On Mon, Apr 20, 2026 at 07:33:38PM -0400, Michael S. Tsirkin wrote:
> >> On Mon, Apr 20, 2026 at 08:20:57PM +0200, David Hildenbrand (Arm) wrote:
> >>
> >>>
> >>> Which would *already* be the case of you use folio_alloc(GFP_ZERO)
> >>> instead of magical vma_alloc_folio() + folio_zero_user().
> >>>
> >>> I don't really see how vma_alloc_folio_hints() -- that also consumes the
> >>> address -- is any better in that regard?
> >>
> >> By itself, it is not. But the issue is propagating the address from
> >> there all over mm. If we miss even one place - we get a subtle cache
> >> corruption on non x86.
> >>
> >
> > Why does it need to propogate?
> >
> > Can we leave folio_zero_user() callers the same, but add a PG_zeroed
> > check in folio_zero_user() that skips the zeroing (but not the cache
> > flush) and clear the PG_zeroed bit?
>
> folio_zero_user() is just an abomination, really.
We can't completely replace it with GFP_ZERO though e.g. because hugetlbfs
has its own pool and needs to zero that.
> --
> Cheers,
>
> David
^ 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