Received-SPF: pass (google.com: domain of linux-kernel+bounces-199536-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 3 Jun 2024 10:54:09 -0700
In-Reply-To: <98ad0dab3a2c66834e50e6d465dcae47dd80758b.1717436464.git.babu.moger@amd.com>
Precedence: bulk
Mime-Version: 1.0
References: <98ad0dab3a2c66834e50e6d465dcae47dd80758b.1717436464.git.babu.moger@amd.com>
Message-ID: <Zl4DQauIgkrjuBjg@google.com>
Subject: Re: [PATCH] KVM: Fix Undefined Behavior Sanitizer(UBSAN) error
From: Sean Christopherson <seanjc@google.com>
To: Babu Moger <babu.moger@amd.com>
Cc: pbonzini@redhat.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="us-ascii"

On Mon, Jun 03, 2024, Babu Moger wrote:
> System throws this following UBSAN: invalid-load error when the very first
> VM is powered up on a freshly booted host machine. Happens only with 2P or
> 4P (multiple sockets) systems.

...

> However, VM boots up fine without any issues and operational.
> 
> The error is due to invalid assignment in kvm invalidate range end path.
> There is no arch specific handler for this case and handler is assigned
> to kvm_null_fn(). This is an empty function and returns void. Return value
> of this function is assigned to boolean variable. UBSAN complains about
> this incompatible assignment when kernel is compiled with CONFIG_UBSAN.
> 
> Fix the issue by adding a check for the null handler.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> Seems straight forward fix to me. Point me if you think otherwise. New
> to this area of the code. First of all not clear to me why handler need
> to be called when memory slot is not found in the hva range.
> ---
>  virt/kvm/kvm_main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 14841acb8b95..ee8be1835214 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -653,7 +653,8 @@ static __always_inline kvm_mn_ret_t __kvm_handle_hva_range(struct kvm *kvm,
>  				if (IS_KVM_NULL_FN(range->handler))
>  					break;
>  			}
> -			r.ret |= range->handler(kvm, &gfn_range);
> +			if (!IS_KVM_NULL_FN(range->handler))
> +				r.ret |= range->handler(kvm, &gfn_range);

Hrm, this should be unreachable, the IS_KVM_NULL_FN() just about is supposed to
bail after locking.

Ah, the "break" will only break out of the memslot loop, it won't break out of
the address space loop.  Stupid SMM.

I think this is what we want.

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index b312d0cbe60b..70f5a39f8302 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -651,7 +651,7 @@ static __always_inline kvm_mn_ret_t __kvm_handle_hva_range(struct kvm *kvm,
                                        range->on_lock(kvm);
 
                                if (IS_KVM_NULL_FN(range->handler))
-                                       break;
+                                       goto mmu_unlock;
                        }
                        r.ret |= range->handler(kvm, &gfn_range);
                }
@@ -660,6 +660,7 @@ static __always_inline kvm_mn_ret_t __kvm_handle_hva_range(struct kvm *kvm,
        if (range->flush_on_ret && r.ret)
                kvm_flush_remote_tlbs(kvm);
 
+mmu_unlock:
        if (r.found_memslot)
                KVM_MMU_UNLOCK(kvm);