lkml.org 
[lkml]   [2015]   [Nov]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 1/4 v5] livepatch: add old_sympos as disambiguator field to klp_func
On Thu 2015-11-12 10:59:50, Chris J Arges wrote:
> In cases of duplicate symbols, old_sympos will be used to disambiguate
> instead of old_addr. By default old_sympos will be 0, and patching will
> only succeed if the symbol is unique. Specifying a positive value will
> ensure that occurrence of the symbol in kallsyms for the patched object
> will be used for patching if it is valid.
>
> In addition, make old_addr an internal structure field not to be specified
> by the user. Finally, remove klp_find_verify_func_addr as it can be
> replaced by klp_find_object_symbol directly.
>
> Signed-off-by: Chris J Arges <chris.j.arges@canonical.com>
> ---
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 6e53441..479d75e 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -159,36 +154,46 @@ static int klp_find_callback(void *data, const char *name,
> return 0;
>
> /*
> - * args->addr might be overwritten if another match is found
> - * but klp_find_object_symbol() handles this and only returns the
> - * addr if count == 1.
> + * Increment and assign the address. Return 1 when the desired symbol
> + * position is found or the search for a unique symbol fails.
> */
> args->addr = addr;
> args->count++;

I would avoid the obvious things and move the comment here:

/*
* Finish the search when the symbol is found on the desired
* position or the position is not defined for non-unique
* symbol.
*/

> + if (((args->pos > 0) && (args->count == args->pos)) ||
> + ((args->pos == 0) && (args->count > 1)))
> + return 1;

I wonder if this is better readable:

if ((args->pos && (args->count == args->pos)) ||
(!args->pos && (args->count > 1)))
return 1;

> return 0;
> }
>
[...]
> @@ -277,7 +261,7 @@ static int klp_find_external_symbol(struct module *pmod, const char *name,
> preempt_enable();
>
> /* otherwise check if it's in another .o within the patch module */
> - return klp_find_object_symbol(pmod->name, name, addr);
> + return klp_find_object_symbol(pmod->name, name, addr, 0);

Please, add a comment here that external symbols always have to be unique.

> }
>
> static int klp_write_object_relocations(struct module *pmod,
> @@ -307,7 +291,7 @@ static int klp_write_object_relocations(struct module *pmod,
> else
> ret = klp_find_object_symbol(obj->mod->name,
> reloc->name,
> - &reloc->val);
> + &reloc->val, 0);

Please, mention in the commit message that the support for sympos will
be added into relocations in a separate patch. Or add a FIXME comment
here. It will make easier the review and git history archaeology.

Thank you,
Petr


\
 
 \ /
  Last update: 2015-11-13 17:41    [W:0.122 / U:1.740 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site