Live-Patching Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/4] livepatch/samples/selftest: Clean up show variables handling
@ 2020-01-16 15:31 Petr Mladek
  2020-01-16 15:31 ` [PATCH 1/4] livepatch/sample: Use the right type for the leaking data pointer Petr Mladek
                   ` (7 more replies)
  0 siblings, 8 replies; 9+ messages in thread
From: Petr Mladek @ 2020-01-16 15:31 UTC (permalink / raw)
  To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
  Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, Dan Carpenter,
	live-patching, linux-kernel, 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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/4] livepatch/sample: Use the right type for the leaking data pointer
  2020-01-16 15:31 [PATCH 0/4] livepatch/samples/selftest: Clean up show variables handling Petr Mladek
@ 2020-01-16 15:31 ` Petr Mladek
  2020-01-16 15:31 ` [PATCH 2/4] livepatch/selftest: Clean up shadow variable names and type Petr Mladek
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Petr Mladek @ 2020-01-16 15:31 UTC (permalink / raw)
  To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
  Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, Dan Carpenter,
	live-patching, linux-kernel, 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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/4] livepatch/selftest: Clean up shadow variable names and type
  2020-01-16 15:31 [PATCH 0/4] livepatch/samples/selftest: Clean up show variables handling Petr Mladek
  2020-01-16 15:31 ` [PATCH 1/4] livepatch/sample: Use the right type for the leaking data pointer Petr Mladek
@ 2020-01-16 15:31 ` Petr Mladek
  2020-01-16 15:31 ` [PATCH 3/4] livepatch/samples/selftest: Use klp_shadow_alloc() API correctly Petr Mladek
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Petr Mladek @ 2020-01-16 15:31 UTC (permalink / raw)
  To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
  Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, Dan Carpenter,
	live-patching, linux-kernel, 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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 3/4] livepatch/samples/selftest: Use klp_shadow_alloc() API correctly
  2020-01-16 15:31 [PATCH 0/4] livepatch/samples/selftest: Clean up show variables handling Petr Mladek
  2020-01-16 15:31 ` [PATCH 1/4] livepatch/sample: Use the right type for the leaking data pointer Petr Mladek
  2020-01-16 15:31 ` [PATCH 2/4] livepatch/selftest: Clean up shadow variable names and type Petr Mladek
@ 2020-01-16 15:31 ` Petr Mladek
  2020-01-16 15:31 ` [PATCH 4/4] livepatch: Handle allocation failure in the sample of shadow variable API Petr Mladek
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Petr Mladek @ 2020-01-16 15:31 UTC (permalink / raw)
  To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
  Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, Dan Carpenter,
	live-patching, linux-kernel, 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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 4/4] livepatch: Handle allocation failure in the sample of shadow variable API
  2020-01-16 15:31 [PATCH 0/4] livepatch/samples/selftest: Clean up show variables handling Petr Mladek
                   ` (2 preceding siblings ...)
  2020-01-16 15:31 ` [PATCH 3/4] livepatch/samples/selftest: Use klp_shadow_alloc() API correctly Petr Mladek
@ 2020-01-16 15:31 ` Petr Mladek
  2020-01-16 19:29 ` [PATCH 0/4] livepatch/samples/selftest: Clean up show variables handling Joe Lawrence
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Petr Mladek @ 2020-01-16 15:31 UTC (permalink / raw)
  To: Jiri Kosina, Josh Poimboeuf, Miroslav Benes
  Cc: Joe Lawrence, Kamalesh Babulal, Nicolai Stange, Dan Carpenter,
	live-patching, linux-kernel, 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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/4] livepatch/samples/selftest: Clean up show variables handling
  2020-01-16 15:31 [PATCH 0/4] livepatch/samples/selftest: Clean up show variables handling Petr Mladek
                   ` (3 preceding siblings ...)
  2020-01-16 15:31 ` [PATCH 4/4] livepatch: Handle allocation failure in the sample of shadow variable API Petr Mladek
@ 2020-01-16 19:29 ` Joe Lawrence
  2020-01-17  8:26 ` Miroslav Benes
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 9+ messages in thread
From: Joe Lawrence @ 2020-01-16 19:29 UTC (permalink / raw)
  To: Petr Mladek, Jiri Kosina, Josh Poimboeuf, Miroslav Benes
  Cc: Kamalesh Babulal, Nicolai Stange, Dan Carpenter, live-patching,
	linux-kernel

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/4] livepatch/samples/selftest: Clean up show variables handling
  2020-01-16 15:31 [PATCH 0/4] livepatch/samples/selftest: Clean up show variables handling Petr Mladek
                   ` (4 preceding siblings ...)
  2020-01-16 19:29 ` [PATCH 0/4] livepatch/samples/selftest: Clean up show variables handling Joe Lawrence
@ 2020-01-17  8:26 ` Miroslav Benes
  2020-01-17  9:41 ` Kamalesh Babulal
  2020-01-17 10:13 ` Jiri Kosina
  7 siblings, 0 replies; 9+ messages in thread
From: Miroslav Benes @ 2020-01-17  8:26 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Jiri Kosina, Josh Poimboeuf, Joe Lawrence, Kamalesh Babulal,
	Nicolai Stange, Dan Carpenter, live-patching, linux-kernel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/4] livepatch/samples/selftest: Clean up show variables handling
  2020-01-16 15:31 [PATCH 0/4] livepatch/samples/selftest: Clean up show variables handling Petr Mladek
                   ` (5 preceding siblings ...)
  2020-01-17  8:26 ` Miroslav Benes
@ 2020-01-17  9:41 ` Kamalesh Babulal
  2020-01-17 10:13 ` Jiri Kosina
  7 siblings, 0 replies; 9+ messages in thread
From: Kamalesh Babulal @ 2020-01-17  9:41 UTC (permalink / raw)
  To: Petr Mladek, Jiri Kosina, Josh Poimboeuf, Miroslav Benes
  Cc: Joe Lawrence, Nicolai Stange, Dan Carpenter, live-patching, linux-kernel

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/4] livepatch/samples/selftest: Clean up show variables handling
  2020-01-16 15:31 [PATCH 0/4] livepatch/samples/selftest: Clean up show variables handling Petr Mladek
                   ` (6 preceding siblings ...)
  2020-01-17  9:41 ` Kamalesh Babulal
@ 2020-01-17 10:13 ` Jiri Kosina
  7 siblings, 0 replies; 9+ messages in thread
From: Jiri Kosina @ 2020-01-17 10:13 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Josh Poimboeuf, Miroslav Benes, Joe Lawrence, Kamalesh Babulal,
	Nicolai Stange, Dan Carpenter, live-patching, linux-kernel

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-16 15:31 [PATCH 0/4] livepatch/samples/selftest: Clean up show variables handling Petr Mladek
2020-01-16 15:31 ` [PATCH 1/4] livepatch/sample: Use the right type for the leaking data pointer Petr Mladek
2020-01-16 15:31 ` [PATCH 2/4] livepatch/selftest: Clean up shadow variable names and type Petr Mladek
2020-01-16 15:31 ` [PATCH 3/4] livepatch/samples/selftest: Use klp_shadow_alloc() API correctly Petr Mladek
2020-01-16 15:31 ` [PATCH 4/4] livepatch: Handle allocation failure in the sample of shadow variable API Petr Mladek
2020-01-16 19:29 ` [PATCH 0/4] livepatch/samples/selftest: Clean up show variables handling Joe Lawrence
2020-01-17  8:26 ` Miroslav Benes
2020-01-17  9:41 ` Kamalesh Babulal
2020-01-17 10:13 ` Jiri Kosina

Live-Patching Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/live-patching/0 live-patching/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 live-patching live-patching/ https://lore.kernel.org/live-patching \
		live-patching@vger.kernel.org
	public-inbox-index live-patching

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.live-patching


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git