Re: [PATCH] KVM: arm64: arch_timer shouldn't assume the vcpu is loaded

From: Marc Zyngier <maz@kernel.org>
To: James Morse <james.morse@arm.com>
Cc: Andre Przywara <andre.przywara@arm.com>,
	Julien Thierry <julien.thierry.kdev@gmail.com>,
	kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org,
	Suzuki K Poulose <suzuki.poulose@arm.com>
Subject: Re: [PATCH] KVM: arm64: arch_timer shouldn't assume the vcpu is loaded
Date: Wed, 8 Apr 2020 11:07:26 +0100	[thread overview]
Message-ID: <20200408110726.4d81bc3b@why> (raw)
In-Reply-To: <20200406150355.4859-1-james.morse@arm.com>

Hi James,

Thanks for looking into this.

On Mon,  6 Apr 2020 16:03:55 +0100
James Morse <james.morse@arm.com> wrote:

> kvm_arch_timer_get_input_level() needs to get the arch_timer_context for
> a particular vcpu, and uses kvm_get_running_vcpu() to find it.
> 
> kvm_arch_timer_get_input_level() may be called to handle a user-space
> write to the redistributor, where the vcpu is not loaded. This causes
> kvm_get_running_vcpu() to return NULL:
> | Unable to handle kernel paging request at virtual address 0000000000001ec0
> | Mem abort info:
> |   ESR = 0x96000004
> |   EC = 0x25: DABT (current EL), IL = 32 bits
> |   SET = 0, FnV = 0
> |   EA = 0, S1PTW = 0
> | Data abort info:
> |   ISV = 0, ISS = 0x00000004
> |   CM = 0, WnR = 0
> | user pgtable: 4k pages, 48-bit VAs, pgdp=000000003cbf9000
> | [0000000000001ec0] pgd=0000000000000000
> | Internal error: Oops: 96000004 [#1] PREEMPT SMP
> | Modules linked in: r8169 realtek efivarfs ip_tables x_tables
> | CPU: 1 PID: 2615 Comm: qemu-system-aar Not tainted 5.6.0-rc7 #30
> | Hardware name: Marvell mvebu_armada-37xx/mvebu_armada-37xx, BIOS 2018.03-devel-18.12.3-gc9aa92c-armbian 02/20/2019
> | pstate: 00000085 (nzcv daIf -PAN -UAO)
> | pc : kvm_arch_timer_get_input_level+0x1c/0x68
> | lr : kvm_arch_timer_get_input_level+0x1c/0x68
> 
> | Call trace:
> |  kvm_arch_timer_get_input_level+0x1c/0x68
> |  vgic_get_phys_line_level+0x3c/0x90
> |  vgic_mmio_write_senable+0xe4/0x130
> |  vgic_uaccess+0xe0/0x100
> |  vgic_v3_redist_uaccess+0x5c/0x80
> |  vgic_v3_attr_regs_access+0xf0/0x200
> |  nvgic_v3_set_attr+0x234/0x250
> |  kvm_device_ioctl_attr+0xa4/0xf8
> |  kvm_device_ioctl+0x7c/0xc0
> |  ksys_ioctl+0x1fc/0xc18
> |  __arm64_sys_ioctl+0x24/0x30
> |  do_el0_svc+0x7c/0x148
> |  el0_sync_handler+0x138/0x258
> |  el0_sync+0x140/0x180
> | Code: 910003fd f9000bf3 2a0003f3 97ff650c (b95ec001)
> | ---[ end trace 81287612d93f1e70 ]---
> | note: qemu-system-aar[2615] exited with preempt_count 1
> 
> Loading the vcpu doesn't make a lot of sense for handling a device ioctl(),
> so instead pass the vcpu through to kvm_arch_timer_get_input_level(). Its
> not clear that an intid makes much sense without the paired vcpu.

I don't fully agree with the analysis, Remember we are looking at the
state of the physical interrupt associated with a virtual interrupt, so
the vcpu doesn't quite make sense here if it isn't loaded.

What does it mean to look at the HW timer when we are not in the right
context? For all we know, it is completely random (the only guarantee
we have is that it is disabled, actually).

My gut feeling is that this is another instance where we should provide
specific userspace accessors that would only deal with the virtual
state, and leave anything that deals with the physical state of the
interrupt to be exercised only by the guest.

Does it make sense?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel