[PATCH 0/4] livepatch/samples/selftest: Clean up show variables handling

[PATCH 0/4] livepatch/samples/selftest: Clean up show variables handling

[Petr Mladek]

Dan Carpenter reported suspicious allocations of shadow variables
in the sample module, see
https://lkml.kernel.org/r/20200107132929.ficffmrm5ntpzcqa@kili.mountain

The code did not cause a real problem. But it was indeed misleading
and semantically wrong. I got confused several times when cleaning it.
So I decided to split the change into few steps. I hope that
it will help reviewers and future readers.

The changes of the sample module are basically the same as in the RFC.
In addition, there is a clean up of the module used by the selftest.


Petr Mladek (4):
  livepatch/sample: Use the right type for the leaking data pointer
  livepatch/selftest: Clean up shadow variable names and type
  livepatch/samples/selftest: Use klp_shadow_alloc() API correctly
  livepatch: Handle allocation failure in the sample of shadow variable
    API

 lib/livepatch/test_klp_shadow_vars.c      | 119 +++++++++++++++++-------------
 samples/livepatch/livepatch-shadow-fix1.c |  39 ++++++----
 samples/livepatch/livepatch-shadow-fix2.c |   4 +-
 samples/livepatch/livepatch-shadow-mod.c  |   4 +-
 4 files changed, 99 insertions(+), 67 deletions(-)

-- 
2.16.4

[PATCH 1/4] livepatch/sample: Use the right type for the leaking data pointer

[Petr Mladek]

