Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Petr Mladek <pmladek@suse.com>
To:     Jiri Kosina <jikos@kernel.org>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Miroslav Benes <mbenes@suse.cz>
Cc:     Jason Baron <jbaron@akamai.com>,
        Joe Lawrence <joe.lawrence@redhat.com>,
        Jessica Yu <jeyu@kernel.org>,
        Evgenii Shatokhin <eshatokhin@virtuozzo.com>,
        live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
        Petr Mladek <pmladek@suse.com>
Subject: [PATCH v12 11/12] livepatch: Remove ordering and refuse loading conflicting patches
Date:   Tue, 28 Aug 2018 16:36:02 +0200
Message-Id: <20180828143603.4442-12-pmladek@suse.com>
In-Reply-To: <20180828143603.4442-1-pmladek@suse.com>
References: <20180828143603.4442-1-pmladek@suse.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

The atomic replace and cumulative patches were introduced as a more secure
way to handle dependent patches. They simplify the logic:

  + Any new cumulative patch is supposed to take over shadow variables
    and changes made by callbacks from previous livepatches.

  + All replaced patches are discarded and the modules can be unloaded.
    As a result, there is only one scenario when a cumulative livepatch
    gets disabled.

The different handling of "normal" and cumulative patches might cause
confusion. It would make sense to keep only one mode. On the other hand,
it would be rude to enforce using the cumulative livepatches even for
trivial and independent (hot) fixes.

This patch removes the stack of patches. The list of enabled patches
is still needed but the ordering is not longer enforced.

Note that it is not possible to catch all possible dependencies. It is
the responsibility of the livepatch authors to decide.

Nevertheless this patch prevents having two patches for the same function
enabled at the same time after the transition finishes. It might help
to catch obvious mistakes. But more importantly, we do not need to
handle situation when a patch in the middle of the function stack
(ops->func_stack) is being removed.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 Documentation/livepatch/livepatch.txt | 30 +++++++++++--------
 kernel/livepatch/core.c               | 56 +++++++++++++++++++++++++++++++----
 2 files changed, 68 insertions(+), 18 deletions(-)

diff --git a/Documentation/livepatch/livepatch.txt b/Documentation/livepatch/livepatch.txt
index 7fb01d27d81d..8d985cab0a21 100644
--- a/Documentation/livepatch/livepatch.txt
+++ b/Documentation/livepatch/livepatch.txt
@@ -141,9 +141,9 @@ without HAVE_RELIABLE_STACKTRACE are not considered fully supported by
 the kernel livepatching.
 
 The /sys/kernel/livepatch/<patch>/transition file shows whether a patch
-is in transition.  Only a single patch (the topmost patch on the stack)
-can be in transition at a given time.  A patch can remain in transition
-indefinitely, if any of the tasks are stuck in the initial patch state.
+is in transition.  Only a single patch can be in transition at a given
+time.  A patch can remain in transition indefinitely, if any of the tasks
+are stuck in the initial patch state.
 
 A transition can be reversed and effectively canceled by writing the
 opposite value to the /sys/kernel/livepatch/<patch>/enabled file while
@@ -327,9 +327,10 @@ successfully disabled via the sysfs interface.
 Livepatch modules have to call klp_enable_patch() in module_init() callback.
 This function is rather complex and might even fail in the early phase.
 
-First, the addresses of the patched functions are found according to their
-names. The special relocations, mentioned in the section "New functions",
-are applied. The relevant entries are created under
+First, possible conflicts are checked for non-cummulative patches with
+disabled replace flag. The addresses of the patched functions are found
+according to their names. The special relocations, mentioned in the section
+"New functions", are applied. The relevant entries are created under
 /sys/kernel/livepatch/<name>. The patch is rejected when any above
 operation fails.
 
@@ -343,11 +344,11 @@ this process, see the "Consistency model" section.
 Finally, once all tasks have been patched, the 'transition' value changes
 to '0'.
 
