| Message ID | 20220402174044.2263418-3-oupton@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Fix use-after-free in debugfs | expand |
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?
Hi Sean, On Mon, Apr 04, 2022 at 05:33:29PM +0000, Sean Christopherson wrote: > 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. Good point. > > 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? I'd really like to drop it altogether. I've actually looked at several instances of this printk firing internally, and all of it had to do with some leak in userspace. I'll pull this patch out of the series for v2 and maybe just propose we drop it altogether. -- Thanks, Oliver
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); dput(dent); mutex_unlock(&kvm_debugfs_lock); return 0;
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") Signed-off-by: Oliver Upton <oupton@google.com> --- virt/kvm/kvm_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)