Re: [PATCH 4/4] selftests/livepatch: fix mem leaks in test-klp-shadow-vars
From: Petr Mladek <pmladek@suse.com>
Date: 2020-06-02 09:57:53
Also in:
linux-kselftest
On Thu 2020-05-28 09:48:49, Yannick Cote wrote:
quoted hunk ↗ jump to hunk
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 <redacted> --- 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.
quoted hunk ↗ jump to hunk
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