[PATCH] livepatch: add module locking around kallsyms calls

[PATCH] livepatch: add module locking around kallsyms calls

[Miroslav Benes]

The list of loaded modules is walked through in
module_kallsyms_on_each_symbol (called by kallsyms_on_each_symbol). The
module_mutex lock should be acquired to prevent potential corruptions
in the list.

This was uncovered with new lockdep asserts in module code introduced by
the commit 0be964be0d45 ("module: Sanitize RCU usage and locking") in
recent next- trees.

Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
 kernel/livepatch/core.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index e6c8d54..c40ebcc 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -179,7 +179,9 @@ static int klp_find_object_symbol(const char *objname, const char *name,
 		.count = 0
 	};
 
+	mutex_lock(&module_mutex);
 	kallsyms_on_each_symbol(klp_find_callback, &args);
+	mutex_unlock(&module_mutex);
 
 	if (args.count == 0)
 		pr_err("symbol '%s' not found in symbol table\n", name);
@@ -219,13 +221,19 @@ static int klp_verify_vmlinux_symbol(const char *name, unsigned long addr)
 		.name = name,
 		.addr = addr,
 	};
+	int ret;
 
-	if (kallsyms_on_each_symbol(klp_verify_callback, &args))
-		return 0;
+	mutex_lock(&module_mutex);
+	ret = kallsyms_on_each_symbol(klp_verify_callback, &args);
+	mutex_unlock(&module_mutex);
 
-	pr_err("symbol '%s' not found at specified address 0x%016lx, kernel mismatch?\n",
-		name, addr);
-	return -EINVAL;
+	if (!ret) {
+		pr_err("symbol '%s' not found at specified address 0x%016lx, kernel mismatch?\n",
+			name, addr);
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
 static int klp_find_verify_func_addr(struct klp_object *obj,
-- 
2.1.4

Re: [PATCH] livepatch: add module locking around kallsyms calls

[Minfei Huang]

On Mon, Jun 1, 2015 at 11:48 PM, Miroslav Benes <mbenes@suse.cz> wrote:
> The list of loaded modules is walked through in
> module_kallsyms_on_each_symbol (called by kallsyms_on_each_symbol). The
> module_mutex lock should be acquired to prevent potential corruptions
> in the list.
>
> This was uncovered with new lockdep asserts in module code introduced by
> the commit 0be964be0d45 ("module: Sanitize RCU usage and locking") in
> recent next- trees.
>
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> ---
>  kernel/livepatch/core.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index e6c8d54..c40ebcc 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -179,7 +179,9 @@ static int klp_find_object_symbol(const char *objname, const char *name,
>                 .count = 0
>         };
>
> +       mutex_lock(&module_mutex);
>         kallsyms_on_each_symbol(klp_find_callback, &args);
> +       mutex_unlock(&module_mutex);
>
>         if (args.count == 0)
>                 pr_err("symbol '%s' not found in symbol table\n", name);
> @@ -219,13 +221,19 @@ static int klp_verify_vmlinux_symbol(const char *name, unsigned long addr)
>                 .name = name,
>                 .addr = addr,
>         };
> +       int ret;
>
> -       if (kallsyms_on_each_symbol(klp_verify_callback, &args))
> -               return 0;
> +       mutex_lock(&module_mutex);
> +       ret = kallsyms_on_each_symbol(klp_verify_callback, &args);
> +       mutex_unlock(&module_mutex);
>

Hi.
In livepatch code path, returning value 0 may represent the right, but
sometime represent wrong, like the above function.

Is it possible that we can wrap such function and return the unified
value? Thus we can not confuse the returning value any more.

Otherwise annotation is appreciate.

Thanks
Minfei

> -       pr_err("symbol '%s' not found at specified address 0x%016lx, kernel mismatch?\n",
> -               name, addr);
> -       return -EINVAL;
> +       if (!ret) {
> +               pr_err("symbol '%s' not found at specified address 0x%016lx, kernel mismatch?\n",
> +                       name, addr);
> +               return -EINVAL;
> +       }
> +
> +       return 0;
>  }
>
>  static int klp_find_verify_func_addr(struct klp_object *obj,
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe live-patching" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] livepatch: add module locking around kallsyms calls

[Miroslav Benes]

On Tue, 2 Jun 2015, Minfei Huang wrote:

> On Mon, Jun 1, 2015 at 11:48 PM, Miroslav Benes <mbenes@suse.cz> wrote:
> > The list of loaded modules is walked through in
> > module_kallsyms_on_each_symbol (called by kallsyms_on_each_symbol). The
> > module_mutex lock should be acquired to prevent potential corruptions
> > in the list.
> >
> > This was uncovered with new lockdep asserts in module code introduced by
> > the commit 0be964be0d45 ("module: Sanitize RCU usage and locking") in
> > recent next- trees.
> >
> > Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> > ---
> >  kernel/livepatch/core.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> > index e6c8d54..c40ebcc 100644
> > --- a/kernel/livepatch/core.c
> > +++ b/kernel/livepatch/core.c
> > @@ -179,7 +179,9 @@ static int klp_find_object_symbol(const char *objname, const char *name,
> >                 .count = 0
> >         };
> >
> > +       mutex_lock(&module_mutex);
> >         kallsyms_on_each_symbol(klp_find_callback, &args);
> > +       mutex_unlock(&module_mutex);
> >
> >         if (args.count == 0)
> >                 pr_err("symbol '%s' not found in symbol table\n", name);
> > @@ -219,13 +221,19 @@ static int klp_verify_vmlinux_symbol(const char *name, unsigned long addr)
> >                 .name = name,
> >                 .addr = addr,
> >         };
> > +       int ret;
> >
> > -       if (kallsyms_on_each_symbol(klp_verify_callback, &args))
> > -               return 0;
> > +       mutex_lock(&module_mutex);
> > +       ret = kallsyms_on_each_symbol(klp_verify_callback, &args);
> > +       mutex_unlock(&module_mutex);
> >
> 
> Hi.
> In livepatch code path, returning value 0 may represent the right, but
> sometime represent wrong, like the above function.
> 
> Is it possible that we can wrap such function and return the unified
> value? Thus we can not confuse the returning value any more.

