The test-klp-callbacks change implement a synchronization replacement of initial code to use completion variables instead of delays. The completion variable interlocks the busy module with the concurrent loading of the target livepatch patches which works with the execution flow instead of estimated time delays.
The test-klp-shadow-vars changes first refactors the code to be more of a readable example as well as continuing to verify the component code. The patch is broken in two to display the renaming and restructuring in part 1 and the addition and change of logic in part 2. The last change frees memory before bailing in case of errors.
Patchset to be merged via the livepatching tree is against: livepatching/for-next
Joe Lawrence (1): selftests/livepatch: rework test-klp-callbacks to use completion variables
Yannick Cote (3): selftests/livepatch: rework test-klp-shadow-vars selftests/livepatch: more verification in test-klp-shadow-vars selftests/livepatch: fix mem leaks in test-klp-shadow-vars
lib/livepatch/test_klp_callbacks_busy.c | 42 +++- lib/livepatch/test_klp_shadow_vars.c | 222 +++++++++--------- .../selftests/livepatch/test-callbacks.sh | 29 ++- .../selftests/livepatch/test-shadow-vars.sh | 85 ++++--- 4 files changed, 214 insertions(+), 164 deletions(-)
From: Joe Lawrence joe.lawrence@redhat.com
The test-klp-callbacks script includes a few tests which rely on kernel task timings that may not always execute as expected under system load. These will generate out of sequence kernel log messages that result in test failure.
Instead of using sleep timing windows to orchestrate the test, rework the test_klp_callbacks_busy module to use completion variables.
Signed-off-by: Joe Lawrence joe.lawrence@redhat.com Signed-off-by: Yannick Cote ycote@redhat.com --- lib/livepatch/test_klp_callbacks_busy.c | 42 +++++++++++++++---- .../selftests/livepatch/test-callbacks.sh | 29 +++++++------ 2 files changed, 47 insertions(+), 24 deletions(-)
diff --git a/lib/livepatch/test_klp_callbacks_busy.c b/lib/livepatch/test_klp_callbacks_busy.c index 40beddf8a0e2..c3df12f47e5e 100644 --- a/lib/livepatch/test_klp_callbacks_busy.c +++ b/lib/livepatch/test_klp_callbacks_busy.c @@ -5,34 +5,58 @@
#include <linux/module.h> #include <linux/kernel.h> +#include <linux/sched.h> #include <linux/workqueue.h> #include <linux/delay.h>
-static int sleep_secs; -module_param(sleep_secs, int, 0644); -MODULE_PARM_DESC(sleep_secs, "sleep_secs (default=0)"); +/* load/run-time control from sysfs writer */ +static bool block_transition; +module_param(block_transition, bool, 0644); +MODULE_PARM_DESC(block_transition, "block_transition (default=false)");
static void busymod_work_func(struct work_struct *work); -static DECLARE_DELAYED_WORK(work, busymod_work_func); +static DECLARE_WORK(work, busymod_work_func); +static DECLARE_COMPLETION(busymod_work_complete);
static void busymod_work_func(struct work_struct *work) { - pr_info("%s, sleeping %d seconds ...\n", __func__, sleep_secs); - msleep(sleep_secs * 1000); + bool early_complete = block_transition; + + pr_info("%s enter\n", __func__); + + /* + * When blocking the livepatch transition, set completion flag + * early so subsequent test commands see the transition. + */ + if (early_complete) + complete(&busymod_work_complete); + + while (block_transition) + msleep(1000); + pr_info("%s exit\n", __func__); + + /* + * In non-blocking case, wait until we're done to complete to + * ensure kernel log ordering + */ + if (!early_complete) + complete(&busymod_work_complete); }
static int test_klp_callbacks_busy_init(void) { pr_info("%s\n", __func__); - schedule_delayed_work(&work, - msecs_to_jiffies(1000 * 0)); + schedule_work(&work); + wait_for_completion(&busymod_work_complete); + return 0; }
static void test_klp_callbacks_busy_exit(void) { - cancel_delayed_work_sync(&work); + block_transition = false; + cancel_work_sync(&work); pr_info("%s\n", __func__); }
diff --git a/tools/testing/selftests/livepatch/test-callbacks.sh b/tools/testing/selftests/livepatch/test-callbacks.sh index a35289b13c9c..32b57ba07f4f 100755 --- a/tools/testing/selftests/livepatch/test-callbacks.sh +++ b/tools/testing/selftests/livepatch/test-callbacks.sh @@ -356,9 +356,7 @@ livepatch: '$MOD_LIVEPATCH': unpatching complete echo -n "TEST: multiple target modules ... " dmesg -C
-load_mod $MOD_TARGET_BUSY sleep_secs=0 -# give $MOD_TARGET_BUSY::busymod_work_func() a chance to run -sleep 5 +load_mod $MOD_TARGET_BUSY block_transition=N load_lp $MOD_LIVEPATCH load_mod $MOD_TARGET unload_mod $MOD_TARGET @@ -366,9 +364,9 @@ disable_lp $MOD_LIVEPATCH unload_lp $MOD_LIVEPATCH unload_mod $MOD_TARGET_BUSY
-check_result "% modprobe $MOD_TARGET_BUSY sleep_secs=0 +check_result "% modprobe $MOD_TARGET_BUSY block_transition=N $MOD_TARGET_BUSY: ${MOD_TARGET_BUSY}_init -$MOD_TARGET_BUSY: busymod_work_func, sleeping 0 seconds ... +$MOD_TARGET_BUSY: busymod_work_func enter $MOD_TARGET_BUSY: busymod_work_func exit % modprobe $MOD_LIVEPATCH livepatch: enabling patch '$MOD_LIVEPATCH' @@ -404,11 +402,10 @@ livepatch: '$MOD_LIVEPATCH': unpatching complete $MOD_TARGET_BUSY: ${MOD_TARGET_BUSY}_exit"
- # TEST: busy target module # # A similar test as the previous one, but force the "busy" kernel module -# to do longer work. +# to block the livepatch transition. # # The livepatching core will refuse to patch a task that is currently # executing a to-be-patched function -- the consistency model stalls the @@ -417,8 +414,7 @@ $MOD_TARGET_BUSY: ${MOD_TARGET_BUSY}_exit" # function for a long time. Meanwhile, load and unload other target # kernel modules while the livepatch transition is in progress. # -# - Load the "busy" kernel module, this time make it do 10 seconds worth -# of work. +# - Load the "busy" kernel module, this time make its work function loop # # - Meanwhile, the livepatch is loaded. Notice that the patch # transition does not complete as the targeted "busy" module is @@ -438,20 +434,23 @@ $MOD_TARGET_BUSY: ${MOD_TARGET_BUSY}_exit" echo -n "TEST: busy target module ... " dmesg -C
-load_mod $MOD_TARGET_BUSY sleep_secs=10 +load_mod $MOD_TARGET_BUSY block_transition=Y load_lp_nowait $MOD_LIVEPATCH -# Don't wait for transition, load $MOD_TARGET while the transition -# is still stalled in $MOD_TARGET_BUSY::busymod_work_func() -sleep 5 + +# Wait until the livepatch reports in-transition state, i.e. that it's +# stalled on $MOD_TARGET_BUSY::busymod_work_func() +loop_until 'grep -q '^1$' /sys/kernel/livepatch/$MOD_LIVEPATCH/transition' || + die "failed to stall transition" + load_mod $MOD_TARGET unload_mod $MOD_TARGET disable_lp $MOD_LIVEPATCH unload_lp $MOD_LIVEPATCH unload_mod $MOD_TARGET_BUSY
-check_result "% modprobe $MOD_TARGET_BUSY sleep_secs=10 +check_result "% modprobe $MOD_TARGET_BUSY block_transition=Y $MOD_TARGET_BUSY: ${MOD_TARGET_BUSY}_init -$MOD_TARGET_BUSY: busymod_work_func, sleeping 10 seconds ... +$MOD_TARGET_BUSY: busymod_work_func enter % modprobe $MOD_LIVEPATCH livepatch: enabling patch '$MOD_LIVEPATCH' livepatch: '$MOD_LIVEPATCH': initializing patching transition
On Thu 2020-05-28 09:48:46, Yannick Cote wrote:
From: Joe Lawrence joe.lawrence@redhat.com
The test-klp-callbacks script includes a few tests which rely on kernel task timings that may not always execute as expected under system load. These will generate out of sequence kernel log messages that result in test failure.
Instead of using sleep timing windows to orchestrate the test, rework the test_klp_callbacks_busy module to use completion variables.
Signed-off-by: Joe Lawrence joe.lawrence@redhat.com Signed-off-by: Yannick Cote ycote@redhat.com
lib/livepatch/test_klp_callbacks_busy.c | 42 +++++++++++++++---- .../selftests/livepatch/test-callbacks.sh | 29 +++++++------ 2 files changed, 47 insertions(+), 24 deletions(-)
diff --git a/lib/livepatch/test_klp_callbacks_busy.c b/lib/livepatch/test_klp_callbacks_busy.c index 40beddf8a0e2..c3df12f47e5e 100644 --- a/lib/livepatch/test_klp_callbacks_busy.c +++ b/lib/livepatch/test_klp_callbacks_busy.c @@ -5,34 +5,58 @@ #include <linux/module.h> #include <linux/kernel.h> +#include <linux/sched.h> #include <linux/workqueue.h> #include <linux/delay.h> -static int sleep_secs; -module_param(sleep_secs, int, 0644); -MODULE_PARM_DESC(sleep_secs, "sleep_secs (default=0)"); +/* load/run-time control from sysfs writer */ +static bool block_transition; +module_param(block_transition, bool, 0644); +MODULE_PARM_DESC(block_transition, "block_transition (default=false)"); static void busymod_work_func(struct work_struct *work); -static DECLARE_DELAYED_WORK(work, busymod_work_func); +static DECLARE_WORK(work, busymod_work_func); +static DECLARE_COMPLETION(busymod_work_complete); static void busymod_work_func(struct work_struct *work) {
- pr_info("%s, sleeping %d seconds ...\n", __func__, sleep_secs);
- msleep(sleep_secs * 1000);
- bool early_complete = block_transition;
- pr_info("%s enter\n", __func__);
- /*
* When blocking the livepatch transition, set completion flag* early so subsequent test commands see the transition.*/- if (early_complete)
complete(&busymod_work_complete);
I have to say that the code is really confusing. A completion called "work_complete" is completed when the work gets actually. It is completed later when the work is done immediately.
Do we need the completion at all? See below.
- while (block_transition)
The compiler might optimize the code and avoid the re-reads. Please, use:
/* Re-read variable in each cycle */ while (READ_ONCE(block_transition))
msleep(1000);
Nit: This is still a busy wait even though there is a big delay between waits. The right solution would be using wait_event(). But feel free to keep msleep(). It is good enough for selftests.
- pr_info("%s exit\n", __func__);
- /*
* In non-blocking case, wait until we're done to complete to* ensure kernel log ordering*/- if (!early_complete)
complete(&busymod_work_complete);} static int test_klp_callbacks_busy_init(void) { pr_info("%s\n", __func__);
- schedule_delayed_work(&work,
msecs_to_jiffies(1000 * 0));
- schedule_work(&work);
- wait_for_completion(&busymod_work_complete);
IMHO, the completion is not needed when using:
schedule_work(&work); /* * Print all messages from the work before returning from init(). * It helps to serialize messages from the loaded modules. */ if (!block_transition) flush_work(&work);
- return 0;
} static void test_klp_callbacks_busy_exit(void) {
- cancel_delayed_work_sync(&work);
- block_transition = false;
The compiler could move this assignment after the following call. Please, use:
/* Make sure that the variable is set before flushing work. */ WRITE_ONCE(block_transition, false);
- cancel_work_sync(&work);
The work is not longer canceled. flush_work() better fits here. Also I would do this only when the transition is blocked:
if (block_transition) { /* Make sure that the variable is set before flushing work. */ WRITE_ONCE(block_transition, false); flush_work(&work); }
Otherwise this is a nice improvement.
Best Regards, Petr
The initial idea was to make a change to please cppcheck and remove void pointer arithmetics found a few times:
portability: 'obj' is of type 'void *'. When using void pointers in calculations, the behaviour is undefined. [arithOperationsOnVoidPointer]
The rest of the changes are to help make the test read as an example while continuing to verify the shadow variable code. The logic of the test is unchanged but restructured to use descriptive names.
Signed-off-by: Yannick Cote ycote@redhat.com --- lib/livepatch/test_klp_shadow_vars.c | 101 +++++++++++++++------------ 1 file changed, 57 insertions(+), 44 deletions(-)
diff --git a/lib/livepatch/test_klp_shadow_vars.c b/lib/livepatch/test_klp_shadow_vars.c index f0b5a1d24e55..ec2635cff974 100644 --- a/lib/livepatch/test_klp_shadow_vars.c +++ b/lib/livepatch/test_klp_shadow_vars.c @@ -109,8 +109,7 @@ static void shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor) static void shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor) { klp_shadow_free_all(id, dtor); - pr_info("klp_%s(id=0x%lx, dtor=PTR%d)\n", - __func__, id, ptr_id(dtor)); + pr_info("klp_%s(id=0x%lx, dtor=PTR%d)\n", __func__, id, ptr_id(dtor)); }
@@ -124,8 +123,7 @@ static int shadow_ctor(void *obj, void *shadow_data, void *ctor_data) return -EINVAL;
*sv = *var; - pr_info("%s: PTR%d -> PTR%d\n", - __func__, ptr_id(sv), ptr_id(*var)); + pr_info("%s: PTR%d -> PTR%d\n", __func__, ptr_id(sv), ptr_id(*var));
return 0; } @@ -138,49 +136,63 @@ static void shadow_dtor(void *obj, void *shadow_data) __func__, ptr_id(obj), ptr_id(sv)); }
-static int test_klp_shadow_vars_init(void) -{ - void *obj = THIS_MODULE; - int id = 0x1234; - gfp_t gfp_flags = GFP_KERNEL; +/* dynamically created obj fields have the following shadow var id values */ +#define SV_ID1 0x1234 +#define SV_ID2 0x1235
- int var1, var2, var3, var4; - int *pv1, *pv2, *pv3, *pv4; - int **sv1, **sv2, **sv3, **sv4; +/* + * The main test case adds/removes new fields (shadow var) to each of these + * test structure instances. The last group of fields in the struct represent + * the idea that shadow variables may be added and removed to and from the + * struct during execution. + */ +struct test_object { + /* add anything here below and avoid to define an empty struct */ + struct shadow_ptr sp;
- int **sv; + /* these represent shadow vars added and removed with SV_ID{1,2} */ + /* char nfield1; */ + /* int nfield2; */ +};
- pv1 = &var1; - pv2 = &var2; - pv3 = &var3; - pv4 = &var4; +static int test_klp_shadow_vars_init(void) +{ + struct test_object obj1, obj2, obj3; + char nfield1, nfield2, *pnfield1, *pnfield2, **sv1, **sv2; + int nfield3, nfield4, *pnfield3, *pnfield4, **sv3, **sv4; + void **sv; + + pnfield1 = &nfield1; + pnfield2 = &nfield2; + pnfield3 = &nfield3; + pnfield4 = &nfield4;
ptr_id(NULL); - ptr_id(pv1); - ptr_id(pv2); - ptr_id(pv3); - ptr_id(pv4); + ptr_id(pnfield1); + ptr_id(pnfield2); + ptr_id(pnfield3); + ptr_id(pnfield4);
/* * With an empty shadow variable hash table, expect not to find * any matches. */ - sv = shadow_get(obj, id); + sv = shadow_get(&obj1, SV_ID1); if (!sv) pr_info(" got expected NULL result\n");
/* * Allocate a few shadow variables with different <obj> and <id>. */ - sv1 = shadow_alloc(obj, id, sizeof(pv1), gfp_flags, shadow_ctor, &pv1); + sv1 = shadow_alloc(&obj1, SV_ID1, sizeof(pnfield1), GFP_KERNEL, shadow_ctor, &pnfield1); if (!sv1) return -ENOMEM;
- sv2 = shadow_alloc(obj + 1, id, sizeof(pv2), gfp_flags, shadow_ctor, &pv2); + sv2 = shadow_alloc(&obj2, SV_ID1, sizeof(pnfield2), GFP_KERNEL, shadow_ctor, &pnfield2); if (!sv2) return -ENOMEM;
- sv3 = shadow_alloc(obj, id + 1, sizeof(pv3), gfp_flags, shadow_ctor, &pv3); + sv3 = shadow_alloc(&obj1, SV_ID2, sizeof(pnfield3), GFP_KERNEL, shadow_ctor, &pnfield3); if (!sv3) return -ENOMEM;
@@ -188,23 +200,24 @@ static int test_klp_shadow_vars_init(void) * Verify we can find our new shadow variables and that they point * to expected data. */ - sv = shadow_get(obj, id); + sv = shadow_get(&obj1, SV_ID1); if (!sv) return -EINVAL; - if (sv == sv1 && *sv1 == pv1) + if ((char **)sv == sv1 && *sv1 == pnfield1) pr_info(" got expected PTR%d -> PTR%d result\n", ptr_id(sv1), ptr_id(*sv1));
- sv = shadow_get(obj + 1, id); + sv = shadow_get(&obj2, SV_ID1); if (!sv) return -EINVAL; - if (sv == sv2 && *sv2 == pv2) + if ((char **)sv == sv2 && *sv2 == pnfield2) pr_info(" got expected PTR%d -> PTR%d result\n", ptr_id(sv2), ptr_id(*sv2)); - sv = shadow_get(obj, id + 1); + + sv = shadow_get(&obj1, SV_ID2); if (!sv) return -EINVAL; - if (sv == sv3 && *sv3 == pv3) + if ((int **)sv == sv3 && *sv3 == pnfield3) pr_info(" got expected PTR%d -> PTR%d result\n", ptr_id(sv3), ptr_id(*sv3));
@@ -212,14 +225,14 @@ static int test_klp_shadow_vars_init(void) * Allocate or get a few more, this time with the same <obj>, <id>. * The second invocation should return the same shadow var. */ - sv4 = shadow_get_or_alloc(obj + 2, id, sizeof(pv4), gfp_flags, shadow_ctor, &pv4); + sv4 = shadow_get_or_alloc(&obj3, SV_ID1, sizeof(pnfield4), GFP_KERNEL, shadow_ctor, &pnfield4); if (!sv4) return -ENOMEM;
- sv = shadow_get_or_alloc(obj + 2, id, sizeof(pv4), gfp_flags, shadow_ctor, &pv4); + sv = shadow_get_or_alloc(&obj3, SV_ID1, sizeof(pnfield4), GFP_KERNEL, shadow_ctor, &pnfield4); if (!sv) return -EINVAL; - if (sv == sv4 && *sv4 == pv4) + if ((int **)sv == sv4 && *sv4 == pnfield4) pr_info(" got expected PTR%d -> PTR%d result\n", ptr_id(sv4), ptr_id(*sv4));
@@ -227,36 +240,36 @@ static int test_klp_shadow_vars_init(void) * Free the <obj=*, id> shadow variables and check that we can no * longer find them. */ - shadow_free(obj, id, shadow_dtor); /* sv1 */ - sv = shadow_get(obj, id); + shadow_free(&obj1, SV_ID1, shadow_dtor); /* sv1 */ + sv = shadow_get(&obj1, SV_ID1); if (!sv) pr_info(" got expected NULL result\n");
- shadow_free(obj + 1, id, shadow_dtor); /* sv2 */ - sv = shadow_get(obj + 1, id); + shadow_free(&obj2, SV_ID1, shadow_dtor); /* sv2 */ + sv = shadow_get(&obj2, SV_ID1); if (!sv) pr_info(" got expected NULL result\n");
- shadow_free(obj + 2, id, shadow_dtor); /* sv4 */ - sv = shadow_get(obj + 2, id); + shadow_free(&obj3, SV_ID1, shadow_dtor); /* sv4 */ + sv = shadow_get(&obj3, SV_ID1); if (!sv) pr_info(" got expected NULL result\n");
/* * We should still find an <id+1> variable. */ - sv = shadow_get(obj, id + 1); + sv = shadow_get(&obj1, SV_ID2); if (!sv) return -EINVAL; - if (sv == sv3 && *sv3 == pv3) + if ((int **)sv == sv3 && *sv3 == pnfield3) pr_info(" got expected PTR%d -> PTR%d result\n", ptr_id(sv3), ptr_id(*sv3));
/* * Free all the <id+1> variables, too. */ - shadow_free_all(id + 1, shadow_dtor); /* sv3 */ - sv = shadow_get(obj, id); + shadow_free_all(SV_ID2, shadow_dtor); /* sv3 */ + sv = shadow_get(&obj1, SV_ID1); if (!sv) pr_info(" shadow_get() got expected NULL result\n");
On Thu 2020-05-28 09:48:47, Yannick Cote wrote:
The initial idea was to make a change to please cppcheck and remove void pointer arithmetics found a few times:
portability: 'obj' is of type 'void *'. When using void pointers in calculations, the behaviour is undefined. [arithOperationsOnVoidPointer]
The rest of the changes are to help make the test read as an example while continuing to verify the shadow variable code. The logic of the test is unchanged but restructured to use descriptive names.
First, I really like this change. There are only few comments, see below.
Signed-off-by: Yannick Cote ycote@redhat.com
lib/livepatch/test_klp_shadow_vars.c | 101 +++++++++++++++------------ 1 file changed, 57 insertions(+), 44 deletions(-)
diff --git a/lib/livepatch/test_klp_shadow_vars.c b/lib/livepatch/test_klp_shadow_vars.c index f0b5a1d24e55..ec2635cff974 100644 --- a/lib/livepatch/test_klp_shadow_vars.c +++ b/lib/livepatch/test_klp_shadow_vars.c
- The main test case adds/removes new fields (shadow var) to each of these
- test structure instances. The last group of fields in the struct represent
- the idea that shadow variables may be added and removed to and from the
- struct during execution.
- */
+struct test_object {
/* add anything here below and avoid to define an empty struct */- struct shadow_ptr sp;
I was confused why the orignal structure included shadow pointer from the start. And it is because struct shadow_ptr is unrelated to the tested livepatch shadow variable API.
This selftest is quite complicated even without this confusion. It would be nice to rename struct shadow_ptr to struct ptr_id and the varible to p_id. Or something to something like this.
It should be done in separate patch before or on top of this patchset. I could send it if you do not want to invest more time into it.
- int **sv;
- /* these represent shadow vars added and removed with SV_ID{1,2} */
- /* char nfield1; */
- /* int nfield2; */
+}; @@ -188,23 +200,24 @@ static int test_klp_shadow_vars_init(void) * Verify we can find our new shadow variables and that they point * to expected data. */
- sv = shadow_get(obj, id);
- sv = shadow_get(&obj1, SV_ID1); if (!sv) return -EINVAL;
- if (sv == sv1 && *sv1 == pv1)
- if ((char **)sv == sv1 && *sv1 == pnfield1)
Are these casts of "sv" variable enough to get rid of the cppcheck warnings? It would have been better to split the two changes (warning fixes and code refactoring into two patches).
It is a hint for future patchse. I do not want to block this one because of it. Feel free to use:
Reviewed-by: Petr Mladek pmladek@suse.com
Best Regards, Petr
This change makes the test feel more familiar with narrowing to a typical usage by operating on a number of identical structure instances and populating the same two new shadow variables symmetrically while keeping the same testing and verification criteria for the extra variables.
Signed-off-by: Yannick Cote ycote@redhat.com --- lib/livepatch/test_klp_shadow_vars.c | 182 ++++++++---------- .../selftests/livepatch/test-shadow-vars.sh | 85 +++++--- 2 files changed, 135 insertions(+), 132 deletions(-)
diff --git a/lib/livepatch/test_klp_shadow_vars.c b/lib/livepatch/test_klp_shadow_vars.c index ec2635cff974..195309e1edf3 100644 --- a/lib/livepatch/test_klp_shadow_vars.c +++ b/lib/livepatch/test_klp_shadow_vars.c @@ -128,6 +128,11 @@ static int shadow_ctor(void *obj, void *shadow_data, void *ctor_data) return 0; }
+/* + * With more than one item to free in the list, order is not determined and + * shadow_dtor will not be passed to shadow_free_all() which would make the + * test fail. (see pass 6) + */ static void shadow_dtor(void *obj, void *shadow_data) { int **sv = shadow_data; @@ -136,6 +141,9 @@ static void shadow_dtor(void *obj, void *shadow_data) __func__, ptr_id(obj), ptr_id(sv)); }
+/* number of objects we simulate that need kpatch-patch shadow vars */ +#define NUM_OBJS 3 + /* dynamically created obj fields have the following shadow var id values */ #define SV_ID1 0x1234 #define SV_ID2 0x1235 @@ -157,122 +165,96 @@ struct test_object {
static int test_klp_shadow_vars_init(void) { - struct test_object obj1, obj2, obj3; - char nfield1, nfield2, *pnfield1, *pnfield2, **sv1, **sv2; - int nfield3, nfield4, *pnfield3, *pnfield4, **sv3, **sv4; + struct test_object objs[NUM_OBJS]; + char nfields1[NUM_OBJS], *pnfields1[NUM_OBJS], **sv1[NUM_OBJS]; + char *pndup[NUM_OBJS]; + int nfields2[NUM_OBJS], *pnfields2[NUM_OBJS], **sv2[NUM_OBJS]; void **sv; - - pnfield1 = &nfield1; - pnfield2 = &nfield2; - pnfield3 = &nfield3; - pnfield4 = &nfield4; + int i;
ptr_id(NULL); - ptr_id(pnfield1); - ptr_id(pnfield2); - ptr_id(pnfield3); - ptr_id(pnfield4);
/* * With an empty shadow variable hash table, expect not to find * any matches. */ - sv = shadow_get(&obj1, SV_ID1); + sv = shadow_get(&objs[0], SV_ID1); if (!sv) pr_info(" got expected NULL result\n");
- /* - * Allocate a few shadow variables with different <obj> and <id>. - */ - sv1 = shadow_alloc(&obj1, SV_ID1, sizeof(pnfield1), GFP_KERNEL, shadow_ctor, &pnfield1); - if (!sv1) - return -ENOMEM; - - sv2 = shadow_alloc(&obj2, SV_ID1, sizeof(pnfield2), GFP_KERNEL, shadow_ctor, &pnfield2); - if (!sv2) - return -ENOMEM; - - sv3 = shadow_alloc(&obj1, SV_ID2, sizeof(pnfield3), GFP_KERNEL, shadow_ctor, &pnfield3); - if (!sv3) - return -ENOMEM; - - /* - * Verify we can find our new shadow variables and that they point - * to expected data. - */ - sv = shadow_get(&obj1, SV_ID1); - if (!sv) - return -EINVAL; - if ((char **)sv == sv1 && *sv1 == pnfield1) - pr_info(" got expected PTR%d -> PTR%d result\n", - ptr_id(sv1), ptr_id(*sv1)); - - sv = shadow_get(&obj2, SV_ID1); - if (!sv) - return -EINVAL; - if ((char **)sv == sv2 && *sv2 == pnfield2) - pr_info(" got expected PTR%d -> PTR%d result\n", - ptr_id(sv2), ptr_id(*sv2)); - - sv = shadow_get(&obj1, SV_ID2); - if (!sv) - return -EINVAL; - if ((int **)sv == sv3 && *sv3 == pnfield3) - pr_info(" got expected PTR%d -> PTR%d result\n", - ptr_id(sv3), ptr_id(*sv3)); - - /* - * Allocate or get a few more, this time with the same <obj>, <id>. - * The second invocation should return the same shadow var. - */ - sv4 = shadow_get_or_alloc(&obj3, SV_ID1, sizeof(pnfield4), GFP_KERNEL, shadow_ctor, &pnfield4); - if (!sv4) - return -ENOMEM; - - sv = shadow_get_or_alloc(&obj3, SV_ID1, sizeof(pnfield4), GFP_KERNEL, shadow_ctor, &pnfield4); - if (!sv) - return -EINVAL; - if ((int **)sv == sv4 && *sv4 == pnfield4) - pr_info(" got expected PTR%d -> PTR%d result\n", - ptr_id(sv4), ptr_id(*sv4)); - - /* - * Free the <obj=*, id> shadow variables and check that we can no - * longer find them. - */ - shadow_free(&obj1, SV_ID1, shadow_dtor); /* sv1 */ - sv = shadow_get(&obj1, SV_ID1); - if (!sv) - pr_info(" got expected NULL result\n"); + /* pass 1: init & alloc a char+int pair of svars for each objs */ + for (i = 0; i < NUM_OBJS; i++) { + pnfields1[i] = &nfields1[i]; + pnfields2[i] = &nfields2[i]; + ptr_id(pnfields1[i]); + ptr_id(pnfields2[i]); + + /* 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; + sv2[i] = shadow_alloc(&objs[i], SV_ID2, sizeof(pnfields2[i]), + GFP_KERNEL, shadow_ctor, &pnfields2[i]); + if (!sv2[i]) + return -ENOMEM; + }
- shadow_free(&obj2, SV_ID1, shadow_dtor); /* sv2 */ - sv = shadow_get(&obj2, SV_ID1); - if (!sv) - pr_info(" got expected NULL result\n"); + /* pass 2: verify we find allocated svars and where they point to */ + for (i = 0; i < NUM_OBJS; i++) { + /* check the "char" svar for all objects */ + sv = shadow_get(&objs[i], SV_ID1); + if (!sv) + return -EINVAL; + 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])); + + /* check the "int" svar for all objects */ + sv = shadow_get(&objs[i], SV_ID2); + if (!sv) + return -EINVAL; + 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])); + }
- shadow_free(&obj3, SV_ID1, shadow_dtor); /* sv4 */ - sv = shadow_get(&obj3, SV_ID1); - if (!sv) - pr_info(" got expected NULL result\n"); + /* pass 3: verify that 'get_of_alloc' returns already allocated svars */ + for (i = 0; i < NUM_OBJS; i++) { + sv = shadow_get_or_alloc(&objs[i], SV_ID1, sizeof(pndup[i]), + GFP_KERNEL, shadow_ctor, &pndup[i]); + if (!sv) + return -EINVAL; + 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])); + }
- /* - * We should still find an <id+1> variable. - */ - sv = shadow_get(&obj1, SV_ID2); - if (!sv) - return -EINVAL; - if ((int **)sv == sv3 && *sv3 == pnfield3) - pr_info(" got expected PTR%d -> PTR%d result\n", - ptr_id(sv3), ptr_id(*sv3)); + /* pass 4: free <objs[*], SV_ID1> pairs of svars, verify removal */ + for (i = 0; i < NUM_OBJS; i++) { + shadow_free(&objs[i], SV_ID1, shadow_dtor); /* 'char' pairs */ + sv = shadow_get(&objs[i], SV_ID1); + if (!sv) + pr_info(" got expected NULL result\n"); + }
- /* - * Free all the <id+1> variables, too. - */ - shadow_free_all(SV_ID2, shadow_dtor); /* sv3 */ - sv = shadow_get(&obj1, SV_ID1); - if (!sv) - pr_info(" shadow_get() got expected NULL result\n"); + /* pass 5: check we still find <objs[*], SV_ID2> svar pairs */ + for (i = 0; i < NUM_OBJS; i++) { + sv = shadow_get(&objs[i], SV_ID2); /* 'int' pairs */ + if (!sv) + return -EINVAL; + 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])); + }
+ /* pass 6: free all the <objs[*], SV_ID2> svar pairs too. */ + shadow_free_all(SV_ID2, NULL); /* 'int' pairs */ + for (i = 0; i < NUM_OBJS; i++) { + sv = shadow_get(&objs[i], SV_ID2); + if (!sv) + pr_info(" shadow_get() got expected NULL result\n"); + }
free_ptr_list();
diff --git a/tools/testing/selftests/livepatch/test-shadow-vars.sh b/tools/testing/selftests/livepatch/test-shadow-vars.sh index 1aae73299114..469ceb345b3a 100755 --- a/tools/testing/selftests/livepatch/test-shadow-vars.sh +++ b/tools/testing/selftests/livepatch/test-shadow-vars.sh @@ -19,42 +19,63 @@ load_mod $MOD_TEST unload_mod $MOD_TEST
check_result "% modprobe $MOD_TEST -$MOD_TEST: klp_shadow_get(obj=PTR5, id=0x1234) = PTR0 +$MOD_TEST: klp_shadow_get(obj=PTR1, id=0x1234) = PTR0 $MOD_TEST: got expected NULL result -$MOD_TEST: shadow_ctor: PTR6 -> PTR1 -$MOD_TEST: klp_shadow_alloc(obj=PTR5, id=0x1234, size=8, gfp_flags=GFP_KERNEL), ctor=PTR7, ctor_data=PTR1 = PTR6 -$MOD_TEST: shadow_ctor: PTR8 -> PTR2 -$MOD_TEST: klp_shadow_alloc(obj=PTR9, id=0x1234, size=8, gfp_flags=GFP_KERNEL), ctor=PTR7, ctor_data=PTR2 = PTR8 -$MOD_TEST: shadow_ctor: PTR10 -> PTR3 -$MOD_TEST: klp_shadow_alloc(obj=PTR5, id=0x1235, size=8, gfp_flags=GFP_KERNEL), ctor=PTR7, ctor_data=PTR3 = PTR10 -$MOD_TEST: klp_shadow_get(obj=PTR5, id=0x1234) = PTR6 -$MOD_TEST: got expected PTR6 -> PTR1 result -$MOD_TEST: klp_shadow_get(obj=PTR9, id=0x1234) = PTR8 -$MOD_TEST: got expected PTR8 -> PTR2 result -$MOD_TEST: klp_shadow_get(obj=PTR5, id=0x1235) = PTR10 -$MOD_TEST: got expected PTR10 -> PTR3 result -$MOD_TEST: shadow_ctor: PTR11 -> PTR4 -$MOD_TEST: klp_shadow_get_or_alloc(obj=PTR12, id=0x1234, size=8, gfp_flags=GFP_KERNEL), ctor=PTR7, ctor_data=PTR4 = PTR11 -$MOD_TEST: klp_shadow_get_or_alloc(obj=PTR12, id=0x1234, size=8, gfp_flags=GFP_KERNEL), ctor=PTR7, ctor_data=PTR4 = PTR11 -$MOD_TEST: got expected PTR11 -> PTR4 result -$MOD_TEST: shadow_dtor(obj=PTR5, shadow_data=PTR6) -$MOD_TEST: klp_shadow_free(obj=PTR5, id=0x1234, dtor=PTR13) -$MOD_TEST: klp_shadow_get(obj=PTR5, id=0x1234) = PTR0 +$MOD_TEST: shadow_ctor: PTR4 -> PTR2 +$MOD_TEST: klp_shadow_alloc(obj=PTR1, id=0x1234, size=8, gfp_flags=GFP_KERNEL), ctor=PTR5, ctor_data=PTR2 = PTR4 +$MOD_TEST: shadow_ctor: PTR6 -> PTR3 +$MOD_TEST: klp_shadow_alloc(obj=PTR1, id=0x1235, size=8, gfp_flags=GFP_KERNEL), ctor=PTR5, ctor_data=PTR3 = PTR6 +$MOD_TEST: shadow_ctor: PTR9 -> PTR7 +$MOD_TEST: klp_shadow_alloc(obj=PTR10, id=0x1234, size=8, gfp_flags=GFP_KERNEL), ctor=PTR5, ctor_data=PTR7 = PTR9 +$MOD_TEST: shadow_ctor: PTR11 -> PTR8 +$MOD_TEST: klp_shadow_alloc(obj=PTR10, id=0x1235, size=8, gfp_flags=GFP_KERNEL), ctor=PTR5, ctor_data=PTR8 = PTR11 +$MOD_TEST: shadow_ctor: PTR14 -> PTR12 +$MOD_TEST: klp_shadow_alloc(obj=PTR15, id=0x1234, size=8, gfp_flags=GFP_KERNEL), ctor=PTR5, ctor_data=PTR12 = PTR14 +$MOD_TEST: shadow_ctor: PTR16 -> PTR13 +$MOD_TEST: klp_shadow_alloc(obj=PTR15, id=0x1235, size=8, gfp_flags=GFP_KERNEL), ctor=PTR5, ctor_data=PTR13 = PTR16 +$MOD_TEST: klp_shadow_get(obj=PTR1, id=0x1234) = PTR4 +$MOD_TEST: got expected PTR4 -> PTR2 result +$MOD_TEST: klp_shadow_get(obj=PTR1, id=0x1235) = PTR6 +$MOD_TEST: got expected PTR6 -> PTR3 result +$MOD_TEST: klp_shadow_get(obj=PTR10, id=0x1234) = PTR9 +$MOD_TEST: got expected PTR9 -> PTR7 result +$MOD_TEST: klp_shadow_get(obj=PTR10, id=0x1235) = PTR11 +$MOD_TEST: got expected PTR11 -> PTR8 result +$MOD_TEST: klp_shadow_get(obj=PTR15, id=0x1234) = PTR14 +$MOD_TEST: got expected PTR14 -> PTR12 result +$MOD_TEST: klp_shadow_get(obj=PTR15, id=0x1235) = PTR16 +$MOD_TEST: got expected PTR16 -> PTR13 result +$MOD_TEST: klp_shadow_get_or_alloc(obj=PTR1, id=0x1234, size=8, gfp_flags=GFP_KERNEL), ctor=PTR5, ctor_data=PTR17 = PTR4 +$MOD_TEST: got expected PTR4 -> PTR2 result +$MOD_TEST: klp_shadow_get_or_alloc(obj=PTR10, id=0x1234, size=8, gfp_flags=GFP_KERNEL), ctor=PTR5, ctor_data=PTR18 = PTR9 +$MOD_TEST: got expected PTR9 -> PTR7 result +$MOD_TEST: klp_shadow_get_or_alloc(obj=PTR15, id=0x1234, size=8, gfp_flags=GFP_KERNEL), ctor=PTR5, ctor_data=PTR19 = PTR14 +$MOD_TEST: got expected PTR14 -> PTR12 result +$MOD_TEST: shadow_dtor(obj=PTR1, shadow_data=PTR4) +$MOD_TEST: klp_shadow_free(obj=PTR1, id=0x1234, dtor=PTR20) +$MOD_TEST: klp_shadow_get(obj=PTR1, id=0x1234) = PTR0 $MOD_TEST: got expected NULL result -$MOD_TEST: shadow_dtor(obj=PTR9, shadow_data=PTR8) -$MOD_TEST: klp_shadow_free(obj=PTR9, id=0x1234, dtor=PTR13) -$MOD_TEST: klp_shadow_get(obj=PTR9, id=0x1234) = PTR0 +$MOD_TEST: shadow_dtor(obj=PTR10, shadow_data=PTR9) +$MOD_TEST: klp_shadow_free(obj=PTR10, id=0x1234, dtor=PTR20) +$MOD_TEST: klp_shadow_get(obj=PTR10, id=0x1234) = PTR0 $MOD_TEST: got expected NULL result -$MOD_TEST: shadow_dtor(obj=PTR12, shadow_data=PTR11) -$MOD_TEST: klp_shadow_free(obj=PTR12, id=0x1234, dtor=PTR13) -$MOD_TEST: klp_shadow_get(obj=PTR12, id=0x1234) = PTR0 +$MOD_TEST: shadow_dtor(obj=PTR15, shadow_data=PTR14) +$MOD_TEST: klp_shadow_free(obj=PTR15, id=0x1234, dtor=PTR20) +$MOD_TEST: klp_shadow_get(obj=PTR15, id=0x1234) = PTR0 $MOD_TEST: got expected NULL result -$MOD_TEST: klp_shadow_get(obj=PTR5, id=0x1235) = PTR10 -$MOD_TEST: got expected PTR10 -> PTR3 result -$MOD_TEST: shadow_dtor(obj=PTR5, shadow_data=PTR10) -$MOD_TEST: klp_shadow_free_all(id=0x1235, dtor=PTR13) -$MOD_TEST: klp_shadow_get(obj=PTR5, id=0x1234) = PTR0 +$MOD_TEST: klp_shadow_get(obj=PTR1, id=0x1235) = PTR6 +$MOD_TEST: got expected PTR6 -> PTR3 result +$MOD_TEST: klp_shadow_get(obj=PTR10, id=0x1235) = PTR11 +$MOD_TEST: got expected PTR11 -> PTR8 result +$MOD_TEST: klp_shadow_get(obj=PTR15, id=0x1235) = PTR16 +$MOD_TEST: got expected PTR16 -> PTR13 result +$MOD_TEST: klp_shadow_free_all(id=0x1235, dtor=PTR0) +$MOD_TEST: klp_shadow_get(obj=PTR1, id=0x1235) = PTR0 $MOD_TEST: shadow_get() got expected NULL result -% rmmod test_klp_shadow_vars" +$MOD_TEST: klp_shadow_get(obj=PTR10, id=0x1235) = PTR0 +$MOD_TEST: shadow_get() got expected NULL result +$MOD_TEST: klp_shadow_get(obj=PTR15, id=0x1235) = PTR0 +$MOD_TEST: shadow_get() got expected NULL result +% rmmod $MOD_TEST"
exit 0
On Thu, 28 May 2020, Yannick Cote wrote:
This change makes the test feel more familiar with narrowing to a typical usage by operating on a number of identical structure instances and populating the same two new shadow variables symmetrically while keeping the same testing and verification criteria for the extra variables.
It changes the order of shadow vars allocation and freeing (now we allocate everything first, while previously the order was more complicated), but it does not matter, I think.
One nit below and I have to trust you with the output.
Signed-off-by: Yannick Cote ycote@redhat.com
lib/livepatch/test_klp_shadow_vars.c | 182 ++++++++---------- .../selftests/livepatch/test-shadow-vars.sh | 85 +++++--- 2 files changed, 135 insertions(+), 132 deletions(-)
diff --git a/lib/livepatch/test_klp_shadow_vars.c b/lib/livepatch/test_klp_shadow_vars.c index ec2635cff974..195309e1edf3 100644 --- a/lib/livepatch/test_klp_shadow_vars.c +++ b/lib/livepatch/test_klp_shadow_vars.c @@ -128,6 +128,11 @@ static int shadow_ctor(void *obj, void *shadow_data, void *ctor_data) return 0; } +/*
- With more than one item to free in the list, order is not determined and
- shadow_dtor will not be passed to shadow_free_all() which would make the
- test fail. (see pass 6)
- */
static void shadow_dtor(void *obj, void *shadow_data) { int **sv = shadow_data; @@ -136,6 +141,9 @@ static void shadow_dtor(void *obj, void *shadow_data) __func__, ptr_id(obj), ptr_id(sv)); } +/* number of objects we simulate that need kpatch-patch shadow vars */
s/kpatch-patch// ?
Miroslav
Noticed one more thing nevertheless...
- /* pass 6: free all the <objs[*], SV_ID2> svar pairs too. */
- shadow_free_all(SV_ID2, NULL); /* 'int' pairs */
- for (i = 0; i < NUM_OBJS; i++) {
sv = shadow_get(&objs[i], SV_ID2);if (!sv)pr_info(" shadow_get() got expected NULL result\n");- }
I think that shadow_get() should be removed from the message to make it consistent with the rest.
Miroslav
On Thu 2020-05-28 09:48:48, Yannick Cote wrote:
This change makes the test feel more familiar with narrowing to a typical usage by operating on a number of identical structure instances and populating the same two new shadow variables symmetrically while keeping the same testing and verification criteria for the extra variables.
@@ -157,122 +165,96 @@ struct test_object { static int test_klp_shadow_vars_init(void) {
- struct test_object obj1, obj2, obj3;
- char nfield1, nfield2, *pnfield1, *pnfield2, **sv1, **sv2;
- int nfield3, nfield4, *pnfield3, *pnfield4, **sv3, **sv4;
- struct test_object objs[NUM_OBJS];
- char nfields1[NUM_OBJS], *pnfields1[NUM_OBJS], **sv1[NUM_OBJS];
- char *pndup[NUM_OBJS];
- int nfields2[NUM_OBJS], *pnfields2[NUM_OBJS], **sv2[NUM_OBJS]; void **sv;
- /* pass 1: init & alloc a char+int pair of svars for each objs */
- for (i = 0; i < NUM_OBJS; i++) {
pnfields1[i] = &nfields1[i];pnfields2[i] = &nfields2[i];ptr_id(pnfields1[i]);ptr_id(pnfields2[i]);/* 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;
Please, put empty line here to delimit ID1 ID2 handling a bit.
Also I have got a bit more predictable PTR IDs when I moved pnfields2 initialization here:
pnfields2[i] = &nfields2[i]; ptr_id(pnfields2[i]);
sv2[i] = shadow_alloc(&objs[i], SV_ID2, sizeof(pnfields2[i]),GFP_KERNEL, shadow_ctor, &pnfields2[i]);if (!sv2[i])return -ENOMEM;- }
It looks like:
test_klp_shadow_vars: klp_shadow_alloc(obj=PTR1, id=0x1234, size=8, gfp_flags=GFP_KERNEL), ctor=PTR4, ctor_data=PTR2 = PTR3 test_klp_shadow_vars: shadow_ctor: PTR6 -> PTR5 test_klp_shadow_vars: klp_shadow_alloc(obj=PTR1, id=0x1235, size=8, gfp_flags=GFP_KERNEL), ctor=PTR4, ctor_data=PTR5 = PTR6 test_klp_shadow_vars: shadow_ctor: PTR8 -> PTR7 test_klp_shadow_vars: klp_shadow_alloc(obj=PTR9, id=0x1234, size=8, gfp_flags=GFP_KERNEL), ctor=PTR4, ctor_data=PTR7 = PTR8 test_klp_shadow_vars: shadow_ctor: PTR11 -> PTR10
instead of
test_klp_shadow_vars: klp_shadow_alloc(obj=PTR1, id=0x1234, size=8, gfp_flags=GFP_KERNEL), ctor=PTR5, ctor_data=PTR2 = PTR4 test_klp_shadow_vars: shadow_ctor: PTR6 -> PTR3 test_klp_shadow_vars: klp_shadow_alloc(obj=PTR1, id=0x1235, size=8, gfp_flags=GFP_KERNEL), ctor=PTR5, ctor_data=PTR3 = PTR6 test_klp_shadow_vars: shadow_ctor: PTR9 -> PTR7 test_klp_shadow_vars: klp_shadow_alloc(obj=PTR10, id=0x1234, size=8, gfp_flags=GFP_KERNEL), ctor=PTR5, ctor_data=PTR7 = PTR9 test_klp_shadow_vars: shadow_ctor: PTR11 -> PTR8
By other words, the PTR IDs are incrementing by the same offset for both SV_ID1 and SV_ID2. It looks better even later in the log.
- /* pass 3: verify that 'get_of_alloc' returns already allocated svars */
- for (i = 0; i < NUM_OBJS; i++) {
sv = shadow_get_or_alloc(&objs[i], SV_ID1, sizeof(pndup[i]),GFP_KERNEL, shadow_ctor, &pndup[i]);
First, the test failed on my system. I have got:
# --- expected # +++ result # @@ -27,20 +27,20 @@ test_klp_shadow_vars: klp_shadow_get(obj # test_klp_shadow_vars: got expected PTR16 -> PTR13 result # test_klp_shadow_vars: klp_shadow_get_or_alloc(obj=PTR1, id=0x1234, size=8, gfp_flags=GFP_KERNEL), ctor=PTR5, ctor_data=PTR17 = PTR4 # test_klp_shadow_vars: got expected PTR4 -> PTR2 result # -test_klp_shadow_vars: klp_shadow_get_or_alloc(obj=PTR10, id=0x1234, size=8, gfp_flags=GFP_KERNEL), ctor=PTR5, ctor_data=PTR18 = PTR9 # +test_klp_shadow_vars: klp_shadow_get_or_alloc(obj=PTR10, id=0x1234, size=8, gfp_flags=GFP_KERNEL), ctor=PTR5, ctor_data=PTR0 = PTR9 # test_klp_shadow_vars: got expected PTR9 -> PTR7 result # -test_klp_shadow_vars: klp_shadow_get_or_alloc(obj=PTR15, id=0x1234, size=8, gfp_flags=GFP_KERNEL), ctor=PTR5, ctor_data=PTR19 = PTR14 # +test_klp_shadow_vars: klp_shadow_get_or_alloc(obj=PTR15, id=0x1234, size=8, gfp_flags=GFP_KERNEL), ctor=PTR5, ctor_data=PTR0 = PTR14
In my build, it uses PTR0 for ctor_data. But it takes a new pointer in your case.
It is because pndup[i] was not initialized. Note that it is the value (data) that is stored in the shadow variable.
The solution is to initialize pndup[i] here:
pndup[i] = &nfields1[i]; ptr_id(pndup[i]);
2nd problem, klp_shadow_get_or_alloc() is always called for already allocated values now. It would be great to test that they can be created when they are not available.
A solution might be to allocate half of the variables by shadow_alloc() and the other half with shadow_get_or_alloc(). I would do this in the first cycle, using:
if (i % 2) { sv1[i] = shadow_alloc(&objs[i], SV_ID1, sizeof(pnfields1[i]), GFP_KERNEL, shadow_ctor, &pnfields1[i]); } else { sv1[i] = shadow_get_or_alloc(&objs[i], SV_ID1, sizeof(pnfields1[i]), GFP_KERNEL, shadow_ctor, &pnfields1[i]); }
Otherwise, it is a nice clean up.
Best Regards, Petr
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; 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)
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;}
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
On 5/28/20 9:48 AM, Yannick Cote wrote:
The test-klp-callbacks change implement a synchronization replacement of initial code to use completion variables instead of delays. The completion variable interlocks the busy module with the concurrent loading of the target livepatch patches which works with the execution flow instead of estimated time delays.
For more context: we had been seeing occasional glitches with this test in our continuous kernel integration suite. In every case, it seemed that the worker thread wasn't running when expected, so I assumed that system load had something to do with it. We shuffled the ordering of tests, but still encountered issues and I decided life was too sort to continue remotely debugging sleep-"synchronized" code.
The test-klp-shadow-vars changes first refactors the code to be more of a readable example as well as continuing to verify the component code. The patch is broken in two to display the renaming and restructuring in part 1 and the addition and change of logic in part 2. The last change frees memory before bailing in case of errors.
Yannick's patches look fine to me, so for those:
Acked-by: Joe Lawrence joe.lawrence@redhat.com
(I can ack individually if required, let me know.)
-- Joe
On Thu, 28 May 2020, Yannick Cote wrote:
The test-klp-callbacks change implement a synchronization replacement of initial code to use completion variables instead of delays. The completion variable interlocks the busy module with the concurrent loading of the target livepatch patches which works with the execution flow instead of estimated time delays.
The test-klp-shadow-vars changes first refactors the code to be more of a readable example as well as continuing to verify the component code. The patch is broken in two to display the renaming and restructuring in part 1 and the addition and change of logic in part 2. The last change frees memory before bailing in case of errors.
With the small comments, I made, fixed
Acked-by: Miroslav Benes mbenes@suse.cz
M
On 5/28/20 7:18 PM, Yannick Cote wrote:
The test-klp-callbacks change implement a synchronization replacement of initial code to use completion variables instead of delays. The completion variable interlocks the busy module with the concurrent loading of the target livepatch patches which works with the execution flow instead of estimated time delays.
The test-klp-shadow-vars changes first refactors the code to be more of a readable example as well as continuing to verify the component code. The patch is broken in two to display the renaming and restructuring in part 1 and the addition and change of logic in part 2. The last change frees memory before bailing in case of errors.
Patchset to be merged via the livepatching tree is against: livepatching/for-next
Joe Lawrence (1): selftests/livepatch: rework test-klp-callbacks to use completion variables
Yannick Cote (3): selftests/livepatch: rework test-klp-shadow-vars selftests/livepatch: more verification in test-klp-shadow-vars selftests/livepatch: fix mem leaks in test-klp-shadow-vars
lib/livepatch/test_klp_callbacks_busy.c | 42 +++- lib/livepatch/test_klp_shadow_vars.c | 222 +++++++++--------- .../selftests/livepatch/test-callbacks.sh | 29 ++- .../selftests/livepatch/test-shadow-vars.sh | 85 ++++--- 4 files changed, 214 insertions(+), 164 deletions(-)
Series looks good to me, with one minor typo in patch 3 (s/kpatch-patch//), which Miroslav as already mentioned.
Reviewed-by: Kamalesh Babulal kamalesh@linux.vnet.ibm.com
linux-kselftest-mirror@lists.linaro.org
-
Joe Lawrence -
Kamalesh Babulal -
Miroslav Benes -
Petr Mladek -
Yannick Cote