| Message ID | 20230201054609.14575-1-quic_satyap@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] remoteproc: sysfs/debugfs: fix race while updating recovery flag | expand |
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);
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);
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 >
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);
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(+)