Re: [PATCH 2/4] KVM: Only log about debugfs directory collision once

From: Sean Christopherson <seanjc@google.com>
To: Oliver Upton <oupton@google.com>
Cc: kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org,
	Marc Zyngier <maz@kernel.org>, James Morse <james.morse@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	linux-arm-kernel@lists.infradead.org,
	Peter Shier <pshier@google.com>,
	Ricardo Koller <ricarkol@google.com>,
	Reiji Watanabe <reijiw@google.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	stable@kernel.org
Subject: Re: [PATCH 2/4] KVM: Only log about debugfs directory collision once
Date: Mon, 4 Apr 2022 17:33:29 +0000	[thread overview]
Message-ID: <Yksr6etwnN0iW8ZH@google.com> (raw)
In-Reply-To: <20220402174044.2263418-3-oupton@google.com>

On Sat, Apr 02, 2022, Oliver Upton wrote:
> In all likelihood, a debugfs directory name collision is the result of a
> userspace bug. If userspace closes the VM fd without releasing all
> references to said VM then the debugfs directory is never cleaned.
> 
> Even a ratelimited print statement can fill up dmesg, making it
> particularly annoying for the person debugging what exactly went wrong.
> Furthermore, a userspace that wants to be a nuisance could clog up the
> logs by deliberately holding a VM reference after closing the VM fd.
> 
> Dial back logging to print at most once, given that userspace is most
> likely to blame. Leave the statement in place for the small chance that
> KVM actually got it wrong.
> 
> Cc: stable@kernel.org
> Fixes: 85cd39af14f4 ("KVM: Do not leak memory for duplicate debugfs directories")

I don't think this warrants Cc: stable@, the whole point of ratelimiting printk is
to guard against this sort of thing.  If a ratelimited printk can bring down the
kernel and/or logging infrastructure, then the kernel is misconfigured for the
environment.

> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  virt/kvm/kvm_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 69c318fdff61..38b30bd60f34 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -959,7 +959,7 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
>  	mutex_lock(&kvm_debugfs_lock);
>  	dent = debugfs_lookup(dir_name, kvm_debugfs_dir);
>  	if (dent) {
> -		pr_warn_ratelimited("KVM: debugfs: duplicate directory %s\n", dir_name);
> +		pr_warn_once("KVM: debugfs: duplicate directory %s\n", dir_name);

I don't see how printing once is going to be usefull for a human debugger.  If we
want to get rid of the ratelimited print, why not purge it entirely?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel