Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <b2d4af96-ea66-982f-54c0-919546d39aa3@ozlabs.ru>
Date:   Thu, 5 Aug 2021 15:49:05 +1000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:88.0) Gecko/20100101
 Thunderbird/88.0
Subject: Re: [PATCH] KVM: Do not leak memory for duplicate debugfs directories
Content-Language: en-US
To:     Paolo Bonzini <pbonzini@redhat.com>, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org
Cc:     stable@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
References: <20210804093737.2536206-1-pbonzini@redhat.com>
From:   Alexey Kardashevskiy <aik@ozlabs.ru>
In-Reply-To: <20210804093737.2536206-1-pbonzini@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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