Re: [PATCH] tests: kvm: Check for a kernel warning

[Krish Sadhukhan <krish.sadhukhan@oracle.com>]

On 6/20/19 7:12 AM, Aaron Lewis wrote:
> On Tue, Jun 18, 2019 at 12:38 PM Krish Sadhukhan
> <krish.sadhukhan@oracle.com> wrote:
>>
>>
>> On 06/18/2019 07:13 AM, Aaron Lewis wrote:
>>> On Fri, May 31, 2019 at 7:14 AM Aaron Lewis <aaronlewis@google.com> wrote:
>>>> When running with /sys/module/kvm_intel/parameters/unrestricted_guest=N,
>>>> test that a kernel warning does not occur informing us that
>>>> vcpu->mmio_needed=1.  This can happen when KVM_RUN is called after a
>>>> triple fault.
>>>> This test was made to detect a bug that was reported by Syzkaller
>>>> (https://groups.google.com/forum/#!topic/syzkaller/lHfau8E3SOE) and
>>>> fixed with commit bbeac2830f4de ("KVM: X86: Fix residual mmio emulation
>>>> request to userspace").
>>>>
>>>> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
>>>> Reviewed-by: Jim Mattson <jmattson@google.com>
>>>> Reviewed-by: Peter Shier <pshier@google.com>
>>>> ---
>>>>    tools/testing/selftests/kvm/.gitignore        |   1 +
>>>>    tools/testing/selftests/kvm/Makefile          |   1 +
>>>>    .../testing/selftests/kvm/include/kvm_util.h  |   2 +
>>>>    .../selftests/kvm/include/x86_64/processor.h  |   2 +
>>>>    tools/testing/selftests/kvm/lib/kvm_util.c    |  36 +++++
>>>>    .../selftests/kvm/lib/x86_64/processor.c      |  16 +++
>>>>    .../selftests/kvm/x86_64/mmio_warning_test.c  | 126 ++++++++++++++++++
>>>>    7 files changed, 184 insertions(+)
>>>>    create mode 100644 tools/testing/selftests/kvm/x86_64/mmio_warning_test.c
>>>>
>>>> diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
>>>> index df1bf9230a74..41266af0d3dc 100644
>>>> --- a/tools/testing/selftests/kvm/.gitignore
>>>> +++ b/tools/testing/selftests/kvm/.gitignore
>>>> @@ -2,6 +2,7 @@
>>>>    /x86_64/evmcs_test
>>>>    /x86_64/hyperv_cpuid
>>>>    /x86_64/kvm_create_max_vcpus
>>>> +/x86_64/mmio_warning_test
>>>>    /x86_64/platform_info_test
>>>>    /x86_64/set_sregs_test
>>>>    /x86_64/smm_test
>>>> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
>>>> index 79c524395ebe..670b938f1049 100644
>>>> --- a/tools/testing/selftests/kvm/Makefile
>>>> +++ b/tools/testing/selftests/kvm/Makefile
>>>> @@ -22,6 +22,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/vmx_close_while_nested_test
>>>>    TEST_GEN_PROGS_x86_64 += x86_64/smm_test
>>>>    TEST_GEN_PROGS_x86_64 += x86_64/kvm_create_max_vcpus
>>>>    TEST_GEN_PROGS_x86_64 += x86_64/vmx_set_nested_state_test
>>>> +TEST_GEN_PROGS_x86_64 += x86_64/mmio_warning_test
>>>>    TEST_GEN_PROGS_x86_64 += dirty_log_test
>>>>    TEST_GEN_PROGS_x86_64 += clear_dirty_log_test
>>>>
>>>> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
>>>> index 8c6b9619797d..c5c427c86598 100644
>>>> --- a/tools/testing/selftests/kvm/include/kvm_util.h
>>>> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
>>>> @@ -137,6 +137,8 @@ struct kvm_vm *vm_create_default(uint32_t vcpuid, uint64_t extra_mem_size,
>>>>                                    void *guest_code);
>>>>    void vm_vcpu_add_default(struct kvm_vm *vm, uint32_t vcpuid, void *guest_code);
>>>>
>>>> +bool vm_is_unrestricted_guest(struct kvm_vm *vm);
>>>> +
>>>>    struct kvm_userspace_memory_region *
>>>>    kvm_userspace_memory_region_find(struct kvm_vm *vm, uint64_t start,
>>>>                                    uint64_t end);
>>>> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
>>>> index 6063d5b2f356..af4d26de32d1 100644
>>>> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
>>>> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
>>>> @@ -303,6 +303,8 @@ static inline unsigned long get_xmm(int n)
>>>>           return 0;
>>>>    }
>>>>
>>>> +bool is_intel_cpu(void);
>>>> +
>>>>    struct kvm_x86_state;
>>>>    struct kvm_x86_state *vcpu_save_state(struct kvm_vm *vm, uint32_t vcpuid);
>>>>    void vcpu_load_state(struct kvm_vm *vm, uint32_t vcpuid,
>>>> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
>>>> index e9113857f44e..b93b09ad9a11 100644
>>>> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
>>>> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
>>>> @@ -1584,3 +1584,39 @@ void *addr_gva2hva(struct kvm_vm *vm, vm_vaddr_t gva)
>>>>    {
>>>>           return addr_gpa2hva(vm, addr_gva2gpa(vm, gva));
>>>>    }
>>>> +
>>>> +/*
>>>> + * Is Unrestricted Guest
>>>> + *
>>>> + * Input Args:
>>>> + *   vm - Virtual Machine
>>>> + *
>>>> + * Output Args: None
>>>> + *
>>>> + * Return: True if the unrestricted guest is set to 'Y', otherwise return false.
>>>> + *
>>>> + * Check if the unrestricted guest flag is enabled.
>>>> + */
>>>> +bool vm_is_unrestricted_guest(struct kvm_vm *vm)
>>>> +{
>>>> +       char val = 'N';
>>>> +       size_t count;
>>>> +       FILE *f;
>>>> +
>>>> +       if (vm == NULL) {
>>>> +               /* Ensure that the KVM vendor-specific module is loaded. */
>>>> +               f = fopen(KVM_DEV_PATH, "r");
>>>> +               TEST_ASSERT(f != NULL, "Error in opening KVM dev file: %d",
>>>> +                           errno);
>>>> +               fclose(f);
>>>> +       }
>>>> +
>>>> +       f = fopen("/sys/module/kvm_intel/parameters/unrestricted_guest", "r");
>>>> +       if (f) {
>>>> +               count = fread(&val, sizeof(char), 1, f);
>>>> +               TEST_ASSERT(count == 1, "Unable to read from param file.");
>>>> +               fclose(f);
>>>> +       }
>>>> +
>>>> +       return val == 'Y';
>>>> +}
>>>> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
>>>> index dc7fae9fa424..bcc0e70e1856 100644
>>>> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
>>>> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
>>>> @@ -1139,3 +1139,19 @@ void vcpu_load_state(struct kvm_vm *vm, uint32_t vcpuid, struct kvm_x86_state *s
>>>>                           r);
>>>>           }
>>>>    }
>>>> +
>>>> +bool is_intel_cpu(void)
>>>> +{
>>>> +       int eax, ebx, ecx, edx;
>>>> +       const uint32_t *chunk;
>>>> +       const int leaf = 0;
>>>> +
>>>> +       __asm__ __volatile__(
>>>> +               "cpuid"
>>>> +               : /* output */ "=a"(eax), "=b"(ebx),
>>>> +                 "=c"(ecx), "=d"(edx)
>>>> +               : /* input */ "0"(leaf), "2"(0));
>>>> +
>>>> +       chunk = (const uint32_t *)("GenuineIntel");
>>>> +       return (ebx == chunk[0] && edx == chunk[1] && ecx == chunk[2]);
>>>> +}
>>>> diff --git a/tools/testing/selftests/kvm/x86_64/mmio_warning_test.c b/tools/testing/selftests/kvm/x86_64/mmio_warning_test.c
>>>> new file mode 100644
>>>> index 000000000000..00bb97d76000
>>>> --- /dev/null
>>>> +++ b/tools/testing/selftests/kvm/x86_64/mmio_warning_test.c
>>>> @@ -0,0 +1,126 @@
>>>> +/*
>>>> + * mmio_warning_test
>>>> + *
>>>> + * Copyright (C) 2019, Google LLC.
>>>> + *
>>>> + * This work is licensed under the terms of the GNU GPL, version 2.
>>>> + *
>>>> + * Test that we don't get a kernel warning when we call KVM_RUN after a
>>>> + * triple fault occurs.  To get the triple fault to occur we call KVM_RUN
>>>> + * on a VCPU that hasn't been properly setup.
>>>> + *
>>>> + */
>>>> +
>>>> +#define _GNU_SOURCE
>>>> +#include <fcntl.h>
>>>> +#include <kvm_util.h>
>>>> +#include <linux/kvm.h>
>>>> +#include <processor.h>
>>>> +#include <pthread.h>
>>>> +#include <stdio.h>
>>>> +#include <stdlib.h>
>>>> +#include <string.h>
>>>> +#include <sys/ioctl.h>
>>>> +#include <sys/mman.h>
>>>> +#include <sys/stat.h>
>>>> +#include <sys/types.h>
>>>> +#include <sys/wait.h>
>>>> +#include <test_util.h>
>>>> +#include <unistd.h>
>>>> +
>>>> +#define NTHREAD 4
>>>> +#define NPROCESS 5
>>>> +
>>>> +struct thread_context {
>>>> +       int kvmcpu;
>>>> +       struct kvm_run *run;
>>>> +};
>>>> +
>>>> +void *thr(void *arg)
>>>> +{
>>>> +       struct thread_context *tc = (struct thread_context *)arg;
>>>> +       int res;
>>>> +       int kvmcpu = tc->kvmcpu;
>>>> +       struct kvm_run *run = tc->run;
>>>> +
>>>> +       res = ioctl(kvmcpu, KVM_RUN, 0);
>>>> +       printf("ret1=%d exit_reason=%d suberror=%d\n",
>>>> +               res, run->exit_reason, run->internal.suberror);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +void test(void)
>>>> +{
>>>> +       int i, kvm, kvmvm, kvmcpu;
>>>> +       pthread_t th[NTHREAD];
>>>> +       struct kvm_run *run;
>>>> +       struct thread_context tc;
>>>> +
>>>> +       kvm = open("/dev/kvm", O_RDWR);
>>>> +       TEST_ASSERT(kvm != -1, "failed to open /dev/kvm");
>>>> +       kvmvm = ioctl(kvm, KVM_CREATE_VM, 0);
>>>> +       TEST_ASSERT(kvmvm != -1, "KVM_CREATE_VM failed");
>>>> +       kvmcpu = ioctl(kvmvm, KVM_CREATE_VCPU, 0);
>>>> +       TEST_ASSERT(kvmcpu != -1, "KVM_CREATE_VCPU failed");
>>>> +       run = (struct kvm_run *)mmap(0, 4096, PROT_READ|PROT_WRITE, MAP_SHARED,
>>>> +                                   kvmcpu, 0);
>>>> +       tc.kvmcpu = kvmcpu;
>>>> +       tc.run = run;
>>>> +       srand(getpid());
>>>> +       for (i = 0; i < NTHREAD; i++) {
>>>> +               pthread_create(&th[i], NULL, thr, (void *)(uintptr_t)&tc);
>>>> +               usleep(rand() % 10000);
>>>> +       }
>>>> +       for (i = 0; i < NTHREAD; i++)
>>>> +               pthread_join(th[i], NULL);
>>>> +}
>>>> +
>>>> +int get_warnings_count(void)
>>>> +{
>>>> +       int warnings;
>>>> +       FILE *f;
>>>> +
>>>> +       f = popen("dmesg | grep \"WARNING:\" | wc -l", "r");
>>>> +       fscanf(f, "%d", &warnings);
>>>> +       fclose(f);
>>>> +
>>>> +       return warnings;
>>>> +}
>>>> +
>>>> +int main(void)
>>>> +{
>>>> +       int warnings_before, warnings_after;
>>>> +
>>>> +       if (!is_intel_cpu()) {
>>>> +               printf("Must be run on an Intel CPU, skipping test\n");
>>>> +               exit(KSFT_SKIP);
>>>> +       }
>>>> +
>>>> +       if (vm_is_unrestricted_guest(NULL)) {
>>>> +               printf("Unrestricted guest must be disabled, skipping test\n");
>>>> +               exit(KSFT_SKIP);
>>>> +       }
>>>> +
>>>> +       warnings_before = get_warnings_count();
>>>> +
>>>> +       for (int i = 0; i < NPROCESS; ++i) {
>>>> +               int status;
>>>> +               int pid = fork();
>>>> +
>>>> +               if (pid < 0)
>>>> +                       exit(1);
>>>> +               if (pid == 0) {
>>>> +                       test();
>>>> +                       exit(0);
>>>> +               }
>>>> +               while (waitpid(pid, &status, __WALL) != pid)
>>>> +                       ;
>>>> +       }
>>>> +
>>>> +       warnings_after = get_warnings_count();
>> Since you are grep'ing for the word "WARNING",  is there a possibility
>> that the test can detect a false positive based on Warnings generated
>> due to some other cause while it ran ?
>>
> Yes, this is a possibility, however, it is still a warning and should
> still be dealt with.  We could special case the grep message to be
> more specific to the case we are dealing with here, but I'd prefer to
> keep it this way to alert on any warning.  That way other warnings,
> should they occur, are brought to our attention.


OK.  In that case, does it make sense to provide some additional info in 
the ASSERT message below ? dmesg can contain Warnings that may have 
occurred either before or after the run of this test and so we can at 
least link the false positives to this test.


>
>>>> +       TEST_ASSERT(warnings_before == warnings_after,
>>>> +                  "Warnings found in kernel.  Run 'dmesg' to inspect them.");
>>>> +
>>>> +       return 0;
>>>> +}
>>>> --
>>>> 2.22.0.rc1.311.g5d7573a151-goog
>>>>
>>> ping