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 <[email protected]>) is used
for the statically declared functions.
Signed-off-by: Nicholas Mc Guire <[email protected]>
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
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 <[email protected]> for
catching this) and NULL returned.
Signed-off-by: Nicholas Mc Guire <[email protected]>
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
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 <[email protected]>) is used
> for the statically declared functions.
>
> Signed-off-by: Nicholas Mc Guire <[email protected]>
> 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
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 <[email protected]> for
> catching this) and NULL returned.
>
> Signed-off-by: Nicholas Mc Guire <[email protected]>
> 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 <[email protected]>
-- Joe
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 <[email protected]>) is used
> > for the statically declared functions.
> >
> > Signed-off-by: Nicholas Mc Guire <[email protected]>
> > Link: https://lkml.org/lkml/2018/12/13/827
Needs a:
Suggested-by: Joe Lawrence <[email protected]>
> >
> > 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
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
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 <[email protected]>) is used
> > for the statically declared functions.
> >
> > Signed-off-by: Nicholas Mc Guire <[email protected]>
> > 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
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
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 <[email protected]> for
> catching this) and NULL returned.
>
> Signed-off-by: Nicholas Mc Guire <[email protected]>
> Fixes: 439e7271dc2b ("livepatch: introduce shadow variable API")
Acked-by: Miroslav Benes <[email protected]>
M
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
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 <[email protected]> for
> catching this) and NULL returned.
>
> Signed-off-by: Nicholas Mc Guire <[email protected]>
> 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 <[email protected]>
Best Regards,
Petr
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 <[email protected]> for
> catching this) and NULL returned.
>
> Signed-off-by: Nicholas Mc Guire <[email protected]>
> Fixes: 439e7271dc2b ("livepatch: introduce shadow variable API")
Applied to for-4.21/upstream, thanks.
--
Jiri Kosina
SUSE Labs