LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] KVM: Do not leak memory for duplicate debugfs directories
@ 2021-08-04  9:37 Paolo Bonzini
  2021-08-04  9:56 ` Greg Kroah-Hartman
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Paolo Bonzini @ 2021-08-04  9:37 UTC (permalink / raw)
  To: linux-kernel, kvm; +Cc: stable, Alexey Kardashevskiy, Greg Kroah-Hartman

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(-)

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) {
-- 
2.27.0


^ 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: 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

end of thread, other threads:[~2021-08-05  5:49 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).