* FAILED: patch "[PATCH] test_firmware: prevent race conditions by a correct" failed to apply to 5.15-stable tree
@ 2023-06-07 12:22 gregkh
2023-06-10 9:35 ` Mirsad Goran Todorovac
0 siblings, 1 reply; 3+ messages in thread
From: gregkh @ 2023-06-07 12:22 UTC (permalink / raw)
To: mirsad.todorovac, colin.i.king, error27, gregkh, mcgrof, rdunlap,
russell.h.weight, shuah, tianfei.zhang, tiwai
Cc: stable
The patch below does not apply to the 5.15-stable tree.
If someone wants it applied there, or to any other stable or longterm
tree, then please email the backport, including the original git commit
id to <stable@vger.kernel.org>.
To reproduce the conflict and resubmit, you may use the following commands:
git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-5.15.y
git checkout FETCH_HEAD
git cherry-pick -x 4acfe3dfde685a5a9eaec5555351918e2d7266a1
# <resolve conflicts, build, test, etc.>
git commit -s
git send-email --to '<stable@vger.kernel.org>' --in-reply-to '2023060753-dowry-untried-a3d2@gregkh' --subject-prefix 'PATCH 5.15.y' HEAD^..
Possible dependencies:
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
From 4acfe3dfde685a5a9eaec5555351918e2d7266a1 Mon Sep 17 00:00:00 2001
From: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
Date: Tue, 9 May 2023 10:47:45 +0200
Subject: [PATCH] test_firmware: prevent race conditions by a correct
implementation of locking
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);
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.
The similar approach was applied to all functions called from the locked
and the unlocked context, which safely mitigates both deadlocks and race
conditions in the driver.
__test_dev_config_update_bool(), __test_dev_config_update_u8() and
__test_dev_config_update_size_t() unlocked versions of the functions
were introduced to be called from the locked contexts as a workaround
without releasing the main driver's lock and thereof causing a race
condition.
The test_dev_config_update_bool(), test_dev_config_update_u8() and
test_dev_config_update_size_t() locked versions of the functions
are being called from driver methods without the unnecessary multiplying
of the locking and unlocking code for each method, and complicating
the code with saving of the return value across lock.
Fixes: 7feebfa487b92 ("test_firmware: add support for request_firmware_into_buf")
Cc: Luis Chamberlain <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
Suggested-by: Dan Carpenter <error27@gmail.com>
Signed-off-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
Link: https://lore.kernel.org/r/20230509084746.48259-1-mirsad.todorovac@alu.unizg.hr
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index 05ed84c2fc4c..35417e0af3f4 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -353,16 +353,26 @@ static ssize_t config_test_show_str(char *dst,
return len;
}
+static inline int __test_dev_config_update_bool(const char *buf, size_t size,
+ bool *cfg)
+{
+ int ret;
+
+ if (kstrtobool(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);
- if (kstrtobool(buf, cfg) < 0)
- ret = -EINVAL;
- else
- ret = size;
+ ret = __test_dev_config_update_bool(buf, size, cfg);
mutex_unlock(&test_fw_mutex);
return ret;
@@ -373,7 +383,8 @@ static ssize_t test_dev_config_show_bool(char *buf, bool val)
return snprintf(buf, PAGE_SIZE, "%d\n", val);
}
-static int test_dev_config_update_size_t(const char *buf,
+static int __test_dev_config_update_size_t(
+ const char *buf,
size_t size,
size_t *cfg)
{
@@ -384,9 +395,7 @@ static int test_dev_config_update_size_t(const char *buf,
if (ret)
return ret;
- mutex_lock(&test_fw_mutex);
*(size_t *)cfg = new;
- mutex_unlock(&test_fw_mutex);
/* Always return full write size even if we didn't consume all */
return size;
@@ -402,7 +411,7 @@ static ssize_t test_dev_config_show_int(char *buf, int val)
return snprintf(buf, PAGE_SIZE, "%d\n", val);
}
-static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
+static int __test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
{
u8 val;
int ret;
@@ -411,14 +420,23 @@ static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
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 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 val)
{
return snprintf(buf, PAGE_SIZE, "%u\n", val);
@@ -471,10 +489,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;
@@ -518,10 +536,10 @@ static ssize_t config_buf_size_store(struct device *dev,
mutex_unlock(&test_fw_mutex);
goto out;
}
- mutex_unlock(&test_fw_mutex);
- rc = test_dev_config_update_size_t(buf, count,
- &test_fw_config->buf_size);
+ rc = __test_dev_config_update_size_t(buf, count,
+ &test_fw_config->buf_size);
+ mutex_unlock(&test_fw_mutex);
out:
return rc;
@@ -548,10 +566,10 @@ static ssize_t config_file_offset_store(struct device *dev,
mutex_unlock(&test_fw_mutex);
goto out;
}
- mutex_unlock(&test_fw_mutex);
- rc = test_dev_config_update_size_t(buf, count,
- &test_fw_config->file_offset);
+ rc = __test_dev_config_update_size_t(buf, count,
+ &test_fw_config->file_offset);
+ mutex_unlock(&test_fw_mutex);
out:
return rc;
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: FAILED: patch "[PATCH] test_firmware: prevent race conditions by a correct" failed to apply to 5.15-stable tree
2023-06-07 12:22 FAILED: patch "[PATCH] test_firmware: prevent race conditions by a correct" failed to apply to 5.15-stable tree gregkh
@ 2023-06-10 9:35 ` Mirsad Goran Todorovac
2023-06-10 10:15 ` Greg KH
0 siblings, 1 reply; 3+ messages in thread
From: Mirsad Goran Todorovac @ 2023-06-10 9:35 UTC (permalink / raw)
To: gregkh, colin.i.king, error27, mcgrof, rdunlap, russell.h.weight,
shuah, tianfei.zhang, tiwai
Cc: stable
On 6/7/23 14:22, gregkh@linuxfoundation.org wrote:
>
> The patch below does not apply to the 5.15-stable tree.
> If someone wants it applied there, or to any other stable or longterm
> tree, then please email the backport, including the original git commit
> id to <stable@vger.kernel.org>.
>
> To reproduce the conflict and resubmit, you may use the following commands:
>
> git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-5.15.y
> git checkout FETCH_HEAD
> git cherry-pick -x 4acfe3dfde685a5a9eaec5555351918e2d7266a1
> # <resolve conflicts, build, test, etc.>
> git commit -s
> git send-email --to '<stable@vger.kernel.org>' --in-reply-to '2023060753-dowry-untried-a3d2@gregkh' --subject-prefix 'PATCH 5.15.y' HEAD^..
>
> Possible dependencies:
>
>
>
> thanks,
>
> greg k-h
Hi, Mr. Greg,
Is there something wrong with the patch and do I need to resubmit?
Regards,
Mirsad
> ------------------ original commit in Linus's tree ------------------
>
>>From 4acfe3dfde685a5a9eaec5555351918e2d7266a1 Mon Sep 17 00:00:00 2001
> From: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
> Date: Tue, 9 May 2023 10:47:45 +0200
> Subject: [PATCH] test_firmware: prevent race conditions by a correct
> implementation of locking
>
> 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);
>
> 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.
>
> The similar approach was applied to all functions called from the locked
> and the unlocked context, which safely mitigates both deadlocks and race
> conditions in the driver.
>
> __test_dev_config_update_bool(), __test_dev_config_update_u8() and
> __test_dev_config_update_size_t() unlocked versions of the functions
> were introduced to be called from the locked contexts as a workaround
> without releasing the main driver's lock and thereof causing a race
> condition.
>
> The test_dev_config_update_bool(), test_dev_config_update_u8() and
> test_dev_config_update_size_t() locked versions of the functions
> are being called from driver methods without the unnecessary multiplying
> of the locking and unlocking code for each method, and complicating
> the code with saving of the return value across lock.
>
> Fixes: 7feebfa487b92 ("test_firmware: add support for request_firmware_into_buf")
> Cc: Luis Chamberlain <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
> Suggested-by: Dan Carpenter <error27@gmail.com>
> Signed-off-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
> Link: https://lore.kernel.org/r/20230509084746.48259-1-mirsad.todorovac@alu.unizg.hr
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> diff --git a/lib/test_firmware.c b/lib/test_firmware.c
> index 05ed84c2fc4c..35417e0af3f4 100644
> --- a/lib/test_firmware.c
> +++ b/lib/test_firmware.c
> @@ -353,16 +353,26 @@ static ssize_t config_test_show_str(char *dst,
> return len;
> }
>
> +static inline int __test_dev_config_update_bool(const char *buf, size_t size,
> + bool *cfg)
> +{
> + int ret;
> +
> + if (kstrtobool(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);
> - if (kstrtobool(buf, cfg) < 0)
> - ret = -EINVAL;
> - else
> - ret = size;
> + ret = __test_dev_config_update_bool(buf, size, cfg);
> mutex_unlock(&test_fw_mutex);
>
> return ret;
> @@ -373,7 +383,8 @@ static ssize_t test_dev_config_show_bool(char *buf, bool val)
> return snprintf(buf, PAGE_SIZE, "%d\n", val);
> }
>
> -static int test_dev_config_update_size_t(const char *buf,
> +static int __test_dev_config_update_size_t(
> + const char *buf,
> size_t size,
> size_t *cfg)
> {
> @@ -384,9 +395,7 @@ static int test_dev_config_update_size_t(const char *buf,
> if (ret)
> return ret;
>
> - mutex_lock(&test_fw_mutex);
> *(size_t *)cfg = new;
> - mutex_unlock(&test_fw_mutex);
>
> /* Always return full write size even if we didn't consume all */
> return size;
> @@ -402,7 +411,7 @@ static ssize_t test_dev_config_show_int(char *buf, int val)
> return snprintf(buf, PAGE_SIZE, "%d\n", val);
> }
>
> -static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
> +static int __test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
> {
> u8 val;
> int ret;
> @@ -411,14 +420,23 @@ static int test_dev_config_update_u8(const char *buf, size_t size, u8 *cfg)
> 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 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 val)
> {
> return snprintf(buf, PAGE_SIZE, "%u\n", val);
> @@ -471,10 +489,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;
> @@ -518,10 +536,10 @@ static ssize_t config_buf_size_store(struct device *dev,
> mutex_unlock(&test_fw_mutex);
> goto out;
> }
> - mutex_unlock(&test_fw_mutex);
>
> - rc = test_dev_config_update_size_t(buf, count,
> - &test_fw_config->buf_size);
> + rc = __test_dev_config_update_size_t(buf, count,
> + &test_fw_config->buf_size);
> + mutex_unlock(&test_fw_mutex);
>
> out:
> return rc;
> @@ -548,10 +566,10 @@ static ssize_t config_file_offset_store(struct device *dev,
> mutex_unlock(&test_fw_mutex);
> goto out;
> }
> - mutex_unlock(&test_fw_mutex);
>
> - rc = test_dev_config_update_size_t(buf, count,
> - &test_fw_config->file_offset);
> + rc = __test_dev_config_update_size_t(buf, count,
> + &test_fw_config->file_offset);
> + mutex_unlock(&test_fw_mutex);
>
> out:
> return rc;
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: FAILED: patch "[PATCH] test_firmware: prevent race conditions by a correct" failed to apply to 5.15-stable tree
2023-06-10 9:35 ` Mirsad Goran Todorovac
@ 2023-06-10 10:15 ` Greg KH
0 siblings, 0 replies; 3+ messages in thread
From: Greg KH @ 2023-06-10 10:15 UTC (permalink / raw)
To: Mirsad Goran Todorovac
Cc: colin.i.king, error27, mcgrof, rdunlap, russell.h.weight, shuah,
tianfei.zhang, tiwai, stable
On Sat, Jun 10, 2023 at 11:35:34AM +0200, Mirsad Goran Todorovac wrote:
> On 6/7/23 14:22, gregkh@linuxfoundation.org wrote:
> >
> > The patch below does not apply to the 5.15-stable tree.
> > If someone wants it applied there, or to any other stable or longterm
> > tree, then please email the backport, including the original git commit
> > id to <stable@vger.kernel.org>.
> >
> > To reproduce the conflict and resubmit, you may use the following commands:
> >
> > git fetch https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/ linux-5.15.y
> > git checkout FETCH_HEAD
> > git cherry-pick -x 4acfe3dfde685a5a9eaec5555351918e2d7266a1
> > # <resolve conflicts, build, test, etc.>
> > git commit -s
> > git send-email --to '<stable@vger.kernel.org>' --in-reply-to '2023060753-dowry-untried-a3d2@gregkh' --subject-prefix 'PATCH 5.15.y' HEAD^..
> >
> > Possible dependencies:
> >
> >
> >
> > thanks,
> >
> > greg k-h
>
> Hi, Mr. Greg,
>
> Is there something wrong with the patch and do I need to resubmit?
Yes, and yes. It does not apply, as the text here says, please follow
the above steps if you wish to have it applied to the 5.15.y kernel
tree.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-06-10 10:15 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-07 12:22 FAILED: patch "[PATCH] test_firmware: prevent race conditions by a correct" failed to apply to 5.15-stable tree gregkh
2023-06-10 9:35 ` Mirsad Goran Todorovac
2023-06-10 10:15 ` Greg KH
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).