[PATCH 1/2] livepatch: fix non-static warnings

[PATCH 1/2] livepatch: fix non-static warnings

[Nicholas Mc Guire]

Sparse reported warnings about non-static symbols. For the variables a
simple static attribute is fine - for those symbols referenced by
livepatch via klp_func the symbol-names must be unmodified in the
relocation table - to resolve this the __noclone attribute (as 
suggested by Joe Lawrence <joe.lawrence@redhat.com>) is used
for the statically declared functions.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
Link: https://lkml.org/lkml/2018/12/13/827
---

sparse reported the following warnings:

  CHECK   samples/livepatch/livepatch-shadow-fix1.c
samples/livepatch/livepatch-shadow-fix1.c:74:14: warning: symbol
 'livepatch_fix1_dummy alloc' was not declared. Should it be static?
samples/livepatch/livepatch-shadow-fix1.c:116:6: warning: symbol
 'livepatch_fix1_dummy free' was not declared. Should it be static?

  CHECK   samples/livepatch/livepatch-shadow-mod.c
samples/livepatch/livepatch-shadow-mod.c:99:1: warning: symbol
 'dummy_list' was not declared. Should it be static?
samples/livepatch/livepatch-shadow-mod.c:100:1: warning: symbol
 'dummy_list_mutex' was not declared. Should it be static?
samples/livepatch/livepatch-shadow-mod.c:107:23: warning: symbol
 'dummy_alloc' was not declared. Should it be static?
samples/livepatch/livepatch-shadow-mod.c:132:15: warning: symbol
 'dummy_free' was not declared. Should it be static?
samples/livepatch/livepatch-shadow-mod.c:140:15: warning: symbol
 'dummy_check' was not declared. Should it be static?

Patch was compile tested with: x86_64_defconfig + FTRACE=y
FUNCTION_TRACER=y, EXPERT=y, LATENCYTOP=y, SAMPLES=y,
SAMPLE_LIVEPATCH=y

Patch was runtested on an Intel i3 with:
   insmod samples/livepatch/livepatch-shadow-mod.ko
   insmod samples/livepatch/livepatch-shadow-fix1.ko
   insmod samples/livepatch/livepatch-shadow-fix2.ko
   echo 0 > /sys/kernel/livepatch/livepatch_shadow_fix2/enabled
   echo 0 > /sys/kernel/livepatch/livepatch_shadow_fix1/enabled
   rmmod livepatch-shadow-fix2
   rmmod livepatch-shadow-fix1
   rmmod livepatch-shadow-mod
and dmesg output checked.

Patch is against 4.20-rc6 (localversion-next is next-20181214)

 samples/livepatch/livepatch-shadow-fix1.c |  4 ++--
 samples/livepatch/livepatch-shadow-mod.c  | 16 +++++++++++-----
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c
index 49b1355..eaab10f 100644
--- a/samples/livepatch/livepatch-shadow-fix1.c
+++ b/samples/livepatch/livepatch-shadow-fix1.c
@@ -71,7 +71,7 @@ static int shadow_leak_ctor(void *obj, void *shadow_data, void *ctor_data)
 	return 0;
 }
 
-struct dummy *livepatch_fix1_dummy_alloc(void)
+static __noclone struct dummy *livepatch_fix1_dummy_alloc(void)
 {
 	struct dummy *d;
 	void *leak;
@@ -108,7 +108,7 @@ static void livepatch_fix1_dummy_leak_dtor(void *obj, void *shadow_data)
 			 __func__, d, *shadow_leak);
 }
 
