Re: [RFC PATCH 1/1] kvm-all.c: hint Valgrind that kvm_get_one_reg() inits memory

[David Gibson <david@gibson.dropbear.id.au>]

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

On Tue, Apr 05, 2022 at 10:04:39AM -0300, Daniel Henrique Barboza wrote:
> There is a lot of Valgrind warnings about conditional jump depending on
> unintialized values like this one (taken from a pSeries guest):
> 
>  Conditional jump or move depends on uninitialised value(s)
>     at 0xB011DC: kvmppc_enable_cap_large_decr (kvm.c:2544)
>     by 0x92F28F: cap_large_decr_cpu_apply (spapr_caps.c:523)
>     by 0x930C37: spapr_caps_cpu_apply (spapr_caps.c:921)
>     by 0x955D3B: spapr_reset_vcpu (spapr_cpu_core.c:73)
> (...)
>   Uninitialised value was created by a stack allocation
>     at 0xB01150: kvmppc_enable_cap_large_decr (kvm.c:2538)
> 
> In this case, the alleged unintialized value is the 'lpcr' variable that
> is written by kvm_get_one_reg() and then used in an if clause:
> 
> int kvmppc_enable_cap_large_decr(PowerPCCPU *cpu, int enable)
> {
>     CPUState *cs = CPU(cpu);
>     uint64_t lpcr;
> 
>     kvm_get_one_reg(cs, KVM_REG_PPC_LPCR_64, &lpcr);
>     /* Do we need to modify the LPCR? */
>     if (!!(lpcr & LPCR_LD) != !!enable) { <---- Valgrind warns here
> (...)
> 
> A quick fix is to init the variable that kvm_get_one_reg() is going to
> write ('lpcr' in the example above). Another idea is to convince
> Valgrind that kvm_get_one_reg() inits the 'void *target' memory in case
> the ioctl() is successful. This will put some boilerplate in the
> function but it will bring benefit for its other callers.
> 
> This patch uses the memcheck VALGRING_MAKE_MEM_DEFINED() to mark the
> 'target' variable as initialized if the ioctl is successful.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  accel/kvm/kvm-all.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 5f1377ca04..d9acba23c7 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -53,6 +53,10 @@
>  #include <sys/eventfd.h>
>  #endif
>  
> +#ifdef CONFIG_VALGRIND_H
> +#include <valgrind/memcheck.h>
> +#endif
> +
>  /* KVM uses PAGE_SIZE in its definition of KVM_COALESCED_MMIO_MAX. We
>   * need to use the real host PAGE_SIZE, as that's what KVM will use.
>   */
> @@ -3504,6 +3508,19 @@ int kvm_get_one_reg(CPUState *cs, uint64_t id, void *target)
>      if (r) {
>          trace_kvm_failed_reg_get(id, strerror(-r));
>      }
> +
> +#ifdef CONFIG_VALGRIND_H
> +    if (r == 0) {
> +        switch (id & KVM_REG_SIZE_MASK) {
> +        case KVM_REG_SIZE_U32:
> +            VALGRIND_MAKE_MEM_DEFINED(target, sizeof(uint32_t));
> +            break;
> +        case KVM_REG_SIZE_U64:
> +            VALGRIND_MAKE_MEM_DEFINED(target, sizeof(uint64_t));
> +            break;
> +        }
> +    }
> +#endif
>      return r;
>  }
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]