Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Marcos Paulo de Souza <mpdesouza@suse.com>
To:     linux-kernel@vger.kernel.org, live-patching@vger.kernel.org
Cc:     jpoimboe@redhat.com, joe.lawrence@redhat.com, pmladek@suse.com,
        Marcos Paulo de Souza <mpdesouza@suse.com>
Subject: [PATCH v2 1/4] livepatch/shadow: Separate code to get or use pre-allocated shadow variable
Date:   Wed, 26 Oct 2022 16:41:19 -0300
Message-Id: <20221026194122.11761-2-mpdesouza@suse.com>
In-Reply-To: <20221026194122.11761-1-mpdesouza@suse.com>
References: <20221026194122.11761-1-mpdesouza@suse.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

From: Petr Mladek <pmladek@suse.com>

Separate code that is used in klp_shadow_get_or_alloc() under klp_mutex.
It splits a long spaghetti function into two. Also it unifies the error
handling. The old used a mix of duplicated code, direct returns,
and goto. The new code has only one unlock, free, and return calls.

It is code refactoring without any functional changes.

Signed-off-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Marcos Paulo de Souza <mpdesouza@suse.com>
---
 Changes from v1:
 * Change __klp_shadow_get_or_use to __klp_shadow_get_or_add_locked and add a comment
   about it's meaning (Petr)
 * Add lockdep_assert_held on __klp_shadow_get_or_add_locked
 * Reworked the commit message (Petr)
 * Added my SoB (Josh)

 kernel/livepatch/shadow.c | 78 ++++++++++++++++++++++-----------------
 1 file changed, 45 insertions(+), 33 deletions(-)

diff --git a/kernel/livepatch/shadow.c b/kernel/livepatch/shadow.c
index c2e724d97ddf..81ad7cbbd124 100644
--- a/kernel/livepatch/shadow.c
+++ b/kernel/livepatch/shadow.c
@@ -101,41 +101,22 @@ void *klp_shadow_get(void *obj, unsigned long id)
 }
 EXPORT_SYMBOL_GPL(klp_shadow_get);
 
-static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
-				       size_t size, gfp_t gfp_flags,
-				       klp_shadow_ctor_t ctor, void *ctor_data,
-				       bool warn_on_exist)
+/* Check if the variable exists. Otherwise, add the pre-allocated one. */
+static void *__klp_shadow_get_or_add_locked(void *obj, unsigned long id,
+				     struct klp_shadow *new_shadow,
+				     klp_shadow_ctor_t ctor, void *ctor_data,
+				     bool *exist)
 {
-	struct klp_shadow *new_shadow;
 	void *shadow_data;
-	unsigned long flags;
 
-	/* Check if the shadow variable already exists */
-	shadow_data = klp_shadow_get(obj, id);
-	if (shadow_data)
-		goto exists;
-
-	/*
-	 * Allocate a new shadow variable.  Fill it with zeroes by default.
-	 * More complex setting can be done by @ctor function.  But it is
-	 * called only when the buffer is really used (under klp_shadow_lock).
-	 */
-	new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);
-	if (!new_shadow)
-		return NULL;
+	lockdep_assert_held(&klp_shadow_lock);
 
-	/* Look for <obj, id> again under the lock */
-	spin_lock_irqsave(&klp_shadow_lock, flags);
 	shadow_data = klp_shadow_get(obj, id);
 	if (unlikely(shadow_data)) {
-		/*
-		 * Shadow variable was found, throw away speculative
-		 * allocation.
-		 */
-		spin_unlock_irqrestore(&klp_shadow_lock, flags);
-		kfree(new_shadow);
-		goto exists;
+		*exist = true;
+		return shadow_data;
 	}
+	*exist = false;
 
 	new_shadow->obj = obj;
 	new_shadow->id = id;
@@ -145,8 +126,6 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
 
 		err = ctor(obj, new_shadow->data, ctor_data);
 		if (err) {
-			spin_unlock_irqrestore(&klp_shadow_lock, flags);
-			kfree(new_shadow);
 			pr_err("Failed to construct shadow variable <%p, %lx> (%d)\n",
 			       obj, id, err);
 			return NULL;
@@ -156,12 +135,45 @@ static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
 	/* No <obj, id> found, so attach the newly allocated one */
 	hash_add_rcu(klp_shadow_hash, &new_shadow->node,
 		     (unsigned long)new_shadow->obj);
-	spin_unlock_irqrestore(&klp_shadow_lock, flags);
 
 	return new_shadow->data;
+}
+
+static void *__klp_shadow_get_or_alloc(void *obj, unsigned long id,
+				       size_t size, gfp_t gfp_flags,
+				       klp_shadow_ctor_t ctor, void *ctor_data,
+				       bool warn_on_exist)
+{
+	struct klp_shadow *new_shadow;
+	void *shadow_data;
+	bool exist;
+	unsigned long flags;
+
+	/* Check if the shadow variable already exists */
+	shadow_data = klp_shadow_get(obj, id);
+	if (shadow_data)
+		return shadow_data;
+
+	/*
+	 * Allocate a new shadow variable.  Fill it with zeroes by default.
+	 * More complex setting can be done by @ctor function.  But it is
+	 * called only when the buffer is really used (under klp_shadow_lock).
+	 */
+	new_shadow = kzalloc(size + sizeof(*new_shadow), gfp_flags);
+	if (!new_shadow)
+		return NULL;
+
+	/* Look for <obj, id> again under the lock */
+	spin_lock_irqsave(&klp_shadow_lock, flags);
+	shadow_data = __klp_shadow_get_or_add_locked(obj, id, new_shadow,
+					      ctor, ctor_data, &exist);
+	spin_unlock_irqrestore(&klp_shadow_lock, flags);
+
+	/* Throw away unused speculative allocation. */
+	if (!shadow_data || exist)
+		kfree(new_shadow);
 
-exists:
-	if (warn_on_exist) {
+	if (exist && warn_on_exist) {
 		WARN(1, "Duplicate shadow variable <%p, %lx>\n", obj, id);
 		return NULL;
 	}
-- 
2.37.3