From: Alexey Kardashevskiy <aik@ozlabs.ru>
To: Paolo Bonzini <pbonzini@redhat.com>,
Greg KH <gregkh@linuxfoundation.org>
Cc: "Kernel Mailing List, Linux" <linux-kernel@vger.kernel.org>,
kvm <kvm@vger.kernel.org>
Subject: Re: [RFC PATCH kernel] KVM: Stop leaking memory in debugfs
Date: Tue, 3 Aug 2021 23:13:57 +1000 [thread overview]
Message-ID: <a0665d21-a390-6263-0018-09b4cb57e87b@ozlabs.ru> (raw)
In-Reply-To: <CABgObfb+M9Qeow1EZy+eQwM1jwoZY3zdPJfZW+Q+MoWmkaqcFw@mail.gmail.com>
On 03/08/2021 22:52, 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);
> }
> }
oh nice demo :)
although I still think there was a race when I saw it as there was no
fork() in the picture but continuous create/destroy VM in 16 threads on
16 VCPUs with no KVM_RUN in between.
>
> So Alexey's patch is okay and I've queued it, though with pr_warn_ratelimited
> instead of pr_err.
Makes sense with your reproducer. Thanks,
--
Alexey
prev parent reply other threads:[~2021-08-03 13:14 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-30 4:32 [RFC PATCH kernel] KVM: Stop leaking memory in debugfs Alexey Kardashevskiy
2021-08-03 11:16 ` Greg KH
2021-08-03 12:52 ` Paolo Bonzini
2021-08-03 13:11 ` Greg KH
2021-08-03 13:29 ` Paolo Bonzini
2021-08-03 13:13 ` Alexey Kardashevskiy [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a0665d21-a390-6263-0018-09b4cb57e87b@ozlabs.ru \
--to=aik@ozlabs.ru \
--cc=gregkh@linuxfoundation.org \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pbonzini@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.