Re: [RFC PATCH kernel] KVM: Stop leaking memory in debugfs

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Aug 03, 2021 at 02:52:17PM +0200, Paolo Bonzini wrote:
> On Tue, Aug 3, 2021 at 1:16 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Fri, Jul 30, 2021 at 02:32:17PM +1000, Alexey Kardashevskiy wrote:
> > >       snprintf(dir_name, sizeof(dir_name), "%d-%d", task_pid_nr(current), fd);
> > >       kvm->debugfs_dentry = debugfs_create_dir(dir_name, kvm_debugfs_dir);
> > > +     if (IS_ERR_OR_NULL(kvm->debugfs_dentry)) {
> > > +             pr_err("Failed to create %s\n", dir_name);
> > > +             return 0;
> > > +     }
> >
> > It should not matter if you fail a debugfs call at all.
> >
> > If there is a larger race at work here, please fix that root cause, do
> > not paper over it by attempting to have debugfs catch the issue for you.
> 
> I don't think it's a race, it's really just a bug that is intrinsic in how
> the debugfs files are named.  You can just do something like this:
> 
> #include <unistd.h>
> #include <stdio.h>
> #include <fcntl.h>
> #include <sys/wait.h>
> #include <sys/ioctl.h>
> #include <linux/kvm.h>
> #include <stdlib.h>
> int main() {
>         int kvmfd = open("/dev/kvm", O_RDONLY);
>         int fd = ioctl(kvmfd, KVM_CREATE_VM, 0);
>         if (fork() == 0) {
>                 printf("before: %d\n", fd);
>                 sleep(2);
>         } else {
>                 close(fd);
>                 sleep(1);
>                 int fd = ioctl(kvmfd, KVM_CREATE_VM, 0);
>                 printf("after: %d\n", fd);
>                 wait(NULL);
>         }
> }
> 
> So Alexey's patch is okay and I've queued it, though with pr_warn_ratelimited
> instead of pr_err.

So userspace can create kvm resources with duplicate names?  That feels
wrong to me.

But if all that is "duplicate" is the debugfs kvm directory, why not ask
debugfs if it is already present before trying to create it again?  That
way you will not have debugfs complain about duplicate
files/directories.

thanks,

greg k-h