* Re: [PATCH] KVM: Do not leak memory for duplicate debugfs directories
2021-08-04 9:37 [PATCH] KVM: Do not leak memory for duplicate debugfs directories Paolo Bonzini
@ 2021-08-04 9:56 ` Greg Kroah-Hartman
2021-08-04 23:32 ` Alexey Kardashevskiy
2021-08-05 5:49 ` Alexey Kardashevskiy
2 siblings, 0 replies; 5+ messages in thread
From: Greg Kroah-Hartman @ 2021-08-04 9:56 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, kvm, stable, Alexey Kardashevskiy
On Wed, Aug 04, 2021 at 05:37:37AM -0400, Paolo Bonzini wrote:
> KVM creates a debugfs directory for each VM in order to store statistics
> about the virtual machine. The directory name is built from the process
> pid and a VM fd. While generally unique, it is possible to keep a
> file descriptor alive in a way that causes duplicate directories, which
> manifests as these messages:
>
> [ 471.846235] debugfs: Directory '20245-4' with parent 'kvm' already present!
>
> Even though this should not happen in practice, it is more or less
> expected in the case of KVM for testcases that call KVM_CREATE_VM and
> close the resulting file descriptor repeatedly and in parallel.
>
> When this happens, debugfs_create_dir() returns an error but
> kvm_create_vm_debugfs() goes on to allocate stat data structs which are
> later leaked. The slow memory leak was spotted by syzkaller, where it
> caused OOM reports.
>
> Since the issue only affects debugfs, do a lookup before calling
> debugfs_create_dir, so that the message is downgraded and rate-limited.
> While at it, ensure kvm->debugfs_dentry is NULL rather than an error
> if it is not created. This fixes kvm_destroy_vm_debugfs, which was not
> checking IS_ERR_OR_NULL correctly.
>
> Cc: stable@vger.kernel.org
> Fixes: 536a6f88c49d ("KVM: Create debugfs dir and stat files for each VM")
> Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> virt/kvm/kvm_main.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] KVM: Do not leak memory for duplicate debugfs directories
2021-08-04 9:37 [PATCH] KVM: Do not leak memory for duplicate debugfs directories Paolo Bonzini
2021-08-04 9:56 ` Greg Kroah-Hartman
@ 2021-08-04 23:32 ` Alexey Kardashevskiy
2021-08-04 23:53 ` Alexey Kardashevskiy
2021-08-05 5:49 ` Alexey Kardashevskiy
2 siblings, 1 reply; 5+ messages in thread
From: Alexey Kardashevskiy @ 2021-08-04 23:32 UTC (permalink / raw)
To: Paolo Bonzini, linux-kernel, kvm; +Cc: stable, Greg Kroah-Hartman
On 04/08/2021 19:37, Paolo Bonzini wrote:
> KVM creates a debugfs directory for each VM in order to store statistics
> about the virtual machine. The directory name is built from the process
> pid and a VM fd. While generally unique, it is possible to keep a
> file descriptor alive in a way that causes duplicate directories, which
> manifests as these messages:
>
> [ 471.846235] debugfs: Directory '20245-4' with parent 'kvm' already present!
>
> Even though this should not happen in practice, it is more or less
> expected in the case of KVM for testcases that call KVM_CREATE_VM and
> close the resulting file descriptor repeatedly and in parallel.
>
> When this happens, debugfs_create_dir() returns an error but
> kvm_create_vm_debugfs() goes on to allocate stat data structs which are
> later leaked.
Rather the already allocated srructs leak, no?
> The slow memory leak was spotted by syzkaller, where it
> caused OOM reports.
I gave it a try and almost replied with "tested-by" but after running it
over night I got 1 of these with followed OOM:
[ 1104.951394][ T544] debugfs: Directory '544-4' with parent 'kvm'
already present!
[ 1104.951600][ T544] Failed to create 544-4
This is definitely an improvement as this used to happen a few times
every hour but still puzzling :-/
>
> Since the issue only affects debugfs, do a lookup before calling
> debugfs_create_dir, so that the message is downgraded and rate-limited.
> While at it, ensure kvm->debugfs_dentry is NULL rather than an error
> if it is not created. This fixes kvm_destroy_vm_debugfs, which was not
> checking IS_ERR_OR_NULL correctly.
>
> Cc: stable@vger.kernel.org
> Fixes: 536a6f88c49d ("KVM: Create debugfs dir and stat files for each VM")
> Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> virt/kvm/kvm_main.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index d20fba0fc290..b50dbe269f4b 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -892,6 +892,8 @@ static void kvm_destroy_vm_debugfs(struct kvm *kvm)
>
> static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
> {
> + static DEFINE_MUTEX(kvm_debugfs_lock);
> + struct dentry *dent;
> char dir_name[ITOA_MAX_LEN * 2];
> struct kvm_stat_data *stat_data;
> const struct _kvm_stats_desc *pdesc;
> @@ -903,8 +905,20 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
> return 0;
>
> snprintf(dir_name, sizeof(dir_name), "%d-%d", task_pid_nr(current), fd);
> - kvm->debugfs_dentry = debugfs_create_dir(dir_name, kvm_debugfs_dir);
> + 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);
> + dput(dent);
> + mutex_unlock(&kvm_debugfs_lock);
> + return 0;
> + }
> + dent = debugfs_create_dir(dir_name, kvm_debugfs_dir);
> + mutex_unlock(&kvm_debugfs_lock);
> + if (IS_ERR(dent))
> + return 0;
>
> + kvm->debugfs_dentry = dent;
> kvm->debugfs_stat_data = kcalloc(kvm_debugfs_num_entries,
> sizeof(*kvm->debugfs_stat_data),
> GFP_KERNEL_ACCOUNT);
> @@ -5201,7 +5215,7 @@ static void kvm_uevent_notify_change(unsigned int type, struct kvm *kvm)
> }
> add_uevent_var(env, "PID=%d", kvm->userspace_pid);
>
> - if (!IS_ERR_OR_NULL(kvm->debugfs_dentry)) {
> + if (kvm->debugfs_dentry) {
> char *tmp, *p = kmalloc(PATH_MAX, GFP_KERNEL_ACCOUNT);
>
> if (p) {
>
--
Alexey
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] KVM: Do not leak memory for duplicate debugfs directories
2021-08-04 23:32 ` Alexey Kardashevskiy
@ 2021-08-04 23:53 ` Alexey Kardashevskiy
0 siblings, 0 replies; 5+ messages in thread
From: Alexey Kardashevskiy @ 2021-08-04 23:53 UTC (permalink / raw)
To: Paolo Bonzini, linux-kernel, kvm; +Cc: stable, Greg Kroah-Hartman
On 05/08/2021 09:32, Alexey Kardashevskiy wrote:
>
>
> On 04/08/2021 19:37, Paolo Bonzini wrote:
>> KVM creates a debugfs directory for each VM in order to store statistics
>> about the virtual machine. The directory name is built from the process
>> pid and a VM fd. While generally unique, it is possible to keep a
>> file descriptor alive in a way that causes duplicate directories, which
>> manifests as these messages:
>>
>> [ 471.846235] debugfs: Directory '20245-4' with parent 'kvm'
>> already present!
>>
>> Even though this should not happen in practice, it is more or less
>> expected in the case of KVM for testcases that call KVM_CREATE_VM and
>> close the resulting file descriptor repeatedly and in parallel.
>>
>> When this happens, debugfs_create_dir() returns an error but
>> kvm_create_vm_debugfs() goes on to allocate stat data structs which are
>> later leaked.
>
> Rather the already allocated srructs leak, no?
>
>> The slow memory leak was spotted by syzkaller, where it
>> caused OOM reports.
>
> I gave it a try and almost replied with "tested-by" but after running it
> over night I got 1 of these with followed OOM:
>
> [ 1104.951394][ T544] debugfs: Directory '544-4' with parent 'kvm'
> already present!
> [ 1104.951600][ T544] Failed to create 544-4
>
> This is definitely an improvement as this used to happen a few times
> every hour but still puzzling :-/
ah rats, I was running a wrong kernel, retrying now. sorry for the noise.
--
Alexey
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] KVM: Do not leak memory for duplicate debugfs directories
2021-08-04 9:37 [PATCH] KVM: Do not leak memory for duplicate debugfs directories Paolo Bonzini
2021-08-04 9:56 ` Greg Kroah-Hartman
2021-08-04 23:32 ` Alexey Kardashevskiy
@ 2021-08-05 5:49 ` Alexey Kardashevskiy
2 siblings, 0 replies; 5+ messages in thread
From: Alexey Kardashevskiy @ 2021-08-05 5:49 UTC (permalink / raw)
To: Paolo Bonzini, linux-kernel, kvm; +Cc: stable, Greg Kroah-Hartman
On 8/4/21 19:37, Paolo Bonzini wrote:
> KVM creates a debugfs directory for each VM in order to store statistics
> about the virtual machine. The directory name is built from the process
> pid and a VM fd. While generally unique, it is possible to keep a
> file descriptor alive in a way that causes duplicate directories, which
> manifests as these messages:
>
> [ 471.846235] debugfs: Directory '20245-4' with parent 'kvm' already present!
>
> Even though this should not happen in practice, it is more or less
> expected in the case of KVM for testcases that call KVM_CREATE_VM and
> close the resulting file descriptor repeatedly and in parallel.
>
> When this happens, debugfs_create_dir() returns an error but
> kvm_create_vm_debugfs() goes on to allocate stat data structs which are
> later leaked. The slow memory leak was spotted by syzkaller, where it
> caused OOM reports.
>
> Since the issue only affects debugfs, do a lookup before calling
> debugfs_create_dir, so that the message is downgraded and rate-limited.
> While at it, ensure kvm->debugfs_dentry is NULL rather than an error
> if it is not created. This fixes kvm_destroy_vm_debugfs, which was not
> checking IS_ERR_OR_NULL correctly.
>
> Cc: stable@vger.kernel.org
> Fixes: 536a6f88c49d ("KVM: Create debugfs dir and stat files for each VM")
> Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
after another try, works brilliant.
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Tested-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Thanks,
> ---
> virt/kvm/kvm_main.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index d20fba0fc290..b50dbe269f4b 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -892,6 +892,8 @@ static void kvm_destroy_vm_debugfs(struct kvm *kvm)
>
> static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
> {
> + static DEFINE_MUTEX(kvm_debugfs_lock);
> + struct dentry *dent;
> char dir_name[ITOA_MAX_LEN * 2];
> struct kvm_stat_data *stat_data;
> const struct _kvm_stats_desc *pdesc;
> @@ -903,8 +905,20 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
> return 0;
>
> snprintf(dir_name, sizeof(dir_name), "%d-%d", task_pid_nr(current), fd);
> - kvm->debugfs_dentry = debugfs_create_dir(dir_name, kvm_debugfs_dir);
> + 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);
> + dput(dent);
> + mutex_unlock(&kvm_debugfs_lock);
> + return 0;
> + }
> + dent = debugfs_create_dir(dir_name, kvm_debugfs_dir);
> + mutex_unlock(&kvm_debugfs_lock);
> + if (IS_ERR(dent))
> + return 0;
>
> + kvm->debugfs_dentry = dent;
> kvm->debugfs_stat_data = kcalloc(kvm_debugfs_num_entries,
> sizeof(*kvm->debugfs_stat_data),
> GFP_KERNEL_ACCOUNT);
> @@ -5201,7 +5215,7 @@ static void kvm_uevent_notify_change(unsigned int type, struct kvm *kvm)
> }
> add_uevent_var(env, "PID=%d", kvm->userspace_pid);
>
> - if (!IS_ERR_OR_NULL(kvm->debugfs_dentry)) {
> + if (kvm->debugfs_dentry) {
> char *tmp, *p = kmalloc(PATH_MAX, GFP_KERNEL_ACCOUNT);
>
> if (p) {
>
--
Alexey
^ permalink raw reply [flat|nested] 5+ messages in thread