linux-kernel - Re: [RFC PATCH 3/9] livepatch: move patching functions into patch.c

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LNX.2.00.1502131508510.14133@pobox.suse.cz>
Date:	Fri, 13 Feb 2015 15:28:28 +0100 (CET)
From:	Miroslav Benes <mbenes@...e.cz>
To:	Josh Poimboeuf <jpoimboe@...hat.com>
cc:	Seth Jennings <sjenning@...hat.com>, Jiri Kosina <jkosina@...e.cz>,
	Vojtech Pavlik <vojtech@...e.cz>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 3/9] livepatch: move patching functions into
 patch.c

On Mon, 9 Feb 2015, Josh Poimboeuf wrote:

> Move functions related to the actual patching of functions and objects
> into a new patch.c file.

I am definitely for splitting the code to several different files. 
Otherwise it would be soon unmanageable. However I don't know if this 
patch is the best possible. Maybe it is just nitpicking so let's not spend 
too much time on this :)

Without this patch there are several different groups of functions in 
core.c:
1. infrastructure such as global variables, klp_init and some helper 
   functions
2. (un)registration and initialization of the patch
3. enable/disable with patching/unpatching, ftrace handler
4. sysfs code
5. module notifier
6. relocations

I would move sysfs code away to separate file. 

If we decide to move patching code I think it would make sense to move 
enable/disable functions along with it. Or perhaps __klp_enable_patch and 
__klp_disable_patch only. It is possible though that the result would be 
much worse.

Or we can move some other group of functions...

[...]

> diff --git a/kernel/livepatch/patch.h b/kernel/livepatch/patch.h
> new file mode 100644
> index 0000000..bb34bd3
> --- /dev/null
> +++ b/kernel/livepatch/patch.h
> @@ -0,0 +1,25 @@
> +#include <linux/livepatch.h>
> +
> +/**
> + * struct klp_ops - structure for tracking registered ftrace ops structs
> + *
> + * A single ftrace_ops is shared between all enabled replacement functions
> + * (klp_func structs) which have the same old_addr.  This allows the switch
> + * between function versions to happen instantaneously by updating the klp_ops
> + * struct's func_stack list.  The winner is the klp_func at the top of the
> + * func_stack (front of the list).
> + *
> + * @node:	node for the global klp_ops list
> + * @func_stack:	list head for the stack of klp_func's (active func is on top)
> + * @fops:	registered ftrace ops struct
> + */
> +struct klp_ops {
> +	struct list_head node;
> +	struct list_head func_stack;
> +	struct ftrace_ops fops;
> +};
> +
> +struct klp_ops *klp_find_ops(unsigned long old_addr);
> +
> +extern int klp_patch_object(struct klp_object *obj);
> +extern void klp_unpatch_object(struct klp_object *obj);

Is there a reason why klp_find_ops is not extern and the other two 
functions are? I think it is redundant and it is better to be consistent.

Regards,
Miroslav
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/