remoteproc: sysfs/debugfs: fix race while updating recovery flag

[PATCH v2] remoteproc: sysfs/debugfs: fix race while updating recovery flag

Posted by Satya Durga Srinivasu Prabhala 2 years, 7 months ago

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

Re: [PATCH v2] remoteproc: sysfs/debugfs: fix race while updating recovery flag

Posted by Bjorn Andersson 2 years, 7 months ago

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
>

Re: [PATCH v2] remoteproc: sysfs/debugfs: fix race while updating recovery flag

Posted by Mukesh Ojha 2 years, 7 months ago


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);

Re: [PATCH v2] remoteproc: sysfs/debugfs: fix race while updating recovery flag

Posted by Satya Durga Srinivasu Prabhala 2 years, 7 months ago


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);