* [PATCH v2] remoteproc: sysfs/debugfs: fix race while updating recovery flag
@ 2023-02-01 5:46 Satya Durga Srinivasu Prabhala
2023-02-01 12:05 ` Mukesh Ojha
2023-02-14 16:50 ` Bjorn Andersson
0 siblings, 2 replies; 4+ messages in thread
From: Satya Durga Srinivasu Prabhala @ 2023-02-01 5:46 UTC (permalink / raw)
To: andersson, mathieu.poirier
Cc: Satya Durga Srinivasu Prabhala, linux-remoteproc, linux-kernel,
linux-arm-msm
When multiple clients try to update the recovery flag, it is
possible that, race condition would lead to undesired results
as updates to recovery flag isn't protected by any mechanism
today. To avoid such issues, take remoteproc mutex lock before
updating recovery flag and release the lock once done.
Signed-off-by: Satya Durga Srinivasu Prabhala <quic_satyap@quicinc.com>
---
v1 -> v2:
- addressed comments from Mukesh Ojha
1. take & release lock only while updating recovery flag
2. update debugfs
drivers/remoteproc/remoteproc_debugfs.c | 4 ++++
drivers/remoteproc/remoteproc_sysfs.c | 4 ++++
2 files changed, 8 insertions(+)
diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
index b86c1d09c70c..2c44d375024e 100644
--- a/drivers/remoteproc/remoteproc_debugfs.c
+++ b/drivers/remoteproc/remoteproc_debugfs.c
@@ -226,10 +226,14 @@ rproc_recovery_write(struct file *filp, const char __user *user_buf,
if (!strncmp(buf, "enabled", count)) {
/* change the flag and begin the recovery process if needed */
+ mutex_lock(&rproc->lock);
rproc->recovery_disabled = false;
+ mutex_unlock(&rproc->lock);
rproc_trigger_recovery(rproc);
} else if (!strncmp(buf, "disabled", count)) {
+ mutex_lock(&rproc->lock);
rproc->recovery_disabled = true;
+ mutex_unlock(&rproc->lock);
} else if (!strncmp(buf, "recover", count)) {
/* begin the recovery process without changing the flag */
rproc_trigger_recovery(rproc);
diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index 8c7ea8922638..628e0de9a132 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -50,10 +50,14 @@ static ssize_t recovery_store(struct device *dev,
if (sysfs_streq(buf, "enabled")) {
/* change the flag and begin the recovery process if needed */
+ mutex_lock(&rproc->lock);
rproc->recovery_disabled = false;
+ mutex_unlock(&rproc->lock);
rproc_trigger_recovery(rproc);
} else if (sysfs_streq(buf, "disabled")) {
+ mutex_lock(&rproc->lock);
rproc->recovery_disabled = true;
+ mutex_unlock(&rproc->lock);
} else if (sysfs_streq(buf, "recover")) {
/* begin the recovery process without changing the flag */
rproc_trigger_recovery(rproc);
--
2.38.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] remoteproc: sysfs/debugfs: fix race while updating recovery flag
2023-02-01 5:46 [PATCH v2] remoteproc: sysfs/debugfs: fix race while updating recovery flag Satya Durga Srinivasu Prabhala
@ 2023-02-01 12:05 ` Mukesh Ojha
2023-02-07 2:12 ` Satya Durga Srinivasu Prabhala
2023-02-14 16:50 ` Bjorn Andersson
1 sibling, 1 reply; 4+ messages in thread
From: Mukesh Ojha @ 2023-02-01 12:05 UTC (permalink / raw)
To: Satya Durga Srinivasu Prabhala, andersson, mathieu.poirier
Cc: linux-remoteproc, linux-kernel, linux-arm-msm
On 2/1/2023 11:16 AM, Satya Durga Srinivasu Prabhala wrote:
> When multiple clients try to update the recovery flag, it is
> possible that, race condition would lead to undesired results
> as updates to recovery flag isn't protected by any mechanism
> today. To avoid such issues, take remoteproc mutex lock before
> updating recovery flag and release the lock once done.
>
> Signed-off-by: Satya Durga Srinivasu Prabhala <quic_satyap@quicinc.com>
LGTM.
Reviewed-by: Mukesh Ojha <quic_mojha@quicinc.com>
-Mukesh
> ---
> v1 -> v2:
> - addressed comments from Mukesh Ojha
> 1. take & release lock only while updating recovery flag
> 2. update debugfs
>
> drivers/remoteproc/remoteproc_debugfs.c | 4 ++++
> drivers/remoteproc/remoteproc_sysfs.c | 4 ++++
> 2 files changed, 8 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
> index b86c1d09c70c..2c44d375024e 100644
> --- a/drivers/remoteproc/remoteproc_debugfs.c
> +++ b/drivers/remoteproc/remoteproc_debugfs.c
> @@ -226,10 +226,14 @@ rproc_recovery_write(struct file *filp, const char __user *user_buf,
>
> if (!strncmp(buf, "enabled", count)) {
> /* change the flag and begin the recovery process if needed */
> + mutex_lock(&rproc->lock);
> rproc->recovery_disabled = false;
> + mutex_unlock(&rproc->lock);
> rproc_trigger_recovery(rproc);
> } else if (!strncmp(buf, "disabled", count)) {
> + mutex_lock(&rproc->lock);
> rproc->recovery_disabled = true;
> + mutex_unlock(&rproc->lock);
> } else if (!strncmp(buf, "recover", count)) {
> /* begin the recovery process without changing the flag */
> rproc_trigger_recovery(rproc);
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index 8c7ea8922638..628e0de9a132 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -50,10 +50,14 @@ static ssize_t recovery_store(struct device *dev,
>
> if (sysfs_streq(buf, "enabled")) {
> /* change the flag and begin the recovery process if needed */
> + mutex_lock(&rproc->lock);
> rproc->recovery_disabled = false;
> + mutex_unlock(&rproc->lock);
> rproc_trigger_recovery(rproc);
> } else if (sysfs_streq(buf, "disabled")) {
> + mutex_lock(&rproc->lock);
> rproc->recovery_disabled = true;
> + mutex_unlock(&rproc->lock);
> } else if (sysfs_streq(buf, "recover")) {
> /* begin the recovery process without changing the flag */
> rproc_trigger_recovery(rproc);
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] remoteproc: sysfs/debugfs: fix race while updating recovery flag
2023-02-01 12:05 ` Mukesh Ojha
@ 2023-02-07 2:12 ` Satya Durga Srinivasu Prabhala
0 siblings, 0 replies; 4+ messages in thread
From: Satya Durga Srinivasu Prabhala @ 2023-02-07 2:12 UTC (permalink / raw)
To: Mukesh Ojha, andersson, mathieu.poirier
Cc: linux-remoteproc, linux-kernel, linux-arm-msm
On 2/1/23 4:05 AM, Mukesh Ojha wrote:
>
>
> On 2/1/2023 11:16 AM, Satya Durga Srinivasu Prabhala wrote:
>> When multiple clients try to update the recovery flag, it is
>> possible that, race condition would lead to undesired results
>> as updates to recovery flag isn't protected by any mechanism
>> today. To avoid such issues, take remoteproc mutex lock before
>> updating recovery flag and release the lock once done.
>>
>> Signed-off-by: Satya Durga Srinivasu Prabhala <quic_satyap@quicinc.com>
>
> LGTM.
>
> Reviewed-by: Mukesh Ojha <quic_mojha@quicinc.com>
Thanks Mukesh.
Hi Bjorn,
Can you help cross check?
Best,
Satya
>
> -Mukesh
>> ---
>> v1 -> v2:
>> - addressed comments from Mukesh Ojha
>> 1. take & release lock only while updating recovery flag
>> 2. update debugfs
>>
>> drivers/remoteproc/remoteproc_debugfs.c | 4 ++++
>> drivers/remoteproc/remoteproc_sysfs.c | 4 ++++
>> 2 files changed, 8 insertions(+)
>>
>> diff --git a/drivers/remoteproc/remoteproc_debugfs.c
>> b/drivers/remoteproc/remoteproc_debugfs.c
>> index b86c1d09c70c..2c44d375024e 100644
>> --- a/drivers/remoteproc/remoteproc_debugfs.c
>> +++ b/drivers/remoteproc/remoteproc_debugfs.c
>> @@ -226,10 +226,14 @@ rproc_recovery_write(struct file *filp, const
>> char __user *user_buf,
>>
>> if (!strncmp(buf, "enabled", count)) {
>> /* change the flag and begin the recovery process if needed */
>> + mutex_lock(&rproc->lock);
>> rproc->recovery_disabled = false;
>> + mutex_unlock(&rproc->lock);
>> rproc_trigger_recovery(rproc);
>> } else if (!strncmp(buf, "disabled", count)) {
>> + mutex_lock(&rproc->lock);
>> rproc->recovery_disabled = true;
>> + mutex_unlock(&rproc->lock);
>> } else if (!strncmp(buf, "recover", count)) {
>> /* begin the recovery process without changing the flag */
>> rproc_trigger_recovery(rproc);
>> diff --git a/drivers/remoteproc/remoteproc_sysfs.c
>> b/drivers/remoteproc/remoteproc_sysfs.c
>> index 8c7ea8922638..628e0de9a132 100644
>> --- a/drivers/remoteproc/remoteproc_sysfs.c
>> +++ b/drivers/remoteproc/remoteproc_sysfs.c
>> @@ -50,10 +50,14 @@ static ssize_t recovery_store(struct device *dev,
>>
>> if (sysfs_streq(buf, "enabled")) {
>> /* change the flag and begin the recovery process if needed */
>> + mutex_lock(&rproc->lock);
>> rproc->recovery_disabled = false;
>> + mutex_unlock(&rproc->lock);
>> rproc_trigger_recovery(rproc);
>> } else if (sysfs_streq(buf, "disabled")) {
>> + mutex_lock(&rproc->lock);
>> rproc->recovery_disabled = true;
>> + mutex_unlock(&rproc->lock);
>> } else if (sysfs_streq(buf, "recover")) {
>> /* begin the recovery process without changing the flag */
>> rproc_trigger_recovery(rproc);
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] remoteproc: sysfs/debugfs: fix race while updating recovery flag
2023-02-01 5:46 [PATCH v2] remoteproc: sysfs/debugfs: fix race while updating recovery flag Satya Durga Srinivasu Prabhala
2023-02-01 12:05 ` Mukesh Ojha
@ 2023-02-14 16:50 ` Bjorn Andersson
1 sibling, 0 replies; 4+ messages in thread
From: Bjorn Andersson @ 2023-02-14 16:50 UTC (permalink / raw)
To: Satya Durga Srinivasu Prabhala
Cc: mathieu.poirier, linux-remoteproc, linux-kernel, linux-arm-msm
On Tue, Jan 31, 2023 at 09:46:08PM -0800, Satya Durga Srinivasu Prabhala wrote:
> When multiple clients try to update the recovery flag, it is
> possible that, race condition would lead to undesired results
> as updates to recovery flag isn't protected by any mechanism
> today. To avoid such issues, take remoteproc mutex lock before
> updating recovery flag and release the lock once done.
>
The only query of recovery_disabled that I can see is in
rproc_crash_handler_work(), outside of any lock. So I'm not able to see
the issue you're referring to.
Can you please help me understand better?
Thanks,
Bjorn
> Signed-off-by: Satya Durga Srinivasu Prabhala <quic_satyap@quicinc.com>
> ---
> v1 -> v2:
> - addressed comments from Mukesh Ojha
> 1. take & release lock only while updating recovery flag
> 2. update debugfs
>
> drivers/remoteproc/remoteproc_debugfs.c | 4 ++++
> drivers/remoteproc/remoteproc_sysfs.c | 4 ++++
> 2 files changed, 8 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
> index b86c1d09c70c..2c44d375024e 100644
> --- a/drivers/remoteproc/remoteproc_debugfs.c
> +++ b/drivers/remoteproc/remoteproc_debugfs.c
> @@ -226,10 +226,14 @@ rproc_recovery_write(struct file *filp, const char __user *user_buf,
>
> if (!strncmp(buf, "enabled", count)) {
> /* change the flag and begin the recovery process if needed */
> + mutex_lock(&rproc->lock);
> rproc->recovery_disabled = false;
> + mutex_unlock(&rproc->lock);
> rproc_trigger_recovery(rproc);
> } else if (!strncmp(buf, "disabled", count)) {
> + mutex_lock(&rproc->lock);
> rproc->recovery_disabled = true;
> + mutex_unlock(&rproc->lock);
> } else if (!strncmp(buf, "recover", count)) {
> /* begin the recovery process without changing the flag */
> rproc_trigger_recovery(rproc);
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index 8c7ea8922638..628e0de9a132 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -50,10 +50,14 @@ static ssize_t recovery_store(struct device *dev,
>
> if (sysfs_streq(buf, "enabled")) {
> /* change the flag and begin the recovery process if needed */
> + mutex_lock(&rproc->lock);
> rproc->recovery_disabled = false;
> + mutex_unlock(&rproc->lock);
> rproc_trigger_recovery(rproc);
> } else if (sysfs_streq(buf, "disabled")) {
> + mutex_lock(&rproc->lock);
> rproc->recovery_disabled = true;
> + mutex_unlock(&rproc->lock);
> } else if (sysfs_streq(buf, "recover")) {
> /* begin the recovery process without changing the flag */
> rproc_trigger_recovery(rproc);
> --
> 2.38.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-02-14 16:48 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-01 5:46 [PATCH v2] remoteproc: sysfs/debugfs: fix race while updating recovery flag Satya Durga Srinivasu Prabhala
2023-02-01 12:05 ` Mukesh Ojha
2023-02-07 2:12 ` Satya Durga Srinivasu Prabhala
2023-02-14 16:50 ` Bjorn Andersson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.