[PATCH v3 1/2] test_firmware: prevent race conditions by a correct implementation of locking

[PATCH v3 1/2] test_firmware: prevent race conditions by a correct implementation of locking

[Mirsad Todorovac]

[ Upstream commit 4acfe3dfde685a5a9eaec5555351918e2d7266a1 ]

Dan Carpenter spotted a race condition in a couple of situations like
these in the test_firmware driver:

static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
{
        u8 val;
        int ret;

        ret = kstrtou8(buf, 10, &val);
        if (ret)
                return ret;

        mutex_lock(&test_fw_mutex);
        *(u8 *)cfg = val;
        mutex_unlock(&test_fw_mutex);

        /* Always return full write size even if we didn't consume all */
        return size;
}

static ssize_t config_num_requests_store(struct device *dev,
                                         struct device_attribute *attr,
                                         const char *buf, size_t count)
{
        int rc;

        mutex_lock(&test_fw_mutex);
        if (test_fw_config->reqs) {
                pr_err("Must call release_all_firmware prior to changing config\n");
                rc = -EINVAL;
                mutex_unlock(&test_fw_mutex);
                goto out;
        }
        mutex_unlock(&test_fw_mutex);

        // NOTE: HERE is the race!!! Function can be preempted!

        // test_fw_config->reqs can change between the release of
        // the lock about and acquire of the lock in the
        // test_dev_config_update_u8()

        rc = test_dev_config_update_u8(buf, count,
                                       &test_fw_config->num_requests);

out:
        return rc;
}

static ssize_t config_read_fw_idx_store(struct device *dev,
                                        struct device_attribute *attr,
                                        const char *buf, size_t count)
{
        return test_dev_config_update_u8(buf, count,
                                         &test_fw_config->read_fw_idx);
}

The function test_dev_config_update_u8() is called from both the locked
and the unlocked context, function config_num_requests_store() and
config_read_fw_idx_store() which can both be called asynchronously as
they are driver's methods, while test_dev_config_update_u8() and siblings
change their argument pointed to by u8 *cfg or similar pointer.

To avoid deadlock on test_fw_mutex, the lock is dropped before calling
test_dev_config_update_u8() and re-acquired within test_dev_config_update_u8()
itself, but alas this creates a race condition.

Having two locks wouldn't assure a race-proof mutual exclusion.

This situation is best avoided by the introduction of a new, unlocked
function __test_dev_config_update_u8() which can be called from the locked
context and reducing test_dev_config_update_u8() to:

static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
{
        int ret;

        mutex_lock(&test_fw_mutex);
        ret = __test_dev_config_update_u8(buf, size, cfg);
        mutex_unlock(&test_fw_mutex);

        return ret;
}

doing the locking and calling the unlocked primitive, which enables both
locked and unlocked versions without duplication of code.

Fixes: c92316bf8e948 ("test_firmware: add batched firmware tests")
Cc: Luis R. Rodriguez <mcgrof@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Russ Weight <russell.h.weight@intel.com>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Tianfei Zhang <tianfei.zhang@intel.com>
Cc: Shuah Khan <shuah@kernel.org>
Cc: Colin Ian King <colin.i.king@gmail.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: linux-kselftest@vger.kernel.org
Cc: stable@vger.kernel.org # v5.4, 4.19, 4.14
Suggested-by: Dan Carpenter <error27@gmail.com>
Link: https://lore.kernel.org/r/20230509084746.48259-1-mirsad.todorovac@alu.unizg.hr
Signed-off-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>

[ This is the patch to fix the racing condition in locking for the 5.4,	]
[ 4.19 and 4.14 stable branches. Not all the fixes from the upstream	]
[ commit apply, but those which do are verbatim equal to those in the	]
[ upstream commit.							]
---
 v3:
 minor bug fixes in the commit description. no change to the code.
 5.4, 4.19 and 4.14 passed build, 5.4 and 4.19 passed kselftest.
 unable to boot 4.14, should work (no changes to lib/test_firmware.c).

 v2:
 bundled locking and ENOSPC patches together.
 tested on 5.4 and 4.19 stable.

 lib/test_firmware.c | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index 38553944e967..92d7195d5b5b 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -301,16 +301,26 @@ static ssize_t config_test_show_str(char *dst,
 	return len;
 }
 
-static int test_dev_config_update_bool(const char *buf, size_t size,
-				       bool *cfg)
+static inline int __test_dev_config_update_bool(const char *buf, size_t size,
+						bool *cfg)
 {
 	int ret;
 
-	mutex_lock(&test_fw_mutex);
 	if (strtobool(buf, cfg) < 0)
 		ret = -EINVAL;
 	else
 		ret = size;
+
+	return ret;
+}
+
+static int test_dev_config_update_bool(const char *buf, size_t size,
+				       bool *cfg)
+{
+	int ret;
+
+	mutex_lock(&test_fw_mutex);
+	ret = __test_dev_config_update_bool(buf, size, cfg);
 	mutex_unlock(&test_fw_mutex);
 
 	return ret;
@@ -340,7 +350,7 @@ static ssize_t test_dev_config_show_int(char *buf, int cfg)
 	return snprintf(buf, PAGE_SIZE, "%d\n", val);
 }
 
-static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
+static inline int __test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
 {
 	int ret;
 	long new;
@@ -352,14 +362,23 @@ static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
 	if (new > U8_MAX)
 		return -EINVAL;
 
-	mutex_lock(&test_fw_mutex);
 	*(u8 *)cfg = new;
-	mutex_unlock(&test_fw_mutex);
 
 	/* Always return full write size even if we didn't consume all */
 	return size;
 }
 
+static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
+{
+	int ret;
+
+	mutex_lock(&test_fw_mutex);
+	ret = __test_dev_config_update_u8(buf, size, cfg);
+	mutex_unlock(&test_fw_mutex);
+
+	return ret;
+}
+
 static ssize_t test_dev_config_show_u8(char *buf, u8 cfg)
 {
 	u8 val;
@@ -392,10 +411,10 @@ static ssize_t config_num_requests_store(struct device *dev,
 		mutex_unlock(&test_fw_mutex);
 		goto out;
 	}
-	mutex_unlock(&test_fw_mutex);
 
-	rc = test_dev_config_update_u8(buf, count,
-				       &test_fw_config->num_requests);
+	rc = __test_dev_config_update_u8(buf, count,
+					 &test_fw_config->num_requests);
+	mutex_unlock(&test_fw_mutex);
 
 out:
 	return rc;
-- 
2.34.1

Re: [PATCH v3 1/2] test_firmware: prevent race conditions by a correct implementation of locking

[Greg Kroah-Hartman]

On Thu, Aug 03, 2023 at 06:53:04PM +0200, Mirsad Todorovac wrote:
> [ Upstream commit 4acfe3dfde685a5a9eaec5555351918e2d7266a1 ]

<snip>

Ok, I am totally confused as to what patch is the newest one, and which
is for what branches, sorry.

So, I've deleted all of the patches from my review queue and I would ask
that you start over.

Note, if you put the kernel version in the subject line, it makes it
simpler for me to understand what goes where.

Here is an example to follow:
	https://lore.kernel.org/r/20230802170227.1590187-1-eahariha@linux.microsoft.com

There are loads of other examples on the stable mailing list, please
don't make it hard for me to understand what to do here, make it obvious
as I'm dealing with hundreds of patches a day.

thanks,

greg k-h

Re: [PATCH v3 1/2] [DONE REQ CHG] test_firmware: prevent race conditions by a correct implementation of locking

[Mirsad Todorovac]

Hi, Mr. Greg,

On 8/4/23 13:22, Greg Kroah-Hartman wrote:
> On Thu, Aug 03, 2023 at 06:53:04PM +0200, Mirsad Todorovac wrote:
>> [ Upstream commit 4acfe3dfde685a5a9eaec5555351918e2d7266a1 ]
> 
> <snip>
> 
> Ok, I am totally confused as to what patch is the newest one, and which
> is for what branches, sorry.
> 
> So, I've deleted all of the patches from my review queue and I would ask
> that you start over.
> 
> Note, if you put the kernel version in the subject line, it makes it
> simpler for me to understand what goes where.
> 
> Here is an example to follow:
> 	https://lore.kernel.org/r/20230802170227.1590187-1-eahariha@linux.microsoft.com
> 
> There are loads of other examples on the stable mailing list, please
> don't make it hard for me to understand what to do here, make it obvious
> as I'm dealing with hundreds of patches a day.

Done requested changes and resubmitted. I hope it is done right this time. Of course,
it could be better still, but this should work out.

It is a paramount that we make your life with applying patches easier and this is
a new situation that I haven't got covered yet.

I will also try to imitate the skilled patch submitters the next time before wasting
your time or reinventing the wheel.

> thanks,

You're welcome.

Kind regards,
Mirsad