The "leak" pointer, in the sample of shadow variable API, is allocated
as sizeof(int). Let's help developers and static analyzers with
understanding the code by using the appropriate pointer type.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 samples/livepatch/livepatch-shadow-fix1.c | 12 ++++++------
 samples/livepatch/livepatch-shadow-fix2.c |  4 ++--
 samples/livepatch/livepatch-shadow-mod.c  |  4 ++--
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c
index e89ca4546114..bab12bdb753f 100644
--- a/samples/livepatch/livepatch-shadow-fix1.c
+++ b/samples/livepatch/livepatch-shadow-fix1.c
@@ -52,8 +52,8 @@ struct dummy {
  */
 static int shadow_leak_ctor(void *obj, void *shadow_data, void *ctor_data)
 {
-	void **shadow_leak = shadow_data;
-	void *leak = ctor_data;
+	int **shadow_leak = shadow_data;
+	int *leak = ctor_data;
 
 	*shadow_leak = leak;
 	return 0;
@@ -62,7 +62,7 @@ static int shadow_leak_ctor(void *obj, void *shadow_data, void *ctor_data)
 static struct dummy *livepatch_fix1_dummy_alloc(void)
 {
 	struct dummy *d;
-	void *leak;
+	int *leak;
 
 	d = kzalloc(sizeof(*d), GFP_KERNEL);
 	if (!d)
@@ -76,7 +76,7 @@ static struct dummy *livepatch_fix1_dummy_alloc(void)
 	 * variable.  A patched dummy_free routine can later fetch this
 	 * pointer to handle resource release.
 	 */
-	leak = kzalloc(sizeof(int), GFP_KERNEL);
+	leak = kzalloc(sizeof(*leak), GFP_KERNEL);
 	if (!leak) {
 		kfree(d);
 		return NULL;
@@ -94,7 +94,7 @@ static struct dummy *livepatch_fix1_dummy_alloc(void)
 static void livepatch_fix1_dummy_leak_dtor(void *obj, void *shadow_data)
 {
 	void *d = obj;
-	void **shadow_leak = shadow_data;
+	int **shadow_leak = shadow_data;
 
 	kfree(*shadow_leak);
 	pr_info("%s: dummy @ %p, prevented leak @ %p\n",
@@ -103,7 +103,7 @@ static void livepatch_fix1_dummy_leak_dtor(void *obj, void *shadow_data)
 
 static void livepatch_fix1_dummy_free(struct dummy *d)
 {
-	void **shadow_leak;
+	int **shadow_leak;
 
 	/*
 	 * Patch: fetch the saved SV_LEAK shadow variable, detach and
diff --git a/samples/livepatch/livepatch-shadow-fix2.c b/samples/livepatch/livepatch-shadow-fix2.c
index 50d223b82e8b..29fe5cd42047 100644
--- a/samples/livepatch/livepatch-shadow-fix2.c
+++ b/samples/livepatch/livepatch-shadow-fix2.c
@@ -59,7 +59,7 @@ static bool livepatch_fix2_dummy_check(struct dummy *d, unsigned long jiffies)
 static void livepatch_fix2_dummy_leak_dtor(void *obj, void *shadow_data)
 {
 	void *d = obj;
-	void **shadow_leak = shadow_data;
+	int **shadow_leak = shadow_data;
 
 	kfree(*shadow_leak);
 	pr_info("%s: dummy @ %p, prevented leak @ %p\n",
@@ -68,7 +68,7 @@ static void livepatch_fix2_dummy_leak_dtor(void *obj, void *shadow_data)
 
 static void livepatch_fix2_dummy_free(struct dummy *d)
 {
-	void **shadow_leak;
+	int **shadow_leak;
 	int *shadow_count;
 
 	/* Patch: copy the memory leak patch from the fix1 module. */
diff --git a/samples/livepatch/livepatch-shadow-mod.c b/samples/livepatch/livepatch-shadow-mod.c
index ecfe83a943a7..7e753b0d2fa6 100644
--- a/samples/livepatch/livepatch-shadow-mod.c
+++ b/samples/livepatch/livepatch-shadow-mod.c
@@ -95,7 +95,7 @@ struct dummy {
 static __used noinline struct dummy *dummy_alloc(void)
 {
 	struct dummy *d;
-	void *leak;
+	int *leak;
 
 	d = kzalloc(sizeof(*d), GFP_KERNEL);
 	if (!d)
@@ -105,7 +105,7 @@ static __used noinline struct dummy *dummy_alloc(void)
 		msecs_to_jiffies(1000 * EXPIRE_PERIOD);
 
 	/* Oops, forgot to save leak! */
-	leak = kzalloc(sizeof(int), GFP_KERNEL);
+	leak = kzalloc(sizeof(*leak), GFP_KERNEL);
 	if (!leak) {
 		kfree(d);
 		return NULL;
-- 
2.16.4

[PATCH 2/4] livepatch/selftest: Clean up shadow variable names and type

[Petr Mladek]

The shadow variable selftest is quite tricky. Especially it is problematic
to understand what values are stored, returned, and printed.

Make it easier to understand by using "int *var, **sv" variables
consistently everywhere instead of the generic "void *", "ret",
and "ctor_data".

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 lib/livepatch/test_klp_shadow_vars.c | 93 ++++++++++++++++++++----------------
 1 file changed, 52 insertions(+), 41 deletions(-)

diff --git a/lib/livepatch/test_klp_shadow_vars.c b/lib/livepatch/test_klp_shadow_vars.c
index fe5c413efe96..4e94f46234e8 100644
--- a/lib/livepatch/test_klp_shadow_vars.c
+++ b/lib/livepatch/test_klp_shadow_vars.c
@@ -60,36 +60,43 @@ static int ptr_id(void *ptr)
  */
 static void *shadow_get(void *obj, unsigned long id)
 {
-	void *ret = klp_shadow_get(obj, id);
+	int **sv;
 
+	sv = klp_shadow_get(obj, id);
 	pr_info("klp_%s(obj=PTR%d, id=0x%lx) = PTR%d\n",
-		__func__, ptr_id(obj), id, ptr_id(ret));
+		__func__, ptr_id(obj), id, ptr_id(sv));
 
-	return ret;
+	return sv;
 }
 
 static void *shadow_alloc(void *obj, unsigned long id, size_t size,
 			  gfp_t gfp_flags, klp_shadow_ctor_t ctor,
 			  void *ctor_data)
 {
-	void *ret = klp_shadow_alloc(obj, id, size, gfp_flags, ctor,
-				     ctor_data);
+	int *var = ctor_data;
+	int **sv;
+
+	sv = klp_shadow_alloc(obj, id, size, gfp_flags, ctor, var);
 	pr_info("klp_%s(obj=PTR%d, id=0x%lx, size=%zx, gfp_flags=%pGg), ctor=PTR%d, ctor_data=PTR%d = PTR%d\n",
 		__func__, ptr_id(obj), id, size, &gfp_flags, ptr_id(ctor),
-		ptr_id(ctor_data), ptr_id(ret));
-	return ret;
+		ptr_id(var), ptr_id(sv));
+
+	return sv;
 }
 
 static void *shadow_get_or_alloc(void *obj, unsigned long id, size_t size,
 				 gfp_t gfp_flags, klp_shadow_ctor_t ctor,
 				 void *ctor_data)
 {
-	void *ret = klp_shadow_get_or_alloc(obj, id, size, gfp_flags, ctor,
-					    ctor_data);
+	int *var = ctor_data;
+	int **sv;
+
+	sv = klp_shadow_get_or_alloc(obj, id, size, gfp_flags, ctor, var);
 	pr_info("klp_%s(obj=PTR%d, id=0x%lx, size=%zx, gfp_flags=%pGg), ctor=PTR%d, ctor_data=PTR%d = PTR%d\n",
 		__func__, ptr_id(obj), id, size, &gfp_flags, ptr_id(ctor),
-		ptr_id(ctor_data), ptr_id(ret));
-	return ret;
+		ptr_id(var), ptr_id(sv));
+
+	return sv;
 }
 
 static void shadow_free(void *obj, unsigned long id, klp_shadow_dtor_t dtor)
@@ -110,18 +117,22 @@ static void shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor)
 /* Shadow variable constructor - remember simple pointer data */
 static int shadow_ctor(void *obj, void *shadow_data, void *ctor_data)
 {
-	int **shadow_int = shadow_data;
-	*shadow_int = ctor_data;
+	int **sv = shadow_data;
+	int *var = ctor_data;
+
+	*sv = var;
 	pr_info("%s: PTR%d -> PTR%d\n",
-		__func__, ptr_id(shadow_int), ptr_id(ctor_data));
+		__func__, ptr_id(sv), ptr_id(var));
 
 	return 0;
 }
 
 static void shadow_dtor(void *obj, void *shadow_data)
 {
+	int **sv = shadow_data;
+
 	pr_info("%s(obj=PTR%d, shadow_data=PTR%d)\n",
-		__func__, ptr_id(obj), ptr_id(shadow_data));
+		__func__, ptr_id(obj), ptr_id(sv));
 }
 
 static int test_klp_shadow_vars_init(void)
@@ -134,7 +145,7 @@ static int test_klp_shadow_vars_init(void)
 	int var1, var2, var3, var4;
 	int **sv1, **sv2, **sv3, **sv4;
 
-	void *ret;
+	int **sv;
 
 	ptr_id(NULL);
 	ptr_id(&var1);
@@ -146,8 +157,8 @@ static int test_klp_shadow_vars_init(void)
 	 * With an empty shadow variable hash table, expect not to find
 	 * any matches.
 	 */
-	ret = shadow_get(obj, id);
-	if (!ret)
+	sv = shadow_get(obj, id);
+	if (!sv)
 		pr_info("  got expected NULL result\n");
 
 	/*
@@ -169,23 +180,23 @@ static int test_klp_shadow_vars_init(void)
 	 * Verify we can find our new shadow variables and that they point
 	 * to expected data.
 	 */
-	ret = shadow_get(obj, id);
-	if (!ret)
+	sv = shadow_get(obj, id);
+	if (!sv)
 		return -EINVAL;
-	if (ret == sv1 && *sv1 == &var1)
+	if (sv == sv1 && *sv1 == &var1)
 		pr_info("  got expected PTR%d -> PTR%d result\n",
 			ptr_id(sv1), ptr_id(*sv1));
 
-	ret = shadow_get(obj + 1, id);
-	if (!ret)
+	sv = shadow_get(obj + 1, id);
+	if (!sv)
 		return -EINVAL;
-	if (ret == sv2 && *sv2 == &var2)
+	if (sv == sv2 && *sv2 == &var2)
 		pr_info("  got expected PTR%d -> PTR%d result\n",
 			ptr_id(sv2), ptr_id(*sv2));
-	ret = shadow_get(obj, id + 1);
-	if (!ret)
+	sv = shadow_get(obj, id + 1);
+	if (!sv)
 		return -EINVAL;
-	if (ret == sv3 && *sv3 == &var3)
+	if (sv == sv3 && *sv3 == &var3)
 		pr_info("  got expected PTR%d -> PTR%d result\n",
 			ptr_id(sv3), ptr_id(*sv3));
 
@@ -197,10 +208,10 @@ static int test_klp_shadow_vars_init(void)
 	if (!sv4)
 		return -ENOMEM;
 
-	ret = shadow_get_or_alloc(obj + 2, id, size, gfp_flags, shadow_ctor, &var4);
-	if (!ret)
+	sv = shadow_get_or_alloc(obj + 2, id, size, gfp_flags, shadow_ctor, &var4);
+	if (!sv)
 		return -EINVAL;
-	if (ret == sv4 && *sv4 == &var4)
+	if (sv == sv4 && *sv4 == &var4)
 		pr_info("  got expected PTR%d -> PTR%d result\n",
 			ptr_id(sv4), ptr_id(*sv4));
 
@@ -209,27 +220,27 @@ static int test_klp_shadow_vars_init(void)
 	 * longer find them.
 	 */
 	shadow_free(obj, id, shadow_dtor);			/* sv1 */
-	ret = shadow_get(obj, id);
-	if (!ret)
+	sv = shadow_get(obj, id);
+	if (!sv)
 		pr_info("  got expected NULL result\n");
 
 	shadow_free(obj + 1, id, shadow_dtor);			/* sv2 */
-	ret = shadow_get(obj + 1, id);
-	if (!ret)
+	sv = shadow_get(obj + 1, id);
+	if (!sv)
 		pr_info("  got expected NULL result\n");
 
 	shadow_free(obj + 2, id, shadow_dtor);			/* sv4 */
-	ret = shadow_get(obj + 2, id);
-	if (!ret)
+	sv = shadow_get(obj + 2, id);
+	if (!sv)
 		pr_info("  got expected NULL result\n");
 
 	/*
 	 * We should still find an <id+1> variable.
 	 */
-	ret = shadow_get(obj, id + 1);
-	if (!ret)
+	sv = shadow_get(obj, id + 1);
+	if (!sv)
 		return -EINVAL;
-	if (ret == sv3 && *sv3 == &var3)
+	if (sv == sv3 && *sv3 == &var3)
 		pr_info("  got expected PTR%d -> PTR%d result\n",
 			ptr_id(sv3), ptr_id(*sv3));
 
@@ -237,8 +248,8 @@ static int test_klp_shadow_vars_init(void)
 	 * Free all the <id+1> variables, too.
 	 */
 	shadow_free_all(id + 1, shadow_dtor);			/* sv3 */
-	ret = shadow_get(obj, id);
-	if (!ret)
+	sv = shadow_get(obj, id);
+	if (!sv)
 		pr_info("  shadow_get() got expected NULL result\n");
 
 
-- 
2.16.4

[PATCH 3/4] livepatch/samples/selftest: Use klp_shadow_alloc() API correctly

[Petr Mladek]

The commit e91c2518a5d22a ("livepatch: Initialize shadow variables
safely by a custom callback") leads to the following static checker
warning:

  samples/livepatch/livepatch-shadow-fix1.c:86 livepatch_fix1_dummy_alloc()
  error: 'klp_shadow_alloc()' 'leak' too small (4 vs 8)

It is because klp_shadow_alloc() is used a wrong way:

  int *leak;
  shadow_leak = klp_shadow_alloc(d, SV_LEAK, sizeof(leak), GFP_KERNEL,
				 shadow_leak_ctor, leak);

The code is supposed to store the "leak" pointer into the shadow variable.
3rd parameter correctly passes size of the data (size of pointer). But
the 5th parameter is wrong. It should pass pointer to the data (pointer
to the pointer) but it passes the pointer directly.

It works because shadow_leak_ctor() handle "ctor_data" as the data
instead of pointer to the data. But it is semantically wrong and
confusing.

The same problem is also in the module used by selftests. In this case,
"pvX" variables are introduced. They represent the data stored in
the shadow variables.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 lib/livepatch/test_klp_shadow_vars.c      | 52 ++++++++++++++++++-------------
 samples/livepatch/livepatch-shadow-fix1.c |  9 ++++--
 2 files changed, 36 insertions(+), 25 deletions(-)

diff --git a/lib/livepatch/test_klp_shadow_vars.c b/lib/livepatch/test_klp_shadow_vars.c
index 4e94f46234e8..f0b5a1d24e55 100644
--- a/lib/livepatch/test_klp_shadow_vars.c
+++ b/lib/livepatch/test_klp_shadow_vars.c
@@ -73,13 +73,13 @@ static void *shadow_alloc(void *obj, unsigned long id, size_t size,
 			  gfp_t gfp_flags, klp_shadow_ctor_t ctor,
 			  void *ctor_data)
 {
-	int *var = ctor_data;
+	int **var = ctor_data;
 	int **sv;
 
 	sv = klp_shadow_alloc(obj, id, size, gfp_flags, ctor, var);
 	pr_info("klp_%s(obj=PTR%d, id=0x%lx, size=%zx, gfp_flags=%pGg), ctor=PTR%d, ctor_data=PTR%d = PTR%d\n",
 		__func__, ptr_id(obj), id, size, &gfp_flags, ptr_id(ctor),
-		ptr_id(var), ptr_id(sv));
+		ptr_id(*var), ptr_id(sv));
 
 	return sv;
 }
@@ -88,13 +88,13 @@ static void *shadow_get_or_alloc(void *obj, unsigned long id, size_t size,
 				 gfp_t gfp_flags, klp_shadow_ctor_t ctor,
 				 void *ctor_data)
 {
-	int *var = ctor_data;
+	int **var = ctor_data;
 	int **sv;
 
 	sv = klp_shadow_get_or_alloc(obj, id, size, gfp_flags, ctor, var);
 	pr_info("klp_%s(obj=PTR%d, id=0x%lx, size=%zx, gfp_flags=%pGg), ctor=PTR%d, ctor_data=PTR%d = PTR%d\n",
 		__func__, ptr_id(obj), id, size, &gfp_flags, ptr_id(ctor),
-		ptr_id(var), ptr_id(sv));
+		ptr_id(*var), ptr_id(sv));
 
 	return sv;
 }
@@ -118,11 +118,14 @@ static void shadow_free_all(unsigned long id, klp_shadow_dtor_t dtor)
 static int shadow_ctor(void *obj, void *shadow_data, void *ctor_data)
 {
 	int **sv = shadow_data;
-	int *var = ctor_data;
+	int **var = ctor_data;
 
-	*sv = var;
+	if (!var)
+		return -EINVAL;
+
+	*sv = *var;
 	pr_info("%s: PTR%d -> PTR%d\n",
-		__func__, ptr_id(sv), ptr_id(var));
+		__func__, ptr_id(sv), ptr_id(*var));
 
 	return 0;
 }
@@ -139,19 +142,24 @@ static int test_klp_shadow_vars_init(void)
 {
 	void *obj			= THIS_MODULE;
 	int id			= 0x1234;
-	size_t size		= sizeof(int *);
 	gfp_t gfp_flags		= GFP_KERNEL;
 
 	int var1, var2, var3, var4;
+	int *pv1, *pv2, *pv3, *pv4;
 	int **sv1, **sv2, **sv3, **sv4;
 
 	int **sv;
 
+	pv1 = &var1;
+	pv2 = &var2;
+	pv3 = &var3;
+	pv4 = &var4;
+
 	ptr_id(NULL);
-	ptr_id(&var1);
-	ptr_id(&var2);
-	ptr_id(&var3);
-	ptr_id(&var4);
+	ptr_id(pv1);
+	ptr_id(pv2);
+	ptr_id(pv3);
+	ptr_id(pv4);
 
 	/*
 	 * With an empty shadow variable hash table, expect not to find
@@ -164,15 +172,15 @@ static int test_klp_shadow_vars_init(void)
 	/*
 	 * Allocate a few shadow variables with different <obj> and <id>.
 	 */
-	sv1 = shadow_alloc(obj, id, size, gfp_flags, shadow_ctor, &var1);
+	sv1 = shadow_alloc(obj, id, sizeof(pv1), gfp_flags, shadow_ctor, &pv1);
 	if (!sv1)
 		return -ENOMEM;
 
-	sv2 = shadow_alloc(obj + 1, id, size, gfp_flags, shadow_ctor, &var2);
+	sv2 = shadow_alloc(obj + 1, id, sizeof(pv2), gfp_flags, shadow_ctor, &pv2);
 	if (!sv2)
 		return -ENOMEM;
 
-	sv3 = shadow_alloc(obj, id + 1, size, gfp_flags, shadow_ctor, &var3);
+	sv3 = shadow_alloc(obj, id + 1, sizeof(pv3), gfp_flags, shadow_ctor, &pv3);
 	if (!sv3)
 		return -ENOMEM;
 
@@ -183,20 +191,20 @@ static int test_klp_shadow_vars_init(void)
 	sv = shadow_get(obj, id);
 	if (!sv)
 		return -EINVAL;
-	if (sv == sv1 && *sv1 == &var1)
+	if (sv == sv1 && *sv1 == pv1)
 		pr_info("  got expected PTR%d -> PTR%d result\n",
 			ptr_id(sv1), ptr_id(*sv1));
 
 	sv = shadow_get(obj + 1, id);
 	if (!sv)
 		return -EINVAL;
-	if (sv == sv2 && *sv2 == &var2)
+	if (sv == sv2 && *sv2 == pv2)
 		pr_info("  got expected PTR%d -> PTR%d result\n",
 			ptr_id(sv2), ptr_id(*sv2));
 	sv = shadow_get(obj, id + 1);
 	if (!sv)
 		return -EINVAL;
-	if (sv == sv3 && *sv3 == &var3)
+	if (sv == sv3 && *sv3 == pv3)
 		pr_info("  got expected PTR%d -> PTR%d result\n",
 			ptr_id(sv3), ptr_id(*sv3));
 
@@ -204,14 +212,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, size, gfp_flags, shadow_ctor, &var4);
+	sv4 = shadow_get_or_alloc(obj + 2, id, sizeof(pv4), gfp_flags, shadow_ctor, &pv4);
 	if (!sv4)
 		return -ENOMEM;
 
-	sv = shadow_get_or_alloc(obj + 2, id, size, gfp_flags, shadow_ctor, &var4);
+	sv = shadow_get_or_alloc(obj + 2, id, sizeof(pv4), gfp_flags, shadow_ctor, &pv4);
 	if (!sv)
 		return -EINVAL;
-	if (sv == sv4 && *sv4 == &var4)
+	if (sv == sv4 && *sv4 == pv4)
 		pr_info("  got expected PTR%d -> PTR%d result\n",
 			ptr_id(sv4), ptr_id(*sv4));
 
@@ -240,7 +248,7 @@ static int test_klp_shadow_vars_init(void)
 	sv = shadow_get(obj, id + 1);
 	if (!sv)
 		return -EINVAL;
-	if (sv == sv3 && *sv3 == &var3)
+	if (sv == sv3 && *sv3 == pv3)
 		pr_info("  got expected PTR%d -> PTR%d result\n",
 			ptr_id(sv3), ptr_id(*sv3));
 
diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c
index bab12bdb753f..de0363b288a7 100644
--- a/samples/livepatch/livepatch-shadow-fix1.c
+++ b/samples/livepatch/livepatch-shadow-fix1.c
@@ -53,9 +53,12 @@ struct dummy {
 static int shadow_leak_ctor(void *obj, void *shadow_data, void *ctor_data)
 {
 	int **shadow_leak = shadow_data;
-	int *leak = ctor_data;
+	int **leak = ctor_data;
 
-	*shadow_leak = leak;
+	if (!ctor_data)
+		return -EINVAL;
+
+	*shadow_leak = *leak;
 	return 0;
 }
 
@@ -83,7 +86,7 @@ static struct dummy *livepatch_fix1_dummy_alloc(void)
 	}
 
 	klp_shadow_alloc(d, SV_LEAK, sizeof(leak), GFP_KERNEL,
-			 shadow_leak_ctor, leak);
+			 shadow_leak_ctor, &leak);
 
 	pr_info("%s: dummy @ %p, expires @ %lx\n",
 		__func__, d, d->jiffies_expire);
-- 
2.16.4

[PATCH 4/4] livepatch: Handle allocation failure in the sample of shadow variable API

[Petr Mladek]

klp_shadow_alloc() is not handled in the sample of shadow variable API.
It is not strictly necessary because livepatch_fix1_dummy_free() is
able to handle the potential failure. But it is an example and it should
use the API a clean way.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 samples/livepatch/livepatch-shadow-fix1.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c
index de0363b288a7..918ce17b43fd 100644
--- a/samples/livepatch/livepatch-shadow-fix1.c
+++ b/samples/livepatch/livepatch-shadow-fix1.c
@@ -66,6 +66,7 @@ static struct dummy *livepatch_fix1_dummy_alloc(void)
 {
 	struct dummy *d;
 	int *leak;
+	int **shadow_leak;
 
 	d = kzalloc(sizeof(*d), GFP_KERNEL);
 	if (!d)
@@ -80,18 +81,27 @@ static struct dummy *livepatch_fix1_dummy_alloc(void)
 	 * pointer to handle resource release.
 	 */
 	leak = kzalloc(sizeof(*leak), GFP_KERNEL);
-	if (!leak) {
-		kfree(d);
-		return NULL;
+	if (!leak)
+		goto err_leak;
+
+	shadow_leak = klp_shadow_alloc(d, SV_LEAK, sizeof(leak), GFP_KERNEL,
+				       shadow_leak_ctor, &leak);
+	if (!shadow_leak) {
+		pr_err("%s: failed to allocate shadow variable for the leaking pointer: dummy @ %p, leak @ %p\n",
+		       __func__, d, leak);
+		goto err_shadow;
 	}
 
-	klp_shadow_alloc(d, SV_LEAK, sizeof(leak), GFP_KERNEL,
-			 shadow_leak_ctor, &leak);
-
 	pr_info("%s: dummy @ %p, expires @ %lx\n",
 		__func__, d, d->jiffies_expire);
 
 	return d;
+
+err_shadow:
+	kfree(leak);
+err_leak:
+	kfree(d);
+	return NULL;
 }
 
 static void livepatch_fix1_dummy_leak_dtor(void *obj, void *shadow_data)
-- 
2.16.4

Re: [PATCH 0/4] livepatch/samples/selftest: Clean up show variables handling

[Joe Lawrence]

On 1/16/20 10:31 AM, Petr Mladek wrote:
> Dan Carpenter reported suspicious allocations of shadow variables
> in the sample module, see
> https://lkml.kernel.org/r/20200107132929.ficffmrm5ntpzcqa@kili.mountain
> 
> The code did not cause a real problem. But it was indeed misleading
> and semantically wrong. I got confused several times when cleaning it.
> So I decided to split the change into few steps. I hope that
> it will help reviewers and future readers.
> 
> The changes of the sample module are basically the same as in the RFC.
> In addition, there is a clean up of the module used by the selftest.
> 
> 
> Petr Mladek (4):
>    livepatch/sample: Use the right type for the leaking data pointer
>    livepatch/selftest: Clean up shadow variable names and type
>    livepatch/samples/selftest: Use klp_shadow_alloc() API correctly
>    livepatch: Handle allocation failure in the sample of shadow variable
>      API
> 
>   lib/livepatch/test_klp_shadow_vars.c      | 119 +++++++++++++++++-------------
>   samples/livepatch/livepatch-shadow-fix1.c |  39 ++++++----
>   samples/livepatch/livepatch-shadow-fix2.c |   4 +-
>   samples/livepatch/livepatch-shadow-mod.c  |   4 +-
>   4 files changed, 99 insertions(+), 67 deletions(-)
> 

Hi Petr,

These are good cleanups, thanks for the fixes and tidying up all the 
pointer/value indirections.

Reviewed-by: Joe Lawrence <joe.lawrence@redhat.com>

-- Joe

Re: [PATCH 0/4] livepatch/samples/selftest: Clean up show variables handling

[Miroslav Benes]

On Thu, 16 Jan 2020, Petr Mladek wrote:

> Dan Carpenter reported suspicious allocations of shadow variables
> in the sample module, see
> https://lkml.kernel.org/r/20200107132929.ficffmrm5ntpzcqa@kili.mountain
> 
> The code did not cause a real problem. But it was indeed misleading
> and semantically wrong. I got confused several times when cleaning it.
> So I decided to split the change into few steps. I hope that
> it will help reviewers and future readers.
> 
> The changes of the sample module are basically the same as in the RFC.
> In addition, there is a clean up of the module used by the selftest.
> 
> 
> Petr Mladek (4):
>   livepatch/sample: Use the right type for the leaking data pointer
>   livepatch/selftest: Clean up shadow variable names and type
>   livepatch/samples/selftest: Use klp_shadow_alloc() API correctly
>   livepatch: Handle allocation failure in the sample of shadow variable
>     API
> 
>  lib/livepatch/test_klp_shadow_vars.c      | 119 +++++++++++++++++-------------
>  samples/livepatch/livepatch-shadow-fix1.c |  39 ++++++----
>  samples/livepatch/livepatch-shadow-fix2.c |   4 +-
>  samples/livepatch/livepatch-shadow-mod.c  |   4 +-
>  4 files changed, 99 insertions(+), 67 deletions(-)

Acked-by: Miroslav Benes <mbenes@suse.cz>

M

Re: [PATCH 0/4] livepatch/samples/selftest: Clean up show variables handling

[Kamalesh Babulal]

On 1/16/20 9:01 PM, Petr Mladek wrote:
> Dan Carpenter reported suspicious allocations of shadow variables
> in the sample module, see
> https://lkml.kernel.org/r/20200107132929.ficffmrm5ntpzcqa@kili.mountain
> 
> The code did not cause a real problem. But it was indeed misleading
> and semantically wrong. I got confused several times when cleaning it.
> So I decided to split the change into few steps. I hope that
> it will help reviewers and future readers.
> 
> The changes of the sample module are basically the same as in the RFC.
> In addition, there is a clean up of the module used by the selftest.
> 
> 
> Petr Mladek (4):
>   livepatch/sample: Use the right type for the leaking data pointer
>   livepatch/selftest: Clean up shadow variable names and type
>   livepatch/samples/selftest: Use klp_shadow_alloc() API correctly
>   livepatch: Handle allocation failure in the sample of shadow variable
>     API
> 
>  lib/livepatch/test_klp_shadow_vars.c      | 119 +++++++++++++++++-------------
>  samples/livepatch/livepatch-shadow-fix1.c |  39 ++++++----
>  samples/livepatch/livepatch-shadow-fix2.c |   4 +-
>  samples/livepatch/livepatch-shadow-mod.c  |   4 +-
>  4 files changed, 99 insertions(+), 67 deletions(-)
> 

Reviewed-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>

-- 
Kamalesh

Re: [PATCH 0/4] livepatch/samples/selftest: Clean up show variables handling

[Jiri Kosina]

On Thu, 16 Jan 2020, Petr Mladek wrote:

> Dan Carpenter reported suspicious allocations of shadow variables
> in the sample module, see
> https://lkml.kernel.org/r/20200107132929.ficffmrm5ntpzcqa@kili.mountain
> 
> The code did not cause a real problem. But it was indeed misleading
> and semantically wrong. I got confused several times when cleaning it.
> So I decided to split the change into few steps. I hope that
> it will help reviewers and future readers.
> 
> The changes of the sample module are basically the same as in the RFC.
> In addition, there is a clean up of the module used by the selftest.
> 
> 
> Petr Mladek (4):
>   livepatch/sample: Use the right type for the leaking data pointer
>   livepatch/selftest: Clean up shadow variable names and type
>   livepatch/samples/selftest: Use klp_shadow_alloc() API correctly
>   livepatch: Handle allocation failure in the sample of shadow variable
>     API
> 
>  lib/livepatch/test_klp_shadow_vars.c      | 119 +++++++++++++++++-------------
>  samples/livepatch/livepatch-shadow-fix1.c |  39 ++++++----
>  samples/livepatch/livepatch-shadow-fix2.c |   4 +-
>  samples/livepatch/livepatch-shadow-mod.c  |   4 +-
>  4 files changed, 99 insertions(+), 67 deletions(-)

I've pushed this to for-5.6/selftests. Thanks,

-- 
Jiri Kosina
SUSE Labs