linux-kernel - Re: [PATCH v3] selftests/x86/lam: fix resource leak in do_uring() and allocate_dsa

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z_TZ138UxQ_uZzys@gmail.com>
Date: Tue, 8 Apr 2025 10:09:59 +0200
From: Ingo Molnar <mingo@...nel.org>
To: Malaya Kumar Rout <malayarout91@...il.com>
Cc: Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
	Borislav Petkov <bp@...en8.de>,
	Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
	"H. Peter Anvin" <hpa@...or.com>, Shuah Khan <shuah@...nel.org>,
	linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v3] selftests/x86/lam: fix resource leak in do_uring()
 and allocate_dsa_pasid()


* Malaya Kumar Rout <malayarout91@...il.com> wrote:

> Exception branch returns without closing
> the file descriptors 'file_fd' and 'fd'
> 
> Signed-off-by: Malaya Kumar Rout <malayarout91@...il.com>
> ---
>  tools/testing/selftests/x86/lam.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/x86/lam.c b/tools/testing/selftests/x86/lam.c
> index 18d736640ece..88482d8112de 100644
> --- a/tools/testing/selftests/x86/lam.c
> +++ b/tools/testing/selftests/x86/lam.c
> @@ -682,7 +682,7 @@ int do_uring(unsigned long lam)
>  		return 1;
>  
>  	if (fstat(file_fd, &st) < 0)
> -		return 1;
> +		goto cleanup;
>  
>  	off_t file_sz = st.st_size;
>  
> @@ -690,7 +690,7 @@ int do_uring(unsigned long lam)
>  
>  	fi = malloc(sizeof(*fi) + sizeof(struct iovec) * blocks);
>  	if (!fi)
> -		return 1;
> +		goto cleanup;
>  
>  	fi->file_sz = file_sz;
>  	fi->file_fd = file_fd;
> @@ -698,7 +698,7 @@ int do_uring(unsigned long lam)
>  	ring = malloc(sizeof(*ring));
>  	if (!ring) {
>  		free(fi);
> -		return 1;
> +		goto cleanup;
>  	}
>  
>  	memset(ring, 0, sizeof(struct io_ring));
> @@ -729,6 +729,8 @@ int do_uring(unsigned long lam)
>  	}
>  
>  	free(fi);
> +cleanup:
> +	close(file_fd);
>  
>  	return ret;
>  }
> @@ -1192,6 +1194,7 @@ void *allocate_dsa_pasid(void)
>  	if (wq == MAP_FAILED)
>  		perror("mmap");
>  
> +	close(fd);
>  	return wq;

So in your previous patch you closed the file before the perror(), 
presumably so that file-leak detection in Valgrind or whatever tool you 
are using doesn't trigger.

But here it's done after the perror() call, why? It's perfectly fine to 
close the mapping fd straight after an mmap() call.

Finally, it would be nice to quote the before/after output of the leak 
detection tool you are using.

Thanks,

	Ingo