Re: [PATCH 4/4] selftests/livepatch: fix mem leaks in test-klp-shadow-vars

[Petr Mladek <pmladek@suse.com>]

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