[PATCH] selftests: memcg: uninitialized variable in test_memcg_reclaim()

[PATCH] selftests: memcg: uninitialized variable in test_memcg_reclaim()

[Dan Carpenter]

The "fd" is used on the clean up path without ever being initialized.

Fixes: eae3cb2e87ff ("selftests: cgroup: add a selftest for memory.reclaim")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
I kind of went over kill on fixing this as if it were real code which
matters.  :P

 .../selftests/cgroup/test_memcontrol.c        | 23 +++++++++++--------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
index 8833359556f3..08681699c2f9 100644
--- a/tools/testing/selftests/cgroup/test_memcontrol.c
+++ b/tools/testing/selftests/cgroup/test_memcontrol.c
@@ -658,18 +658,18 @@ static int test_memcg_reclaim(const char *root)
 
 	memcg = cg_name(root, "memcg_test");
 	if (!memcg)
-		goto cleanup;
+		return KSFT_FAIL;
 
 	if (cg_create(memcg))
-		goto cleanup;
+		goto free_memcg;
 
 	current = cg_read_long(memcg, "memory.current");
 	if (current != 0)
-		goto cleanup;
+		goto destroy_memcg;
 
 	fd = get_temp_fd();
 	if (fd < 0)
-		goto cleanup;
+		goto destroy_memcg;
 
 	cg_run_nowait(memcg, alloc_pagecache_50M_noexit, (void *)(long)fd);
 
@@ -697,7 +697,7 @@ static int test_memcg_reclaim(const char *root)
 			fprintf(stderr,
 				"failed to allocate %ld for memcg reclaim test\n",
 				expected_usage);
-			goto cleanup;
+			goto close;
 		}
 	}
 
@@ -717,7 +717,7 @@ static int test_memcg_reclaim(const char *root)
 		 * not reclaim the full amount.
 		 */
 		if (to_reclaim <= 0)
-			goto cleanup;
+			goto close;
 
 
 		snprintf(buf, sizeof(buf), "%ld", to_reclaim);
@@ -729,7 +729,7 @@ static int test_memcg_reclaim(const char *root)
 			 */
 			current = cg_read_long(memcg, "memory.current");
 			if (!values_close(current, MB(30), 3) && current > MB(30))
-				goto cleanup;
+				goto close;
 			break;
 		}
 
@@ -738,14 +738,17 @@ static int test_memcg_reclaim(const char *root)
 			continue;
 
 		/* We got an unexpected error or ran out of retries. */
-		goto cleanup;
+		goto close;
 	}
 
 	ret = KSFT_PASS;
-cleanup:
+
+close:
+	close(fd);
+destroy_memcg:
 	cg_destroy(memcg);
+free_memcg:
 	free(memcg);
-	close(fd);
 
 	return ret;
 }
-- 
2.35.1

Re: [PATCH] selftests: memcg: uninitialized variable in test_memcg_reclaim()

[Yosry Ahmed]

Hi Dan!

On Tue, Jul 19, 2022 at 2:46 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> The "fd" is used on the clean up path without ever being initialized.
>
> Fixes: eae3cb2e87ff ("selftests: cgroup: add a selftest for memory.reclaim")

Thanks for fixing this :)

> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> I kind of went over kill on fixing this as if it were real code which
> matters.  :P
>
>  .../selftests/cgroup/test_memcontrol.c        | 23 +++++++++++--------
>  1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/tools/testing/selftests/cgroup/test_memcontrol.c b/tools/testing/selftests/cgroup/test_memcontrol.c
> index 8833359556f3..08681699c2f9 100644
> --- a/tools/testing/selftests/cgroup/test_memcontrol.c
> +++ b/tools/testing/selftests/cgroup/test_memcontrol.c
> @@ -658,18 +658,18 @@ static int test_memcg_reclaim(const char *root)
>
>         memcg = cg_name(root, "memcg_test");
>         if (!memcg)
> -               goto cleanup;
> +               return KSFT_FAIL;

Nit: Just goto free here as well, free ignores NULLs anyway. It's
easier to have fewer return paths and more consistent with other
tests.