-void livepatch_fix1_dummy_free(struct dummy *d)
+static __noclone void livepatch_fix1_dummy_free(struct dummy *d)
 {
 	void **shadow_leak;
 
diff --git a/samples/livepatch/livepatch-shadow-mod.c b/samples/livepatch/livepatch-shadow-mod.c
index 4c54b25..0a72bc2 100644
--- a/samples/livepatch/livepatch-shadow-mod.c
+++ b/samples/livepatch/livepatch-shadow-mod.c
@@ -30,6 +30,11 @@
  * memory leak, please load these modules at your own risk -- some
  * amount of memory may leaked before the bug is patched.
  *
+ * NOTE - the __noclone attribute to those functions that are to be
+ * shared with other modules while being declared static. As livepatch
+ * needs the unmodified symbol names and the usual "static" would
+ * invoke gccs cloning mechanism that renames the functions this
+ * needs to be suppressed with the additional __noclone attribute.
  *
  * Usage
  * -----
@@ -96,15 +101,15 @@ MODULE_DESCRIPTION("Buggy module for shadow variable demo");
  * Keep a list of all the dummies so we can clean up any residual ones
  * on module exit
  */
-LIST_HEAD(dummy_list);
-DEFINE_MUTEX(dummy_list_mutex);
+static LIST_HEAD(dummy_list);
+static DEFINE_MUTEX(dummy_list_mutex);
 
 struct dummy {
 	struct list_head list;
 	unsigned long jiffies_expire;
 };
 
-noinline struct dummy *dummy_alloc(void)
+static __noclone noinline struct dummy *dummy_alloc(void)
 {
 	struct dummy *d;
 	void *leak;
@@ -125,7 +130,7 @@ noinline struct dummy *dummy_alloc(void)
 	return d;
 }
 
-noinline void dummy_free(struct dummy *d)
+static __noclone noinline void dummy_free(struct dummy *d)
 {
 	pr_info("%s: dummy @ %p, expired = %lx\n",
 		__func__, d, d->jiffies_expire);
@@ -133,7 +138,8 @@ noinline void dummy_free(struct dummy *d)
 	kfree(d);
 }
 
-noinline bool dummy_check(struct dummy *d, unsigned long jiffies)
+static __noclone noinline bool dummy_check(struct dummy *d,
+					   unsigned long jiffies)
 {
 	return time_after(jiffies, d->jiffies_expire);
 }
-- 
2.1.4

[PATCH 2/2] livepatch: check kzalloc return values

[Nicholas Mc Guire]

kzalloc() return should always be checked - notably in example code
where this may be seen as reference. On failure of allocation in
livepatch_fix1_dummy_alloc() respectively dummy_alloc() previous
allocation is freed (thanks to Petr Mladek <pmladek@suse.com> for
catching this) and NULL returned.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
Fixes: 439e7271dc2b ("livepatch: introduce shadow variable API")
---

Problem located with an experimental coccinelle script

Patch was compile tested with: x86_64_defconfig + FTRACE=y
FUNCTION_TRACER=y, EXPERT=y, LATENCYTOP=y, SAMPLES=y,
SAMPLE_LIVEPATCH=y

Patch is against 4.20-rc6 (localversion-next is next-20181214)
on top of 0001-livepatch-fix-non-static-warnings.patch 

 samples/livepatch/livepatch-shadow-fix1.c | 5 +++++
 samples/livepatch/livepatch-shadow-mod.c  | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c
index eaab10f..1974eb5 100644
--- a/samples/livepatch/livepatch-shadow-fix1.c
+++ b/samples/livepatch/livepatch-shadow-fix1.c
@@ -89,6 +89,11 @@ static __noclone struct dummy *livepatch_fix1_dummy_alloc(void)
 	 * pointer to handle resource release.
 	 */
 	leak = kzalloc(sizeof(int), GFP_KERNEL);
+	if (!leak) {
+		kfree(d);
+		return NULL;
+	}
+
 	klp_shadow_alloc(d, SV_LEAK, sizeof(leak), GFP_KERNEL,
 			 shadow_leak_ctor, leak);
 
diff --git a/samples/livepatch/livepatch-shadow-mod.c b/samples/livepatch/livepatch-shadow-mod.c
index dc69da0..b4ece36 100644
--- a/samples/livepatch/livepatch-shadow-mod.c
+++ b/samples/livepatch/livepatch-shadow-mod.c
@@ -123,6 +123,10 @@ static __noclone noinline struct dummy *dummy_alloc(void)
 
 	/* Oops, forgot to save leak! */
 	leak = kzalloc(sizeof(int), GFP_KERNEL);
+	if (!leak) {
+		kfree(d);
+		return NULL;
+	}
 
 	pr_info("%s: dummy @ %p, expires @ %lx\n",
 		__func__, d, d->jiffies_expire);
-- 
2.1.4

Re: [PATCH 2/2] livepatch: check kzalloc return values

[Joe Lawrence]

On 12/14/2018 11:56 AM, Nicholas Mc Guire wrote:
> kzalloc() return should always be checked - notably in example code
> where this may be seen as reference. On failure of allocation in
> livepatch_fix1_dummy_alloc() respectively dummy_alloc() previous
> allocation is freed (thanks to Petr Mladek <pmladek@suse.com> for
> catching this) and NULL returned.
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> Fixes: 439e7271dc2b ("livepatch: introduce shadow variable API")
> ---
> 
> Problem located with an experimental coccinelle script
> 
> Patch was compile tested with: x86_64_defconfig + FTRACE=y
> FUNCTION_TRACER=y, EXPERT=y, LATENCYTOP=y, SAMPLES=y,
> SAMPLE_LIVEPATCH=y
> 
> Patch is against 4.20-rc6 (localversion-next is next-20181214)
> on top of 0001-livepatch-fix-non-static-warnings.patch 
> 
>  samples/livepatch/livepatch-shadow-fix1.c | 5 +++++
>  samples/livepatch/livepatch-shadow-mod.c  | 4 ++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c
> index eaab10f..1974eb5 100644
> --- a/samples/livepatch/livepatch-shadow-fix1.c
> +++ b/samples/livepatch/livepatch-shadow-fix1.c
> @@ -89,6 +89,11 @@ static __noclone struct dummy *livepatch_fix1_dummy_alloc(void)
>  	 * pointer to handle resource release.
>  	 */
>  	leak = kzalloc(sizeof(int), GFP_KERNEL);
> +	if (!leak) {
> +		kfree(d);
> +		return NULL;
> +	}
> +
>  	klp_shadow_alloc(d, SV_LEAK, sizeof(leak), GFP_KERNEL,
>  			 shadow_leak_ctor, leak);
>  
> diff --git a/samples/livepatch/livepatch-shadow-mod.c b/samples/livepatch/livepatch-shadow-mod.c
> index dc69da0..b4ece36 100644
> --- a/samples/livepatch/livepatch-shadow-mod.c
> +++ b/samples/livepatch/livepatch-shadow-mod.c
> @@ -123,6 +123,10 @@ static __noclone noinline struct dummy *dummy_alloc(void)
>  
>  	/* Oops, forgot to save leak! */
>  	leak = kzalloc(sizeof(int), GFP_KERNEL);
> +	if (!leak) {
> +		kfree(d);
> +		return NULL;
> +	}
>  
>  	pr_info("%s: dummy @ %p, expires @ %lx\n",
>  		__func__, d, d->jiffies_expire);
> 

Patch v2 2/2 looks good, thanks for combining.

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

-- Joe

Re: [PATCH 2/2] livepatch: check kzalloc return values

[Petr Mladek]

On Fri 2018-12-14 17:56:10, Nicholas Mc Guire wrote:
> kzalloc() return should always be checked - notably in example code
> where this may be seen as reference. On failure of allocation in
> livepatch_fix1_dummy_alloc() respectively dummy_alloc() previous
> allocation is freed (thanks to Petr Mladek <pmladek@suse.com> for
> catching this) and NULL returned.
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> Fixes: 439e7271dc2b ("livepatch: introduce shadow variable API")

It is a bit confusing that it was not re-send together with v2
of the other patch. It should not get lost!

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH 2/2] livepatch: check kzalloc return values