Hi,

I must admit I do not understand. Both klp_find_object_symbol and 
klp_verify_vmlinux_symbol return 0 on success or -EINVAL. It is true that 
kallsyms_on_each_symbol and module_kallsyms_on_each symbol are different. 
That is why our kallsyms callbacks are different. See the implementation 
of those. But that is the API. Is this what you are worried about?

> Otherwise annotation is appreciate.

Thanks,
Miroslav

Re: [PATCH] livepatch: add module locking around kallsyms calls

[Minfei Huang]

On 06/02/15 at 11:15am, Miroslav Benes wrote:
> On Tue, 2 Jun 2015, Minfei Huang wrote:
> > > -       if (kallsyms_on_each_symbol(klp_verify_callback, &args))
> > > -               return 0;
> > > +       mutex_lock(&module_mutex);
> > > +       ret = kallsyms_on_each_symbol(klp_verify_callback, &args);
> > > +       mutex_unlock(&module_mutex);
> > >
> > 
> > Hi.
> > In livepatch code path, returning value 0 may represent the right, but
> > sometime represent wrong, like the above function.
> > 
> > Is it possible that we can wrap such function and return the unified
> > value? Thus we can not confuse the returning value any more.
> 
> Hi,
> 
> I must admit I do not understand. Both klp_find_object_symbol and 
> klp_verify_vmlinux_symbol return 0 on success or -EINVAL. It is true that 
> kallsyms_on_each_symbol and module_kallsyms_on_each symbol are different. 
> That is why our kallsyms callbacks are different. See the implementation 
> of those. But that is the API. Is this what you are worried about?
> 

Sorry to confuse you about the unclear description.

Yes. kallsyms_on_each_symbol return 0 to imply the failure. I know we
should comply the API which we call from the other module, but it may be
better to wrap the API as a function, if the return value conflicts with
current rule.

Otherwise it may confuse someone that the error message will be printed,
although the return value is 0, like kallsyms_on_each_symbol.

But I do not insist my view.

Thanks
Minfei

> > Otherwise annotation is appreciate.

Re: [PATCH] livepatch: add module locking around kallsyms calls

[Josh Poimboeuf]

On Mon, Jun 01, 2015 at 05:48:37PM +0200, Miroslav Benes wrote:
> The list of loaded modules is walked through in
> module_kallsyms_on_each_symbol (called by kallsyms_on_each_symbol). The
> module_mutex lock should be acquired to prevent potential corruptions
> in the list.
> 
> This was uncovered with new lockdep asserts in module code introduced by
> the commit 0be964be0d45 ("module: Sanitize RCU usage and locking") in
> recent next- trees.
> 
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>

Thanks!

Should we add a comment to kallsyms_on_each_symbol() so that others
don't make this mistake?

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

Re: [PATCH] livepatch: add module locking around kallsyms calls

[Jiri Kosina]

On Tue, 2 Jun 2015, Josh Poimboeuf wrote:

> On Mon, Jun 01, 2015 at 05:48:37PM +0200, Miroslav Benes wrote:
> > The list of loaded modules is walked through in
> > module_kallsyms_on_each_symbol (called by kallsyms_on_each_symbol). The
> > module_mutex lock should be acquired to prevent potential corruptions
> > in the list.
> > 
> > This was uncovered with new lockdep asserts in module code introduced by
> > the commit 0be964be0d45 ("module: Sanitize RCU usage and locking") in
> > recent next- trees.
> > 
> > Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> 
> Thanks!
> 
> Should we add a comment to kallsyms_on_each_symbol() so that others
> don't make this mistake?

Yeah, the locking rules in module loader are not really crystal clear (not 
even after Peterz's revamp in -next), so some comment / lockdep assertion 
might be helpful. But let's keep that separate from this functional fix in 
klp.

> Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

I have now queued this in for-4.1/upstream-fixes, but I don't think I'll 
rush this in for -final now, therefore I added Cc: stable as well. If 
there is -rc7, I'll push it to Linus for -final early next week.

Thanks,

-- 
Jiri Kosina
SUSE Labs