On Wed 2019-10-23 16:15:28, Josh Poimboeuf wrote:

Hi Petr,

Sorry for taking so long...

On Thu, Oct 03, 2019 at 11:01:35AM +0200, Petr Mladek wrote:

quoted

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 726947338fd5..42907c4a0ce8 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h

@@ -133,10 +133,12 @@ struct klp_object {
 /**
  * struct klp_state - state of the system modified by the livepatch
  * @id:		system state identifier (non-zero)
+ * @version:	version of the change (non-zero)

Is it necessary to assume that 'version' is non-zero?  It would be easy
for a user to not realize that and start with version 0.  Then the patch
state would be silently ignored.

I have the same concern about 'id', but I guess at least one of them has
to be non-zero to differentiate valid entries from the array terminator.

Exactly. At least one struct member must be non-zero to differentiate
the array terminator.

I do not mind to allow zero version. Will do so in v4.

quoted

+/* Check if the patch is able to deal with the given system state. */
+static bool klp_is_state_compatible(struct klp_patch *patch,
+				    struct klp_state *state)
+{
+	struct klp_state *new_state;
+
+	new_state = klp_get_state(patch, state->id);
+
+	if (new_state)
+		return new_state->version >= state->version;
+
+	/* Cumulative livepatch must handle all already modified states. */
+	return !patch->replace;
+}

quoted

From my perspective I view '!new_state' as an error condition.  I'd find

it easier to read if the ordering were changed to check for the error
first:

	if (!new_state) {
		/*
		 * A cumulative livepatch must handle all already
		 * modified states.
		 */
		return !patch->replace;
	}

	return new_state->version >= state->version;

-> v4

quoted

+
+/*
+ * Check that the new livepatch will not break the existing system states.
+ * Cumulative patches must handle all already modified states.
+ * Non-cumulative patches can touch already modified states.
+ */
+bool klp_is_patch_compatible(struct klp_patch *patch)
+{
+	struct klp_patch *old_patch;
+	struct klp_state *state;
+
+
+	klp_for_each_patch(old_patch) {

Extra newline above.

quoted

+		klp_for_each_state(old_patch, state) {
+			if (!klp_is_state_compatible(patch, state))
+				return false;
+		}
+	}

I think renaming 'state' to 'old_state' would make the intention a
little clearer, and would be consistent with 'old_patch'.

Makes sense. I'll make the names consistent also in klp_is_state_compatible():


/* Check if the patch is able to deal with the given system state. */
static bool klp_is_state_compatible(struct klp_patch *patch,
				    struct klp_state *old_state)
{
	struct klp_state *state = klp_get_state(patch, state->id);

	if (!state) {
		/*
		 * A cumulative livepatch must handle all already
		 * modified states.
		 */
		return !patch->replace;
	}

	return state->version >= old_state->version;
}

Best Regards,
Petr