* [PATCH] KVM: s390: vsie: fix race during shadow creation
@ 2023-12-20 7:34 Christian Borntraeger
2023-12-20 7:35 ` Christian Borntraeger
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Christian Borntraeger @ 2023-12-20 7:34 UTC (permalink / raw)
To: KVM
Cc: Christian Borntraeger, Janosch Frank, David Hildenbrand,
linux-s390, Thomas Huth, Claudio Imbrenda, Heiko Carstens,
Vasily Gorbik, Alexander Gordeev
Right now it is possible to see gmap->private being zero in
kvm_s390_vsie_gmap_notifier resulting in a crash. This is due to the
fact that we add gmap->private == kvm after creation:
static int acquire_gmap_shadow(struct kvm_vcpu *vcpu,
struct vsie_page *vsie_page)
{
[...]
gmap = gmap_shadow(vcpu->arch.gmap, asce, edat);
if (IS_ERR(gmap))
return PTR_ERR(gmap);
gmap->private = vcpu->kvm;
Instead of tracking kvm in the shadow gmap, simply use the parent one.
Cc: David Hildenbrand <david@redhat.com>
Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>
---
arch/s390/kvm/vsie.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
index 8207a892bbe2..6b06d8ec41b5 100644
--- a/arch/s390/kvm/vsie.c
+++ b/arch/s390/kvm/vsie.c
@@ -579,14 +579,17 @@ static int shadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
void kvm_s390_vsie_gmap_notifier(struct gmap *gmap, unsigned long start,
unsigned long end)
{
- struct kvm *kvm = gmap->private;
struct vsie_page *cur;
unsigned long prefix;
struct page *page;
+ struct kvm *kvm;
int i;
if (!gmap_is_shadow(gmap))
return;
+
+ kvm = gmap->parent->private;
+
/*
* Only new shadow blocks are added to the list during runtime,
* therefore we can safely reference them all the time.
@@ -1220,7 +1223,6 @@ static int acquire_gmap_shadow(struct kvm_vcpu *vcpu,
gmap = gmap_shadow(vcpu->arch.gmap, asce, edat);
if (IS_ERR(gmap))
return PTR_ERR(gmap);
- gmap->private = vcpu->kvm;
vcpu->kvm->stat.gmap_shadow_create++;
WRITE_ONCE(vsie_page->gmap, gmap);
return 0;
--
2.41.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: s390: vsie: fix race during shadow creation
2023-12-20 7:34 [PATCH] KVM: s390: vsie: fix race during shadow creation Christian Borntraeger
@ 2023-12-20 7:35 ` Christian Borntraeger
2023-12-20 10:34 ` Janosch Frank
2023-12-20 10:39 ` David Hildenbrand
2 siblings, 0 replies; 6+ messages in thread
From: Christian Borntraeger @ 2023-12-20 7:35 UTC (permalink / raw)
To: KVM
Cc: Janosch Frank, David Hildenbrand, linux-s390, Thomas Huth,
Claudio Imbrenda, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev
Am 20.12.23 um 08:34 schrieb Christian Borntraeger:
> Right now it is possible to see gmap->private being zero in
> kvm_s390_vsie_gmap_notifier resulting in a crash. This is due to the
> fact that we add gmap->private == kvm after creation:
>
> static int acquire_gmap_shadow(struct kvm_vcpu *vcpu,
> struct vsie_page *vsie_page)
> {
> [...]
> gmap = gmap_shadow(vcpu->arch.gmap, asce, edat);
> if (IS_ERR(gmap))
> return PTR_ERR(gmap);
> gmap->private = vcpu->kvm;
>
> Instead of tracking kvm in the shadow gmap, simply use the parent one.
>
> Cc: David Hildenbrand <david@redhat.com>
I forgot to add
Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: s390: vsie: fix race during shadow creation
2023-12-20 7:34 [PATCH] KVM: s390: vsie: fix race during shadow creation Christian Borntraeger
2023-12-20 7:35 ` Christian Borntraeger
@ 2023-12-20 10:34 ` Janosch Frank
2023-12-20 10:39 ` David Hildenbrand
2 siblings, 0 replies; 6+ messages in thread
From: Janosch Frank @ 2023-12-20 10:34 UTC (permalink / raw)
To: Christian Borntraeger, KVM
Cc: David Hildenbrand, linux-s390, Thomas Huth, Claudio Imbrenda,
Heiko Carstens, Vasily Gorbik, Alexander Gordeev
On 12/20/23 08:34, Christian Borntraeger wrote:
> Right now it is possible to see gmap->private being zero in
> kvm_s390_vsie_gmap_notifier resulting in a crash. This is due to the
> fact that we add gmap->private == kvm after creation:
>
> static int acquire_gmap_shadow(struct kvm_vcpu *vcpu,
> struct vsie_page *vsie_page)
> {
> [...]
> gmap = gmap_shadow(vcpu->arch.gmap, asce, edat);
> if (IS_ERR(gmap))
> return PTR_ERR(gmap);
> gmap->private = vcpu->kvm;
>
> Instead of tracking kvm in the shadow gmap, simply use the parent one.
>
> Cc: David Hildenbrand <david@redhat.com>
> Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>
I'm not completely happy with fixing this in the notifier by accessing
the parent although I do understand, that we want to have a fix that's
easily backportable.
From what I see "private" is currently not guarded by gmap->initialized
but maybe we should do that. This of course might bring a bit more pain
when backporting.
The other option is to retry which might burn a few cycles.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: s390: vsie: fix race during shadow creation
2023-12-20 7:34 [PATCH] KVM: s390: vsie: fix race during shadow creation Christian Borntraeger
2023-12-20 7:35 ` Christian Borntraeger
2023-12-20 10:34 ` Janosch Frank
@ 2023-12-20 10:39 ` David Hildenbrand
2023-12-20 12:26 ` Christian Borntraeger
2 siblings, 1 reply; 6+ messages in thread
From: David Hildenbrand @ 2023-12-20 10:39 UTC (permalink / raw)
To: Christian Borntraeger, KVM
Cc: Janosch Frank, linux-s390, Thomas Huth, Claudio Imbrenda,
Heiko Carstens, Vasily Gorbik, Alexander Gordeev
On 20.12.23 08:34, Christian Borntraeger wrote:
> Right now it is possible to see gmap->private being zero in
> kvm_s390_vsie_gmap_notifier resulting in a crash. This is due to the
> fact that we add gmap->private == kvm after creation:
>
> static int acquire_gmap_shadow(struct kvm_vcpu *vcpu,
> struct vsie_page *vsie_page)
> {
> [...]
> gmap = gmap_shadow(vcpu->arch.gmap, asce, edat);
> if (IS_ERR(gmap))
> return PTR_ERR(gmap);
> gmap->private = vcpu->kvm;
>
> Instead of tracking kvm in the shadow gmap, simply use the parent one.
>
> Cc: David Hildenbrand <david@redhat.com>
> Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>
> ---
> arch/s390/kvm/vsie.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
> index 8207a892bbe2..6b06d8ec41b5 100644
> --- a/arch/s390/kvm/vsie.c
> +++ b/arch/s390/kvm/vsie.c
> @@ -579,14 +579,17 @@ static int shadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
> void kvm_s390_vsie_gmap_notifier(struct gmap *gmap, unsigned long start,
> unsigned long end)
> {
> - struct kvm *kvm = gmap->private;
> struct vsie_page *cur;
> unsigned long prefix;
> struct page *page;
> + struct kvm *kvm;
> int i;
>
> if (!gmap_is_shadow(gmap))
> return;
> +
> + kvm = gmap->parent->private;
> +
> /*
> * Only new shadow blocks are added to the list during runtime,
> * therefore we can safely reference them all the time.
> @@ -1220,7 +1223,6 @@ static int acquire_gmap_shadow(struct kvm_vcpu *vcpu,
> gmap = gmap_shadow(vcpu->arch.gmap, asce, edat);
> if (IS_ERR(gmap))
> return PTR_ERR(gmap);
> - gmap->private = vcpu->kvm;
> vcpu->kvm->stat.gmap_shadow_create++;
> WRITE_ONCE(vsie_page->gmap, gmap);
> return 0;
Why not let gmap_shadow handle it? Simply clone the parent private field.
diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
index 6f96b5a71c63..e083fade7a5d 100644
--- a/arch/s390/mm/gmap.c
+++ b/arch/s390/mm/gmap.c
@@ -1691,6 +1691,7 @@ struct gmap *gmap_shadow(struct gmap *parent,
unsigned long asce,
return ERR_PTR(-ENOMEM);
new->mm = parent->mm;
new->parent = gmap_get(parent);
+ new->private = patent->private;
new->orig_asce = asce;
new->edat_level = edat_level;
new->initialized = false;
Or am I missing something?
--
Cheers,
David / dhildenb
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: s390: vsie: fix race during shadow creation
2023-12-20 10:39 ` David Hildenbrand
@ 2023-12-20 12:26 ` Christian Borntraeger
2023-12-20 13:11 ` David Hildenbrand
0 siblings, 1 reply; 6+ messages in thread
From: Christian Borntraeger @ 2023-12-20 12:26 UTC (permalink / raw)
To: David Hildenbrand, KVM
Cc: Janosch Frank, linux-s390, Thomas Huth, Claudio Imbrenda,
Heiko Carstens, Vasily Gorbik, Alexander Gordeev
Am 20.12.23 um 11:39 schrieb David Hildenbrand:
> On 20.12.23 08:34, Christian Borntraeger wrote:
>> Right now it is possible to see gmap->private being zero in
>> kvm_s390_vsie_gmap_notifier resulting in a crash. This is due to the
>> fact that we add gmap->private == kvm after creation:
>>
>> static int acquire_gmap_shadow(struct kvm_vcpu *vcpu,
>> struct vsie_page *vsie_page)
>> {
>> [...]
>> gmap = gmap_shadow(vcpu->arch.gmap, asce, edat);
>> if (IS_ERR(gmap))
>> return PTR_ERR(gmap);
>> gmap->private = vcpu->kvm;
>>
>> Instead of tracking kvm in the shadow gmap, simply use the parent one.
>>
>> Cc: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Christian Borntraeger <borntraeger@linux.ibm.com>
>> ---
>> arch/s390/kvm/vsie.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c
>> index 8207a892bbe2..6b06d8ec41b5 100644
>> --- a/arch/s390/kvm/vsie.c
>> +++ b/arch/s390/kvm/vsie.c
>> @@ -579,14 +579,17 @@ static int shadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page)
>> void kvm_s390_vsie_gmap_notifier(struct gmap *gmap, unsigned long start,
>> unsigned long end)
>> {
>> - struct kvm *kvm = gmap->private;
>> struct vsie_page *cur;
>> unsigned long prefix;
>> struct page *page;
>> + struct kvm *kvm;
>> int i;
>> if (!gmap_is_shadow(gmap))
>> return;
>> +
>> + kvm = gmap->parent->private;
>> +
>> /*
>> * Only new shadow blocks are added to the list during runtime,
>> * therefore we can safely reference them all the time.
>> @@ -1220,7 +1223,6 @@ static int acquire_gmap_shadow(struct kvm_vcpu *vcpu,
>> gmap = gmap_shadow(vcpu->arch.gmap, asce, edat);
>> if (IS_ERR(gmap))
>> return PTR_ERR(gmap);
>> - gmap->private = vcpu->kvm;
>> vcpu->kvm->stat.gmap_shadow_create++;
>> WRITE_ONCE(vsie_page->gmap, gmap);
>> return 0;
>
> Why not let gmap_shadow handle it? Simply clone the parent private field.
>
> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
> index 6f96b5a71c63..e083fade7a5d 100644
> --- a/arch/s390/mm/gmap.c
> +++ b/arch/s390/mm/gmap.c
> @@ -1691,6 +1691,7 @@ struct gmap *gmap_shadow(struct gmap *parent, unsigned long asce,
> return ERR_PTR(-ENOMEM);
> new->mm = parent->mm;
> new->parent = gmap_get(parent);
> + new->private = patent->private;
> new->orig_asce = asce;
> new->edat_level = edat_level;
> new->initialized = false;
>
> Or am I missing something?
That would work as well. I discussed several alternatives with Janosch.
The only thing that bothers me is that the owner should define private. So an
alternative would be to have a parameter for gmap_shadow. On the other hand I
like the simplicity of this patch. (we need to get rid of the 2nd assignment
in acquire_gmap_shadow to make it complete.
So I can spin a v2 with this variant if Janosch is ok with it as well.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: s390: vsie: fix race during shadow creation
2023-12-20 12:26 ` Christian Borntraeger
@ 2023-12-20 13:11 ` David Hildenbrand
0 siblings, 0 replies; 6+ messages in thread
From: David Hildenbrand @ 2023-12-20 13:11 UTC (permalink / raw)
To: Christian Borntraeger, KVM
Cc: Janosch Frank, linux-s390, Thomas Huth, Claudio Imbrenda,
Heiko Carstens, Vasily Gorbik, Alexander Gordeev
>> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
>> index 6f96b5a71c63..e083fade7a5d 100644
>> --- a/arch/s390/mm/gmap.c
>> +++ b/arch/s390/mm/gmap.c
>> @@ -1691,6 +1691,7 @@ struct gmap *gmap_shadow(struct gmap *parent, unsigned long asce,
>> return ERR_PTR(-ENOMEM);
>> new->mm = parent->mm;
>> new->parent = gmap_get(parent);
>> + new->private = patent->private;
>> new->orig_asce = asce;
>> new->edat_level = edat_level;
>> new->initialized = false;
>>
>> Or am I missing something?
>
> That would work as well. I discussed several alternatives with Janosch.
> The only thing that bothers me is that the owner should define private. So an
> alternative would be to have a parameter for gmap_shadow. On the other hand I
> like the simplicity of this patch. (we need to get rid of the 2nd assignment
> in acquire_gmap_shadow to make it complete.
Right. Conceptually, the owner setup parent->private and the owner
requests to create a shadow. So inheriting the ->private setting does
not sound completely wrong.
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-12-20 13:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-20 7:34 [PATCH] KVM: s390: vsie: fix race during shadow creation Christian Borntraeger
2023-12-20 7:35 ` Christian Borntraeger
2023-12-20 10:34 ` Janosch Frank
2023-12-20 10:39 ` David Hildenbrand
2023-12-20 12:26 ` Christian Borntraeger
2023-12-20 13:11 ` David Hildenbrand
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox