| Message ID | 20200528134849.7890-5-ycote@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/livepatch: rework of test-klp-{callbacks,shadow_vars} | expand |
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
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)
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(-)