[LTP] [PATCH v2 1/3] memcg/memcontrol02: fix fd leak in cleanup path

[LTP] [PATCH v2 1/3] memcg/memcontrol02: fix fd leak in cleanup path

[Disha Goel]

Initialize fd to -1 to distinguish an unopened descriptor, and add
a cleanup guard so SAFE_CLOSE is only called when fd was actually
opened. Without this, cleanup would call close(-1) causing TBROK
when the anon test variant (n=0) runs and never opens fd.

Signed-off-by: Disha Goel <disgoel@linux.ibm.com>
---
v1 -> v2:
- Fix commit message: clarify that fd=0 would close stdin (not silently
  skip), and that the guard is added by this patch not pre-existing
- Use 'fd != -1' instead of 'fd > -1' per LTP convention

 testcases/kernel/controllers/memcg/memcontrol02.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/controllers/memcg/memcontrol02.c b/testcases/kernel/controllers/memcg/memcontrol02.c
index 0b79403c9..52096f697 100644
--- a/testcases/kernel/controllers/memcg/memcontrol02.c
+++ b/testcases/kernel/controllers/memcg/memcontrol02.c
@@ -29,7 +29,7 @@
 
 static size_t page_size;
 static struct tst_cg_group *cg_child;
-static int fd;
+static int fd = -1;
 static int file_to_all_error = 10;
 
 static void alloc_anon_50M_check(void)
@@ -128,6 +128,8 @@ static void cleanup(void)
 {
 	if (cg_child)
 		cg_child = tst_cg_group_rm(cg_child);
+	if (fd != -1)
+		SAFE_CLOSE(fd);
 }
 
 static struct tst_test test = {
-- 
2.45.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v2 2/3] memcg/memcontrol03: fix typos, redundant define and format specifiers

[Disha Goel]

Fix typos in doc comment ('tempfs' -> 'tmpfs', 'becaue' -> 'because'),
remove '#define TMPDIR \"mntdir\"' which duplicates the definition already
in memcontrol_common.h, replace PRIdPTR with %zu for size_t arguments
(PRIdPTR is for signed intptr_t, not unsigned size_t), fix continuation
line indentation in tst_res() calls, and remove now-unused <inttypes.h>
include.

Signed-off-by: Disha Goel <disgoel@linux.ibm.com>
---
v1 -> v2:
- Remove now-unused <inttypes.h> include (PRIdPTR was its only user)
- Split from combined memcontrol03+04 patch into separate patch per file

 testcases/kernel/controllers/memcg/memcontrol03.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/testcases/kernel/controllers/memcg/memcontrol03.c b/testcases/kernel/controllers/memcg/memcontrol03.c
index 493e970ab..96224c4ee 100644
--- a/testcases/kernel/controllers/memcg/memcontrol03.c
+++ b/testcases/kernel/controllers/memcg/memcontrol03.c
@@ -27,8 +27,8 @@
  * pagecache even in this case."
  *
  * memory.min doesn't appear to exist on V1 so we only test on V2 like
- * the selftest. We do test on more file systems, but not tempfs
- * becaue it can't evict the page cache without swap. Also we avoid
+ * the selftest. We do test on more file systems, but not tmpfs
+ * because it can't evict the page cache without swap. Also we avoid
  * filesystems which allocate extra memory for buffer heads.
  *
  * The tolerances have been increased from the self tests.
@@ -36,12 +36,8 @@
 
 #define _GNU_SOURCE
 
-#include <inttypes.h>
-
 #include "memcontrol_common.h"
 
-#define TMPDIR "mntdir"
-
 static struct tst_cg_group *trunk_cg[3];
 static struct tst_cg_group *leaf_cg[4];
 static int fd = -1;
