Hi Andrew,
On 2019/7/17 2:50, Andrew Murray wrote:
On Tue, Jul 16, 2019 at 11:14:37PM +0800, Zenghui Yu wrote:
On 2019/7/16 23:05, Zenghui Yu wrote:
Hi folks,
Running the latest kernel with KASAN enabled, we will hit the following
KASAN BUG during guest's boot process.
I'm in commit 9637d517347e80ee2fe1c5d8ce45ba1b88d8b5cd.
Any problems in the chained PMU code? Or just a false positive?
---8<---
[ 654.706268]
==================================================================
[ 654.706280] BUG: KASAN: slab-out-of-bounds in
kvm_pmu_get_canonical_pmc+0x48/0x78
[ 654.706286] Read of size 8 at addr ffff801d6c8fea38 by task
qemu-kvm/23268
[ 654.706296] CPU: 2 PID: 23268 Comm: qemu-kvm Not tainted 5.2.0+ #178
[ 654.706301] Hardware name: Huawei TaiShan 2280 /BC11SPCD, BIOS 1.58
10/24/2018
[ 654.706305] Call trace:
[ 654.706311] dump_backtrace+0x0/0x238
[ 654.706317] show_stack+0x24/0x30
[ 654.706325] dump_stack+0xe0/0x134
[ 654.706332] print_address_description+0x80/0x408
[ 654.706338] __kasan_report+0x164/0x1a0
[ 654.706343] kasan_report+0xc/0x18
[ 654.706348] __asan_load8+0x88/0xb0
[ 654.706353] kvm_pmu_get_canonical_pmc+0x48/0x78
I noticed that we will use "pmc->idx" and the "chained" bitmap to
determine if the pmc is chained, in kvm_pmu_pmc_is_chained().
Should we initialize the idx and the bitmap appropriately before
doing kvm_pmu_stop_counter()? Like:
Hi Zenghui,
Thanks for spotting this and investigating - I'll make sure to use KASAN
in the future when testing...
diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
index 3dd8238..cf3119a 100644
--- a/virt/kvm/arm/pmu.c
+++ b/virt/kvm/arm/pmu.c
@@ -224,12 +224,12 @@ void kvm_pmu_vcpu_reset(struct kvm_vcpu *vcpu)
int i;
struct kvm_pmu *pmu = &vcpu->arch.pmu;
+ bitmap_zero(vcpu->arch.pmu.chained, ARMV8_PMU_MAX_COUNTER_PAIRS);
+
for (i = 0; i < ARMV8_PMU_MAX_COUNTERS; i++) {
- kvm_pmu_stop_counter(vcpu, &pmu->pmc[i]);
pmu->pmc[i].idx = i;
+ kvm_pmu_stop_counter(vcpu, &pmu->pmc[i]);
}
-
- bitmap_zero(vcpu->arch.pmu.chained, ARMV8_PMU_MAX_COUNTER_PAIRS);
}
We have to be a little careful here, as the vcpu may be reset after use.
Upon resetting we must ensure that any existing perf_events are released -
this is why kvm_pmu_stop_counter is called before bitmap_zero (as
kvm_pmu_stop_counter relies on kvm_pmu_pmc_is_chained).
(For example, by clearing the bitmap before stopping the counters, we will
attempt to release the perf event for both pmc's in a chained pair. Whereas
we should only release the canonical pmc. It's actually OK right now as we
set the non-canonical pmc perf_event will be NULL - but who knows that this
will hold true in the future. The code makes the assumption that the
non-canonical perf event isn't touched on a chained pair).
Indeed!
The KASAN bug gets fixed by moving the assignment of idx before
kvm_pmu_stop_counter. Therefore I'd suggest you drop the bitmap_zero hunks.
Can you send a patch with just the idx assignment hunk please?
Sure. I will send a patch with your Suggested-by, after some tests.
Thanks,
zenghui
