[4/4] selftests/livepatch: fix mem leaks in test-klp-shadow-vars
diff mbox series

Message ID 20200528134849.7890-5-ycote@redhat.com
State New
Headers show
Series
  • selftests/livepatch: rework of test-klp-{callbacks,shadow_vars}
Related show

Commit Message

Yannick Cote May 28, 2020, 1:48 p.m. UTC
In some cases, when an error occurs during testing and the main test
routine returns, a memory leak occurs via leaving previously registered
shadow variables allocated in the kernel as well as shadow_ptr list
elements. From now on, in case of error, remove all allocated shadow
variables and shadow_ptr struct elements.

Signed-off-by: Yannick Cote <ycote@redhat.com>
---
 lib/livepatch/test_klp_shadow_vars.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

Comments

Petr Mladek June 2, 2020, 9:57 a.m. UTC | #1
On Thu 2020-05-28 09:48:49, Yannick Cote wrote:
> In some cases, when an error occurs during testing and the main test
> routine returns, a memory leak occurs via leaving previously registered
> shadow variables allocated in the kernel as well as shadow_ptr list
> elements. From now on, in case of error, remove all allocated shadow
> variables and shadow_ptr struct elements.
> 
> Signed-off-by: Yannick Cote <ycote@redhat.com>
> ---
>  lib/livepatch/test_klp_shadow_vars.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/livepatch/test_klp_shadow_vars.c b/lib/livepatch/test_klp_shadow_vars.c
> index 195309e1edf3..c6d631d826e0 100644
> --- a/lib/livepatch/test_klp_shadow_vars.c
> +++ b/lib/livepatch/test_klp_shadow_vars.c
> @@ -170,6 +170,7 @@ static int test_klp_shadow_vars_init(void)
>  	char *pndup[NUM_OBJS];
>  	int nfields2[NUM_OBJS], *pnfields2[NUM_OBJS], **sv2[NUM_OBJS];
>  	void **sv;
> +	int ret = -EINVAL;

IMHO, this predefined error value adds more hard than good. It makes
the code hard to read. One has to jump up and down to check what error
will get returned. Also it is safe only when "goto out" is used right
after setting this variable.


>  	int i;
>  
>  	ptr_id(NULL);
> @@ -192,12 +193,16 @@ static int test_klp_shadow_vars_init(void)
>  		/* alloc a few svars with different <obj> and <id>. */
>  		sv1[i] = shadow_alloc(&objs[i], SV_ID1, sizeof(pnfields1[i]),
>  					GFP_KERNEL, shadow_ctor, &pnfields1[i]);
> -		if (!sv1[i])
> -			return -ENOMEM;
> +		if (!sv1[i]) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
>  		sv2[i] = shadow_alloc(&objs[i], SV_ID2, sizeof(pnfields2[i]),
>  					GFP_KERNEL, shadow_ctor, &pnfields2[i]);
> -		if (!sv2[i])
> -			return -ENOMEM;
> +		if (!sv2[i]) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
>  	}
>  
>  	/* pass 2: verify we find allocated svars and where they point to */
> @@ -205,7 +210,7 @@ static int test_klp_shadow_vars_init(void)
>  		/* check the "char" svar for all objects */
>  		sv = shadow_get(&objs[i], SV_ID1);
>  		if (!sv)
> -			return -EINVAL;
> +			goto out;

Please, replace also return -EINVAL with

			ret = -EINVAL;
			goto out;


With this change:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Patch
diff mbox series

diff --git a/lib/livepatch/test_klp_shadow_vars.c b/lib/livepatch/test_klp_shadow_vars.c
index 195309e1edf3..c6d631d826e0 100644
--- a/lib/livepatch/test_klp_shadow_vars.c
+++ b/lib/livepatch/test_klp_shadow_vars.c
@@ -170,6 +170,7 @@  static int test_klp_shadow_vars_init(void)
 	char *pndup[NUM_OBJS];
 	int nfields2[NUM_OBJS], *pnfields2[NUM_OBJS], **sv2[NUM_OBJS];
 	void **sv;
+	int ret = -EINVAL;
 	int i;
 
 	ptr_id(NULL);
@@ -192,12 +193,16 @@  static int test_klp_shadow_vars_init(void)
 		/* alloc a few svars with different <obj> and <id>. */
 		sv1[i] = shadow_alloc(&objs[i], SV_ID1, sizeof(pnfields1[i]),
 					GFP_KERNEL, shadow_ctor, &pnfields1[i]);
-		if (!sv1[i])
-			return -ENOMEM;
+		if (!sv1[i]) {
+			ret = -ENOMEM;
+			goto out;
+		}
 		sv2[i] = shadow_alloc(&objs[i], SV_ID2, sizeof(pnfields2[i]),
 					GFP_KERNEL, shadow_ctor, &pnfields2[i]);
-		if (!sv2[i])
-			return -ENOMEM;
+		if (!sv2[i]) {
+			ret = -ENOMEM;
+			goto out;
+		}
 	}
 
 	/* pass 2: verify we find allocated svars and where they point to */
@@ -205,7 +210,7 @@  static int test_klp_shadow_vars_init(void)
 		/* check the "char" svar for all objects */
 		sv = shadow_get(&objs[i], SV_ID1);
 		if (!sv)
-			return -EINVAL;
+			goto out;
 		if ((char **)sv == sv1[i] && *sv1[i] == pnfields1[i])
 			pr_info("  got expected PTR%d -> PTR%d result\n",
 				ptr_id(sv1[i]), ptr_id(*sv1[i]));
@@ -213,7 +218,7 @@  static int test_klp_shadow_vars_init(void)
 		/* check the "int" svar for all objects */
 		sv = shadow_get(&objs[i], SV_ID2);
 		if (!sv)
-			return -EINVAL;
+			goto out;
 		if ((int **)sv == sv2[i] && *sv2[i] == pnfields2[i])
 			pr_info("  got expected PTR%d -> PTR%d result\n",
 				ptr_id(sv2[i]), ptr_id(*sv2[i]));
@@ -224,7 +229,7 @@  static int test_klp_shadow_vars_init(void)
 		sv = shadow_get_or_alloc(&objs[i], SV_ID1, sizeof(pndup[i]),
 					GFP_KERNEL, shadow_ctor, &pndup[i]);
 		if (!sv)
-			return -EINVAL;
+			goto out;
 		if ((char **)sv == sv1[i] && *sv1[i] == pnfields1[i])
 			pr_info("  got expected PTR%d -> PTR%d result\n",
 					ptr_id(sv1[i]), ptr_id(*sv1[i]));
@@ -242,7 +247,7 @@  static int test_klp_shadow_vars_init(void)
 	for (i = 0; i < NUM_OBJS; i++) {
 		sv = shadow_get(&objs[i], SV_ID2);	/* 'int' pairs */
 		if (!sv)
-			return -EINVAL;
+			goto out;
 		if ((int **)sv == sv2[i] && *sv2[i] == pnfields2[i])
 			pr_info("  got expected PTR%d -> PTR%d result\n",
 					ptr_id(sv2[i]), ptr_id(*sv2[i]));
@@ -259,6 +264,12 @@  static int test_klp_shadow_vars_init(void)
 	free_ptr_list();
 
 	return 0;
+out:
+	shadow_free_all(SV_ID1, NULL);		/* 'char' pairs */
+	shadow_free_all(SV_ID2, NULL);		/* 'int' pairs */
+	free_ptr_list();
+
+	return ret;
 }
 
 static void test_klp_shadow_vars_exit(void)