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 <pbonz...@redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb...@gmail.com>
Reviewed-by: David Gibson <da...@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
signature.asc
Description: PGP signature
