* index-out-of-range ubsan warnings @ 2016-02-23 9:26 Mike Krinkin 2016-02-23 10:07 ` Jan Kiszka 0 siblings, 1 reply; 8+ messages in thread From: Mike Krinkin @ 2016-02-23 9:26 UTC (permalink / raw) To: kvm Hi, while playing with toy OS project i've hit the two suspicious ubsan warnings. The first one: [ 168.067027] ================================================================================ [ 168.067039] UBSAN: Undefined behaviour in arch/x86/kvm/mmu.c:2011:17 [ 168.067044] index 3 is out of range for type 'kvm_mmu_page *[3]' [ 168.067051] CPU: 1 PID: 2950 Comm: qemu-system-x86 Tainted: G O L 4.5.0-rc5-next-20160222 #7 [ 168.067055] Hardware name: LENOVO 23205NG/23205NG, BIOS G2ET95WW (2.55 ) 07/09/2013 [ 168.067059] 0000000000000000 ffff8801cfcaf438 ffffffff81c9f780 0000000041b58ab3 [ 168.067068] ffffffff82eb2cc1 ffffffff81c9f6b4 ffff8801cfcaf460 ffff8801cfcaf410 [ 168.067076] 0000000000000003 0000000000000001 0000000000000000 ffffffffa1981680 [ 168.067083] Call Trace: [ 168.067093] [<ffffffff81c9f780>] dump_stack+0xcc/0x12c [ 168.067100] [<ffffffff81c9f6b4>] ? _atomic_dec_and_lock+0xc4/0xc4 [ 168.067108] [<ffffffff81da9e81>] ubsan_epilogue+0xd/0x8a [ 168.067115] [<ffffffff81daafa2>] __ubsan_handle_out_of_bounds+0x15c/0x1a3 [ 168.067121] [<ffffffff81daae46>] ? __ubsan_handle_shift_out_of_bounds+0x2bd/0x2bd [ 168.067129] [<ffffffff812e848b>] ? debug_lockdep_rcu_enabled+0x7b/0x90 [ 168.067135] [<ffffffff812a0285>] ? __lock_acquire+0x1f35/0x7260 [ 168.067196] [<ffffffffa1822c27>] mmu_zap_unsync_children+0x567/0x590 [kvm] [ 168.067249] [<ffffffffa18226c0>] ? mmu_shrink_scan+0x7d0/0x7d0 [kvm] [ 168.067255] [<ffffffff8129e350>] ? debug_check_no_locks_freed+0x350/0x350 [ 168.067261] [<ffffffff8129d4af>] ? mark_held_locks+0xff/0x280 [ 168.067267] [<ffffffff812e848b>] ? debug_lockdep_rcu_enabled+0x7b/0x90 [ 168.067272] [<ffffffff812a01d4>] ? __lock_acquire+0x1e84/0x7260 [ 168.067279] [<ffffffff8129d4af>] ? mark_held_locks+0xff/0x280 [ 168.067330] [<ffffffffa1820828>] kvm_mmu_prepare_zap_page+0x118/0x1390 [kvm] [ 168.067337] [<ffffffff812eb9cb>] ? __synchronize_srcu+0x27b/0x570 [ 168.067386] [<ffffffffa1820710>] ? kvm_mmu_page_fault+0x620/0x620 [kvm] [ 168.067434] [<ffffffffa178f650>] ? kvm_vcpu_compat_ioctl+0x460/0x460 [kvm] [ 168.067485] [<ffffffffa1833152>] kvm_mmu_invalidate_zap_all_pages+0x302/0x650 [kvm] [ 168.067535] [<ffffffffa17f599e>] kvm_arch_flush_shadow_memslot+0xe/0x10 [kvm] [ 168.067581] [<ffffffffa179171f>] __kvm_set_memory_region+0x180f/0x27b0 [kvm] [ 168.067587] [<ffffffff8293cf9e>] ? mutex_lock_nested+0x50e/0x970 [ 168.067634] [<ffffffffa178ff10>] ? kvm_kvzalloc+0x30/0x30 [kvm] [ 168.067640] [<ffffffff812e848b>] ? debug_lockdep_rcu_enabled+0x7b/0x90 [ 168.067647] [<ffffffff8162accd>] ? __might_fault+0x12d/0x240 [ 168.067693] [<ffffffffa17926ed>] kvm_set_memory_region+0x2d/0x50 [kvm] [ 168.067738] [<ffffffffa1793022>] kvm_vm_ioctl+0x912/0x1610 [kvm] [ 168.067784] [<ffffffffa1792710>] ? kvm_set_memory_region+0x50/0x50 [kvm] [ 168.067790] [<ffffffff8129ee99>] ? __lock_acquire+0xb49/0x7260 [ 168.067797] [<ffffffff812e848b>] ? debug_lockdep_rcu_enabled+0x7b/0x90 [ 168.067803] [<ffffffff81659828>] ? page_add_new_anon_rmap+0x188/0x2d0 [ 168.067810] [<ffffffff8129e350>] ? debug_check_no_locks_freed+0x350/0x350 [ 168.067816] [<ffffffff82946087>] ? _raw_spin_unlock+0x27/0x40 [ 168.067821] [<ffffffff8163a943>] ? handle_mm_fault+0x1673/0x2e40 [ 168.067828] [<ffffffff812e848b>] ? debug_lockdep_rcu_enabled+0x7b/0x90 [ 168.067834] [<ffffffff81725a80>] do_vfs_ioctl+0x1b0/0x12b0 [ 168.067840] [<ffffffff817258d0>] ? ioctl_preallocate+0x210/0x210 [ 168.067847] [<ffffffff8174aef3>] ? __fget+0x273/0x4a0 [ 168.067852] [<ffffffff8174acd0>] ? __fget+0x50/0x4a0 [ 168.067858] [<ffffffff8174b1f6>] ? __fget_light+0x96/0x2b0 [ 168.067864] [<ffffffff81726bf9>] SyS_ioctl+0x79/0x90 [ 168.067870] [<ffffffff82946880>] entry_SYSCALL_64_fastpath+0x23/0xc1 [ 168.067875] ================================================================================ I'm not familiar with the code (and so i don't send a patch), but, AFAICS, except the assignment in kvm_mmu_pages_init, which cases the warning above, there is only one assignment to struct mmu_page_path parent field in the mmu_pages_next which doesn't produce warnings, so i applied the following diff and the warning gone: diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c index 95a955d..cc673f1 100644 --- a/arch/x86/kvm/mmu.c +++ b/arch/x86/kvm/mmu.c @@ -2008,7 +2008,7 @@ static void kvm_mmu_pages_init(struct kvm_mmu_page *parent, struct mmu_page_path *parents, struct kvm_mmu_pages *pvec) { - parents->parent[parent->role.level-1] = NULL; + parents->parent[parent->role.level-2] = NULL; pvec->nr = 0; } The second one is: [ 168.791851] ================================================================================ [ 168.791862] UBSAN: Undefined behaviour in arch/x86/kvm/paging_tmpl.h:252:15 [ 168.791866] index 4 is out of range for type 'u64 [4]' [ 168.791871] CPU: 0 PID: 2950 Comm: qemu-system-x86 Tainted: G O L 4.5.0-rc5-next-20160222 #7 [ 168.791873] Hardware name: LENOVO 23205NG/23205NG, BIOS G2ET95WW (2.55 ) 07/09/2013 [ 168.791876] 0000000000000000 ffff8801cfcaf208 ffffffff81c9f780 0000000041b58ab3 [ 168.791882] ffffffff82eb2cc1 ffffffff81c9f6b4 ffff8801cfcaf230 ffff8801cfcaf1e0 [ 168.791886] 0000000000000004 0000000000000001 0000000000000000 ffffffffa1981600 [ 168.791891] Call Trace: [ 168.791899] [<ffffffff81c9f780>] dump_stack+0xcc/0x12c [ 168.791904] [<ffffffff81c9f6b4>] ? _atomic_dec_and_lock+0xc4/0xc4 [ 168.791910] [<ffffffff81da9e81>] ubsan_epilogue+0xd/0x8a [ 168.791914] [<ffffffff81daafa2>] __ubsan_handle_out_of_bounds+0x15c/0x1a3 [ 168.791918] [<ffffffff81daae46>] ? __ubsan_handle_shift_out_of_bounds+0x2bd/0x2bd [ 168.791922] [<ffffffff811287ef>] ? get_user_pages_fast+0x2bf/0x360 [ 168.791954] [<ffffffffa1794050>] ? kvm_largepages_enabled+0x30/0x30 [kvm] [ 168.791958] [<ffffffff81128530>] ? __get_user_pages_fast+0x360/0x360 [ 168.791987] [<ffffffffa181b818>] paging64_walk_addr_generic+0x1b28/0x2600 [kvm] [ 168.792014] [<ffffffffa1819cf0>] ? init_kvm_mmu+0x1100/0x1100 [kvm] [ 168.792019] [<ffffffff8129e350>] ? debug_check_no_locks_freed+0x350/0x350 [ 168.792044] [<ffffffffa1819cf0>] ? init_kvm_mmu+0x1100/0x1100 [kvm] [ 168.792076] [<ffffffffa181c36d>] paging64_gva_to_gpa+0x7d/0x110 [kvm] [ 168.792121] [<ffffffffa181c2f0>] ? paging64_walk_addr_generic+0x2600/0x2600 [kvm] [ 168.792130] [<ffffffff812e848b>] ? debug_lockdep_rcu_enabled+0x7b/0x90 [ 168.792178] [<ffffffffa17d9a4a>] emulator_read_write_onepage+0x27a/0x1150 [kvm] [ 168.792208] [<ffffffffa1794d44>] ? __kvm_read_guest_page+0x54/0x70 [kvm] [ 168.792234] [<ffffffffa17d97d0>] ? kvm_task_switch+0x160/0x160 [kvm] [ 168.792238] [<ffffffff812e848b>] ? debug_lockdep_rcu_enabled+0x7b/0x90 [ 168.792263] [<ffffffffa17daa07>] emulator_read_write+0xe7/0x6d0 [kvm] [ 168.792290] [<ffffffffa183b620>] ? em_cr_write+0x230/0x230 [kvm] [ 168.792314] [<ffffffffa17db005>] emulator_write_emulated+0x15/0x20 [kvm] [ 168.792340] [<ffffffffa18465f8>] segmented_write+0xf8/0x130 [kvm] [ 168.792367] [<ffffffffa1846500>] ? em_lgdt+0x20/0x20 [kvm] [ 168.792374] [<ffffffffa14db512>] ? vmx_read_guest_seg_ar+0x42/0x1e0 [kvm_intel] [ 168.792400] [<ffffffffa1846d82>] writeback+0x3f2/0x700 [kvm] [ 168.792424] [<ffffffffa1846990>] ? em_sidt+0xa0/0xa0 [kvm] [ 168.792449] [<ffffffffa185554d>] ? x86_decode_insn+0x1b3d/0x4f70 [kvm] [ 168.792474] [<ffffffffa1859032>] x86_emulate_insn+0x572/0x3010 [kvm] [ 168.792499] [<ffffffffa17e71dd>] x86_emulate_instruction+0x3bd/0x2110 [kvm] [ 168.792524] [<ffffffffa17e6e20>] ? reexecute_instruction.part.110+0x2e0/0x2e0 [kvm] [ 168.792532] [<ffffffffa14e9a81>] handle_ept_misconfig+0x61/0x460 [kvm_intel] [ 168.792539] [<ffffffffa14e9a20>] ? handle_pause+0x450/0x450 [kvm_intel] [ 168.792546] [<ffffffffa15130ea>] vmx_handle_exit+0xd6a/0x1ad0 [kvm_intel] [ 168.792572] [<ffffffffa17f6a6c>] ? kvm_arch_vcpu_ioctl_run+0xbdc/0x6090 [kvm] [ 168.792597] [<ffffffffa17f6bcd>] kvm_arch_vcpu_ioctl_run+0xd3d/0x6090 [kvm] [ 168.792621] [<ffffffffa17f6a6c>] ? kvm_arch_vcpu_ioctl_run+0xbdc/0x6090 [kvm] [ 168.792627] [<ffffffff8293b530>] ? __ww_mutex_lock_interruptible+0x1630/0x1630 [ 168.792651] [<ffffffffa17f5e90>] ? kvm_arch_vcpu_runnable+0x4f0/0x4f0 [kvm] [ 168.792656] [<ffffffff811eeb30>] ? preempt_notifier_unregister+0x190/0x190 [ 168.792681] [<ffffffffa17e0447>] ? kvm_arch_vcpu_load+0x127/0x650 [kvm] [ 168.792704] [<ffffffffa178e9a3>] kvm_vcpu_ioctl+0x553/0xda0 [kvm] [ 168.792727] [<ffffffffa178e450>] ? vcpu_put+0x40/0x40 [kvm] [ 168.792732] [<ffffffff8129e350>] ? debug_check_no_locks_freed+0x350/0x350 [ 168.792735] [<ffffffff82946087>] ? _raw_spin_unlock+0x27/0x40 [ 168.792740] [<ffffffff8163a943>] ? handle_mm_fault+0x1673/0x2e40 [ 168.792744] [<ffffffff8129daa8>] ? trace_hardirqs_on_caller+0x478/0x6c0 [ 168.792747] [<ffffffff8129dcfd>] ? trace_hardirqs_on+0xd/0x10 [ 168.792751] [<ffffffff812e848b>] ? debug_lockdep_rcu_enabled+0x7b/0x90 [ 168.792756] [<ffffffff81725a80>] do_vfs_ioctl+0x1b0/0x12b0 [ 168.792759] [<ffffffff817258d0>] ? ioctl_preallocate+0x210/0x210 [ 168.792763] [<ffffffff8174aef3>] ? __fget+0x273/0x4a0 [ 168.792766] [<ffffffff8174acd0>] ? __fget+0x50/0x4a0 [ 168.792770] [<ffffffff8174b1f6>] ? __fget_light+0x96/0x2b0 [ 168.792773] [<ffffffff81726bf9>] SyS_ioctl+0x79/0x90 [ 168.792777] [<ffffffff82946880>] entry_SYSCALL_64_fastpath+0x23/0xc1 [ 168.792780] ================================================================================ the following fix solved problem for me: diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h index 6c9fed9..2ce4f05 100644 --- a/arch/x86/kvm/paging_tmpl.h +++ b/arch/x86/kvm/paging_tmpl.h @@ -249,7 +249,7 @@ static int FNAME(update_accessed_dirty_bits)(struct kvm_vcpu *vcpu, return ret; kvm_vcpu_mark_page_dirty(vcpu, table_gfn); - walker->ptes[level] = pte; + walker->ptes[level - 1] = pte; } return 0; } ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: index-out-of-range ubsan warnings 2016-02-23 9:26 index-out-of-range ubsan warnings Mike Krinkin @ 2016-02-23 10:07 ` Jan Kiszka 2016-02-23 10:44 ` Xiao Guangrong 0 siblings, 1 reply; 8+ messages in thread From: Jan Kiszka @ 2016-02-23 10:07 UTC (permalink / raw) To: Mike Krinkin, kvm; +Cc: Marcelo Tosatti, guangrong.xiao, Paolo Bonzini On 2016-02-23 10:26, Mike Krinkin wrote: > Hi, > > while playing with toy OS project i've hit the two suspicious ubsan warnings. > The first one: > > [ 168.067027] ================================================================================ > [ 168.067039] UBSAN: Undefined behaviour in arch/x86/kvm/mmu.c:2011:17 > [ 168.067044] index 3 is out of range for type 'kvm_mmu_page *[3]' > [ 168.067051] CPU: 1 PID: 2950 Comm: qemu-system-x86 Tainted: G O L 4.5.0-rc5-next-20160222 #7 > [ 168.067055] Hardware name: LENOVO 23205NG/23205NG, BIOS G2ET95WW (2.55 ) 07/09/2013 > [ 168.067059] 0000000000000000 ffff8801cfcaf438 ffffffff81c9f780 0000000041b58ab3 > [ 168.067068] ffffffff82eb2cc1 ffffffff81c9f6b4 ffff8801cfcaf460 ffff8801cfcaf410 > [ 168.067076] 0000000000000003 0000000000000001 0000000000000000 ffffffffa1981680 > [ 168.067083] Call Trace: > [ 168.067093] [<ffffffff81c9f780>] dump_stack+0xcc/0x12c > [ 168.067100] [<ffffffff81c9f6b4>] ? _atomic_dec_and_lock+0xc4/0xc4 > [ 168.067108] [<ffffffff81da9e81>] ubsan_epilogue+0xd/0x8a > [ 168.067115] [<ffffffff81daafa2>] __ubsan_handle_out_of_bounds+0x15c/0x1a3 > [ 168.067121] [<ffffffff81daae46>] ? __ubsan_handle_shift_out_of_bounds+0x2bd/0x2bd > [ 168.067129] [<ffffffff812e848b>] ? debug_lockdep_rcu_enabled+0x7b/0x90 > [ 168.067135] [<ffffffff812a0285>] ? __lock_acquire+0x1f35/0x7260 > [ 168.067196] [<ffffffffa1822c27>] mmu_zap_unsync_children+0x567/0x590 [kvm] > [ 168.067249] [<ffffffffa18226c0>] ? mmu_shrink_scan+0x7d0/0x7d0 [kvm] > [ 168.067255] [<ffffffff8129e350>] ? debug_check_no_locks_freed+0x350/0x350 > [ 168.067261] [<ffffffff8129d4af>] ? mark_held_locks+0xff/0x280 > [ 168.067267] [<ffffffff812e848b>] ? debug_lockdep_rcu_enabled+0x7b/0x90 > [ 168.067272] [<ffffffff812a01d4>] ? __lock_acquire+0x1e84/0x7260 > [ 168.067279] [<ffffffff8129d4af>] ? mark_held_locks+0xff/0x280 > [ 168.067330] [<ffffffffa1820828>] kvm_mmu_prepare_zap_page+0x118/0x1390 [kvm] > [ 168.067337] [<ffffffff812eb9cb>] ? __synchronize_srcu+0x27b/0x570 > [ 168.067386] [<ffffffffa1820710>] ? kvm_mmu_page_fault+0x620/0x620 [kvm] > [ 168.067434] [<ffffffffa178f650>] ? kvm_vcpu_compat_ioctl+0x460/0x460 [kvm] > [ 168.067485] [<ffffffffa1833152>] kvm_mmu_invalidate_zap_all_pages+0x302/0x650 [kvm] > [ 168.067535] [<ffffffffa17f599e>] kvm_arch_flush_shadow_memslot+0xe/0x10 [kvm] > [ 168.067581] [<ffffffffa179171f>] __kvm_set_memory_region+0x180f/0x27b0 [kvm] > [ 168.067587] [<ffffffff8293cf9e>] ? mutex_lock_nested+0x50e/0x970 > [ 168.067634] [<ffffffffa178ff10>] ? kvm_kvzalloc+0x30/0x30 [kvm] > [ 168.067640] [<ffffffff812e848b>] ? debug_lockdep_rcu_enabled+0x7b/0x90 > [ 168.067647] [<ffffffff8162accd>] ? __might_fault+0x12d/0x240 > [ 168.067693] [<ffffffffa17926ed>] kvm_set_memory_region+0x2d/0x50 [kvm] > [ 168.067738] [<ffffffffa1793022>] kvm_vm_ioctl+0x912/0x1610 [kvm] > [ 168.067784] [<ffffffffa1792710>] ? kvm_set_memory_region+0x50/0x50 [kvm] > [ 168.067790] [<ffffffff8129ee99>] ? __lock_acquire+0xb49/0x7260 > [ 168.067797] [<ffffffff812e848b>] ? debug_lockdep_rcu_enabled+0x7b/0x90 > [ 168.067803] [<ffffffff81659828>] ? page_add_new_anon_rmap+0x188/0x2d0 > [ 168.067810] [<ffffffff8129e350>] ? debug_check_no_locks_freed+0x350/0x350 > [ 168.067816] [<ffffffff82946087>] ? _raw_spin_unlock+0x27/0x40 > [ 168.067821] [<ffffffff8163a943>] ? handle_mm_fault+0x1673/0x2e40 > [ 168.067828] [<ffffffff812e848b>] ? debug_lockdep_rcu_enabled+0x7b/0x90 > [ 168.067834] [<ffffffff81725a80>] do_vfs_ioctl+0x1b0/0x12b0 > [ 168.067840] [<ffffffff817258d0>] ? ioctl_preallocate+0x210/0x210 > [ 168.067847] [<ffffffff8174aef3>] ? __fget+0x273/0x4a0 > [ 168.067852] [<ffffffff8174acd0>] ? __fget+0x50/0x4a0 > [ 168.067858] [<ffffffff8174b1f6>] ? __fget_light+0x96/0x2b0 > [ 168.067864] [<ffffffff81726bf9>] SyS_ioctl+0x79/0x90 > [ 168.067870] [<ffffffff82946880>] entry_SYSCALL_64_fastpath+0x23/0xc1 > [ 168.067875] ================================================================================ > > I'm not familiar with the code (and so i don't send a patch), but, AFAICS, > except the assignment in kvm_mmu_pages_init, which cases the warning above, > there is only one assignment to struct mmu_page_path parent field in the > mmu_pages_next which doesn't produce warnings, so i applied the following diff > and the warning gone: > > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > index 95a955d..cc673f1 100644 > --- a/arch/x86/kvm/mmu.c > +++ b/arch/x86/kvm/mmu.c > @@ -2008,7 +2008,7 @@ static void kvm_mmu_pages_init(struct kvm_mmu_page *parent, > struct mmu_page_path *parents, > struct kvm_mmu_pages *pvec) > { > - parents->parent[parent->role.level-1] = NULL; > + parents->parent[parent->role.level-2] = NULL; > pvec->nr = 0; > } > > > The second one is: > > [ 168.791851] ================================================================================ > [ 168.791862] UBSAN: Undefined behaviour in arch/x86/kvm/paging_tmpl.h:252:15 > [ 168.791866] index 4 is out of range for type 'u64 [4]' > [ 168.791871] CPU: 0 PID: 2950 Comm: qemu-system-x86 Tainted: G O L 4.5.0-rc5-next-20160222 #7 > [ 168.791873] Hardware name: LENOVO 23205NG/23205NG, BIOS G2ET95WW (2.55 ) 07/09/2013 > [ 168.791876] 0000000000000000 ffff8801cfcaf208 ffffffff81c9f780 0000000041b58ab3 > [ 168.791882] ffffffff82eb2cc1 ffffffff81c9f6b4 ffff8801cfcaf230 ffff8801cfcaf1e0 > [ 168.791886] 0000000000000004 0000000000000001 0000000000000000 ffffffffa1981600 > [ 168.791891] Call Trace: > [ 168.791899] [<ffffffff81c9f780>] dump_stack+0xcc/0x12c > [ 168.791904] [<ffffffff81c9f6b4>] ? _atomic_dec_and_lock+0xc4/0xc4 > [ 168.791910] [<ffffffff81da9e81>] ubsan_epilogue+0xd/0x8a > [ 168.791914] [<ffffffff81daafa2>] __ubsan_handle_out_of_bounds+0x15c/0x1a3 > [ 168.791918] [<ffffffff81daae46>] ? __ubsan_handle_shift_out_of_bounds+0x2bd/0x2bd > [ 168.791922] [<ffffffff811287ef>] ? get_user_pages_fast+0x2bf/0x360 > [ 168.791954] [<ffffffffa1794050>] ? kvm_largepages_enabled+0x30/0x30 [kvm] > [ 168.791958] [<ffffffff81128530>] ? __get_user_pages_fast+0x360/0x360 > [ 168.791987] [<ffffffffa181b818>] paging64_walk_addr_generic+0x1b28/0x2600 [kvm] > [ 168.792014] [<ffffffffa1819cf0>] ? init_kvm_mmu+0x1100/0x1100 [kvm] > [ 168.792019] [<ffffffff8129e350>] ? debug_check_no_locks_freed+0x350/0x350 > [ 168.792044] [<ffffffffa1819cf0>] ? init_kvm_mmu+0x1100/0x1100 [kvm] > [ 168.792076] [<ffffffffa181c36d>] paging64_gva_to_gpa+0x7d/0x110 [kvm] > [ 168.792121] [<ffffffffa181c2f0>] ? paging64_walk_addr_generic+0x2600/0x2600 [kvm] > [ 168.792130] [<ffffffff812e848b>] ? debug_lockdep_rcu_enabled+0x7b/0x90 > [ 168.792178] [<ffffffffa17d9a4a>] emulator_read_write_onepage+0x27a/0x1150 [kvm] > [ 168.792208] [<ffffffffa1794d44>] ? __kvm_read_guest_page+0x54/0x70 [kvm] > [ 168.792234] [<ffffffffa17d97d0>] ? kvm_task_switch+0x160/0x160 [kvm] > [ 168.792238] [<ffffffff812e848b>] ? debug_lockdep_rcu_enabled+0x7b/0x90 > [ 168.792263] [<ffffffffa17daa07>] emulator_read_write+0xe7/0x6d0 [kvm] > [ 168.792290] [<ffffffffa183b620>] ? em_cr_write+0x230/0x230 [kvm] > [ 168.792314] [<ffffffffa17db005>] emulator_write_emulated+0x15/0x20 [kvm] > [ 168.792340] [<ffffffffa18465f8>] segmented_write+0xf8/0x130 [kvm] > [ 168.792367] [<ffffffffa1846500>] ? em_lgdt+0x20/0x20 [kvm] > [ 168.792374] [<ffffffffa14db512>] ? vmx_read_guest_seg_ar+0x42/0x1e0 [kvm_intel] > [ 168.792400] [<ffffffffa1846d82>] writeback+0x3f2/0x700 [kvm] > [ 168.792424] [<ffffffffa1846990>] ? em_sidt+0xa0/0xa0 [kvm] > [ 168.792449] [<ffffffffa185554d>] ? x86_decode_insn+0x1b3d/0x4f70 [kvm] > [ 168.792474] [<ffffffffa1859032>] x86_emulate_insn+0x572/0x3010 [kvm] > [ 168.792499] [<ffffffffa17e71dd>] x86_emulate_instruction+0x3bd/0x2110 [kvm] > [ 168.792524] [<ffffffffa17e6e20>] ? reexecute_instruction.part.110+0x2e0/0x2e0 [kvm] > [ 168.792532] [<ffffffffa14e9a81>] handle_ept_misconfig+0x61/0x460 [kvm_intel] > [ 168.792539] [<ffffffffa14e9a20>] ? handle_pause+0x450/0x450 [kvm_intel] > [ 168.792546] [<ffffffffa15130ea>] vmx_handle_exit+0xd6a/0x1ad0 [kvm_intel] > [ 168.792572] [<ffffffffa17f6a6c>] ? kvm_arch_vcpu_ioctl_run+0xbdc/0x6090 [kvm] > [ 168.792597] [<ffffffffa17f6bcd>] kvm_arch_vcpu_ioctl_run+0xd3d/0x6090 [kvm] > [ 168.792621] [<ffffffffa17f6a6c>] ? kvm_arch_vcpu_ioctl_run+0xbdc/0x6090 [kvm] > [ 168.792627] [<ffffffff8293b530>] ? __ww_mutex_lock_interruptible+0x1630/0x1630 > [ 168.792651] [<ffffffffa17f5e90>] ? kvm_arch_vcpu_runnable+0x4f0/0x4f0 [kvm] > [ 168.792656] [<ffffffff811eeb30>] ? preempt_notifier_unregister+0x190/0x190 > [ 168.792681] [<ffffffffa17e0447>] ? kvm_arch_vcpu_load+0x127/0x650 [kvm] > [ 168.792704] [<ffffffffa178e9a3>] kvm_vcpu_ioctl+0x553/0xda0 [kvm] > [ 168.792727] [<ffffffffa178e450>] ? vcpu_put+0x40/0x40 [kvm] > [ 168.792732] [<ffffffff8129e350>] ? debug_check_no_locks_freed+0x350/0x350 > [ 168.792735] [<ffffffff82946087>] ? _raw_spin_unlock+0x27/0x40 > [ 168.792740] [<ffffffff8163a943>] ? handle_mm_fault+0x1673/0x2e40 > [ 168.792744] [<ffffffff8129daa8>] ? trace_hardirqs_on_caller+0x478/0x6c0 > [ 168.792747] [<ffffffff8129dcfd>] ? trace_hardirqs_on+0xd/0x10 > [ 168.792751] [<ffffffff812e848b>] ? debug_lockdep_rcu_enabled+0x7b/0x90 > [ 168.792756] [<ffffffff81725a80>] do_vfs_ioctl+0x1b0/0x12b0 > [ 168.792759] [<ffffffff817258d0>] ? ioctl_preallocate+0x210/0x210 > [ 168.792763] [<ffffffff8174aef3>] ? __fget+0x273/0x4a0 > [ 168.792766] [<ffffffff8174acd0>] ? __fget+0x50/0x4a0 > [ 168.792770] [<ffffffff8174b1f6>] ? __fget_light+0x96/0x2b0 > [ 168.792773] [<ffffffff81726bf9>] SyS_ioctl+0x79/0x90 > [ 168.792777] [<ffffffff82946880>] entry_SYSCALL_64_fastpath+0x23/0xc1 > [ 168.792780] ================================================================================ > > the following fix solved problem for me: > > diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h > index 6c9fed9..2ce4f05 100644 > --- a/arch/x86/kvm/paging_tmpl.h > +++ b/arch/x86/kvm/paging_tmpl.h > @@ -249,7 +249,7 @@ static int FNAME(update_accessed_dirty_bits)(struct kvm_vcpu *vcpu, > return ret; > > kvm_vcpu_mark_page_dirty(vcpu, table_gfn); > - walker->ptes[level] = pte; > + walker->ptes[level - 1] = pte; > } > return 0; > } CC'ing some KVM people with MMU background. Jan -- Siemens AG, Corporate Technology, CT RDA ITP SES-DE Corporate Competence Center Embedded Linux ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: index-out-of-range ubsan warnings 2016-02-23 10:07 ` Jan Kiszka @ 2016-02-23 10:44 ` Xiao Guangrong 2016-02-23 11:13 ` Mike Krinkin 2016-02-23 13:21 ` Paolo Bonzini 0 siblings, 2 replies; 8+ messages in thread From: Xiao Guangrong @ 2016-02-23 10:44 UTC (permalink / raw) To: Jan Kiszka, Mike Krinkin, kvm; +Cc: Marcelo Tosatti, Paolo Bonzini Mike, thank you for the report. On 02/23/2016 06:07 PM, Jan Kiszka wrote: > On 2016-02-23 10:26, Mike Krinkin wrote: >> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c >> index 95a955d..cc673f1 100644 >> --- a/arch/x86/kvm/mmu.c >> +++ b/arch/x86/kvm/mmu.c >> @@ -2008,7 +2008,7 @@ static void kvm_mmu_pages_init(struct kvm_mmu_page *parent, >> struct mmu_page_path *parents, >> struct kvm_mmu_pages *pvec) >> { >> - parents->parent[parent->role.level-1] = NULL; >> + parents->parent[parent->role.level-2] = NULL; >> pvec->nr = 0; >> } >> This is a known issue and i attached a patch to fix it, please refer to: https://groups.google.com/forum/#!msg/syzkaller/4wAzRPswgQ8/sCMTx9RQFQAJ Could you please try it? >> >> The second one is: >> >> [ 168.791851] ================================================================================ >> [ 168.791862] UBSAN: Undefined behaviour in arch/x86/kvm/paging_tmpl.h:252:15 >> [ 168.791866] index 4 is out of range for type 'u64 [4]' >> [ 168.791871] CPU: 0 PID: 2950 Comm: qemu-system-x86 Tainted: G O L 4.5.0-rc5-next-20160222 #7 >> [ 168.791873] Hardware name: LENOVO 23205NG/23205NG, BIOS G2ET95WW (2.55 ) 07/09/2013 >> [ 168.791876] 0000000000000000 ffff8801cfcaf208 ffffffff81c9f780 0000000041b58ab3 >> [ 168.791882] ffffffff82eb2cc1 ffffffff81c9f6b4 ffff8801cfcaf230 ffff8801cfcaf1e0 >> [ 168.791886] 0000000000000004 0000000000000001 0000000000000000 ffffffffa1981600 >> [ 168.791891] Call Trace: >> [ 168.791899] [<ffffffff81c9f780>] dump_stack+0xcc/0x12c >> [ 168.791904] [<ffffffff81c9f6b4>] ? _atomic_dec_and_lock+0xc4/0xc4 >> [ 168.791910] [<ffffffff81da9e81>] ubsan_epilogue+0xd/0x8a >> [ 168.791914] [<ffffffff81daafa2>] __ubsan_handle_out_of_bounds+0x15c/0x1a3 >> [ 168.791918] [<ffffffff81daae46>] ? __ubsan_handle_shift_out_of_bounds+0x2bd/0x2bd >> [ 168.791922] [<ffffffff811287ef>] ? get_user_pages_fast+0x2bf/0x360 >> [ 168.791954] [<ffffffffa1794050>] ? kvm_largepages_enabled+0x30/0x30 [kvm] >> [ 168.791958] [<ffffffff81128530>] ? __get_user_pages_fast+0x360/0x360 >> [ 168.791987] [<ffffffffa181b818>] paging64_walk_addr_generic+0x1b28/0x2600 [kvm] >> [ 168.792014] [<ffffffffa1819cf0>] ? init_kvm_mmu+0x1100/0x1100 [kvm] >> [ 168.792019] [<ffffffff8129e350>] ? debug_check_no_locks_freed+0x350/0x350 >> [ 168.792044] [<ffffffffa1819cf0>] ? init_kvm_mmu+0x1100/0x1100 [kvm] >> [ 168.792076] [<ffffffffa181c36d>] paging64_gva_to_gpa+0x7d/0x110 [kvm] >> [ 168.792121] [<ffffffffa181c2f0>] ? paging64_walk_addr_generic+0x2600/0x2600 [kvm] >> [ 168.792130] [<ffffffff812e848b>] ? debug_lockdep_rcu_enabled+0x7b/0x90 >> [ 168.792178] [<ffffffffa17d9a4a>] emulator_read_write_onepage+0x27a/0x1150 [kvm] >> [ 168.792208] [<ffffffffa1794d44>] ? __kvm_read_guest_page+0x54/0x70 [kvm] >> [ 168.792234] [<ffffffffa17d97d0>] ? kvm_task_switch+0x160/0x160 [kvm] >> [ 168.792238] [<ffffffff812e848b>] ? debug_lockdep_rcu_enabled+0x7b/0x90 >> [ 168.792263] [<ffffffffa17daa07>] emulator_read_write+0xe7/0x6d0 [kvm] >> [ 168.792290] [<ffffffffa183b620>] ? em_cr_write+0x230/0x230 [kvm] >> [ 168.792314] [<ffffffffa17db005>] emulator_write_emulated+0x15/0x20 [kvm] >> [ 168.792340] [<ffffffffa18465f8>] segmented_write+0xf8/0x130 [kvm] >> [ 168.792367] [<ffffffffa1846500>] ? em_lgdt+0x20/0x20 [kvm] >> [ 168.792374] [<ffffffffa14db512>] ? vmx_read_guest_seg_ar+0x42/0x1e0 [kvm_intel] >> [ 168.792400] [<ffffffffa1846d82>] writeback+0x3f2/0x700 [kvm] >> [ 168.792424] [<ffffffffa1846990>] ? em_sidt+0xa0/0xa0 [kvm] >> [ 168.792449] [<ffffffffa185554d>] ? x86_decode_insn+0x1b3d/0x4f70 [kvm] >> [ 168.792474] [<ffffffffa1859032>] x86_emulate_insn+0x572/0x3010 [kvm] >> [ 168.792499] [<ffffffffa17e71dd>] x86_emulate_instruction+0x3bd/0x2110 [kvm] >> [ 168.792524] [<ffffffffa17e6e20>] ? reexecute_instruction.part.110+0x2e0/0x2e0 [kvm] >> [ 168.792532] [<ffffffffa14e9a81>] handle_ept_misconfig+0x61/0x460 [kvm_intel] >> [ 168.792539] [<ffffffffa14e9a20>] ? handle_pause+0x450/0x450 [kvm_intel] >> [ 168.792546] [<ffffffffa15130ea>] vmx_handle_exit+0xd6a/0x1ad0 [kvm_intel] >> [ 168.792572] [<ffffffffa17f6a6c>] ? kvm_arch_vcpu_ioctl_run+0xbdc/0x6090 [kvm] >> [ 168.792597] [<ffffffffa17f6bcd>] kvm_arch_vcpu_ioctl_run+0xd3d/0x6090 [kvm] >> [ 168.792621] [<ffffffffa17f6a6c>] ? kvm_arch_vcpu_ioctl_run+0xbdc/0x6090 [kvm] >> [ 168.792627] [<ffffffff8293b530>] ? __ww_mutex_lock_interruptible+0x1630/0x1630 >> [ 168.792651] [<ffffffffa17f5e90>] ? kvm_arch_vcpu_runnable+0x4f0/0x4f0 [kvm] >> [ 168.792656] [<ffffffff811eeb30>] ? preempt_notifier_unregister+0x190/0x190 >> [ 168.792681] [<ffffffffa17e0447>] ? kvm_arch_vcpu_load+0x127/0x650 [kvm] >> [ 168.792704] [<ffffffffa178e9a3>] kvm_vcpu_ioctl+0x553/0xda0 [kvm] >> [ 168.792727] [<ffffffffa178e450>] ? vcpu_put+0x40/0x40 [kvm] >> [ 168.792732] [<ffffffff8129e350>] ? debug_check_no_locks_freed+0x350/0x350 >> [ 168.792735] [<ffffffff82946087>] ? _raw_spin_unlock+0x27/0x40 >> [ 168.792740] [<ffffffff8163a943>] ? handle_mm_fault+0x1673/0x2e40 >> [ 168.792744] [<ffffffff8129daa8>] ? trace_hardirqs_on_caller+0x478/0x6c0 >> [ 168.792747] [<ffffffff8129dcfd>] ? trace_hardirqs_on+0xd/0x10 >> [ 168.792751] [<ffffffff812e848b>] ? debug_lockdep_rcu_enabled+0x7b/0x90 >> [ 168.792756] [<ffffffff81725a80>] do_vfs_ioctl+0x1b0/0x12b0 >> [ 168.792759] [<ffffffff817258d0>] ? ioctl_preallocate+0x210/0x210 >> [ 168.792763] [<ffffffff8174aef3>] ? __fget+0x273/0x4a0 >> [ 168.792766] [<ffffffff8174acd0>] ? __fget+0x50/0x4a0 >> [ 168.792770] [<ffffffff8174b1f6>] ? __fget_light+0x96/0x2b0 >> [ 168.792773] [<ffffffff81726bf9>] SyS_ioctl+0x79/0x90 >> [ 168.792777] [<ffffffff82946880>] entry_SYSCALL_64_fastpath+0x23/0xc1 >> [ 168.792780] ================================================================================ >> >> the following fix solved problem for me: >> >> diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h >> index 6c9fed9..2ce4f05 100644 >> --- a/arch/x86/kvm/paging_tmpl.h >> +++ b/arch/x86/kvm/paging_tmpl.h >> @@ -249,7 +249,7 @@ static int FNAME(update_accessed_dirty_bits)(struct kvm_vcpu *vcpu, >> return ret; >> >> kvm_vcpu_mark_page_dirty(vcpu, table_gfn); >> - walker->ptes[level] = pte; >> + walker->ptes[level - 1] = pte; >> } >> return 0; >> } > This fix looks correct to me. > CC'ing some KVM people with MMU background. Thank you, Jan! ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: index-out-of-range ubsan warnings 2016-02-23 10:44 ` Xiao Guangrong @ 2016-02-23 11:13 ` Mike Krinkin 2016-02-23 13:21 ` Paolo Bonzini 1 sibling, 0 replies; 8+ messages in thread From: Mike Krinkin @ 2016-02-23 11:13 UTC (permalink / raw) To: Xiao Guangrong; +Cc: Jan Kiszka, kvm, Marcelo Tosatti, Paolo Bonzini On Tue, Feb 23, 2016 at 06:44:03PM +0800, Xiao Guangrong wrote: > > Mike, thank you for the report. > > On 02/23/2016 06:07 PM, Jan Kiszka wrote: > >On 2016-02-23 10:26, Mike Krinkin wrote: > > >>diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > >>index 95a955d..cc673f1 100644 > >>--- a/arch/x86/kvm/mmu.c > >>+++ b/arch/x86/kvm/mmu.c > >>@@ -2008,7 +2008,7 @@ static void kvm_mmu_pages_init(struct kvm_mmu_page *parent, > >> struct mmu_page_path *parents, > >> struct kvm_mmu_pages *pvec) > >> { > >>- parents->parent[parent->role.level-1] = NULL; > >>+ parents->parent[parent->role.level-2] = NULL; > >> pvec->nr = 0; > >> } > >> > > This is a known issue and i attached a patch to fix it, please refer to: > https://groups.google.com/forum/#!msg/syzkaller/4wAzRPswgQ8/sCMTx9RQFQAJ Oh... Sorry, i should have search through archive more carefully. > > Could you please try it? It works too. > > >> > >>The second one is: > >> > >>[ 168.791851] ================================================================================ > >>[ 168.791862] UBSAN: Undefined behaviour in arch/x86/kvm/paging_tmpl.h:252:15 > >>[ 168.791866] index 4 is out of range for type 'u64 [4]' > >>[ 168.791871] CPU: 0 PID: 2950 Comm: qemu-system-x86 Tainted: G O L 4.5.0-rc5-next-20160222 #7 > >>[ 168.791873] Hardware name: LENOVO 23205NG/23205NG, BIOS G2ET95WW (2.55 ) 07/09/2013 > >>[ 168.791876] 0000000000000000 ffff8801cfcaf208 ffffffff81c9f780 0000000041b58ab3 > >>[ 168.791882] ffffffff82eb2cc1 ffffffff81c9f6b4 ffff8801cfcaf230 ffff8801cfcaf1e0 > >>[ 168.791886] 0000000000000004 0000000000000001 0000000000000000 ffffffffa1981600 > >>[ 168.791891] Call Trace: > >>[ 168.791899] [<ffffffff81c9f780>] dump_stack+0xcc/0x12c > >>[ 168.791904] [<ffffffff81c9f6b4>] ? _atomic_dec_and_lock+0xc4/0xc4 > >>[ 168.791910] [<ffffffff81da9e81>] ubsan_epilogue+0xd/0x8a > >>[ 168.791914] [<ffffffff81daafa2>] __ubsan_handle_out_of_bounds+0x15c/0x1a3 > >>[ 168.791918] [<ffffffff81daae46>] ? __ubsan_handle_shift_out_of_bounds+0x2bd/0x2bd > >>[ 168.791922] [<ffffffff811287ef>] ? get_user_pages_fast+0x2bf/0x360 > >>[ 168.791954] [<ffffffffa1794050>] ? kvm_largepages_enabled+0x30/0x30 [kvm] > >>[ 168.791958] [<ffffffff81128530>] ? __get_user_pages_fast+0x360/0x360 > >>[ 168.791987] [<ffffffffa181b818>] paging64_walk_addr_generic+0x1b28/0x2600 [kvm] > >>[ 168.792014] [<ffffffffa1819cf0>] ? init_kvm_mmu+0x1100/0x1100 [kvm] > >>[ 168.792019] [<ffffffff8129e350>] ? debug_check_no_locks_freed+0x350/0x350 > >>[ 168.792044] [<ffffffffa1819cf0>] ? init_kvm_mmu+0x1100/0x1100 [kvm] > >>[ 168.792076] [<ffffffffa181c36d>] paging64_gva_to_gpa+0x7d/0x110 [kvm] > >>[ 168.792121] [<ffffffffa181c2f0>] ? paging64_walk_addr_generic+0x2600/0x2600 [kvm] > >>[ 168.792130] [<ffffffff812e848b>] ? debug_lockdep_rcu_enabled+0x7b/0x90 > >>[ 168.792178] [<ffffffffa17d9a4a>] emulator_read_write_onepage+0x27a/0x1150 [kvm] > >>[ 168.792208] [<ffffffffa1794d44>] ? __kvm_read_guest_page+0x54/0x70 [kvm] > >>[ 168.792234] [<ffffffffa17d97d0>] ? kvm_task_switch+0x160/0x160 [kvm] > >>[ 168.792238] [<ffffffff812e848b>] ? debug_lockdep_rcu_enabled+0x7b/0x90 > >>[ 168.792263] [<ffffffffa17daa07>] emulator_read_write+0xe7/0x6d0 [kvm] > >>[ 168.792290] [<ffffffffa183b620>] ? em_cr_write+0x230/0x230 [kvm] > >>[ 168.792314] [<ffffffffa17db005>] emulator_write_emulated+0x15/0x20 [kvm] > >>[ 168.792340] [<ffffffffa18465f8>] segmented_write+0xf8/0x130 [kvm] > >>[ 168.792367] [<ffffffffa1846500>] ? em_lgdt+0x20/0x20 [kvm] > >>[ 168.792374] [<ffffffffa14db512>] ? vmx_read_guest_seg_ar+0x42/0x1e0 [kvm_intel] > >>[ 168.792400] [<ffffffffa1846d82>] writeback+0x3f2/0x700 [kvm] > >>[ 168.792424] [<ffffffffa1846990>] ? em_sidt+0xa0/0xa0 [kvm] > >>[ 168.792449] [<ffffffffa185554d>] ? x86_decode_insn+0x1b3d/0x4f70 [kvm] > >>[ 168.792474] [<ffffffffa1859032>] x86_emulate_insn+0x572/0x3010 [kvm] > >>[ 168.792499] [<ffffffffa17e71dd>] x86_emulate_instruction+0x3bd/0x2110 [kvm] > >>[ 168.792524] [<ffffffffa17e6e20>] ? reexecute_instruction.part.110+0x2e0/0x2e0 [kvm] > >>[ 168.792532] [<ffffffffa14e9a81>] handle_ept_misconfig+0x61/0x460 [kvm_intel] > >>[ 168.792539] [<ffffffffa14e9a20>] ? handle_pause+0x450/0x450 [kvm_intel] > >>[ 168.792546] [<ffffffffa15130ea>] vmx_handle_exit+0xd6a/0x1ad0 [kvm_intel] > >>[ 168.792572] [<ffffffffa17f6a6c>] ? kvm_arch_vcpu_ioctl_run+0xbdc/0x6090 [kvm] > >>[ 168.792597] [<ffffffffa17f6bcd>] kvm_arch_vcpu_ioctl_run+0xd3d/0x6090 [kvm] > >>[ 168.792621] [<ffffffffa17f6a6c>] ? kvm_arch_vcpu_ioctl_run+0xbdc/0x6090 [kvm] > >>[ 168.792627] [<ffffffff8293b530>] ? __ww_mutex_lock_interruptible+0x1630/0x1630 > >>[ 168.792651] [<ffffffffa17f5e90>] ? kvm_arch_vcpu_runnable+0x4f0/0x4f0 [kvm] > >>[ 168.792656] [<ffffffff811eeb30>] ? preempt_notifier_unregister+0x190/0x190 > >>[ 168.792681] [<ffffffffa17e0447>] ? kvm_arch_vcpu_load+0x127/0x650 [kvm] > >>[ 168.792704] [<ffffffffa178e9a3>] kvm_vcpu_ioctl+0x553/0xda0 [kvm] > >>[ 168.792727] [<ffffffffa178e450>] ? vcpu_put+0x40/0x40 [kvm] > >>[ 168.792732] [<ffffffff8129e350>] ? debug_check_no_locks_freed+0x350/0x350 > >>[ 168.792735] [<ffffffff82946087>] ? _raw_spin_unlock+0x27/0x40 > >>[ 168.792740] [<ffffffff8163a943>] ? handle_mm_fault+0x1673/0x2e40 > >>[ 168.792744] [<ffffffff8129daa8>] ? trace_hardirqs_on_caller+0x478/0x6c0 > >>[ 168.792747] [<ffffffff8129dcfd>] ? trace_hardirqs_on+0xd/0x10 > >>[ 168.792751] [<ffffffff812e848b>] ? debug_lockdep_rcu_enabled+0x7b/0x90 > >>[ 168.792756] [<ffffffff81725a80>] do_vfs_ioctl+0x1b0/0x12b0 > >>[ 168.792759] [<ffffffff817258d0>] ? ioctl_preallocate+0x210/0x210 > >>[ 168.792763] [<ffffffff8174aef3>] ? __fget+0x273/0x4a0 > >>[ 168.792766] [<ffffffff8174acd0>] ? __fget+0x50/0x4a0 > >>[ 168.792770] [<ffffffff8174b1f6>] ? __fget_light+0x96/0x2b0 > >>[ 168.792773] [<ffffffff81726bf9>] SyS_ioctl+0x79/0x90 > >>[ 168.792777] [<ffffffff82946880>] entry_SYSCALL_64_fastpath+0x23/0xc1 > >>[ 168.792780] ================================================================================ > >> > >>the following fix solved problem for me: > >> > >>diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h > >>index 6c9fed9..2ce4f05 100644 > >>--- a/arch/x86/kvm/paging_tmpl.h > >>+++ b/arch/x86/kvm/paging_tmpl.h > >>@@ -249,7 +249,7 @@ static int FNAME(update_accessed_dirty_bits)(struct kvm_vcpu *vcpu, > >> return ret; > >> > >> kvm_vcpu_mark_page_dirty(vcpu, table_gfn); > >>- walker->ptes[level] = pte; > >>+ walker->ptes[level - 1] = pte; > >> } > >> return 0; > >> } > > > > This fix looks correct to me. > > >CC'ing some KVM people with MMU background. > > Thank you, Jan! > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: index-out-of-range ubsan warnings 2016-02-23 10:44 ` Xiao Guangrong 2016-02-23 11:13 ` Mike Krinkin @ 2016-02-23 13:21 ` Paolo Bonzini 2016-02-23 13:56 ` Mike Krinkin 2016-02-24 6:23 ` Xiao Guangrong 1 sibling, 2 replies; 8+ messages in thread From: Paolo Bonzini @ 2016-02-23 13:21 UTC (permalink / raw) To: Xiao Guangrong, Jan Kiszka, Mike Krinkin, kvm Cc: Marcelo Tosatti, Sasha Levin > > +#define INVALID_INDEX (-1) > + > static int mmu_unsync_walk(struct kvm_mmu_page *sp, > struct kvm_mmu_pages *pvec) > { > if (!sp->unsync_children) > return 0; > > - mmu_pages_add(pvec, sp, 0); > + /* > + * do not count the index in the parent of the sp we're > + * walking start from. > + */ > + mmu_pages_add(pvec, sp, INVALID_INDEX); > return __mmu_unsync_walk(sp, pvec); > } > > @@ -1980,8 +1986,11 @@ static int mmu_pages_next(struct kvm_mmu_pages *pvec, > return n; > } > > - parents->parent[sp->role.level-2] = sp; > - parents->idx[sp->role.level-1] = pvec->page[n].idx; > + parents->parent[sp->role.level - 2] = sp; > + > + /* skip setting idex of the sp we start from. */ > + if (pvec->page[n].idx != INVALID_INDEX) > + parents->idx[sp->role.level - 1] = pvec->page[n].idx; So far so good. > } > > return n; > @@ -1999,6 +2008,7 @@ static void mmu_pages_clear_parents(struct mmu_page_path *parents) > if (!sp) > return; > > + WARN_ON(idx != INVALID_INDEX); Shouldn't this warn if idx is *equal* to INVALID_INDEX? > clear_unsync_child_bit(sp, idx); > level++; > } while (level < PT64_ROOT_LEVEL-1 && !sp->unsync_children); > @@ -2008,7 +2018,7 @@ static void kvm_mmu_pages_init(struct kvm_mmu_page *parent, > struct mmu_page_path *parents, > struct kvm_mmu_pages *pvec) > { > - parents->parent[parent->role.level-1] = NULL; > + parents->parent[parent->role.level - 2] = NULL; This is meant to stop mmu_pages_clear_parents _after_ it has processed sp, so the "-1" is correct. The right fix would be: if (parent->role.level < PT64_ROOT_LEVEL-1) parents->parent[parent->role.level - 1] = NULL; and no one has noticed it so far because if the bug hits you just write over the beginning of ->idx (which so far is uninitialized) and nothing bad happens. I'm thinking of the following (untested) patch instead: -------- 8< -------------- From: Paolo Bonzini <pbonzini@redhat.com> Subject: [PATCH] KVM: MMU: Fix ubsan warnings kvm_mmu_pages_init is doing some really yucky stuff. It is setting up a sentinel for mmu_page_clear_parents; however, because of a) the way levels are numbered starting from 1 and b) the way mmu_page_path sizes its arrays with PT64_ROOT_LEVEL-1 elements, the access can be out of bounds. This is harmless because the code overwrites up to the first two elements of parents->idx and these are initialized, and because the sentinel is not needed in this case---mmu_page_clear_parents exits anyway when it gets to the end of the array. However ubsan complains, and everyone else should too. This fix does three things. First it makes the mmu_page_path arrays PT64_ROOT_LEVEL elements in size, so that we can write to them without checking the level in advance. Second it disintegrates kvm_mmu_pages_init between mmu_unsync_walk (which resets struct kvm_mmu_pages) and mmu_pages_next (which unconditionally places a NULL sentinel at the end of the current path). This is okay because the mmu_page_path is only used in mmu_pages_clear_parents; mmu_pages_clear_parents itself is called within a for_each_sp iterator, and hence always after a call to mmu_pages_next. Third it changes mmu_pages_clear_parents to just use the sentinel to stop iteration, without checking the bounds on level. Reported-by: Sasha Levin <sasha.levin@oracle.com> Reported-by: Mike Krinkin <krinkin.m.u@gmail.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c index 33cc9f3f5b02..f4f5e7ca14dd 100644 --- a/arch/x86/kvm/mmu.c +++ b/arch/x86/kvm/mmu.c @@ -1843,6 +1843,7 @@ static int __mmu_unsync_walk(struct kvm_mmu_page *sp, static int mmu_unsync_walk(struct kvm_mmu_page *sp, struct kvm_mmu_pages *pvec) { + pvec->nr = 0; if (!sp->unsync_children) return 0; @@ -1956,8 +1957,8 @@ static void kvm_sync_pages(struct kvm_vcpu *vcpu, gfn_t gfn) } struct mmu_page_path { - struct kvm_mmu_page *parent[PT64_ROOT_LEVEL-1]; - unsigned int idx[PT64_ROOT_LEVEL-1]; + struct kvm_mmu_page *parent[PT64_ROOT_LEVEL]; + unsigned int idx[PT64_ROOT_LEVEL]; }; #define for_each_sp(pvec, sp, parents, i) \ @@ -1971,19 +1972,21 @@ static int mmu_pages_next(struct kvm_mmu_pages *pvec, int i) { int n; + int level = PT_PAGE_TABLE_LEVEL; for (n = i+1; n < pvec->nr; n++) { struct kvm_mmu_page *sp = pvec->page[n].sp; - if (sp->role.level == PT_PAGE_TABLE_LEVEL) { - parents->idx[0] = pvec->page[n].idx; - return n; - } + level = sp->role.level; + parents->idx[level-1] = pvec->page[n].idx; - parents->parent[sp->role.level-2] = sp; - parents->idx[sp->role.level-1] = pvec->page[n].idx; + if (level == PT_PAGE_TABLE_LEVEL) + break; + + parents->parent[level-2] = sp; } + parents->parent[level-1] = NULL; return n; } @@ -1994,22 +1996,13 @@ static void mmu_pages_clear_parents(struct mmu_page_path *parents) do { unsigned int idx = parents->idx[level]; - sp = parents->parent[level]; if (!sp) return; clear_unsync_child_bit(sp, idx); level++; - } while (level < PT64_ROOT_LEVEL-1 && !sp->unsync_children); -} - -static void kvm_mmu_pages_init(struct kvm_mmu_page *parent, - struct mmu_page_path *parents, - struct kvm_mmu_pages *pvec) -{ - parents->parent[parent->role.level-1] = NULL; - pvec->nr = 0; + } while (!sp->unsync_children); } static void mmu_sync_children(struct kvm_vcpu *vcpu, @@ -2021,7 +2014,6 @@ static void mmu_sync_children(struct kvm_vcpu *vcpu, struct kvm_mmu_pages pages; LIST_HEAD(invalid_list); - kvm_mmu_pages_init(parent, &parents, &pages); while (mmu_unsync_walk(parent, &pages)) { bool protected = false; @@ -2037,7 +2029,6 @@ static void mmu_sync_children(struct kvm_vcpu *vcpu, } kvm_mmu_commit_zap_page(vcpu->kvm, &invalid_list); cond_resched_lock(&vcpu->kvm->mmu_lock); - kvm_mmu_pages_init(parent, &parents, &pages); } } @@ -2269,7 +2260,6 @@ static int mmu_zap_unsync_children(struct kvm *kvm, if (parent->role.level == PT_PAGE_TABLE_LEVEL) return 0; - kvm_mmu_pages_init(parent, &parents, &pages); while (mmu_unsync_walk(parent, &pages)) { struct kvm_mmu_page *sp; @@ -2278,7 +2268,6 @@ static int mmu_zap_unsync_children(struct kvm *kvm, mmu_pages_clear_parents(&parents); zapped++; } - kvm_mmu_pages_init(parent, &parents, &pages); } return zapped; Paolo ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: index-out-of-range ubsan warnings 2016-02-23 13:21 ` Paolo Bonzini @ 2016-02-23 13:56 ` Mike Krinkin 2016-02-24 6:23 ` Xiao Guangrong 1 sibling, 0 replies; 8+ messages in thread From: Mike Krinkin @ 2016-02-23 13:56 UTC (permalink / raw) To: Paolo Bonzini Cc: Xiao Guangrong, Jan Kiszka, kvm, Marcelo Tosatti, Sasha Levin On Tue, Feb 23, 2016 at 02:21:07PM +0100, Paolo Bonzini wrote: > > > > +#define INVALID_INDEX (-1) > > + > > static int mmu_unsync_walk(struct kvm_mmu_page *sp, > > struct kvm_mmu_pages *pvec) > > { > > if (!sp->unsync_children) > > return 0; > > > > - mmu_pages_add(pvec, sp, 0); > > + /* > > + * do not count the index in the parent of the sp we're > > + * walking start from. > > + */ > > + mmu_pages_add(pvec, sp, INVALID_INDEX); > > return __mmu_unsync_walk(sp, pvec); > > } > > > > @@ -1980,8 +1986,11 @@ static int mmu_pages_next(struct kvm_mmu_pages *pvec, > > return n; > > } > > > > - parents->parent[sp->role.level-2] = sp; > > - parents->idx[sp->role.level-1] = pvec->page[n].idx; > > + parents->parent[sp->role.level - 2] = sp; > > + > > + /* skip setting idex of the sp we start from. */ > > + if (pvec->page[n].idx != INVALID_INDEX) > > + parents->idx[sp->role.level - 1] = pvec->page[n].idx; > > So far so good. > > > } > > > > return n; > > @@ -1999,6 +2008,7 @@ static void mmu_pages_clear_parents(struct mmu_page_path *parents) > > if (!sp) > > return; > > > > + WARN_ON(idx != INVALID_INDEX); > > Shouldn't this warn if idx is *equal* to INVALID_INDEX? > > > clear_unsync_child_bit(sp, idx); > > level++; > > } while (level < PT64_ROOT_LEVEL-1 && !sp->unsync_children); > > @@ -2008,7 +2018,7 @@ static void kvm_mmu_pages_init(struct kvm_mmu_page *parent, > > struct mmu_page_path *parents, > > struct kvm_mmu_pages *pvec) > > { > > - parents->parent[parent->role.level-1] = NULL; > > + parents->parent[parent->role.level - 2] = NULL; > > This is meant to stop mmu_pages_clear_parents _after_ it has > processed sp, so the "-1" is correct. The right fix would be: > > if (parent->role.level < PT64_ROOT_LEVEL-1) > parents->parent[parent->role.level - 1] = NULL; > > and no one has noticed it so far because if the bug hits you just write > over the beginning of ->idx (which so far is uninitialized) and nothing > bad happens. > > I'm thinking of the following (untested) patch instead: > > -------- 8< -------------- > From: Paolo Bonzini <pbonzini@redhat.com> > Subject: [PATCH] KVM: MMU: Fix ubsan warnings > > kvm_mmu_pages_init is doing some really yucky stuff. It is setting > up a sentinel for mmu_page_clear_parents; however, because of a) the > way levels are numbered starting from 1 and b) the way mmu_page_path > sizes its arrays with PT64_ROOT_LEVEL-1 elements, the access can be > out of bounds. This is harmless because the code overwrites up to the > first two elements of parents->idx and these are initialized, and > because the sentinel is not needed in this case---mmu_page_clear_parents > exits anyway when it gets to the end of the array. However ubsan > complains, and everyone else should too. > > This fix does three things. First it makes the mmu_page_path arrays > PT64_ROOT_LEVEL elements in size, so that we can write to them without > checking the level in advance. Second it disintegrates kvm_mmu_pages_init > between mmu_unsync_walk (which resets struct kvm_mmu_pages) and > mmu_pages_next (which unconditionally places a NULL sentinel at the > end of the current path). This is okay because the mmu_page_path is > only used in mmu_pages_clear_parents; mmu_pages_clear_parents itself > is called within a for_each_sp iterator, and hence always after a > call to mmu_pages_next. Third it changes mmu_pages_clear_parents to > just use the sentinel to stop iteration, without checking the bounds > on level. > > Reported-by: Sasha Levin <sasha.levin@oracle.com> > Reported-by: Mike Krinkin <krinkin.m.u@gmail.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > index 33cc9f3f5b02..f4f5e7ca14dd 100644 > --- a/arch/x86/kvm/mmu.c > +++ b/arch/x86/kvm/mmu.c > @@ -1843,6 +1843,7 @@ static int __mmu_unsync_walk(struct kvm_mmu_page *sp, > static int mmu_unsync_walk(struct kvm_mmu_page *sp, > struct kvm_mmu_pages *pvec) > { > + pvec->nr = 0; > if (!sp->unsync_children) > return 0; > > @@ -1956,8 +1957,8 @@ static void kvm_sync_pages(struct kvm_vcpu *vcpu, gfn_t gfn) > } > > struct mmu_page_path { > - struct kvm_mmu_page *parent[PT64_ROOT_LEVEL-1]; > - unsigned int idx[PT64_ROOT_LEVEL-1]; > + struct kvm_mmu_page *parent[PT64_ROOT_LEVEL]; > + unsigned int idx[PT64_ROOT_LEVEL]; > }; > > #define for_each_sp(pvec, sp, parents, i) \ > @@ -1971,19 +1972,21 @@ static int mmu_pages_next(struct kvm_mmu_pages *pvec, > int i) > { > int n; > + int level = PT_PAGE_TABLE_LEVEL; > > for (n = i+1; n < pvec->nr; n++) { > struct kvm_mmu_page *sp = pvec->page[n].sp; > > - if (sp->role.level == PT_PAGE_TABLE_LEVEL) { > - parents->idx[0] = pvec->page[n].idx; > - return n; > - } > + level = sp->role.level; > + parents->idx[level-1] = pvec->page[n].idx; > > - parents->parent[sp->role.level-2] = sp; > - parents->idx[sp->role.level-1] = pvec->page[n].idx; > + if (level == PT_PAGE_TABLE_LEVEL) > + break; > + > + parents->parent[level-2] = sp; > } > > + parents->parent[level-1] = NULL; > return n; > } > > @@ -1994,22 +1996,13 @@ static void mmu_pages_clear_parents(struct mmu_page_path *parents) > > do { > unsigned int idx = parents->idx[level]; > - > sp = parents->parent[level]; > if (!sp) > return; > > clear_unsync_child_bit(sp, idx); > level++; > - } while (level < PT64_ROOT_LEVEL-1 && !sp->unsync_children); > -} > - > -static void kvm_mmu_pages_init(struct kvm_mmu_page *parent, > - struct mmu_page_path *parents, > - struct kvm_mmu_pages *pvec) > -{ > - parents->parent[parent->role.level-1] = NULL; > - pvec->nr = 0; > + } while (!sp->unsync_children); > } > > static void mmu_sync_children(struct kvm_vcpu *vcpu, > @@ -2021,7 +2014,6 @@ static void mmu_sync_children(struct kvm_vcpu *vcpu, > struct kvm_mmu_pages pages; > LIST_HEAD(invalid_list); > > - kvm_mmu_pages_init(parent, &parents, &pages); > while (mmu_unsync_walk(parent, &pages)) { > bool protected = false; > > @@ -2037,7 +2029,6 @@ static void mmu_sync_children(struct kvm_vcpu *vcpu, > } > kvm_mmu_commit_zap_page(vcpu->kvm, &invalid_list); > cond_resched_lock(&vcpu->kvm->mmu_lock); > - kvm_mmu_pages_init(parent, &parents, &pages); > } > } > > @@ -2269,7 +2260,6 @@ static int mmu_zap_unsync_children(struct kvm *kvm, > if (parent->role.level == PT_PAGE_TABLE_LEVEL) > return 0; > > - kvm_mmu_pages_init(parent, &parents, &pages); > while (mmu_unsync_walk(parent, &pages)) { > struct kvm_mmu_page *sp; > > @@ -2278,7 +2268,6 @@ static int mmu_zap_unsync_children(struct kvm *kvm, > mmu_pages_clear_parents(&parents); > zapped++; > } > - kvm_mmu_pages_init(parent, &parents, &pages); > } > > return zapped; > > > Paolo > This one fix warning for me too, and i haven't noticed any problems, so i'd say Tested-by: Mike Krinkin <krinkin.m.u@gmail.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: index-out-of-range ubsan warnings 2016-02-23 13:21 ` Paolo Bonzini 2016-02-23 13:56 ` Mike Krinkin @ 2016-02-24 6:23 ` Xiao Guangrong 2016-02-24 7:59 ` Paolo Bonzini 1 sibling, 1 reply; 8+ messages in thread From: Xiao Guangrong @ 2016-02-24 6:23 UTC (permalink / raw) To: Paolo Bonzini, Jan Kiszka, Mike Krinkin, kvm; +Cc: Marcelo Tosatti, Sasha Levin On 02/23/2016 09:21 PM, Paolo Bonzini wrote: >> >> +#define INVALID_INDEX (-1) >> + >> static int mmu_unsync_walk(struct kvm_mmu_page *sp, >> struct kvm_mmu_pages *pvec) >> { >> if (!sp->unsync_children) >> return 0; >> >> - mmu_pages_add(pvec, sp, 0); >> + /* >> + * do not count the index in the parent of the sp we're >> + * walking start from. >> + */ >> + mmu_pages_add(pvec, sp, INVALID_INDEX); >> return __mmu_unsync_walk(sp, pvec); >> } >> >> @@ -1980,8 +1986,11 @@ static int mmu_pages_next(struct kvm_mmu_pages *pvec, >> return n; >> } >> >> - parents->parent[sp->role.level-2] = sp; >> - parents->idx[sp->role.level-1] = pvec->page[n].idx; >> + parents->parent[sp->role.level - 2] = sp; >> + >> + /* skip setting idex of the sp we start from. */ >> + if (pvec->page[n].idx != INVALID_INDEX) >> + parents->idx[sp->role.level - 1] = pvec->page[n].idx; > > So far so good. > >> } >> >> return n; >> @@ -1999,6 +2008,7 @@ static void mmu_pages_clear_parents(struct mmu_page_path *parents) >> if (!sp) >> return; >> >> + WARN_ON(idx != INVALID_INDEX); > > Shouldn't this warn if idx is *equal* to INVALID_INDEX? Yes, indeed. It seems i am spending so much time on QEMU and mixed it with assert() :( > >> clear_unsync_child_bit(sp, idx); >> level++; >> } while (level < PT64_ROOT_LEVEL-1 && !sp->unsync_children); >> @@ -2008,7 +2018,7 @@ static void kvm_mmu_pages_init(struct kvm_mmu_page *parent, >> struct mmu_page_path *parents, >> struct kvm_mmu_pages *pvec) >> { >> - parents->parent[parent->role.level-1] = NULL; >> + parents->parent[parent->role.level - 2] = NULL; > > This is meant to stop mmu_pages_clear_parents _after_ it has > processed sp, so the "-1" is correct. The right fix would be: > > if (parent->role.level < PT64_ROOT_LEVEL-1) > parents->parent[parent->role.level - 1] = NULL; > it is okay as mmu_pages_next() will refill the highest level. > and no one has noticed it so far because if the bug hits you just write > over the beginning of ->idx (which so far is uninitialized) and nothing > bad happens. > > I'm thinking of the following (untested) patch instead: > > -------- 8< -------------- > From: Paolo Bonzini <pbonzini@redhat.com> > Subject: [PATCH] KVM: MMU: Fix ubsan warnings > > kvm_mmu_pages_init is doing some really yucky stuff. It is setting > up a sentinel for mmu_page_clear_parents; however, because of a) the > way levels are numbered starting from 1 and b) the way mmu_page_path > sizes its arrays with PT64_ROOT_LEVEL-1 elements, the access can be > out of bounds. This is harmless because the code overwrites up to the > first two elements of parents->idx and these are initialized, and > because the sentinel is not needed in this case---mmu_page_clear_parents > exits anyway when it gets to the end of the array. However ubsan > complains, and everyone else should too. > > This fix does three things. First it makes the mmu_page_path arrays > PT64_ROOT_LEVEL elements in size, so that we can write to them without > checking the level in advance. Second it disintegrates kvm_mmu_pages_init > between mmu_unsync_walk (which resets struct kvm_mmu_pages) and > mmu_pages_next (which unconditionally places a NULL sentinel at the > end of the current path). This is okay because the mmu_page_path is > only used in mmu_pages_clear_parents; mmu_pages_clear_parents itself > is called within a for_each_sp iterator, and hence always after a > call to mmu_pages_next. Third it changes mmu_pages_clear_parents to > just use the sentinel to stop iteration, without checking the bounds > on level. > > Reported-by: Sasha Levin <sasha.levin@oracle.com> > Reported-by: Mike Krinkin <krinkin.m.u@gmail.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > index 33cc9f3f5b02..f4f5e7ca14dd 100644 > --- a/arch/x86/kvm/mmu.c > +++ b/arch/x86/kvm/mmu.c > @@ -1843,6 +1843,7 @@ static int __mmu_unsync_walk(struct kvm_mmu_page *sp, > static int mmu_unsync_walk(struct kvm_mmu_page *sp, > struct kvm_mmu_pages *pvec) > { > + pvec->nr = 0; > if (!sp->unsync_children) > return 0; > > @@ -1956,8 +1957,8 @@ static void kvm_sync_pages(struct kvm_vcpu *vcpu, gfn_t gfn) > } > > struct mmu_page_path { > - struct kvm_mmu_page *parent[PT64_ROOT_LEVEL-1]; > - unsigned int idx[PT64_ROOT_LEVEL-1]; > + struct kvm_mmu_page *parent[PT64_ROOT_LEVEL]; > + unsigned int idx[PT64_ROOT_LEVEL]; So that it wastes 1 entry as we only use it to save parent sp. > }; > > #define for_each_sp(pvec, sp, parents, i) \ > @@ -1971,19 +1972,21 @@ static int mmu_pages_next(struct kvm_mmu_pages *pvec, > int i) > { > int n; > + int level = PT_PAGE_TABLE_LEVEL; > > for (n = i+1; n < pvec->nr; n++) { > struct kvm_mmu_page *sp = pvec->page[n].sp; > > - if (sp->role.level == PT_PAGE_TABLE_LEVEL) { > - parents->idx[0] = pvec->page[n].idx; > - return n; > - } > + level = sp->role.level; > + parents->idx[level-1] = pvec->page[n].idx; > > - parents->parent[sp->role.level-2] = sp; > - parents->idx[sp->role.level-1] = pvec->page[n].idx; > + if (level == PT_PAGE_TABLE_LEVEL) > + break; > + > + parents->parent[level-2] = sp; > } > > + parents->parent[level-1] = NULL; Why? Considering we have already got a unsync page, so break the loop and reach here. It results parents->parent[ 0 ] = NULL and see below...... > return n; > } > > @@ -1994,22 +1996,13 @@ static void mmu_pages_clear_parents(struct mmu_page_path *parents) > > do { > unsigned int idx = parents->idx[level]; > - > sp = parents->parent[level]; > if (!sp) > return; So the first level is NULL, it stops clearing unsync_children. > > clear_unsync_child_bit(sp, idx); > level++; > - } while (level < PT64_ROOT_LEVEL-1 && !sp->unsync_children); > -} > - > -static void kvm_mmu_pages_init(struct kvm_mmu_page *parent, > - struct mmu_page_path *parents, > - struct kvm_mmu_pages *pvec) > -{ > - parents->parent[parent->role.level-1] = NULL; > - pvec->nr = 0; > + } while (!sp->unsync_children); > } > > static void mmu_sync_children(struct kvm_vcpu *vcpu, > @@ -2021,7 +2014,6 @@ static void mmu_sync_children(struct kvm_vcpu *vcpu, > struct kvm_mmu_pages pages; > LIST_HEAD(invalid_list); > > - kvm_mmu_pages_init(parent, &parents, &pages); > while (mmu_unsync_walk(parent, &pages)) { > bool protected = false; > > @@ -2037,7 +2029,6 @@ static void mmu_sync_children(struct kvm_vcpu *vcpu, > } > kvm_mmu_commit_zap_page(vcpu->kvm, &invalid_list); > cond_resched_lock(&vcpu->kvm->mmu_lock); > - kvm_mmu_pages_init(parent, &parents, &pages); > } > } > > @@ -2269,7 +2260,6 @@ static int mmu_zap_unsync_children(struct kvm *kvm, > if (parent->role.level == PT_PAGE_TABLE_LEVEL) > return 0; > > - kvm_mmu_pages_init(parent, &parents, &pages); > while (mmu_unsync_walk(parent, &pages)) { > struct kvm_mmu_page *sp; > > @@ -2278,7 +2268,6 @@ static int mmu_zap_unsync_children(struct kvm *kvm, > mmu_pages_clear_parents(&parents); > zapped++; > } > - kvm_mmu_pages_init(parent, &parents, &pages); > } > > return zapped; > Each time i looked into these functions, it costs my several hours. The trick is that the item, sp and idx, saved to kvm_mmu_pages is the sp and the index in the _parent_ level. How about clarify/simplify it as this patch: diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c index 57cf30b..14b2800 100644 --- a/arch/x86/kvm/mmu.c +++ b/arch/x86/kvm/mmu.c @@ -1848,7 +1848,7 @@ static int __mmu_unsync_walk(struct kvm_mmu_page *sp, child = page_header(ent & PT64_BASE_ADDR_MASK); if (child->unsync_children) { - if (mmu_pages_add(pvec, child, i)) + if (mmu_pages_add(pvec, sp, i)) return -ENOSPC; ret = __mmu_unsync_walk(child, pvec); @@ -1861,7 +1861,14 @@ static int __mmu_unsync_walk(struct kvm_mmu_page *sp, return ret; } else if (child->unsync) { nr_unsync_leaf++; - if (mmu_pages_add(pvec, child, i)) + if (mmu_pages_add(pvec, sp, i)) + return -ENOSPC; + + /* + * the unsync is on the last level so its 'idx' is + * useless, we set it to 0 to catch potential bugs. + */ + if (mmu_pages_add(pvec, child, 0)) return -ENOSPC; } else clear_unsync_child_bit(sp, i); @@ -1876,7 +1883,6 @@ static int mmu_unsync_walk(struct kvm_mmu_page *sp, if (!sp->unsync_children) return 0; - mmu_pages_add(pvec, sp, 0); return __mmu_unsync_walk(sp, pvec); } @@ -2002,16 +2008,18 @@ static int mmu_pages_next(struct kvm_mmu_pages *pvec, { int n; - for (n = i+1; n < pvec->nr; n++) { + for (n = i + 1; n < pvec->nr; n++) { struct kvm_mmu_page *sp = pvec->page[n].sp; + unsigned int idx = pvec->page[n].idx; if (sp->role.level == PT_PAGE_TABLE_LEVEL) { - parents->idx[0] = pvec->page[n].idx; + /* we always set the idex as 0 for the unsync page. */ + WARN_ON(idx != 0); return n; } - parents->parent[sp->role.level-2] = sp; - parents->idx[sp->role.level-1] = pvec->page[n].idx; + parents->parent[sp->role.level - 2] = sp; + parents->idx[sp->role.level - 2] = idx; } return n; @@ -2031,14 +2039,18 @@ static void mmu_pages_clear_parents(struct mmu_page_path *parents) clear_unsync_child_bit(sp, idx); level++; - } while (level < PT64_ROOT_LEVEL-1 && !sp->unsync_children); + } while (level < PT64_ROOT_LEVEL - 1 && !sp->unsync_children); } static void kvm_mmu_pages_init(struct kvm_mmu_page *parent, struct mmu_page_path *parents, struct kvm_mmu_pages *pvec) { - parents->parent[parent->role.level-1] = NULL; + /* + * set the highest level to NULL to stop mmu_pages_clear_parents() + * handing it on 32 bit guest. + */ + parents->parent[parent->role.level - 2] = NULL; pvec->nr = 0; } ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: index-out-of-range ubsan warnings 2016-02-24 6:23 ` Xiao Guangrong @ 2016-02-24 7:59 ` Paolo Bonzini 0 siblings, 0 replies; 8+ messages in thread From: Paolo Bonzini @ 2016-02-24 7:59 UTC (permalink / raw) To: Xiao Guangrong, Jan Kiszka, Mike Krinkin, kvm Cc: Marcelo Tosatti, Sasha Levin >> >> This is meant to stop mmu_pages_clear_parents _after_ it has >> processed sp, so the "-1" is correct. The right fix would be: >> >> if (parent->role.level < PT64_ROOT_LEVEL-1) >> parents->parent[parent->role.level - 1] = NULL; >> > > it is okay as mmu_pages_next() will refill the highest level. That would only happen for 64-bit pages, not for 32-bit (both PAE AND non-PAE, including the case of non-paged mode with !unrestricted_guest). On 24/02/2016 07:23, Xiao Guangrong wrote: >> + parents->parent[level-1] = NULL; > > Why? The idea was to move the NULL down at every step (first at parent[1], then at parent[2], then at parent[3], then at parent[4]) but as you note it is wrong because pages are added starting from the parent rather than the children. I think I can put together my patch and yours to build something that works; I'll post it later today. Thanks for your help! Paolo ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-02-24 7:59 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-02-23 9:26 index-out-of-range ubsan warnings Mike Krinkin 2016-02-23 10:07 ` Jan Kiszka 2016-02-23 10:44 ` Xiao Guangrong 2016-02-23 11:13 ` Mike Krinkin 2016-02-23 13:21 ` Paolo Bonzini 2016-02-23 13:56 ` Mike Krinkin 2016-02-24 6:23 ` Xiao Guangrong 2016-02-24 7:59 ` Paolo Bonzini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox