Re: KVM: Warn if mark_page_dirty() is called without an active vCPU

Re: KVM: Warn if mark_page_dirty() is called without an active vCPU

[Christian Borntraeger]

From: Christian Borntraeger <borntraeger@de.ibm.com>

Quick heads-up.
The new warnon triggers on s390. Here we write to the guest from an
irqfd worker. Since we do not use dirty_ring yet this might be an over-indication.
Still have to look into that.

[ 1801.980777] WARNING: CPU: 12 PID: 117600 at arch/s390/kvm/../../../virt/kvm/kvm_main.c:3166 mark_page_dirty_in_slot+0xa0/0xb0 [kvm]
[ 1801.980839] Modules linked in: xt_CHECKSUM(E) xt_MASQUERADE(E) xt_conntrack(E) ipt_REJECT(E) xt_tcpudp(E) nft_compat(E) nf_nat_tftp(E) nft_objref(E) vhost_vsock(E) vmw_vsock_virtio_transport_common(E) vsock(E) vhost(E) vhost_iotlb(E) nf_conntrack_tftp(E) crc32_generic(E) algif_hash(E) af_alg(E) paes_s390(E) dm_crypt(E) encrypted_keys(E) loop(E) lcs(E) ctcm(E) fsm(E) kvm(E) nft_fib_inet(E) nft_fib_ipv4(E) nft_fib_ipv6(E) nft_fib(E) nft_reject_inet(E) nf_reject_ipv4(E) nf_reject_ipv6(E) nft_reject(E) nft_ct(E) nft_chain_nat(E) nf_nat(E) nf_conntrack(E) nf_defrag_ipv6(E) nf_defrag_ipv4(E) ip_set(E) nf_tables(E) nfnetlink(E) sunrpc(E) dm_service_time(E) dm_multipath(E) scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E) zfcp(E) scsi_transport_fc(E) ism(E) smc(E) ib_core(E) eadm_sch(E) vfio_ccw(E) mdev(E) vfio_iommu_type1(E) vfio(E) sch_fq_codel(E) configfs(E) ip_tables(E) x_tables(E) ghash_s39 [...truncated...]
[ 1801.980915]  sha1_s390(E) sha_common(E) pkey(E) zcrypt(E) rng_core(E) autofs4(E) [last unloaded: vfio_ap]
[ 1801.980931] CPU: 12 PID: 117600 Comm: kworker/12:0 Tainted: G            E     5.17.0-20220113.rc0.git0.32ce2abb03cf.300.fc35.s390x+next #1
[ 1801.980935] Hardware name: IBM 2964 NC9 712 (LPAR)
[ 1801.980938] Workqueue: events irqfd_inject [kvm]
[ 1801.980959] Krnl PSW : 0704e00180000000 000003ff805f0f5c (mark_page_dirty_in_slot+0xa4/0xb0 [kvm])
[ 1801.980981]            R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:2 PM:0 RI:0 EA:3
[ 1801.980985] Krnl GPRS: 000003ff298e9040 000000017754a660 0000000000000000 0000000000000000
[ 1801.980988]            000000003fefcc36 ffffffffffffff68 0000000000000000 0000000177871500
[ 1801.980990]            00000001d1918000 000000003fefcc36 00000001d1918000 0000000000000000
[ 1801.980993]            00000001375b0000 00000001d191a838 000003ff805f0ee6 0000038000babb48
[ 1801.981003] Krnl Code: 000003ff805f0f4c: eb9ff0a00004	lmg	%r9,%r15,160(%r15)
                          000003ff805f0f52: c0f400018c61	brcl	15,000003ff80622814
                         #000003ff805f0f58: af000000		mc	0,0
                         >000003ff805f0f5c: eb9ff0a00004	lmg	%r9,%r15,160(%r15)
                          000003ff805f0f62: c0f400018c59	brcl	15,000003ff80622814
                          000003ff805f0f68: c004ffe37b10	brcl	0,000003ff80260588
                          000003ff805f0f6e: ec360033007c	cgij	%r3,0,6,000003ff805f0fd4
                          000003ff805f0f74: e31020100012	lt	%r1,16(%r2)
[ 1801.981057] Call Trace:
[ 1801.981060]  [<000003ff805f0f5c>] mark_page_dirty_in_slot+0xa4/0xb0 [kvm]
[ 1801.981083]  [<000003ff8060e9fe>] adapter_indicators_set+0xde/0x268 [kvm]
[ 1801.981104]  [<000003ff80613c24>] set_adapter_int+0x64/0xd8 [kvm]
[ 1801.981124]  [<000003ff805fb9aa>] kvm_set_irq+0xc2/0x130 [kvm]
[ 1801.981144]  [<000003ff805f8d86>] irqfd_inject+0x76/0xa0 [kvm]
[ 1801.981164]  [<0000000175e56906>] process_one_work+0x1fe/0x470
[ 1801.981173]  [<0000000175e570a4>] worker_thread+0x64/0x498
[ 1801.981176]  [<0000000175e5ef2c>] kthread+0x10c/0x110
[ 1801.981180]  [<0000000175de73c8>] __ret_from_fork+0x40/0x58
[ 1801.981185]  [<000000017698440a>] ret_from_fork+0xa/0x40

Re: KVM: Warn if mark_page_dirty() is called without an active vCPU

[Paolo Bonzini]

On 1/13/22 13:06, Christian Borntraeger wrote:
> From: Christian Borntraeger<borntraeger@de.ibm.com>
> 
> Quick heads-up.
> The new warnon triggers on s390. Here we write to the guest from an
> irqfd worker. Since we do not use dirty_ring yet this might be an over-indication.
> Still have to look into that.

Yes, it's okay to add an #ifdef around the warning.

Paolo

[PATCH] KVM: avoid warning on s390 in mark_page_dirty

[Christian Borntraeger]

Avoid warnings on s390 like
[ 1801.980931] CPU: 12 PID: 117600 Comm: kworker/12:0 Tainted: G            E     5.17.0-20220113.rc0.git0.32ce2abb03cf.300.fc35.s390x+next #1
[ 1801.980938] Workqueue: events irqfd_inject [kvm]
[...]
[ 1801.981057] Call Trace:
[ 1801.981060]  [<000003ff805f0f5c>] mark_page_dirty_in_slot+0xa4/0xb0 [kvm]
[ 1801.981083]  [<000003ff8060e9fe>] adapter_indicators_set+0xde/0x268 [kvm]
[ 1801.981104]  [<000003ff80613c24>] set_adapter_int+0x64/0xd8 [kvm]
[ 1801.981124]  [<000003ff805fb9aa>] kvm_set_irq+0xc2/0x130 [kvm]
[ 1801.981144]  [<000003ff805f8d86>] irqfd_inject+0x76/0xa0 [kvm]
[ 1801.981164]  [<0000000175e56906>] process_one_work+0x1fe/0x470
[ 1801.981173]  [<0000000175e570a4>] worker_thread+0x64/0x498
[ 1801.981176]  [<0000000175e5ef2c>] kthread+0x10c/0x110
[ 1801.981180]  [<0000000175de73c8>] __ret_from_fork+0x40/0x58
[ 1801.981185]  [<000000017698440a>] ret_from_fork+0xa/0x40

when writing to a guest from an irqfd worker as long as we do not have
the dirty ring.

Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>
---
 virt/kvm/kvm_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 504158f0e131..1a682d3e106d 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3163,8 +3163,10 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
 {
 	struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
 
+#ifdef CONFIG_HAVE_KVM_DIRTY_RING
 	if (WARN_ON_ONCE(!vcpu) || WARN_ON_ONCE(vcpu->kvm != kvm))
 		return;
+#endif
 
 	if (memslot && kvm_slot_dirty_track_enabled(memslot)) {
 		unsigned long rel_gfn = gfn - memslot->base_gfn;
-- 
2.33.1

Re: [PATCH] KVM: avoid warning on s390 in mark_page_dirty

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1301 bytes --]

On Thu, 2022-01-13 at 13:29 +0100, Christian Borntraeger wrote:
> Avoid warnings on s390 like
> [ 1801.980931] CPU: 12 PID: 117600 Comm: kworker/12:0 Tainted: G            E     5.17.0-20220113.rc0.git0.32ce2abb03cf.300.fc35.s390x+next #1
> [ 1801.980938] Workqueue: events irqfd_inject [kvm]
> [...]
> [ 1801.981057] Call Trace:
> [ 1801.981060]  [<000003ff805f0f5c>] mark_page_dirty_in_slot+0xa4/0xb0 [kvm]
> [ 1801.981083]  [<000003ff8060e9fe>] adapter_indicators_set+0xde/0x268 [kvm]
> [ 1801.981104]  [<000003ff80613c24>] set_adapter_int+0x64/0xd8 [kvm]
> [ 1801.981124]  [<000003ff805fb9aa>] kvm_set_irq+0xc2/0x130 [kvm]
> [ 1801.981144]  [<000003ff805f8d86>] irqfd_inject+0x76/0xa0 [kvm]
> [ 1801.981164]  [<0000000175e56906>] process_one_work+0x1fe/0x470
> [ 1801.981173]  [<0000000175e570a4>] worker_thread+0x64/0x498
> [ 1801.981176]  [<0000000175e5ef2c>] kthread+0x10c/0x110
> [ 1801.981180]  [<0000000175de73c8>] __ret_from_fork+0x40/0x58
> [ 1801.981185]  [<000000017698440a>] ret_from_fork+0xa/0x40
> 
> when writing to a guest from an irqfd worker as long as we do not have
> the dirty ring.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>

Reluctantly-acked-by: David Woodhouse <dwmw@amazon.co.uk>

... with a bucket nearby just in case.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH] KVM: avoid warning on s390 in mark_page_dirty

