Re: [PATCH] KVM: s390: vsie: fix race during shadow creation

[David Hildenbrand <david@xxxxxxxxxx>]

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@xxxxxxxxxx>
> Signed-off-by: Christian Borntraeger <borntraeger@xxxxxxxxxxxxx>
> ---
>   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