linux-kernel - [PATCH v4 4/4] firmware: Do not use fw_lock for fw

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: <1473237908-20989-5-git-send-email-wagi@monom.org>
Date:   Wed,  7 Sep 2016 10:45:08 +0200
From:   Daniel Wagner <wagi@...om.org>
To:     linux-kernel@...r.kernel.org
Cc:     Daniel Wagner <daniel.wagner@...-carit.de>,
        Ming Lei <ming.lei@...onical.com>,
        "Luis R . Rodriguez" <mcgrof@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: [PATCH v4 4/4] firmware: Do not use fw_lock for fw_status protection

From: Daniel Wagner <daniel.wagner@...-carit.de>

fw_lock is to use to protect 'corner cases' inside firmware_class. It
is not exactly clear what those corner cases are nor what it exactly
protects. fw_status can be used without needing the fw_lock to protect
its state transition and wake ups.

fw_status is holds the state in status and the completion is used to
wake up all waiters (in this case that is the user land helper so only
one). This operation has to be 'atomic' to avoid races.  We can do this
by using swait which takes care we don't miss any wake up.

We use also swait instead of wait because don't need all the additional
features wait provides.

Signed-off-by: Daniel Wagner <daniel.wagner@...-carit.de>
Cc: Ming Lei <ming.lei@...onical.com>
Cc: Luis R. Rodriguez <mcgrof@...nel.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
---
 drivers/base/firmware_class.c | 53 +++++++++++++------------------------------
 1 file changed, 16 insertions(+), 37 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 7757c03..05fe6ec 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -30,6 +30,7 @@
 #include <linux/syscore_ops.h>
 #include <linux/reboot.h>
 #include <linux/security.h>
+#include <linux/swait.h>
 
 #include <generated/utsrelease.h>
 
@@ -109,13 +110,13 @@ static inline long firmware_loading_timeout(void)
 
 struct fw_status {
 	unsigned long status;
-	struct completion completion;
+	struct swait_queue_head wq;
 };
 
 static void fw_status_init(struct fw_status *fw_st)
 {
 	fw_st->status = FW_STATUS_UNKNOWN;
-	init_completion(&fw_st->completion);
+	init_swait_queue_head(&fw_st->wq);
 }
 
 static int __fw_status_check(struct fw_status *fw_st, unsigned long status)
@@ -123,15 +124,19 @@ static int __fw_status_check(struct fw_status *fw_st, unsigned long status)
 	return fw_st->status == status;
 }
 
+static inline bool __fw_status_is_done(unsigned long status)
+{
+	return status == FW_STATUS_DONE ||
+	       status == FW_STATUS_ABORTED;
+}
+
 static int fw_status_wait_timeout(struct fw_status *fw_st, long timeout)
 {
-	unsigned long status;
 	int ret;
-
-	ret = wait_for_completion_interruptible_timeout(&fw_st->completion,
-							timeout);
-	status = READ_ONCE(fw_st->status);
-	if (ret == 0 && status == FW_STATUS_ABORTED)
+	ret = swait_event_interruptible_timeout(fw_st->wq,
+				__fw_status_is_done(READ_ONCE(fw_st->status)),
+				timeout);
+	if (ret == 0 && fw_st->status == FW_STATUS_ABORTED)
 		return -ENOENT;
 
 	return ret;
@@ -144,7 +149,7 @@ static void __fw_status_set(struct fw_status *fw_st,
 
 	if (status == FW_STATUS_DONE ||
 			status == FW_STATUS_ABORTED)
-		complete_all(&fw_st->completion);
+		swake_up(&fw_st->wq);
 }
 
 #define fw_status_start(fw_st)					\
@@ -370,14 +375,6 @@ static const char * const fw_path[] = {
 module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
 MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
 
-static void fw_finish_direct_load(struct device *device,
-				  struct firmware_buf *buf)
-{
-	mutex_lock(&fw_lock);
-	fw_status_done(&buf->fw_st);
-	mutex_unlock(&fw_lock);
-}
-
 static int
 fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
 {
@@ -424,7 +421,7 @@ fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
 		}
 		dev_dbg(device, "direct-loading %s\n", buf->fw_id);
 		buf->size = size;
-		fw_finish_direct_load(device, buf);
+		fw_status_done(&buf->fw_st);
 		break;
 	}
 	__putname(path);
@@ -1103,24 +1100,6 @@ static inline void kill_requests_without_uevent(void) { }
 
 #endif /* CONFIG_FW_LOADER_USER_HELPER */
 
-/* wait until the shared firmware_buf becomes ready (or error) */
-static int sync_cached_firmware_buf(struct firmware_buf *buf)
-{
-	int ret = 0;
-
-	mutex_lock(&fw_lock);
-	while (!fw_status_is_done(&buf->fw_st)) {
-		if (fw_status_is_aborted(&buf->fw_st)) {
-			ret = -ENOENT;
-			break;
-		}
-		mutex_unlock(&fw_lock);
-		ret = fw_status_wait_timeout(&buf->fw_st, 0);
-		mutex_lock(&fw_lock);
-	}
-	mutex_unlock(&fw_lock);
-	return ret;
-}
 
 /* prepare firmware and firmware_buf structs;
  * return 0 if a firmware is already assigned, 1 if need to load one,
@@ -1155,7 +1134,7 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
 	firmware->priv = buf;
 
 	if (ret > 0) {
-		ret = sync_cached_firmware_buf(buf);
+		ret = fw_status_wait_timeout(&buf->fw_st, 0);
 		if (!ret) {
 			fw_set_page_data(buf, firmware);
 			return 0; /* assigned */
-- 
2.7.4