linux-kernel - Re: RFC: silencing kvm unimplemented msr spew.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 19 Jul 2016 16:24:31 -0400
From:	Bandan Das <bsd@...hat.com>
To:	Dave Jones <dsj@...com>
Cc:	Paolo Bonzini <pbonzini@...hat.com>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	Radim Krčmář <rkrcmar@...hat.com>,
	kvm@...r.kernel.org
Subject: Re: RFC: silencing kvm unimplemented msr spew.

Dave Jones <dsj@...com> writes:

> On Mon, Jul 18, 2016 at 11:26:50AM -0400, Paolo Bonzini wrote:
>  > 
>  > > kvm is pretty noisy when you have guests poking at MSRs that the kernel
>  > > doesn't implement.  The conveniently named 'ignore_msrs' option initially
>  > > seemed
>  > > like it was what I was looking for, but it changes the printk instead
>  > > of eliding it.
>  > > 
>  > > Untested patch below converts ignore_msrs to a bitmask and adds an option to
>  > > be
>  > > completely silent. The idea being if after testing, things still work and you
>  > > don't care about those messages, you can deploy in production with the
>  > > silence option.
>  > > 
>  > > Would something like this be acceptable ?
>  > 
>  > Indeed, ignore_msrs does a completely different thing.  It suppresses
>  > general protection faults in the guest.  It is related to behavior that
>  > KVM injects in the guests, not to the things that KVM spews in the host.
>  > 
>  > What about just downgrading the printf to KERN_DEBUG?  You could simply
>  > change from vcpu_unimpl to vcpu_debug, but it's probably a good idea to
>  > keep the ratelimiting; there's a kvm_pr_unimpl, so maybe add a new
>  > kvm_pr_debug and vcpu_pr_debug.
>
> Hm, we've certainly got a lot of options in terms of print primitives these days.
>
> We could just do this...

Heh, actually after speaking about this to Paolo a while back, I had this sleeping
in my local branch for a while. Same as what you suggested (without the ratelimiting)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index def97b3..c6e6f64 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -4952,7 +4952,7 @@ void kvm_mmu_invalidate_mmio_sptes(struct kvm *kvm, struct kvm_memslots *slots)
         * zap all shadow pages.
         */
        if (unlikely((slots->generation & MMIO_GEN_MASK) == 0)) {
-               printk_ratelimited(KERN_DEBUG "kvm: zapping shadow pages for mmio generation wraparound\n");
+               kvm_debug("zapping shadow pages for mmio generation wraparound\n");
                kvm_mmu_invalidate_zap_all_pages(kvm);
        }
 }
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 7da5dd2..02d09f9 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2229,7 +2229,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
                if (kvm_pmu_is_valid_msr(vcpu, msr))
                        return kvm_pmu_set_msr(vcpu, msr_info);
                if (!ignore_msrs) {
-                       vcpu_unimpl(vcpu, "unhandled wrmsr: 0x%x data %llx\n",
+                       vcpu_debug(vcpu, "unhandled wrmsr: 0x%x data %llx\n",
                                    msr, data);
                        return 1;
                } else {
@@ -2441,7 +2441,7 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
                if (kvm_pmu_is_valid_msr(vcpu, msr_info->index))
                        return kvm_pmu_get_msr(vcpu, msr_info->index, &msr_info->data);
                if (!ignore_msrs) {
-                       vcpu_unimpl(vcpu, "unhandled rdmsr: 0x%x\n", msr_info->index);
+                       vcpu_debug(vcpu, "unhandled rdmsr: 0x%x\n", msr_info->index);
                        return 1;
                } else {
                        vcpu_unimpl(vcpu, "ignored rdmsr: 0x%x\n", msr_info->index);

I had the same reasoning regarding  dynamic debugging which I think is
enabled by default on most builds anyway.

Bandan

> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 1c9c973a7dd9..a80b9a0a5f8c 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -423,7 +423,7 @@ struct kvm {
>  #define kvm_debug(fmt, ...) \
>         pr_debug("kvm [%i]: " fmt, task_pid_nr(current), ## __VA_ARGS__)
>  #define kvm_pr_unimpl(fmt, ...) \
> -       pr_err_ratelimited("kvm [%i]: " fmt, \
> +       pr_debug_ratelimited("kvm [%i]: " fmt, \
>                            task_tgid_nr(current), ## __VA_ARGS__)
>  
>  /* The guest did something we don't support. */
>
>
> Which I think would have the desired effect, and also gets us dynamic debug
> support for free.
>
> Thoughts ?
>
> 	Dave
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html