>
>         if (cg_create(memcg))
> -               goto cleanup;
> +               goto free_memcg;
>
>         current = cg_read_long(memcg, "memory.current");
>         if (current != 0)
> -               goto cleanup;
> +               goto destroy_memcg;
>
>         fd = get_temp_fd();
>         if (fd < 0)
> -               goto cleanup;
> +               goto destroy_memcg;
>
>         cg_run_nowait(memcg, alloc_pagecache_50M_noexit, (void *)(long)fd);
>
> @@ -697,7 +697,7 @@ static int test_memcg_reclaim(const char *root)
>                         fprintf(stderr,
>                                 "failed to allocate %ld for memcg reclaim test\n",
>                                 expected_usage);
> -                       goto cleanup;
> +                       goto close;
>                 }
>         }
>
> @@ -717,7 +717,7 @@ static int test_memcg_reclaim(const char *root)
>                  * not reclaim the full amount.
>                  */
>                 if (to_reclaim <= 0)
> -                       goto cleanup;
> +                       goto close;
>
>
>                 snprintf(buf, sizeof(buf), "%ld", to_reclaim);
> @@ -729,7 +729,7 @@ static int test_memcg_reclaim(const char *root)
>                          */
>                         current = cg_read_long(memcg, "memory.current");
>                         if (!values_close(current, MB(30), 3) && current > MB(30))
> -                               goto cleanup;
> +                               goto close;
>                         break;
>                 }
>
> @@ -738,14 +738,17 @@ static int test_memcg_reclaim(const char *root)
>                         continue;
>
>                 /* We got an unexpected error or ran out of retries. */
> -               goto cleanup;
> +               goto close;
>         }
>
>         ret = KSFT_PASS;
> -cleanup:
> +
> +close:
> +       close(fd);
> +destroy_memcg:
>         cg_destroy(memcg);
> +free_memcg:
>         free(memcg);
> -       close(fd);
>
>         return ret;
>  }

Nit: keep the cleanup_* naming for labels to make it obvious and to be
consistent with the rest of the file (e.g. cleanup_free,
cleanup_memcg, cleanup_file/cleanup_all). See
test_memcg_subtree_control().

I would honestly have one label to cleanup the memcg. Calling
cg_destroy() on a non-existent memcg should be fine. rmdir() will just
fail silently. All other tests do this and it's easier to read when we
have fewer return paths. My advice would be cleanup_file and
cleanup_memcg labels.

Thanks!

With these nits:
Reviewed-by: Yosry Ahmed <yosryahmed@google.com>

> --
> 2.35.1
>

Re: [PATCH] selftests: memcg: uninitialized variable in test_memcg_reclaim()

[Dan Carpenter]

On Tue, Jul 19, 2022 at 10:27:36AM -0700, Yosry Ahmed wrote:
> 
> Nit: keep the cleanup_* naming for labels to make it obvious and to be
> consistent with the rest of the file (e.g. cleanup_free,
> cleanup_memcg, cleanup_file/cleanup_all). See
> test_memcg_subtree_control().
> 
> I would honestly have one label to cleanup the memcg. Calling
> cg_destroy() on a non-existent memcg should be fine. rmdir() will just
> fail silently. All other tests do this and it's easier to read when we
> have fewer return paths. My advice would be cleanup_file and
> cleanup_memcg labels.

One error label handling is very bug prone.  You always end up freeing
things which have not been initialized/allocated.  Or dereferencing
pointers which are NULL.  Or, since most kernel functions clean up
after themselves, you end up double freeing things.

regards,
dan carpenter

Re: [PATCH] selftests: memcg: uninitialized variable in test_memcg_reclaim()

[Yosry Ahmed]

On Wed, Jul 20, 2022 at 2:29 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Tue, Jul 19, 2022 at 10:27:36AM -0700, Yosry Ahmed wrote:
> >
> > Nit: keep the cleanup_* naming for labels to make it obvious and to be
> > consistent with the rest of the file (e.g. cleanup_free,
> > cleanup_memcg, cleanup_file/cleanup_all). See
> > test_memcg_subtree_control().
> >
> > I would honestly have one label to cleanup the memcg. Calling
> > cg_destroy() on a non-existent memcg should be fine. rmdir() will just
> > fail silently. All other tests do this and it's easier to read when we
> > have fewer return paths. My advice would be cleanup_file and
> > cleanup_memcg labels.
>
> One error label handling is very bug prone.  You always end up freeing
> things which have not been initialized/allocated.  Or dereferencing
> pointers which are NULL.  Or, since most kernel functions clean up
> after themselves, you end up double freeing things.

I am not suggesting a single cleanup label, I said "one label to
cleanup the memcg", which is separate from cleaning up the file.
Basically just merging the destroy_memcg and free_memcg labels to be
consistent with other tests. I don't feel strongly about this anyway
:)

>
> regards,
> dan carpenter