[Miroslav Benes]

On Fri, 14 Dec 2018, Nicholas Mc Guire wrote:

> kzalloc() return should always be checked - notably in example code
> where this may be seen as reference. On failure of allocation in
> livepatch_fix1_dummy_alloc() respectively dummy_alloc() previous
> allocation is freed (thanks to Petr Mladek <pmladek@suse.com> for
> catching this) and NULL returned.
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> Fixes: 439e7271dc2b ("livepatch: introduce shadow variable API")

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

M

Re: [PATCH 2/2] livepatch: check kzalloc return values

[Jiri Kosina]

On Fri, 14 Dec 2018, Nicholas Mc Guire wrote:

> kzalloc() return should always be checked - notably in example code
> where this may be seen as reference. On failure of allocation in
> livepatch_fix1_dummy_alloc() respectively dummy_alloc() previous
> allocation is freed (thanks to Petr Mladek <pmladek@suse.com> for
> catching this) and NULL returned.
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> Fixes: 439e7271dc2b ("livepatch: introduce shadow variable API")

Applied to for-4.21/upstream, thanks.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/2] livepatch: fix non-static warnings

[Joe Lawrence]

On 12/14/2018 11:56 AM, Nicholas Mc Guire wrote:
> Sparse reported warnings about non-static symbols. For the variables a
> simple static attribute is fine - for those symbols referenced by
> livepatch via klp_func the symbol-names must be unmodified in the
> relocation table - to resolve this the __noclone attribute (as 
  ^^^^^^^^^^
nit: symbol table

> suggested by Joe Lawrence <joe.lawrence@redhat.com>) is used
> for the statically declared functions.
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> Link: https://lkml.org/lkml/2018/12/13/827
> ---
> 
> sparse reported the following warnings:
> 
>   CHECK   samples/livepatch/livepatch-shadow-fix1.c
> samples/livepatch/livepatch-shadow-fix1.c:74:14: warning: symbol
>  'livepatch_fix1_dummy alloc' was not declared. Should it be static?
> samples/livepatch/livepatch-shadow-fix1.c:116:6: warning: symbol
>  'livepatch_fix1_dummy free' was not declared. Should it be static?
> 
>   CHECK   samples/livepatch/livepatch-shadow-mod.c
> samples/livepatch/livepatch-shadow-mod.c:99:1: warning: symbol
>  'dummy_list' was not declared. Should it be static?
> samples/livepatch/livepatch-shadow-mod.c:100:1: warning: symbol
>  'dummy_list_mutex' was not declared. Should it be static?
> samples/livepatch/livepatch-shadow-mod.c:107:23: warning: symbol
>  'dummy_alloc' was not declared. Should it be static?
> samples/livepatch/livepatch-shadow-mod.c:132:15: warning: symbol
>  'dummy_free' was not declared. Should it be static?
> samples/livepatch/livepatch-shadow-mod.c:140:15: warning: symbol
>  'dummy_check' was not declared. Should it be static?
> 
> Patch was compile tested with: x86_64_defconfig + FTRACE=y
> FUNCTION_TRACER=y, EXPERT=y, LATENCYTOP=y, SAMPLES=y,
> SAMPLE_LIVEPATCH=y
> 
> Patch was runtested on an Intel i3 with:
>    insmod samples/livepatch/livepatch-shadow-mod.ko
>    insmod samples/livepatch/livepatch-shadow-fix1.ko
>    insmod samples/livepatch/livepatch-shadow-fix2.ko
>    echo 0 > /sys/kernel/livepatch/livepatch_shadow_fix2/enabled
>    echo 0 > /sys/kernel/livepatch/livepatch_shadow_fix1/enabled
>    rmmod livepatch-shadow-fix2
>    rmmod livepatch-shadow-fix1
>    rmmod livepatch-shadow-mod
> and dmesg output checked.
> 
> Patch is against 4.20-rc6 (localversion-next is next-20181214)

Great testing notes, thanks for including these!

>  samples/livepatch/livepatch-shadow-fix1.c |  4 ++--
>  samples/livepatch/livepatch-shadow-mod.c  | 16 +++++++++++-----
>  2 files changed, 13 insertions(+), 7 deletions(-)

Almost.  We should only need to annotate with __noclone for those
function definitions which the samples will be patching.  As the commit
message says, these correlate to klp_func.old_name functions found in
klp_object.name objects (.ko modules or NULL for vmlinux).

For the functions defined in samples/livepatch/*.c those would be:

  livepatch-callbacks-busymod.c :: busymod_work_func()
  livepatch-shadow-mod.c :: dummy_alloc()
  livepatch-shadow-mod.c :: dummy_free()
  livepatch-shadow-mod.c :: dummy_check()

So even though livepatch-shadow-fix2 further refines
livepatch-shadow-fix1, the livepatch core is going to redirect the
original dummy_*() calls defined by livepatch-shadow-mod.c in both fix1
and fix2 cases.  Ie, the fixes modules aren't patched, only the original.

> 
> diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c
> index 49b1355..eaab10f 100644
> --- a/samples/livepatch/livepatch-shadow-fix1.c
> +++ b/samples/livepatch/livepatch-shadow-fix1.c
> @@ -71,7 +71,7 @@ static int shadow_leak_ctor(void *obj, void *shadow_data, void *ctor_data)
>  	return 0;
>  }
>  
> -struct dummy *livepatch_fix1_dummy_alloc(void)
> +static __noclone struct dummy *livepatch_fix1_dummy_alloc(void)
>  {
>  	struct dummy *d;
>  	void *leak;
> @@ -108,7 +108,7 @@ static void livepatch_fix1_dummy_leak_dtor(void *obj, void *shadow_data)
>  			 __func__, d, *shadow_leak);
>  }
>  
> -void livepatch_fix1_dummy_free(struct dummy *d)
> +static __noclone void livepatch_fix1_dummy_free(struct dummy *d)
>  {
>  	void **shadow_leak;
>  
> diff --git a/samples/livepatch/livepatch-shadow-mod.c b/samples/livepatch/livepatch-shadow-mod.c
> index 4c54b25..0a72bc2 100644
> --- a/samples/livepatch/livepatch-shadow-mod.c
> +++ b/samples/livepatch/livepatch-shadow-mod.c
> @@ -30,6 +30,11 @@
>   * memory leak, please load these modules at your own risk -- some
>   * amount of memory may leaked before the bug is patched.
>   *
> + * NOTE - the __noclone attribute to those functions that are to be
> + * shared with other modules while being declared static. As livepatch
> + * needs the unmodified symbol names and the usual "static" would
> + * invoke gccs cloning mechanism that renames the functions this
> + * needs to be suppressed with the additional __noclone attribute.

I like the idea of providing the sample code reader this information,
but since the compiler might also optimize livepatch-callbacks-busymod.c
:: busymod_work_func(), it too should be annotated __noclone.  Would
that file deserve a similar comment?

I don't have a strong opinion, but would throw my vote at leaving this
in the commit message only.


BTW, Petr/Miroslav/Josh, should we be annotating the selftests in
similar fashion?

> [ ... snip ... ]

Thanks for working on this,

-- Joe

Re: [PATCH 1/2] livepatch: fix non-static warnings

[Josh Poimboeuf]

On Fri, Dec 14, 2018 at 04:34:23PM -0500, Joe Lawrence wrote:
> On 12/14/2018 11:56 AM, Nicholas Mc Guire wrote:
> > Sparse reported warnings about non-static symbols. For the variables a
> > simple static attribute is fine - for those symbols referenced by
> > livepatch via klp_func the symbol-names must be unmodified in the
> > relocation table - to resolve this the __noclone attribute (as 
>   ^^^^^^^^^^
> nit: symbol table
> 
> > suggested by Joe Lawrence <joe.lawrence@redhat.com>) is used
> > for the statically declared functions.
> > 
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > Link: https://lkml.org/lkml/2018/12/13/827

Needs a:

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


> > 
> > diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c
> > index 49b1355..eaab10f 100644
> > --- a/samples/livepatch/livepatch-shadow-fix1.c
> > +++ b/samples/livepatch/livepatch-shadow-fix1.c
> > @@ -71,7 +71,7 @@ static int shadow_leak_ctor(void *obj, void *shadow_data, void *ctor_data)
> >  	return 0;
> >  }
> >  
> > -struct dummy *livepatch_fix1_dummy_alloc(void)
> > +static __noclone struct dummy *livepatch_fix1_dummy_alloc(void)
> >  {
> >  	struct dummy *d;
> >  	void *leak;
> > @@ -108,7 +108,7 @@ static void livepatch_fix1_dummy_leak_dtor(void *obj, void *shadow_data)
> >  			 __func__, d, *shadow_leak);
> >  }
> >  
> > -void livepatch_fix1_dummy_free(struct dummy *d)
> > +static __noclone void livepatch_fix1_dummy_free(struct dummy *d)
> >  {
> >  	void **shadow_leak;
> >  
> > diff --git a/samples/livepatch/livepatch-shadow-mod.c b/samples/livepatch/livepatch-shadow-mod.c
> > index 4c54b25..0a72bc2 100644
> > --- a/samples/livepatch/livepatch-shadow-mod.c
> > +++ b/samples/livepatch/livepatch-shadow-mod.c
> > @@ -30,6 +30,11 @@
> >   * memory leak, please load these modules at your own risk -- some
> >   * amount of memory may leaked before the bug is patched.
> >   *
> > + * NOTE - the __noclone attribute to those functions that are to be
> > + * shared with other modules while being declared static. As livepatch
> > + * needs the unmodified symbol names and the usual "static" would
> > + * invoke gccs cloning mechanism that renames the functions this
> > + * needs to be suppressed with the additional __noclone attribute.
> 
> I like the idea of providing the sample code reader this information,
> but since the compiler might also optimize livepatch-callbacks-busymod.c
> :: busymod_work_func(), it too should be annotated __noclone.  Would
> that file deserve a similar comment?
> 
> I don't have a strong opinion, but would throw my vote at leaving this
> in the commit message only.

Agreed, IMO the comment isn't needed.

> BTW, Petr/Miroslav/Josh, should we be annotating the selftests in
> similar fashion?

Probably so.

-- 
Josh

Re: [PATCH 1/2] livepatch: fix non-static warnings

[Joe Lawrence]

On 12/14/2018 04:51 PM, Josh Poimboeuf wrote:
> On Fri, Dec 14, 2018 at 04:34:23PM -0500, Joe Lawrence wrote:
>> BTW, Petr/Miroslav/Josh, should we be annotating the selftests in
>> similar fashion?
> 
> Probably so.
> 

Just to clarify for Nicholas, the self-tests we're referring to are part
of another patch series currently under review -- no need to worry about
them for the changes here.

Thanks,

-- Joe

Re: [PATCH 1/2] livepatch: fix non-static warnings

[Nicholas Mc Guire]

On Fri, Dec 14, 2018 at 04:34:23PM -0500, Joe Lawrence wrote:
> On 12/14/2018 11:56 AM, Nicholas Mc Guire wrote:
> > Sparse reported warnings about non-static symbols. For the variables a
> > simple static attribute is fine - for those symbols referenced by
> > livepatch via klp_func the symbol-names must be unmodified in the
> > relocation table - to resolve this the __noclone attribute (as 
>   ^^^^^^^^^^
> nit: symbol table

that should have been  relocation section  as described in
Documentation/livepatch/module-elf-format.txt - atleast that is how
I currently undderstand the livepatch mechanism and its seperate
relocation section.

> 
> > suggested by Joe Lawrence <joe.lawrence@redhat.com>) is used
> > for the statically declared functions.
> > 
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > Link: https://lkml.org/lkml/2018/12/13/827
> > ---
> > 
> > sparse reported the following warnings:
> > 
> >   CHECK   samples/livepatch/livepatch-shadow-fix1.c
> > samples/livepatch/livepatch-shadow-fix1.c:74:14: warning: symbol
> >  'livepatch_fix1_dummy alloc' was not declared. Should it be static?
> > samples/livepatch/livepatch-shadow-fix1.c:116:6: warning: symbol
> >  'livepatch_fix1_dummy free' was not declared. Should it be static?
> > 
> >   CHECK   samples/livepatch/livepatch-shadow-mod.c
> > samples/livepatch/livepatch-shadow-mod.c:99:1: warning: symbol
> >  'dummy_list' was not declared. Should it be static?
> > samples/livepatch/livepatch-shadow-mod.c:100:1: warning: symbol
> >  'dummy_list_mutex' was not declared. Should it be static?
> > samples/livepatch/livepatch-shadow-mod.c:107:23: warning: symbol
> >  'dummy_alloc' was not declared. Should it be static?
> > samples/livepatch/livepatch-shadow-mod.c:132:15: warning: symbol
> >  'dummy_free' was not declared. Should it be static?
> > samples/livepatch/livepatch-shadow-mod.c:140:15: warning: symbol
> >  'dummy_check' was not declared. Should it be static?
> > 
> > Patch was compile tested with: x86_64_defconfig + FTRACE=y
> > FUNCTION_TRACER=y, EXPERT=y, LATENCYTOP=y, SAMPLES=y,
> > SAMPLE_LIVEPATCH=y
> > 
> > Patch was runtested on an Intel i3 with:
> >    insmod samples/livepatch/livepatch-shadow-mod.ko
> >    insmod samples/livepatch/livepatch-shadow-fix1.ko
> >    insmod samples/livepatch/livepatch-shadow-fix2.ko
> >    echo 0 > /sys/kernel/livepatch/livepatch_shadow_fix2/enabled
> >    echo 0 > /sys/kernel/livepatch/livepatch_shadow_fix1/enabled
> >    rmmod livepatch-shadow-fix2
> >    rmmod livepatch-shadow-fix1
> >    rmmod livepatch-shadow-mod
> > and dmesg output checked.
> > 
> > Patch is against 4.20-rc6 (localversion-next is next-20181214)
> 
> Great testing notes, thanks for including these!
> 
> >  samples/livepatch/livepatch-shadow-fix1.c |  4 ++--
> >  samples/livepatch/livepatch-shadow-mod.c  | 16 +++++++++++-----
> >  2 files changed, 13 insertions(+), 7 deletions(-)
> 
> Almost.  We should only need to annotate with __noclone for those
> function definitions which the samples will be patching.  As the commit
> message says, these correlate to klp_func.old_name functions found in
> klp_object.name objects (.ko modules or NULL for vmlinux).
> 
> For the functions defined in samples/livepatch/*.c those would be:
> 
>   livepatch-callbacks-busymod.c :: busymod_work_func()
>   livepatch-shadow-mod.c :: dummy_alloc()
>   livepatch-shadow-mod.c :: dummy_free()
>   livepatch-shadow-mod.c :: dummy_check()
> 
> So even though livepatch-shadow-fix2 further refines
> livepatch-shadow-fix1, the livepatch core is going to redirect the
> original dummy_*() calls defined by livepatch-shadow-mod.c in both fix1
> and fix2 cases.  Ie, the fixes modules aren't patched, only the original.
>

thanks for your patience - so I did not yet understand how this really
works together - will give it a rerun and repost a hopefully proper
solution. 

thx!
hofrat
 
> > 
> > diff --git a/samples/livepatch/livepatch-shadow-fix1.c b/samples/livepatch/livepatch-shadow-fix1.c
> > index 49b1355..eaab10f 100644
> > --- a/samples/livepatch/livepatch-shadow-fix1.c
> > +++ b/samples/livepatch/livepatch-shadow-fix1.c
> > @@ -71,7 +71,7 @@ static int shadow_leak_ctor(void *obj, void *shadow_data, void *ctor_data)
> >  	return 0;
> >  }
> >  
> > -struct dummy *livepatch_fix1_dummy_alloc(void)
> > +static __noclone struct dummy *livepatch_fix1_dummy_alloc(void)
> >  {
> >  	struct dummy *d;
> >  	void *leak;
> > @@ -108,7 +108,7 @@ static void livepatch_fix1_dummy_leak_dtor(void *obj, void *shadow_data)
> >  			 __func__, d, *shadow_leak);
> >  }
> >  
> > -void livepatch_fix1_dummy_free(struct dummy *d)
> > +static __noclone void livepatch_fix1_dummy_free(struct dummy *d)
> >  {
> >  	void **shadow_leak;
> >  
> > diff --git a/samples/livepatch/livepatch-shadow-mod.c b/samples/livepatch/livepatch-shadow-mod.c
> > index 4c54b25..0a72bc2 100644
> > --- a/samples/livepatch/livepatch-shadow-mod.c
> > +++ b/samples/livepatch/livepatch-shadow-mod.c
> > @@ -30,6 +30,11 @@
> >   * memory leak, please load these modules at your own risk -- some
> >   * amount of memory may leaked before the bug is patched.
> >   *
> > + * NOTE - the __noclone attribute to those functions that are to be
> > + * shared with other modules while being declared static. As livepatch
> > + * needs the unmodified symbol names and the usual "static" would
> > + * invoke gccs cloning mechanism that renames the functions this
> > + * needs to be suppressed with the additional __noclone attribute.
> 
> I like the idea of providing the sample code reader this information,
> but since the compiler might also optimize livepatch-callbacks-busymod.c
> :: busymod_work_func(), it too should be annotated __noclone.  Would
> that file deserve a similar comment?
> 
> I don't have a strong opinion, but would throw my vote at leaving this
> in the commit message only.
> 
> 
> BTW, Petr/Miroslav/Josh, should we be annotating the selftests in
> similar fashion?
> 
> > [ ... snip ... ]
> 
> Thanks for working on this,
> 
> -- Joe

Re: [PATCH 1/2] livepatch: fix non-static warnings

[Joe Lawrence]

On Sat, Dec 15, 2018 at 09:50:52AM +0100, Nicholas Mc Guire wrote:
> On Fri, Dec 14, 2018 at 04:34:23PM -0500, Joe Lawrence wrote:
> > On 12/14/2018 11:56 AM, Nicholas Mc Guire wrote:
> > > Sparse reported warnings about non-static symbols. For the variables a
> > > simple static attribute is fine - for those symbols referenced by
> > > livepatch via klp_func the symbol-names must be unmodified in the
> > > relocation table - to resolve this the __noclone attribute (as 
> >   ^^^^^^^^^^
> > nit: symbol table
> 
> that should have been  relocation section  as described in
> Documentation/livepatch/module-elf-format.txt - atleast that is how
> I currently undderstand the livepatch mechanism and its seperate
> relocation section.
> 
Hi Nicholas,

Jessica can explain module-elf-format.txt in more detail, but the
highlight is that it outlines the format for _livepatch_ modules, in
this case livepatch-shadow-fix{1,2}.ko.  The special relocations
detailed in that file are needed when livepatch modules reference
symbols defined elsewhere (vmlinux, other modules) that may not
ordinarily be visible (non-exported, local, etc.) to the module loader.

When livepatch modules register themselves on load, the livepatching
core needs to find the address of the _target_ to-be-patched function
using a kallsyms lookup.  If that can't be found, the kernel will emit
an error, "livepatch: symbol 'dummy_free' not found in symbol table".
The call path is:

  klp_register_patch
    klp_init_object_loaded
      klp_find_object_symbol

if you want to trace the execution path.

> 
> thanks for your patience - so I did not yet understand how this really
> works together - will give it a rerun and repost a hopefully proper
> solution. 
>
> thx!
> hofrat

Thanks for sticking with it and learning some livepatching internals
along the way.

-- Joe

Re: [PATCH 1/2] livepatch: fix non-static warnings

[Petr Mladek]

On Fri 2018-12-14 16:34:23, Joe Lawrence wrote:
> Almost.  We should only need to annotate with __noclone for those
> function definitions which the samples will be patching.  As the commit
> message says, these correlate to klp_func.old_name functions found in
> klp_object.name objects (.ko modules or NULL for vmlinux).
> 
> For the functions defined in samples/livepatch/*.c those would be:
> 
>   livepatch-callbacks-busymod.c :: busymod_work_func()

__noclone is not added to this function in v2. Well, I wonder
if it can be optimized when it is passed as a pointer.

>   livepatch-shadow-mod.c :: dummy_alloc()
>   livepatch-shadow-mod.c :: dummy_free()
>   livepatch-shadow-mod.c :: dummy_check()
> 
> So even though livepatch-shadow-fix2 further refines
> livepatch-shadow-fix1, the livepatch core is going to redirect the
> original dummy_*() calls defined by livepatch-shadow-mod.c in both fix1
> and fix2 cases.  Ie, the fixes modules aren't patched, only the original.

Best Regards,
Petr