-[*] Note that functions might be patched multiple times. The ftrace handler
-    is registered only once for a given function. Further patches just add
-    an entry to the list (see field `func_stack`) of the struct klp_ops.
-    The right implementation is selected by the ftrace handler, see
-    the "Consistency model" section.
+[*] Note that two patches might modify the same function during the transition
+    to a new cumulative patch. The ftrace handler is registered only once
+    for a given function. The new patch just adds an entry to the list
+    (see field `func_stack`) of the struct klp_ops. The right implementation
+    is selected by the ftrace handler, see the "Consistency model" section.
 
 
 5.2. Disabling
@@ -374,8 +375,11 @@ Third, the sysfs interface is destroyed.
 Finally, the module can be removed if the transition was not forced and the
 last sysfs entry has gone.
 
-Note that patches must be disabled in exactly the reverse order in which
-they were enabled. It makes the problem and the implementation much easier.
+Note that any patch dependencies have to be handled by the atomic replace
+and cumulative patches, see Documentation/livepatch/cumulative-patches.txt.
+Therefore there is usually only one patch enabled on the system. There is
+still possibility to have more trivial and independent livepatches enabled
+at the same time. These can be enabled and disabled in any order.
 
 
 6. Sysfs
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 695d565f23c1..f3e199e8b767 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -137,6 +137,47 @@ static struct klp_object *klp_find_object(struct klp_patch *patch,
 	return NULL;
 }
 
+static int klp_check_obj_conflict(struct klp_patch *patch,
+				  struct klp_object *old_obj)
+{
+	struct klp_object *obj;
+	struct klp_func *func, *old_func;
+
+	obj = klp_find_object(patch, old_obj);
+	if (!obj)
+		return 0;
+
+	klp_for_each_func(old_obj, old_func) {
+		func = klp_find_func(obj, old_func);
+		if (!func)
+			continue;
+
+		pr_err("Function '%s,%lu' in object '%s' has already been livepatched.\n",
+		       func->old_name, func->old_sympos ? func->old_sympos : 1,
+		       obj->name ? obj->name : "vmlinux");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int klp_check_patch_conflict(struct klp_patch *patch)
+{
+	struct klp_patch *old_patch;
+	struct klp_object *old_obj;
+	int ret;
+
+	list_for_each_entry(old_patch, &klp_patches, list) {
+		klp_for_each_object(old_patch, old_obj) {
+			ret = klp_check_obj_conflict(patch, old_obj);
+			if (ret)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
 struct klp_find_arg {
 	const char *objname;
 	const char *name;
@@ -888,7 +929,6 @@ static int klp_init_patch(struct klp_patch *patch)
 	patch->module_put = false;
 	INIT_LIST_HEAD(&patch->list);
 	init_completion(&patch->finish);
-	klp_init_lists(patch);
 
 	if (!patch->objs)
 		return -EINVAL;
@@ -934,10 +974,6 @@ static int __klp_disable_patch(struct klp_patch *patch)
 	if (klp_transition_patch)
 		return -EBUSY;
 
-	/* enforce stacking: only the last enabled patch can be disabled */
-	if (!list_is_last(&patch->list, &klp_patches))
-		return -EBUSY;
-
 	klp_init_transition(patch, KLP_UNPATCHED);
 
 	klp_for_each_object(patch, obj)
@@ -1052,8 +1088,18 @@ int klp_enable_patch(struct klp_patch *patch)
 		return -ENOSYS;
 	}
 
+
 	mutex_lock(&klp_mutex);
 
+	/* Allow to use the dynamic lists in the check for conflicts. */
+	klp_init_lists(patch);
+
+	if (!patch->replace && klp_check_patch_conflict(patch)) {
+		pr_err("Use cumulative livepatches for dependent changes.\n");
+		mutex_unlock(&klp_mutex);
+		return -EINVAL;
+	}
+
 	ret = klp_init_patch(patch);
 	if (ret)
 		goto err;
-- 
2.13.7