tools/testing/selftests/x86/lam.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)
Static Analyis for bench_htab_mem.c with cppcheck:error
tools/testing/selftests/x86/lam.c:585:3:
error: Resource leak: file_fd [resourceLeak]
tools/testing/selftests/x86/lam.c:593:3:
error: Resource leak: file_fd [resourceLeak]
tools/testing/selftests/x86/lam.c:600:3:
error: Memory leak: fi [memleak]
tools/testing/selftests/x86/lam.c:1066:2:
error: Resource leak: fd [resourceLeak]
fix the issue by closing the file descriptors and
releasing the allocated memory.
Signed-off-by: Malaya Kumar Rout <malayarout91@gmail.com>
---
tools/testing/selftests/x86/lam.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/x86/lam.c b/tools/testing/selftests/x86/lam.c
index 4d4a76532dc9..0b43b83ad142 100644
--- a/tools/testing/selftests/x86/lam.c
+++ b/tools/testing/selftests/x86/lam.c
@@ -581,24 +581,28 @@ int do_uring(unsigned long lam)
if (file_fd < 0)
return 1;
- if (fstat(file_fd, &st) < 0)
+ if (fstat(file_fd, &st) < 0) {
+ close(file_fd);
return 1;
-
+ }
off_t file_sz = st.st_size;
int blocks = (int)(file_sz + URING_BLOCK_SZ - 1) / URING_BLOCK_SZ;
fi = malloc(sizeof(*fi) + sizeof(struct iovec) * blocks);
- if (!fi)
+ if (!fi) {
+ close(file_fd);
return 1;
-
+ }
fi->file_sz = file_sz;
fi->file_fd = file_fd;
ring = malloc(sizeof(*ring));
- if (!ring)
+ if (!ring) {
+ close(file_fd);
+ free(fi);
return 1;
-
+ }
memset(ring, 0, sizeof(struct io_ring));
if (setup_io_uring(ring))
@@ -1060,8 +1064,10 @@ void *allocate_dsa_pasid(void)
wq = mmap(NULL, 0x1000, PROT_WRITE,
MAP_SHARED | MAP_POPULATE, fd, 0);
- if (wq == MAP_FAILED)
+ if (wq == MAP_FAILED) {
+ close(fd);
perror("mmap");
+ }
return wq;
}
--
2.43.0On Mon, Mar 24, 2025 at 06:17:50PM +0530, Malaya Kumar Rout wrote: > Static Analyis for bench_htab_mem.c with cppcheck:error > tools/testing/selftests/x86/lam.c:585:3: > error: Resource leak: file_fd [resourceLeak] > tools/testing/selftests/x86/lam.c:593:3: > error: Resource leak: file_fd [resourceLeak] > tools/testing/selftests/x86/lam.c:600:3: > error: Memory leak: fi [memleak] > tools/testing/selftests/x86/lam.c:1066:2: > error: Resource leak: fd [resourceLeak] > > fix the issue by closing the file descriptors and > releasing the allocated memory. > But but but, doesn't the program just exit on any of those 'errors' anyway? That is, iirc this is a single shot program.
* Peter Zijlstra <peterz@infradead.org> wrote: > On Mon, Mar 24, 2025 at 06:17:50PM +0530, Malaya Kumar Rout wrote: > > Static Analyis for bench_htab_mem.c with cppcheck:error > > tools/testing/selftests/x86/lam.c:585:3: > > error: Resource leak: file_fd [resourceLeak] > > tools/testing/selftests/x86/lam.c:593:3: > > error: Resource leak: file_fd [resourceLeak] > > tools/testing/selftests/x86/lam.c:600:3: > > error: Memory leak: fi [memleak] > > tools/testing/selftests/x86/lam.c:1066:2: > > error: Resource leak: fd [resourceLeak] > > > > fix the issue by closing the file descriptors and > > releasing the allocated memory. > > > > But but but, doesn't the program just exit on any of those 'errors' > anyway? > > That is, iirc this is a single shot program. While that's true, still proper cleanup of resources is a good practice - and in more complicated tools it's useful to fix even these semi-false-positives, to make sure other warnings don't get missed. Having said that, the error/cleanup control flow here doesn't look overly clean here to begin with, so I'd suggest fixing that (with goto labels or such) - which would fix the file_fd 'leak' as a happy side effect. Thanks, Ingo
I appreciate all the feedback and recommendations provided. We will incorporate the same. Thanks & Regards, Malaya Kumar Rout On Tue, Mar 25, 2025 at 2:59 PM Ingo Molnar <mingo@kernel.org> wrote: > > > * Peter Zijlstra <peterz@infradead.org> wrote: > > > On Mon, Mar 24, 2025 at 06:17:50PM +0530, Malaya Kumar Rout wrote: > > > Static Analyis for bench_htab_mem.c with cppcheck:error > > > tools/testing/selftests/x86/lam.c:585:3: > > > error: Resource leak: file_fd [resourceLeak] > > > tools/testing/selftests/x86/lam.c:593:3: > > > error: Resource leak: file_fd [resourceLeak] > > > tools/testing/selftests/x86/lam.c:600:3: > > > error: Memory leak: fi [memleak] > > > tools/testing/selftests/x86/lam.c:1066:2: > > > error: Resource leak: fd [resourceLeak] > > > > > > fix the issue by closing the file descriptors and > > > releasing the allocated memory. > > > > > > > But but but, doesn't the program just exit on any of those 'errors' > > anyway? > > > > That is, iirc this is a single shot program. > > While that's true, still proper cleanup of resources is a good practice > - and in more complicated tools it's useful to fix even these > semi-false-positives, to make sure other warnings don't get missed. > > Having said that, the error/cleanup control flow here doesn't look > overly clean here to begin with, so I'd suggest fixing that (with goto > labels or such) - which would fix the file_fd 'leak' as a happy side > effect. > > Thanks, > > Ingo
Exception branch returns without closing
the file descriptors 'file_fd' and 'fd'
Signed-off-by: Malaya Kumar Rout <malayarout91@gmail.com>
:wq
---
tools/testing/selftests/x86/lam.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/x86/lam.c b/tools/testing/selftests/x86/lam.c
index 18d736640ece..1d3631ce4b69 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;
}
@@ -1189,9 +1191,10 @@ void *allocate_dsa_pasid(void)
wq = mmap(NULL, 0x1000, PROT_WRITE,
MAP_SHARED | MAP_POPULATE, fd, 0);
- if (wq == MAP_FAILED)
+ if (wq == MAP_FAILED){
+ close(fd);
perror("mmap");
-
+ }
return wq;
}
--
2.43.0
* Malaya Kumar Rout <malayarout91@gmail.com> wrote:
> @@ -1189,9 +1191,10 @@ void *allocate_dsa_pasid(void)
>
> wq = mmap(NULL, 0x1000, PROT_WRITE,
> MAP_SHARED | MAP_POPULATE, fd, 0);
> - if (wq == MAP_FAILED)
> + if (wq == MAP_FAILED){
> + close(fd);
> perror("mmap");
We should unconditionally close 'fd' after the mmap() call, not just in
the perror() branch.
Thanks,
Ingo
Exception branch returns without closing
the file descriptors 'file_fd' and 'fd'
Signed-off-by: Malaya Kumar Rout <malayarout91@gmail.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;
}
--
2.43.0
* Malaya Kumar Rout <malayarout91@gmail.com> wrote:
> Exception branch returns without closing
> the file descriptors 'file_fd' and 'fd'
>
> Signed-off-by: Malaya Kumar Rout <malayarout91@gmail.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
Exception branch returns without closing
the file descriptors 'file_fd' and 'fd'
Signed-off-by: Malaya Kumar Rout <malayarout91@gmail.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..0873b0e5f48b 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;
}
@@ -1189,6 +1191,7 @@ void *allocate_dsa_pasid(void)
wq = mmap(NULL, 0x1000, PROT_WRITE,
MAP_SHARED | MAP_POPULATE, fd, 0);
+ close(fd);
if (wq == MAP_FAILED)
perror("mmap");
--
2.43.0* Malaya Kumar Rout <malayarout91@gmail.com> wrote: > Exception branch returns without closing > the file descriptors 'file_fd' and 'fd' > > Signed-off-by: Malaya Kumar Rout <malayarout91@gmail.com> > --- > tools/testing/selftests/x86/lam.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) Why have you ignored this request I made: >> Finally, it would be nice to quote the before/after output of the >> leak detection tool you are using. ? Thanks, Ingo
This patch resolves resource leaks reported by cppcheck in lam.c.
Specifically, the 'file_fd' and 'fd' were not closed in do_uring()
and allocate_dsa_pasid() functions, respectively.
cppcheck output before this patch:
tools/testing/selftests/x86/lam.c:685:3: error: Resource leak: file_fd [resourceLeak]
tools/testing/selftests/x86/lam.c:693:3: error: Resource leak: file_fd [resourceLeak]
tools/testing/selftests/x86/lam.c:1195:2: error: Resource leak: fd [resourceLeak]
cppcheck output after this patch:
No resource leaks found
Signed-off-by: Malaya Kumar Rout <malayarout91@gmail.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..0873b0e5f48b 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;
}
@@ -1189,6 +1191,7 @@ void *allocate_dsa_pasid(void)
wq = mmap(NULL, 0x1000, PROT_WRITE,
MAP_SHARED | MAP_POPULATE, fd, 0);
+ close(fd);
if (wq == MAP_FAILED)
perror("mmap");
--
2.43.0The following commit has been merged into the x86/mm branch of tip:
Commit-ID: 60567e93c05d7064c93830cf4bf0d2c58f11b2f2
Gitweb: https://git.kernel.org/tip/60567e93c05d7064c93830cf4bf0d2c58f11b2f2
Author: Malaya Kumar Rout <malayarout91@gmail.com>
AuthorDate: Wed, 09 Apr 2025 19:23:37 +05:30
Committer: Ingo Molnar <mingo@kernel.org>
CommitterDate: Wed, 09 Apr 2025 21:30:37 +02:00
selftests/x86/lam: Fix clean up fds in do_uring() and allocate_dsa_pasid()
Resolve minor fd leaks reported by cppcheck in lam.c.
Specifically, the 'file_fd' and 'fd' were not closed in do_uring()
and allocate_dsa_pasid() functions, respectively.
cppcheck output before this patch:
tools/testing/selftests/x86/lam.c:685:3: error: Resource leak: file_fd [resourceLeak]
tools/testing/selftests/x86/lam.c:693:3: error: Resource leak: file_fd [resourceLeak]
tools/testing/selftests/x86/lam.c:1195:2: error: Resource leak: fd [resourceLeak]
cppcheck output after this patch:
No resource leaks found
While this is a standalone test tool that doesn't really leak anything
in practice, as exit() cleans it up all, clean up resources nevertheless.
[ mingo: Updated the changelog. ]
Signed-off-by: Malaya Kumar Rout <malayarout91@gmail.com>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Link: https://lore.kernel.org/r/20250409135341.28987-1-malayarout91@gmail.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 18d7366..0873b0e 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 @@ out:
}
free(fi);
+cleanup:
+ close(file_fd);
return ret;
}
@@ -1189,6 +1191,7 @@ void *allocate_dsa_pasid(void)
wq = mmap(NULL, 0x1000, PROT_WRITE,
MAP_SHARED | MAP_POPULATE, fd, 0);
+ close(fd);
if (wq == MAP_FAILED)
perror("mmap");© 2016 - 2025 Red Hat, Inc.