[Christian Borntraeger]

Am 13.01.22 um 13:29 schrieb Christian Borntraeger:
> Avoid warnings on s390 like
> [ 1801.980931] CPU: 12 PID: 117600 Comm: kworker/12:0 Tainted: G            E     5.17.0-20220113.rc0.git0.32ce2abb03cf.300.fc35.s390x+next #1
> [ 1801.980938] Workqueue: events irqfd_inject [kvm]
> [...]
> [ 1801.981057] Call Trace:
> [ 1801.981060]  [<000003ff805f0f5c>] mark_page_dirty_in_slot+0xa4/0xb0 [kvm]
> [ 1801.981083]  [<000003ff8060e9fe>] adapter_indicators_set+0xde/0x268 [kvm]
> [ 1801.981104]  [<000003ff80613c24>] set_adapter_int+0x64/0xd8 [kvm]
> [ 1801.981124]  [<000003ff805fb9aa>] kvm_set_irq+0xc2/0x130 [kvm]
> [ 1801.981144]  [<000003ff805f8d86>] irqfd_inject+0x76/0xa0 [kvm]
> [ 1801.981164]  [<0000000175e56906>] process_one_work+0x1fe/0x470
> [ 1801.981173]  [<0000000175e570a4>] worker_thread+0x64/0x498
> [ 1801.981176]  [<0000000175e5ef2c>] kthread+0x10c/0x110
> [ 1801.981180]  [<0000000175de73c8>] __ret_from_fork+0x40/0x58
> [ 1801.981185]  [<000000017698440a>] ret_from_fork+0xa/0x40
> 
> when writing to a guest from an irqfd worker as long as we do not have
> the dirty ring.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>
> ---
>   virt/kvm/kvm_main.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 504158f0e131..1a682d3e106d 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3163,8 +3163,10 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
>   {
>   	struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
>   
> +#ifdef CONFIG_HAVE_KVM_DIRTY_RING
>   	if (WARN_ON_ONCE(!vcpu) || WARN_ON_ONCE(vcpu->kvm != kvm))
>   		return;
> +#endif
>   
>   	if (memslot && kvm_slot_dirty_track_enabled(memslot)) {
>   		unsigned long rel_gfn = gfn - memslot->base_gfn;

Paolo, are you going to pick this for next for the time being?

Re: [PATCH] KVM: avoid warning on s390 in mark_page_dirty

[Paolo Bonzini]

On 1/18/22 09:37, Christian Borntraeger wrote:
> Am 13.01.22 um 13:29 schrieb Christian Borntraeger:
>> Avoid warnings on s390 like
>> [ 1801.980931] CPU: 12 PID: 117600 Comm: kworker/12:0 Tainted: 
>> G            E     
>> 5.17.0-20220113.rc0.git0.32ce2abb03cf.300.fc35.s390x+next #1
>> [ 1801.980938] Workqueue: events irqfd_inject [kvm]
>> [...]
>> [ 1801.981057] Call Trace:
>> [ 1801.981060]  [<000003ff805f0f5c>] mark_page_dirty_in_slot+0xa4/0xb0 
>> [kvm]
>> [ 1801.981083]  [<000003ff8060e9fe>] adapter_indicators_set+0xde/0x268 
>> [kvm]
>> [ 1801.981104]  [<000003ff80613c24>] set_adapter_int+0x64/0xd8 [kvm]
>> [ 1801.981124]  [<000003ff805fb9aa>] kvm_set_irq+0xc2/0x130 [kvm]
>> [ 1801.981144]  [<000003ff805f8d86>] irqfd_inject+0x76/0xa0 [kvm]
>> [ 1801.981164]  [<0000000175e56906>] process_one_work+0x1fe/0x470
>> [ 1801.981173]  [<0000000175e570a4>] worker_thread+0x64/0x498
>> [ 1801.981176]  [<0000000175e5ef2c>] kthread+0x10c/0x110
>> [ 1801.981180]  [<0000000175de73c8>] __ret_from_fork+0x40/0x58
>> [ 1801.981185]  [<000000017698440a>] ret_from_fork+0xa/0x40
>>
>> when writing to a guest from an irqfd worker as long as we do not have
>> the dirty ring.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>
>> ---
>>   virt/kvm/kvm_main.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index 504158f0e131..1a682d3e106d 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -3163,8 +3163,10 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
>>   {
>>       struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
>> +#ifdef CONFIG_HAVE_KVM_DIRTY_RING
>>       if (WARN_ON_ONCE(!vcpu) || WARN_ON_ONCE(vcpu->kvm != kvm))
>>           return;
>> +#endif
>>       if (memslot && kvm_slot_dirty_track_enabled(memslot)) {
>>           unsigned long rel_gfn = gfn - memslot->base_gfn;
> 
> Paolo, are you going to pick this for next for the time being?
> 

Yep, done now.

Paolo

Re: [PATCH] KVM: avoid warning on s390 in mark_page_dirty

[Christian Borntraeger]

Am 18.01.22 um 09:44 schrieb Paolo Bonzini:
> On 1/18/22 09:37, Christian Borntraeger wrote:
>> Am 13.01.22 um 13:29 schrieb Christian Borntraeger:
>>> Avoid warnings on s390 like
>>> [ 1801.980931] CPU: 12 PID: 117600 Comm: kworker/12:0 Tainted: G            E 5.17.0-20220113.rc0.git0.32ce2abb03cf.300.fc35.s390x+next #1
>>> [ 1801.980938] Workqueue: events irqfd_inject [kvm]
>>> [...]
>>> [ 1801.981057] Call Trace:
>>> [ 1801.981060]  [<000003ff805f0f5c>] mark_page_dirty_in_slot+0xa4/0xb0 [kvm]
>>> [ 1801.981083]  [<000003ff8060e9fe>] adapter_indicators_set+0xde/0x268 [kvm]
>>> [ 1801.981104]  [<000003ff80613c24>] set_adapter_int+0x64/0xd8 [kvm]
>>> [ 1801.981124]  [<000003ff805fb9aa>] kvm_set_irq+0xc2/0x130 [kvm]
>>> [ 1801.981144]  [<000003ff805f8d86>] irqfd_inject+0x76/0xa0 [kvm]
>>> [ 1801.981164]  [<0000000175e56906>] process_one_work+0x1fe/0x470
>>> [ 1801.981173]  [<0000000175e570a4>] worker_thread+0x64/0x498
>>> [ 1801.981176]  [<0000000175e5ef2c>] kthread+0x10c/0x110
>>> [ 1801.981180]  [<0000000175de73c8>] __ret_from_fork+0x40/0x58
>>> [ 1801.981185]  [<000000017698440a>] ret_from_fork+0xa/0x40
>>>
>>> when writing to a guest from an irqfd worker as long as we do not have
>>> the dirty ring.
>>>
>>> Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>
>>> ---
>>>   virt/kvm/kvm_main.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>>> index 504158f0e131..1a682d3e106d 100644
>>> --- a/virt/kvm/kvm_main.c
>>> +++ b/virt/kvm/kvm_main.c
>>> @@ -3163,8 +3163,10 @@ void mark_page_dirty_in_slot(struct kvm *kvm,
>>>   {
>>>       struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
>>> +#ifdef CONFIG_HAVE_KVM_DIRTY_RING
>>>       if (WARN_ON_ONCE(!vcpu) || WARN_ON_ONCE(vcpu->kvm != kvm))
>>>           return;
>>> +#endif
>>>       if (memslot && kvm_slot_dirty_track_enabled(memslot)) {
>>>           unsigned long rel_gfn = gfn - memslot->base_gfn;
>>
>> Paolo, are you going to pick this for next for the time being?
>>
> 
> Yep, done now.
> 
> Paolo

Thanks. I just realized that Davids patch meanwhile landed in Linus tree. So better
take this via master and not next.
Maybe also add
Fixes: 2efd61a608b0 ("KVM: Warn if mark_page_dirty() is called without an active vCPU")
in case the patch is picked for stable

Re: [PATCH] KVM: avoid warning on s390 in mark_page_dirty

[Paolo Bonzini]

On 1/18/22 09:53, Christian Borntraeger wrote:
>>>
>>> Paolo, are you going to pick this for next for the time being?
>>>
>>
>> Yep, done now.
>>
>> Paolo
> 
> Thanks. I just realized that Davids patch meanwhile landed in Linus 
> tree. So better
> take this via master and not next.

Yeah, I understood what you meant. :)  In fact, "master" right now is 
still on 5.16 (for patches that are destined to stable, but have 
conflicts merge window changes; those are pushed to master and merged 
from there into next).  There will be another pull request this week and 
it will have this patch.

> Maybe also add
> Fixes: 2efd61a608b0 ("KVM: Warn if mark_page_dirty() is called without 
> an active vCPU")
> in case the patch is picked for stable

Ok, will do.

Paolo

Re: KVM: Warn if mark_page_dirty() is called without an active vCPU

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1544 bytes --]

On Thu, 2022-01-13 at 13:14 +0100, Paolo Bonzini wrote:
> On 1/13/22 13:06, Christian Borntraeger wrote:
> > From: Christian Borntraeger<
> > borntraeger@de.ibm.com
> > >
> > 
> > Quick heads-up.
> > The new warnon triggers on s390. Here we write to the guest from an
> > irqfd worker. Since we do not use dirty_ring yet this might be an
> > over-indication.
> > Still have to look into that.
> 
> Yes, it's okay to add an #ifdef around the warning.

That would be #ifndef CONFIG_HAVE_KVM_DIRTY_RING, yes? 

I already found it hard to write down the rules around how
kvm_vcpu_write_guest() doesn't use the vCPU it's passed, and how both
it and kvm_write_guest() need to be invoked on a pCPU which currently
owns *a* vCPU belonging to the same KVM... if we add "unless you're on
an architecture that doesn't support dirty ring logging", you may have
to pass me a bucket.

Are you proposing that as an officially documented part of the already
horrid API, or a temporary measure :)

Btw, that get_map_page() in arch/s390/kvm/interrupt.c looks like it has
the same use-after-free problem that kvm_map_gfn() used to have. It
probably wants converting to the new gfn_to_pfn_cache. 

Take a look at how I resolve the same issue for delivering Xen event
channel interrupts.

Although I gave myself a free pass on the dirty marking in that case,
by declaring that the shinfo page doesn't get marked dirty; it should
be considered *always* dirty. You might have less fun declaring that
retrospectively in your case.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: KVM: Warn if mark_page_dirty() is called without an active vCPU

[Christian Borntraeger]

Am 13.01.22 um 13:30 schrieb David Woodhouse:
> On Thu, 2022-01-13 at 13:14 +0100, Paolo Bonzini wrote:
>> On 1/13/22 13:06, Christian Borntraeger wrote:
>>> From: Christian Borntraeger<
>>> borntraeger@de.ibm.com
>>>>
>>>
>>> Quick heads-up.
>>> The new warnon triggers on s390. Here we write to the guest from an
>>> irqfd worker. Since we do not use dirty_ring yet this might be an
>>> over-indication.
>>> Still have to look into that.
>>
>> Yes, it's okay to add an #ifdef around the warning.
> 
> That would be #ifndef CONFIG_HAVE_KVM_DIRTY_RING, yes?
> 
> I already found it hard to write down the rules around how
> kvm_vcpu_write_guest() doesn't use the vCPU it's passed, and how both
> it and kvm_write_guest() need to be invoked on a pCPU which currently
> owns *a* vCPU belonging to the same KVM... if we add "unless you're on
> an architecture that doesn't support dirty ring logging", you may have
> to pass me a bucket.
> 
> Are you proposing that as an officially documented part of the already
> horrid API, or a temporary measure :)
> 
> Btw, that get_map_page() in arch/s390/kvm/interrupt.c looks like it has
> the same use-after-free problem that kvm_map_gfn() used to have. It
> probably wants converting to the new gfn_to_pfn_cache.
> 
> Take a look at how I resolve the same issue for delivering Xen event
> channel interrupts.

Do you have a commit ID for your Xen event channel fix?

> 
> Although I gave myself a free pass on the dirty marking in that case,
> by declaring that the shinfo page doesn't get marked dirty; it should
> be considered *always* dirty. You might have less fun declaring that
> retrospectively in your case.

Re: KVM: Warn if mark_page_dirty() is called without an active vCPU

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1570 bytes --]

On Thu, 2022-01-13 at 13:51 +0100, Christian Borntraeger wrote:
> > Btw, that get_map_page() in arch/s390/kvm/interrupt.c looks like it has
> > the same use-after-free problem that kvm_map_gfn() used to have. It
> > probably wants converting to the new gfn_to_pfn_cache.
> > 
> > Take a look at how I resolve the same issue for delivering Xen event
> > channel interrupts.
> 
> Do you have a commit ID for your Xen event channel fix?

14243b387137 ("KVM: x86/xen: Add KVM_IRQ_ROUTING_XEN_EVTCHN and event
channel delivery") and the commits reworking the gfn_to_pfn_cache which
lead up to it.

Questions: In your kvm_set_routing_entry() where it calls
gmap_translate() to turn the summary_addr and ind_addr from guest
addresses to userspace virtual addresses, what protects those
translations? If the gmap changes before kvm_set_routing_entry() even
returns, what ensures that the IRQ gets retranslated?


And later in adapter_indicators_set() where you take that userspace
virtual address and pass it to your get_map_page() function, the same
question: what if userspace does a concurrent mmap() and changes the
physical page that the userspace address points to? 

In the latter case, at least it does look like you don't support
external memory accessed only by a PFN without having a corresponding
struct page. So at least you don't end up accessing a page that can now
belong to *another* guest, because the original underlying page is
locked. You're probably OK in that case, so it's just the gmap changing
that we need to focus on?

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: KVM: Warn if mark_page_dirty() is called without an active vCPU

[Christian Borntraeger]

Am 13.01.22 um 14:22 schrieb David Woodhouse:
> On Thu, 2022-01-13 at 13:51 +0100, Christian Borntraeger wrote:
>>> Btw, that get_map_page() in arch/s390/kvm/interrupt.c looks like it has
>>> the same use-after-free problem that kvm_map_gfn() used to have. It
>>> probably wants converting to the new gfn_to_pfn_cache.
>>>
>>> Take a look at how I resolve the same issue for delivering Xen event
>>> channel interrupts.
>>
>> Do you have a commit ID for your Xen event channel fix?
> 
> 14243b387137 ("KVM: x86/xen: Add KVM_IRQ_ROUTING_XEN_EVTCHN and event
> channel delivery") and the commits reworking the gfn_to_pfn_cache which
> lead up to it.
> 
> Questions: In your kvm_set_routing_entry() where it calls
> gmap_translate() to turn the summary_addr and ind_addr from guest
> addresses to userspace virtual addresses, what protects those
> translations? If the gmap changes before kvm_set_routing_entry() even
> returns, what ensures that the IRQ gets retranslated?

In the end the gmap translated between guest physical and host virtual, just
like the kvm memslots. This is done in kvm_arch_commit_memory_region. The gmap
is a method where we share the last level page table between the guest mapping
and the user mapping. That is why our memslots have to be on 1 MB boundary (our
page tables have 256 4k entries). So instead of gmap we could have used the
memslots as well for translation.

I have trouble seeing a kernel integrity issue: worst case is that we point to
a different address in the userspace mapping if qemu would change the memslots
maybe even to an invalid one. But then the access should fail (for invalid) or
you get a self-inflicted bit flips on your own address space if you play such
games.

Well, one thing:  if QEMU changes memslots, it might need to redo the irqfd
registration as well as we do not follow these changes. Maybe this is something
that we could improve as future QEMUs could do more changes regarding memslots.

> 
> And later in adapter_indicators_set() where you take that userspace
> virtual address and pass it to your get_map_page() function, the same
> question: what if userspace does a concurrent mmap() and changes the
> physical page that the userspace address points to?
> 
> In the latter case, at least it does look like you don't support
> external memory accessed only by a PFN without having a corresponding
> struct page. So at least you don't end up accessing a page that can now
> belong to *another* guest, because the original underlying page is
> locked. You're probably OK in that case, so it's just the gmap changing
> that we need to focus on?

Re: KVM: Warn if mark_page_dirty() is called without an active vCPU

[Paolo Bonzini]

On 1/13/22 13:30, David Woodhouse wrote:
> Are you proposing that as an officially documented part of the already
> horrid API, or a temporary measure:)

Hopefully temporary, but honestly you never know how these things go.

Paolo