@@ -105,8 +101,8 @@ static void alloc_anon_in_child(const struct tst_cg_group *const cg,
 		SAFE_CG_SCANF(cg, "memory.current", "%zu", &cgmem);
 		size = size > cgmem ? size - cgmem : 0;
 
-		tst_res(TINFO, "Child %d in %s: Allocating anon: %"PRIdPTR,
-		getpid(), tst_cg_group_name(cg), size);
+		tst_res(TINFO, "Child %d in %s: Allocating anon: %zu",
+			getpid(), tst_cg_group_name(cg), size);
 
 		if (size)
 			alloc_anon(size);
@@ -148,7 +144,7 @@ static void alloc_pagecache_in_child(const struct tst_cg_group *const cg,
 	SAFE_CG_SCANF(cg, "memory.current", "%zu", &cgmem);
 	size = size > cgmem ? size - cgmem : 0;
 
-	tst_res(TINFO, "Child %d in %s: Allocating pagecache: %"PRIdPTR,
+	tst_res(TINFO, "Child %d in %s: Allocating pagecache: %zu",
 		getpid(), tst_cg_group_name(cg), size);
 
 	if (size)
-- 
2.45.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v2 3/3] memcg/memcontrol04: fix typos, redundant define and format specifiers

[Disha Goel]

Fix typos in doc comment ('tempfs' -> 'tmpfs', 'becaue' -> 'because'),
remove '#define TMPDIR \"mntdir\"' which duplicates the definition already
in memcontrol_common.h, replace PRIdPTR with %zu for size_t arguments
(PRIdPTR is for signed intptr_t, not unsigned size_t), fix 'dont not'
-> 'do not' in inline comment, and remove now-unused <inttypes.h>
include.

Signed-off-by: Disha Goel <disgoel@linux.ibm.com>
---
v1 -> v2:
- Remove now-unused <inttypes.h> include (PRIdPTR was its only user)
- Split from combined memcontrol03+04 patch into separate patch per file

 testcases/kernel/controllers/memcg/memcontrol04.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/testcases/kernel/controllers/memcg/memcontrol04.c b/testcases/kernel/controllers/memcg/memcontrol04.c
index 15d8f891c..22ca1df8c 100644
--- a/testcases/kernel/controllers/memcg/memcontrol04.c
+++ b/testcases/kernel/controllers/memcg/memcontrol04.c
@@ -28,7 +28,7 @@
  * The closest thing to memory.low on V1 is soft_limit_in_bytes which
  * uses a different mechanism and has different semantics. So we only
  * test on V2 like the selftest. We do test on more file systems, but
- * not tempfs becaue it can't evict the page cache without swap. Also
+ * not tmpfs because it can't evict the page cache without swap. Also
  * we avoid filesystems which allocate extra memory for buffer heads.
  *
  * The tolerances have been increased from the self tests.
@@ -36,12 +36,8 @@
 
 #define _GNU_SOURCE
 
-#include <inttypes.h>
-
 #include "memcontrol_common.h"
 
-#define TMPDIR "mntdir"
-
 static struct tst_cg_group *trunk_cg[3];
 static struct tst_cg_group *leaf_cg[4];
 static int fd = -1;
@@ -102,7 +98,7 @@ static void alloc_anon_in_child(const struct tst_cg_group *const cg,
 
 	SAFE_CG_PRINTF(cg, "cgroup.procs", "%d", getpid());
 
-	tst_res(TINFO, "Child %d in %s: Allocating anon: %"PRIdPTR,
+	tst_res(TINFO, "Child %d in %s: Allocating anon: %zu",
 		getpid(), tst_cg_group_name(cg), size);
 	alloc_anon(size);
 
@@ -122,7 +118,7 @@ static void alloc_pagecache_in_child(const struct tst_cg_group *const cg,
 
 	SAFE_CG_PRINTF(cg, "cgroup.procs", "%d", getpid());
 
-	tst_res(TINFO, "Child %d in %s: Allocating pagecache: %"PRIdPTR,
+	tst_res(TINFO, "Child %d in %s: Allocating pagecache: %zu",
 		getpid(), tst_cg_group_name(cg), size);
 	alloc_pagecache(fd, size);
 
@@ -227,7 +223,7 @@ static void test_memcg_low(void)
 			TST_EXP_EXPR(low == 0,
 				"(%c low events=%ld) == 0", id, low);
 		} else if (!tst_cg_memory_recursiveprot(leaf_cg[F])) {
-			/* dont not check F when recursive_protection enabled */
+			/* do not check F when recursive_protection enabled */
 			TST_EXP_EXPR(low == 0,
 				"(%c low events=%ld) == 0", id, low);
 		}
-- 
2.45.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] memcg/memcontrol02: fix fd leak in cleanup path

[linuxtestproject.agent]

Hi Disha,

On Aug 18, 2026, Disha Goel wrote:
> memcg/memcontrol02: fix fd leak in cleanup path

--- [PATCH 1/3] ---

> Without this, cleanup would call close(-1) causing TBROK
> when the anon test variant (n=0) runs and never opens fd.

Could this describe the actual failure path instead? Before this patch,
cleanup() does not call close() at all. The descriptor is leaked when an
error after SAFE_OPEN() skips the normal SAFE_CLOSE(fd); the -1 guard is
needed only after adding the cleanup close.

Verdict - Needs revision

---
Note:

The agent can sometimes produce false positives although often its
findings are genuine. If you find issues with the review, please
comment this email or ignore the suggestions.

Regards,
LTP AI Reviewer

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH 1/2] memcg/memcontrol02: fix fd leak in cleanup path

[Disha Goel]

fd is initialized to 0 (a valid file descriptor) so the cleanup()
guard 'if (fd > -1)' would never trigger. Initialize to -1 and
add SAFE_CLOSE to prevent the descriptor leaking when a SAFE_*
macro calls tst_brk() during the pagecache allocation test.

Signed-off-by: Disha Goel <disgoel@linux.ibm.com>
---
 testcases/kernel/controllers/memcg/memcontrol02.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/testcases/kernel/controllers/memcg/memcontrol02.c b/testcases/kernel/controllers/memcg/memcontrol02.c
index 0b79403c9..75a1e6d51 100644
--- a/testcases/kernel/controllers/memcg/memcontrol02.c
+++ b/testcases/kernel/controllers/memcg/memcontrol02.c
@@ -29,7 +29,7 @@
 
 static size_t page_size;
 static struct tst_cg_group *cg_child;
-static int fd;
+static int fd = -1;
 static int file_to_all_error = 10;
 
 static void alloc_anon_50M_check(void)
@@ -128,6 +128,8 @@ static void cleanup(void)
 {
 	if (cg_child)
 		cg_child = tst_cg_group_rm(cg_child);
+	if (fd > -1)
+		SAFE_CLOSE(fd);
 }
 
 static struct tst_test test = {
-- 
2.45.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] memcg/memcontrol02: fix fd leak in cleanup path

[linuxtestproject.agent]

Hi Disha,

On Fri, Aug 7, 2026, Disha Goel wrote:
> memcg/memcontrol02: fix fd leak in cleanup path

--- [PATCH 1/2] ---

> fd is initialized to 0 (a valid file descriptor) so the cleanup()
> guard 'if (fd > -1)' would never trigger.

Could this explanation be corrected? Zero is greater than -1, and the
cleanup guard is added by this patch rather than being an existing guard.
The relevant problem is that -1 is needed to distinguish an unopened
descriptor before cleanup tries to close it.

> +	if (fd > -1)
> +		SAFE_CLOSE(fd);

Could this use `fd != -1` instead? LTP file descriptors use -1 as the
invalid sentinel, and the LTP convention requires this form for cleanup
guards.

--- [PATCH 2/2] ---

> #include <inttypes.h>
>
> -		tst_res(TINFO, "Child %d in %s: Allocating anon: %"PRIdPTR,
> +		tst_res(TINFO, "Child %d in %s: Allocating anon: %zu",

Could the now-unused `<inttypes.h>` include be removed from both
memcontrol03.c and memcontrol04.c? This patch replaces every `PRIdPTR`
use in both files.

Verdict - Needs revision

---
Note:

The agent can sometimes produce false positives although often its
findings are genuine. If you find issues with the review, please
comment this email or ignore the suggestions.

Regards,
LTP AI Reviewer

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] memcg/memcontrol02: fix fd leak in cleanup path

[Andrea Cervesato via ltp]

Hi Disha,

> > fd is initialized to 0 (a valid file descriptor) so the cleanup()
> > guard 'if (fd > -1)' would never trigger.
> 
> Could this explanation be corrected? Zero is greater than -1, and the
> cleanup guard is added by this patch rather than being an existing guard.
> The relevant problem is that -1 is needed to distinguish an unopened
> descriptor before cleanup tries to close it.
> 
> > +	if (fd > -1)
> > +		SAFE_CLOSE(fd);
> 
> Could this use `fd != -1` instead? LTP file descriptors use -1 as the
> invalid sentinel, and the LTP convention requires this form for cleanup
> guards.

this is correct.

> 
> --- [PATCH 2/2] ---
> 
> > #include <inttypes.h>
> >
> > -		tst_res(TINFO, "Child %d in %s: Allocating anon: %"PRIdPTR,
> > +		tst_res(TINFO, "Child %d in %s: Allocating anon: %zu",
> 
> Could the now-unused `<inttypes.h>` include be removed from both
> memcontrol03.c and memcontrol04.c? This patch replaces every `PRIdPTR`
> use in both files.

And also this is correct. We should split the patch in two: one for
memcontrol03 and one for memcontrol04

--
Andrea Cervesato
SUSE QE Automation Engineer Linux
andrea.cervesato@suse.com

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] memcg/memcontrol02: fix fd leak in cleanup path

[Disha Goel]

On 10/08/26 3:11 pm, Andrea Cervesato wrote:
> Hi Disha,
> 
>>> fd is initialized to 0 (a valid file descriptor) so the cleanup()
>>> guard 'if (fd > -1)' would never trigger.
>>
>> Could this explanation be corrected? Zero is greater than -1, and the
>> cleanup guard is added by this patch rather than being an existing guard.
>> The relevant problem is that -1 is needed to distinguish an unopened
>> descriptor before cleanup tries to close it.
>>
>>> +	if (fd > -1)
>>> +		SAFE_CLOSE(fd);
>>
>> Could this use `fd != -1` instead? LTP file descriptors use -1 as the
>> invalid sentinel, and the LTP convention requires this form for cleanup
>> guards.
> 
> this is correct.
> 
>>
>> --- [PATCH 2/2] ---
>>
>>> #include <inttypes.h>
>>>
>>> -		tst_res(TINFO, "Child %d in %s: Allocating anon: %"PRIdPTR,
>>> +		tst_res(TINFO, "Child %d in %s: Allocating anon: %zu",
>>
>> Could the now-unused `<inttypes.h>` include be removed from both
>> memcontrol03.c and memcontrol04.c? This patch replaces every `PRIdPTR`
>> use in both files.
> 
> And also this is correct. We should split the patch in two: one for
> memcontrol03 and one for memcontrol04
> 
Hi Andrea,

Thanks for the review. I've addressed all the comments and sent a v2.

Link to v2: 
https://lore.kernel.org/ltp/20260818121436.1397-1-disgoel@linux.ibm.com/

> --
> Andrea Cervesato
> SUSE QE Automation Engineer Linux
> andrea.cervesato@suse.com

-- 
Regards,
Disha


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp