| 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
| ||
|
Date: Mon, 21 Jan 2019 17:50:50 -0500
From: Joe Lawrence <joe.lawrence@...hat.com>
To: Petr Mladek <pmladek@...e.com>
Cc: Jiri Kosina <jikos@...nel.org>,
Josh Poimboeuf <jpoimboe@...hat.com>,
Miroslav Benes <mbenes@...e.cz>,
Jason Baron <jbaron@...mai.com>,
Evgenii Shatokhin <eshatokhin@...tuozzo.com>,
live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] livepatch: Remove the redundant enabled flag in
struct klp_patch
On Wed, Jan 16, 2019 at 05:17:20PM +0100, Petr Mladek wrote:
> Livepatches can not longer get enabled and disabled repeatedly.
nit: s/not longer/no longer/g
> The list klp_patches contains only enabled patches and eventually
> the patch in transition. As a result, the enabled flag in
> struct klp_patch provides redundant information and can get
> removed.
>
> The flag is replaced by helper function klp_patch_enabled().
> It simplifies the code. Also it helps to understand the semantic,
> especially for the patch in transition.
>
> Alternative solution was to remove klp_target_state. But this
> would be unfortunate. The three state variable helps to
> catch bugs and regressions. Also it makes it easier to get
> the state a lockless way in klp_update_patch_state().
smaller nit: s/get the state/get the state in/
>
> Suggested-by: Josh Poimboeuf <jpoimboe@...hat.com>
> Signed-off-by: Petr Mladek <pmladek@...e.com>
> ---
> include/linux/livepatch.h | 2 --
> kernel/livepatch/core.c | 23 +++++++++++++++--------
> kernel/livepatch/transition.c | 7 +++----
> kernel/livepatch/transition.h | 1 +
> 4 files changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 53551f470722..fa68192e6bb2 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -155,7 +155,6 @@ struct klp_object {
> * @kobj: kobject for sysfs resources
> * @obj_list: dynamic list of the object entries
> * @kobj_added: @kobj has been added and needs freeing
> - * @enabled: the patch is enabled (but operation may be incomplete)
> * @forced: was involved in a forced transition
> * @free_work: patch cleanup from workqueue-context
> * @finish: for waiting till it is safe to remove the patch module
> @@ -171,7 +170,6 @@ struct klp_patch {
> struct kobject kobj;
> struct list_head obj_list;
> bool kobj_added;
> - bool enabled;
> bool forced;
> struct work_struct free_work;
> struct completion finish;
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 684766d306ad..8e644837e668 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -59,6 +59,17 @@ static bool klp_is_module(struct klp_object *obj)
> return obj->name;
> }
>
> +static bool klp_patch_enabled(struct klp_patch *patch)
> +{
> + if (patch == klp_transition_patch) {
> + WARN_ON_ONCE(klp_target_state == KLP_UNDEFINED);
> +
> + return klp_target_state == KLP_PATCHED;
> + }
> +
> + return !list_empty(&patch->list);
> +}
> +
> /* sets obj->mod if object is not vmlinux and module is found */
> static void klp_find_object_module(struct klp_object *obj)
> {
> @@ -335,7 +346,7 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
>
> mutex_lock(&klp_mutex);
>
> - if (patch->enabled == enabled) {
> + if (klp_patch_enabled(patch) == enabled) {
> /* already in requested state */
> ret = -EINVAL;
> goto out;
> @@ -369,7 +380,7 @@ static ssize_t enabled_show(struct kobject *kobj,
> struct klp_patch *patch;
>
> patch = container_of(kobj, struct klp_patch, kobj);
> - return snprintf(buf, PAGE_SIZE-1, "%d\n", patch->enabled);
> + return snprintf(buf, PAGE_SIZE-1, "%d\n", klp_patch_enabled(patch));
> }
>
> static ssize_t transition_show(struct kobject *kobj,
> @@ -862,7 +873,6 @@ static int klp_init_patch_early(struct klp_patch *patch)
> INIT_LIST_HEAD(&patch->list);
> INIT_LIST_HEAD(&patch->obj_list);
> patch->kobj_added = false;
> - patch->enabled = false;
> patch->forced = false;
> INIT_WORK(&patch->free_work, klp_free_patch_work_fn);
> init_completion(&patch->finish);
> @@ -919,7 +929,7 @@ static int __klp_disable_patch(struct klp_patch *patch)
> {
> struct klp_object *obj;
>
> - if (WARN_ON(!patch->enabled))
> + if (WARN_ON(!klp_patch_enabled(patch)))
> return -EINVAL;
>
> if (klp_transition_patch)
> @@ -941,7 +951,6 @@ static int __klp_disable_patch(struct klp_patch *patch)
> smp_wmb();
>
> klp_start_transition();
> - patch->enabled = false;
> klp_try_complete_transition();
>
> return 0;
> @@ -955,7 +964,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
> if (klp_transition_patch)
> return -EBUSY;
>
> - if (WARN_ON(patch->enabled))
> + if (list_empty(&patch->list))
> return -EINVAL;
>
> if (!patch->kobj_added)
> @@ -994,7 +1003,6 @@ static int __klp_enable_patch(struct klp_patch *patch)
> }
>
> klp_start_transition();
> - patch->enabled = true;
> klp_try_complete_transition();
>
> return 0;
> @@ -1093,7 +1101,6 @@ void klp_discard_replaced_patches(struct klp_patch *new_patch)
> if (old_patch == new_patch)
> return;
>
> - old_patch->enabled = false;
> klp_unpatch_objects(old_patch);
> klp_free_patch_start(old_patch);
> schedule_work(&old_patch->free_work);
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index a3a6f32c6fd0..a40b58660640 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -31,7 +31,7 @@
>
> struct klp_patch *klp_transition_patch;
>
> -static int klp_target_state = KLP_UNDEFINED;
> +int klp_target_state = KLP_UNDEFINED;
>
> /*
> * This work can be performed periodically to finish patching or unpatching any
> @@ -354,6 +354,7 @@ static bool klp_try_switch_task(struct task_struct *task)
> void klp_try_complete_transition(void)
> {
> unsigned int cpu;
> + int target_state = klp_target_state;
> struct task_struct *g, *task;
> struct klp_patch *patch;
> bool complete = true;
> @@ -412,7 +413,7 @@ void klp_try_complete_transition(void)
> * klp_complete_transition() but it is called also
> * from klp_cancel_transition().
> */
> - if (!patch->enabled) {
> + if (target_state == KLP_UNPATCHED) {
> klp_free_patch_start(patch);
> schedule_work(&patch->free_work);
> }
> @@ -545,8 +546,6 @@ void klp_reverse_transition(void)
> klp_target_state == KLP_PATCHED ? "patching to unpatching" :
> "unpatching to patching");
>
> - klp_transition_patch->enabled = !klp_transition_patch->enabled;
> -
> klp_target_state = !klp_target_state;
>
> /*
> diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
> index f9d0bc016067..b9f3e96d8c13 100644
> --- a/kernel/livepatch/transition.h
> +++ b/kernel/livepatch/transition.h
> @@ -5,6 +5,7 @@
> #include <linux/livepatch.h>
>
> extern struct klp_patch *klp_transition_patch;
> +extern int klp_target_state;
>
> void klp_init_transition(struct klp_patch *patch, int state);
> void klp_cancel_transition(void);
> --
> 2.13.7
>
With commit msg nits,
Acked-by: Joe Lawrence <joe.lawrence@...hat.com>
-- Joe
Powered by blists - more mailing lists