[PATCH 0/5] remoteproc sysfs fixes/improvements

[PATCH 0/5] remoteproc sysfs fixes/improvements

[Suman Anna]

Hi Bjorn,

The following series includes couple of fixes and improvements
associated with the sysfs interfaces. Patches are based on top
of 4.19-rc1. The first 3 patches are independent of each other.

Following is the patch summary:
 - Patch 1 fixes an issue with unbalanced sysfs start/stop
   and module removal
 - Patch 2 adds a minor additional check on firmware name
   being echoed into the sysfs file
 - Patch 3 is a minor kerneldoc fix.
 - Patches 4 and 5 introduce a new request denial to sysfs
   interfaces to not allow userspace to meddle with a remoteproc
   client driven boot flow, and usage of it by Wkup M3 remoteproc
   driver

regards
Suman

Suman Anna (5):
  remoteproc: Fix unbalanced boot with sysfs for no auto-boot rprocs
  remoteproc: Check for NULL firmwares in sysfs interface
  remoteproc: Add missing kernel-doc comment for auto-boot
  remoteproc: Introduce deny_sysfs_ops flag
  remoteproc/wkup_m3: Set deny_sysfs_ops flag

 drivers/remoteproc/remoteproc_sysfs.c | 29 ++++++++++++++++++++++++++-
 drivers/remoteproc/wkup_m3_rproc.c    |  1 +
 include/linux/remoteproc.h            |  3 +++
 3 files changed, 32 insertions(+), 1 deletion(-)

-- 
2.18.0

[PATCH 1/5] remoteproc: Fix unbalanced boot with sysfs for no auto-boot rprocs

[Suman Anna]

The remoteproc core performs automatic boot and shutdown of a remote
processor during rproc_add() and rproc_del() for remote processors
supporting 'auto-boot'. The remoteproc devices not using 'auto-boot'
require either a remoteproc client driver or a userspace client to
use the sysfs 'state' variable to perform the boot and shutdown. The
in-kernel client drivers hold the corresponding remoteproc driver
module's reference count when they acquire a rproc handle through
the rproc_get_by_phandle() API, but there is no such support for
userspace applications performing the boot through sysfs interface.

The shutdown of a remoteproc upon removing a remoteproc platform
driver is automatic only with 'auto-boot' and this can cause a
remoteproc with no auto-boot to stay powered on and never freed
up if booted using the sysfs interface without a matching stop,
and when the remoteproc driver module is removed or unbound from
the device. This will result in a memory leak as well as the
corresponding remoteproc ida being never deallocated. Fix this
by holding a module reference count for the remoteproc's driver
during a sysfs 'start' and releasing it during the sysfs 'stop'
operation.

Signed-off-by: Suman Anna <s-anna@ti.com>
---
 drivers/remoteproc/remoteproc_sysfs.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index 47be411400e5..2142b3ea726e 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -11,6 +11,7 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/module.h>
 #include <linux/remoteproc.h>
 
 #include "remoteproc_internal.h"
@@ -100,14 +101,27 @@ static ssize_t state_store(struct device *dev,
 		if (rproc->state == RPROC_RUNNING)
 			return -EBUSY;
 
+		/*
+		 * prevent underlying implementation from being removed
+		 * when remoteproc does not support auto-boot
+		 */
+		if (!rproc->auto_boot &&
+		    !try_module_get(dev->parent->driver->owner))
+			return -EINVAL;
+
 		ret = rproc_boot(rproc);
-		if (ret)
+		if (ret) {
 			dev_err(&rproc->dev, "Boot failed: %d\n", ret);
+			if (!rproc->auto_boot)
+				module_put(dev->parent->driver->owner);
+		}
 	} else if (sysfs_streq(buf, "stop")) {
 		if (rproc->state != RPROC_RUNNING)
 			return -EINVAL;
 
 		rproc_shutdown(rproc);
+		if (!rproc->auto_boot)
+			module_put(dev->parent->driver->owner);
 	} else {
 		dev_err(&rproc->dev, "Unrecognised option: %s\n", buf);
 		ret = -EINVAL;
-- 
2.18.0

[PATCH 2/5] remoteproc: Check for NULL firmwares in sysfs interface

[Suman Anna]

The remoteproc framework provides a sysfs file 'firmware'
for modifying the firmware image name from userspace. Add
an additional check to ensure NULL firmwares are errored
out right away, rather than getting a delayed error while
requesting a firmware during the start of a remoteproc
later on.

Signed-off-by: Suman Anna <s-anna@ti.com>
---
 drivers/remoteproc/remoteproc_sysfs.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index 2142b3ea726e..ce93f4d710f3 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -49,6 +49,11 @@ static ssize_t firmware_store(struct device *dev,
 	}
 
 	len = strcspn(buf, "\n");
+	if (!len) {
+		dev_err(dev, "can't provide a NULL firmware\n");
+		err = -EINVAL;
+		goto out;
+	}
 
 	p = kstrndup(buf, len, GFP_KERNEL);
 	if (!p) {
-- 
2.18.0

[PATCH 3/5] remoteproc: Add missing kernel-doc comment for auto-boot

[Suman Anna]

The commit ddf711872c9d ("remoteproc: Introduce auto-boot flag")
introduced the auto-boot flag but missed adding the corresponding
kernel-doc comment. Add the same.

Signed-off-by: Suman Anna <s-anna@ti.com>
---
 include/linux/remoteproc.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index e3c5d856b6da..75f9ca05b865 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -439,6 +439,7 @@ struct rproc_dump_segment {
  * @cached_table: copy of the resource table
  * @table_sz: size of @cached_table
  * @has_iommu: flag to indicate if remote processor is behind an MMU
+ * @auto_boot: flag to indicate if remote processor should be auto-started
  * @dump_segments: list of segments in the firmware
  */
 struct rproc {
-- 
2.18.0

[PATCH 4/5] remoteproc: Introduce deny_sysfs_ops flag

[Suman Anna]

The remoteproc framework provides sysfs interfaces for changing
the firmware name and for starting/stopping a remote processor
through the sysfs files 'state' and 'firmware'. These interfaces
are currently allowed irrespective of how the remoteprocs were
booted (like remoteproc self auto-boot, remoteproc client-driven
boot etc). These interfaces can adversely affect a remoteproc
and its clients especially when a remoteproc is being controlled
by a remoteproc client driver(s). Also, not all remoteproc
drivers may want to support the sysfs interfaces by default.

Add support to deny the sysfs state/firmware change by introducing
a state flag 'deny_sysfs_ops' that the individual remoteproc drivers
can set based on their usage needs. The default behavior is to
allow the sysfs operations as before.

Signed-off-by: Suman Anna <s-anna@ti.com>
---
 drivers/remoteproc/remoteproc_sysfs.c | 8 ++++++++
 include/linux/remoteproc.h            | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index ce93f4d710f3..b2d8c11b89d0 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -36,6 +36,10 @@ static ssize_t firmware_store(struct device *dev,
 	char *p;
 	int err, len = count;
 
+	/* restrict sysfs operations if not allowed by remoteproc drivers */
+	if (rproc->deny_sysfs_ops)
+		return -EPERM;
+
 	err = mutex_lock_interruptible(&rproc->lock);
 	if (err) {
 		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, err);
@@ -102,6 +106,10 @@ static ssize_t state_store(struct device *dev,
 	struct rproc *rproc = to_rproc(dev);
 	int ret = 0;
 
+	/* restrict sysfs operations if not allowed by remoteproc drivers */
+	if (rproc->deny_sysfs_ops)
+		return -EPERM;
+
 	if (sysfs_streq(buf, "start")) {
 		if (rproc->state == RPROC_RUNNING)
 			return -EBUSY;
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 75f9ca05b865..d21252b4f758 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -440,6 +440,7 @@ struct rproc_dump_segment {
  * @table_sz: size of @cached_table
  * @has_iommu: flag to indicate if remote processor is behind an MMU
  * @auto_boot: flag to indicate if remote processor should be auto-started
+ * @deny_sysfs_ops: flag to not permit sysfs operations on state and firmware
  * @dump_segments: list of segments in the firmware
  */
 struct rproc {
@@ -472,6 +473,7 @@ struct rproc {
 	size_t table_sz;
 	bool has_iommu;
 	bool auto_boot;
+	bool deny_sysfs_ops;
 	struct list_head dump_segments;
 };
 
-- 
2.18.0

[PATCH 5/5] remoteproc/wkup_m3: Set deny_sysfs_ops flag

[Suman Anna]

The Wakeup M3 remote processor is controlled by the wkup_m3_ipc
client driver, so set the newly introduced 'deny_sysfs_ops' flag
to not allow any overriding of the remoteproc firmware or state
from userspace.

Signed-off-by: Suman Anna <s-anna@ti.com>
---
 drivers/remoteproc/wkup_m3_rproc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/remoteproc/wkup_m3_rproc.c b/drivers/remoteproc/wkup_m3_rproc.c
index 1ada0e51fef6..327757fd4d8f 100644
--- a/drivers/remoteproc/wkup_m3_rproc.c
+++ b/drivers/remoteproc/wkup_m3_rproc.c
@@ -168,6 +168,7 @@ static int wkup_m3_rproc_probe(struct platform_device *pdev)
 	}
 
 	rproc->auto_boot = false;
+	rproc->deny_sysfs_ops = true;
 
 	wkupm3 = rproc->priv;
 	wkupm3->rproc = rproc;
-- 
2.18.0

Re: [PATCH 0/5] remoteproc sysfs fixes/improvements

[Suman Anna]

Hi Bjorn,

On 09/14/2018 07:37 PM, Suman Anna wrote:
> Hi Bjorn,
> 
> The following series includes couple of fixes and improvements
> associated with the sysfs interfaces. Patches are based on top
> of 4.19-rc1. The first 3 patches are independent of each other.
> 
> Following is the patch summary:
>  - Patch 1 fixes an issue with unbalanced sysfs start/stop
>    and module removal
>  - Patch 2 adds a minor additional check on firmware name
>    being echoed into the sysfs file
>  - Patch 3 is a minor kerneldoc fix.
>  - Patches 4 and 5 introduce a new request denial to sysfs
>    interfaces to not allow userspace to meddle with a remoteproc
>    client driven boot flow, and usage of it by Wkup M3 remoteproc
>    driver

Can you pick up this series for 4.20 please if you don't have any comments?

regards
Suman

> 
> regards
> Suman
> 
> Suman Anna (5):
>   remoteproc: Fix unbalanced boot with sysfs for no auto-boot rprocs
>   remoteproc: Check for NULL firmwares in sysfs interface
>   remoteproc: Add missing kernel-doc comment for auto-boot
>   remoteproc: Introduce deny_sysfs_ops flag
>   remoteproc/wkup_m3: Set deny_sysfs_ops flag
> 
>  drivers/remoteproc/remoteproc_sysfs.c | 29 ++++++++++++++++++++++++++-
>  drivers/remoteproc/wkup_m3_rproc.c    |  1 +
>  include/linux/remoteproc.h            |  3 +++
>  3 files changed, 32 insertions(+), 1 deletion(-)
> 

Re: [PATCH 1/5] remoteproc: Fix unbalanced boot with sysfs for no auto-boot rprocs

[Arnaud Pouliquen]

Hi Suman,

On 09/15/2018 02:37 AM, Suman Anna wrote:
> The remoteproc core performs automatic boot and shutdown of a remote
> processor during rproc_add() and rproc_del() for remote processors
> supporting 'auto-boot'. The remoteproc devices not using 'auto-boot'
> require either a remoteproc client driver or a userspace client to
> use the sysfs 'state' variable to perform the boot and shutdown. The
> in-kernel client drivers hold the corresponding remoteproc driver
> module's reference count when they acquire a rproc handle through
> the rproc_get_by_phandle() API, but there is no such support for
> userspace applications performing the boot through sysfs interface.
> 
> The shutdown of a remoteproc upon removing a remoteproc platform
> driver is automatic only with 'auto-boot' and this can cause a
> remoteproc with no auto-boot to stay powered on and never freed
> up if booted using the sysfs interface without a matching stop,
> and when the remoteproc driver module is removed or unbound from
> the device. This will result in a memory leak as well as the
> corresponding remoteproc ida being never deallocated. Fix this
> by holding a module reference count for the remoteproc's driver
> during a sysfs 'start' and releasing it during the sysfs 'stop'
> operation.
> 
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
>  drivers/remoteproc/remoteproc_sysfs.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index 47be411400e5..2142b3ea726e 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -11,6 +11,7 @@
>   * GNU General Public License for more details.
>   */
>  
> +#include <linux/module.h>
>  #include <linux/remoteproc.h>
>  
>  #include "remoteproc_internal.h"
> @@ -100,14 +101,27 @@ static ssize_t state_store(struct device *dev,
>  		if (rproc->state == RPROC_RUNNING)
>  			return -EBUSY;
>  
> +		/*
> +		 * prevent underlying implementation from being removed
> +		 * when remoteproc does not support auto-boot
> +		 */
> +		if (!rproc->auto_boot &&
> +		    !try_module_get(dev->parent->driver->owner))
> +			return -EINVAL;
> +
>  		ret = rproc_boot(rproc);
> -		if (ret)
> +		if (ret) {
>  			dev_err(&rproc->dev, "Boot failed: %d\n", ret);
> +			if (!rproc->auto_boot)
> +				module_put(dev->parent->driver->owner);
> +		}
>  	} else if (sysfs_streq(buf, "stop")) {
>  		if (rproc->state != RPROC_RUNNING)
>  			return -EINVAL;
>  
>  		rproc_shutdown(rproc);
> +		if (!rproc->auto_boot)
> +			module_put(dev->parent->driver->owner);
>  	} else {
>  		dev_err(&rproc->dev, "Unrecognised option: %s\n", buf);
>  		ret = -EINVAL;
> 
Looks good for me, i tested on ST platform and did not detect any
regression.
Acked-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>

Regards
Arnaud

Re: [PATCH 2/5] remoteproc: Check for NULL firmwares in sysfs interface

[Arnaud Pouliquen]

Hi Suman,

On 09/15/2018 02:37 AM, Suman Anna wrote:
> The remoteproc framework provides a sysfs file 'firmware'
> for modifying the firmware image name from userspace. Add
> an additional check to ensure NULL firmwares are errored
> out right away, rather than getting a delayed error while
> requesting a firmware during the start of a remoteproc
> later on.
> 
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
>  drivers/remoteproc/remoteproc_sysfs.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index 2142b3ea726e..ce93f4d710f3 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -49,6 +49,11 @@ static ssize_t firmware_store(struct device *dev,
>  	}
>  
>  	len = strcspn(buf, "\n");
> +	if (!len) {
> +		dev_err(dev, "can't provide a NULL firmware\n");
> +		err = -EINVAL;
> +		goto out;
> +	}
This patch fixes only the case of a null name but not a bad name. So I'm
not one hundred percent sure that it is relevant, as it treats only a
part of the problem. But it's fine with me if it is accepted.

Tested-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>

Regards
Arnaud

>  
>  	p = kstrndup(buf, len, GFP_KERNEL);
>  	if (!p) {
> 

Re: [PATCH 4/5] remoteproc: Introduce deny_sysfs_ops flag

[Arnaud Pouliquen]

Hi Suman,

On 09/15/2018 02:37 AM, Suman Anna wrote:
> The remoteproc framework provides sysfs interfaces for changing
> the firmware name and for starting/stopping a remote processor
> through the sysfs files 'state' and 'firmware'. These interfaces
> are currently allowed irrespective of how the remoteprocs were
> booted (like remoteproc self auto-boot, remoteproc client-driven
> boot etc). These interfaces can adversely affect a remoteproc
> and its clients especially when a remoteproc is being controlled
> by a remoteproc client driver(s). Also, not all remoteproc
> drivers may want to support the sysfs interfaces by default.
> 
> Add support to deny the sysfs state/firmware change by introducing
> a state flag 'deny_sysfs_ops' that the individual remoteproc drivers
> can set based on their usage needs. The default behavior is to
> allow the sysfs operations as before.
> 
> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
>  drivers/remoteproc/remoteproc_sysfs.c | 8 ++++++++
>  include/linux/remoteproc.h            | 2 ++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index ce93f4d710f3..b2d8c11b89d0 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -36,6 +36,10 @@ static ssize_t firmware_store(struct device *dev,
>  	char *p;
>  	int err, len = count;
>  
> +	/* restrict sysfs operations if not allowed by remoteproc drivers */
> +	if (rproc->deny_sysfs_ops)
> +		return -EPERM;
> +
>  	err = mutex_lock_interruptible(&rproc->lock);
>  	if (err) {
>  		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, err);
> @@ -102,6 +106,10 @@ static ssize_t state_store(struct device *dev,
>  	struct rproc *rproc = to_rproc(dev);
>  	int ret = 0;
>  
> +	/* restrict sysfs operations if not allowed by remoteproc drivers */
> +	if (rproc->deny_sysfs_ops)
> +		return -EPERM;
> +
>  	if (sysfs_streq(buf, "start")) {
>  		if (rproc->state == RPROC_RUNNING)
>  			return -EBUSY;
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 75f9ca05b865..d21252b4f758 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -440,6 +440,7 @@ struct rproc_dump_segment {
>   * @table_sz: size of @cached_table
>   * @has_iommu: flag to indicate if remote processor is behind an MMU
>   * @auto_boot: flag to indicate if remote processor should be auto-started
> + * @deny_sysfs_ops: flag to not permit sysfs operations on state and firmware
>   * @dump_segments: list of segments in the firmware
>   */
>  struct rproc {
> @@ -472,6 +473,7 @@ struct rproc {
>  	size_t table_sz;
>  	bool has_iommu;
>  	bool auto_boot;
> +	bool deny_sysfs_ops;
>  	struct list_head dump_segments;
>  };

I'm concerned about granularity. Are we denying all write access to the
state and the firmware name?
Would it be interesting to have a bit-field to separately allow/deny
write access:
- to change the firmware name
- to start the firmware
- to stop the firmware
?

For instance, if firmware is stored in the file system, the auto_boot
mode is not functional (if remote proc in built-in). We could have to
allow user application to start the firmware, but deny to change the
firmware name or stop it.

Regards
Arnaud

>  
> 

Re: [PATCH 2/5] remoteproc: Check for NULL firmwares in sysfs interface

[Suman Anna]

Hi Arnaud,

On 10/02/2018 04:43 AM, Arnaud Pouliquen wrote:
> Hi Suman,
> 
> On 09/15/2018 02:37 AM, Suman Anna wrote:
>> The remoteproc framework provides a sysfs file 'firmware'
>> for modifying the firmware image name from userspace. Add
>> an additional check to ensure NULL firmwares are errored
>> out right away, rather than getting a delayed error while
>> requesting a firmware during the start of a remoteproc
>> later on.
>>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> ---
>>  drivers/remoteproc/remoteproc_sysfs.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
>> index 2142b3ea726e..ce93f4d710f3 100644
>> --- a/drivers/remoteproc/remoteproc_sysfs.c
>> +++ b/drivers/remoteproc/remoteproc_sysfs.c
>> @@ -49,6 +49,11 @@ static ssize_t firmware_store(struct device *dev,
>>  	}
>>  
>>  	len = strcspn(buf, "\n");
>> +	if (!len) {
>> +		dev_err(dev, "can't provide a NULL firmware\n");
>> +		err = -EINVAL;
>> +		goto out;
>> +	}

> This patch fixes only the case of a null name but not a bad name. So I'm
> not one hundred percent sure that it is relevant, as it treats only a
> part of the problem. But it's fine with me if it is accepted.

Yeah, it is a minor fix catching only NULL names. There is no way for
the interface to know a bad name until the corresponding firmware is
requested.

regards
Suman

> 
> Tested-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> 
> Regards
> Arnaud
> 
>>  
>>  	p = kstrndup(buf, len, GFP_KERNEL);
>>  	if (!p) {
>>

Re: [PATCH 4/5] remoteproc: Introduce deny_sysfs_ops flag

[Suman Anna]

Hi Arnaud,

Thanks for the review and testing of the series.

On 10/02/2018 04:47 AM, Arnaud Pouliquen wrote:
> Hi Suman,
> 
> On 09/15/2018 02:37 AM, Suman Anna wrote:
>> The remoteproc framework provides sysfs interfaces for changing
>> the firmware name and for starting/stopping a remote processor
>> through the sysfs files 'state' and 'firmware'. These interfaces
>> are currently allowed irrespective of how the remoteprocs were
>> booted (like remoteproc self auto-boot, remoteproc client-driven
>> boot etc). These interfaces can adversely affect a remoteproc
>> and its clients especially when a remoteproc is being controlled
>> by a remoteproc client driver(s). Also, not all remoteproc
>> drivers may want to support the sysfs interfaces by default.
>>
>> Add support to deny the sysfs state/firmware change by introducing
>> a state flag 'deny_sysfs_ops' that the individual remoteproc drivers
>> can set based on their usage needs. The default behavior is to
>> allow the sysfs operations as before.
>>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> ---
>>  drivers/remoteproc/remoteproc_sysfs.c | 8 ++++++++
>>  include/linux/remoteproc.h            | 2 ++
>>  2 files changed, 10 insertions(+)
>>
>> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
>> index ce93f4d710f3..b2d8c11b89d0 100644
>> --- a/drivers/remoteproc/remoteproc_sysfs.c
>> +++ b/drivers/remoteproc/remoteproc_sysfs.c
>> @@ -36,6 +36,10 @@ static ssize_t firmware_store(struct device *dev,
>>  	char *p;
>>  	int err, len = count;
>>  
>> +	/* restrict sysfs operations if not allowed by remoteproc drivers */
>> +	if (rproc->deny_sysfs_ops)
>> +		return -EPERM;
>> +
>>  	err = mutex_lock_interruptible(&rproc->lock);
>>  	if (err) {
>>  		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, err);
>> @@ -102,6 +106,10 @@ static ssize_t state_store(struct device *dev,
>>  	struct rproc *rproc = to_rproc(dev);
>>  	int ret = 0;
>>  
>> +	/* restrict sysfs operations if not allowed by remoteproc drivers */
>> +	if (rproc->deny_sysfs_ops)
>> +		return -EPERM;
>> +
>>  	if (sysfs_streq(buf, "start")) {
>>  		if (rproc->state == RPROC_RUNNING)
>>  			return -EBUSY;
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index 75f9ca05b865..d21252b4f758 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -440,6 +440,7 @@ struct rproc_dump_segment {
>>   * @table_sz: size of @cached_table
>>   * @has_iommu: flag to indicate if remote processor is behind an MMU
>>   * @auto_boot: flag to indicate if remote processor should be auto-started
>> + * @deny_sysfs_ops: flag to not permit sysfs operations on state and firmware
>>   * @dump_segments: list of segments in the firmware
>>   */
>>  struct rproc {
>> @@ -472,6 +473,7 @@ struct rproc {
>>  	size_t table_sz;
>>  	bool has_iommu;
>>  	bool auto_boot;
>> +	bool deny_sysfs_ops;
>>  	struct list_head dump_segments;
>>  };
> 
> I'm concerned about granularity. Are we denying all write access to the
> state and the firmware name?

Not by default. This is relevant only when a remoteproc platform driver
has configured these fields.

> Would it be interesting to have a bit-field to separately allow/deny
> write access:
> - to change the firmware name
> - to start the firmware
> - to stop the firmware
> ?

Do you have use-cases for the individual options?

> 
> For instance, if firmware is stored in the file system, the auto_boot
> mode is not functional (if remote proc in built-in).

This has to do with late FS init, the auto-boot would work if you have
your images in early FS like initramfs or initramdisk with built-in
driver (this is not a remoteproc specific problem and is the case with
all drivers using request_firmware()).

 We could have to
> allow user application to start the firmware, but deny to change the
> firmware name or stop it.

I am not sure if we want to differentiate as the above example almost
kinda depends on how you put together the system. It can be easily
expanded for what you are asking if there is a real need/use-case.

regards
Suman

Re: [PATCH 1/5] remoteproc: Fix unbalanced boot with sysfs for no auto-boot rprocs

[Bjorn Andersson]

On Fri 14 Sep 17:37 PDT 2018, Suman Anna wrote:

> The remoteproc core performs automatic boot and shutdown of a remote
> processor during rproc_add() and rproc_del() for remote processors
> supporting 'auto-boot'. The remoteproc devices not using 'auto-boot'
> require either a remoteproc client driver or a userspace client to
> use the sysfs 'state' variable to perform the boot and shutdown. The
> in-kernel client drivers hold the corresponding remoteproc driver
> module's reference count when they acquire a rproc handle through
> the rproc_get_by_phandle() API, but there is no such support for
> userspace applications performing the boot through sysfs interface.
> 
> The shutdown of a remoteproc upon removing a remoteproc platform
> driver is automatic only with 'auto-boot' and this can cause a
> remoteproc with no auto-boot to stay powered on and never freed
> up if booted using the sysfs interface without a matching stop,
> and when the remoteproc driver module is removed or unbound from
> the device. This will result in a memory leak as well as the
> corresponding remoteproc ida being never deallocated. Fix this
> by holding a module reference count for the remoteproc's driver
> during a sysfs 'start' and releasing it during the sysfs 'stop'
> operation.
> 

This prevents you from rmmod'ing the remoteproc driver, but it does not
prevent you from issuing an unbind of the driver - resulting in the same
issue.

I would prefer if we made sure that rproc_del() always cleaned up any
resources (and stopped the remoteproc processor), but I'm uncertain of
how to deal with remote processors that are supposed to survive Linux
shutting down.

But I'm also uncertain how we can make the remoteproc core ensure that
no dynamic resources are leaked in such scenario.

Regards,
Bjorn

> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
>  drivers/remoteproc/remoteproc_sysfs.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index 47be411400e5..2142b3ea726e 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -11,6 +11,7 @@
>   * GNU General Public License for more details.
>   */
>  
> +#include <linux/module.h>
>  #include <linux/remoteproc.h>
>  
>  #include "remoteproc_internal.h"
> @@ -100,14 +101,27 @@ static ssize_t state_store(struct device *dev,
>  		if (rproc->state == RPROC_RUNNING)
>  			return -EBUSY;
>  
> +		/*
> +		 * prevent underlying implementation from being removed
> +		 * when remoteproc does not support auto-boot
> +		 */
> +		if (!rproc->auto_boot &&
> +		    !try_module_get(dev->parent->driver->owner))
> +			return -EINVAL;
> +
>  		ret = rproc_boot(rproc);
> -		if (ret)
> +		if (ret) {
>  			dev_err(&rproc->dev, "Boot failed: %d\n", ret);
> +			if (!rproc->auto_boot)
> +				module_put(dev->parent->driver->owner);
> +		}
>  	} else if (sysfs_streq(buf, "stop")) {
>  		if (rproc->state != RPROC_RUNNING)
>  			return -EINVAL;
>  
>  		rproc_shutdown(rproc);
> +		if (!rproc->auto_boot)
> +			module_put(dev->parent->driver->owner);
>  	} else {
>  		dev_err(&rproc->dev, "Unrecognised option: %s\n", buf);
>  		ret = -EINVAL;
> -- 
> 2.18.0
> 

Re: [PATCH 2/5] remoteproc: Check for NULL firmwares in sysfs interface

[Bjorn Andersson]

On Fri 14 Sep 17:37 PDT 2018, Suman Anna wrote:

> The remoteproc framework provides a sysfs file 'firmware'
> for modifying the firmware image name from userspace. Add
> an additional check to ensure NULL firmwares are errored
> out right away, rather than getting a delayed error while
> requesting a firmware during the start of a remoteproc
> later on.
> 
> Signed-off-by: Suman Anna <s-anna@ti.com>

Applied.

Regards,
Bjorn

> ---
>  drivers/remoteproc/remoteproc_sysfs.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index 2142b3ea726e..ce93f4d710f3 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -49,6 +49,11 @@ static ssize_t firmware_store(struct device *dev,
>  	}
>  
>  	len = strcspn(buf, "\n");
> +	if (!len) {
> +		dev_err(dev, "can't provide a NULL firmware\n");
> +		err = -EINVAL;
> +		goto out;
> +	}
>  
>  	p = kstrndup(buf, len, GFP_KERNEL);
>  	if (!p) {
> -- 
> 2.18.0
> 

Re: [PATCH 3/5] remoteproc: Add missing kernel-doc comment for auto-boot

[Bjorn Andersson]

On Fri 14 Sep 17:37 PDT 2018, Suman Anna wrote:

> The commit ddf711872c9d ("remoteproc: Introduce auto-boot flag")
> introduced the auto-boot flag but missed adding the corresponding
> kernel-doc comment. Add the same.
> 
> Signed-off-by: Suman Anna <s-anna@ti.com>

Applied.

Thanks,
Bjorn

> ---
>  include/linux/remoteproc.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index e3c5d856b6da..75f9ca05b865 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -439,6 +439,7 @@ struct rproc_dump_segment {
>   * @cached_table: copy of the resource table
>   * @table_sz: size of @cached_table
>   * @has_iommu: flag to indicate if remote processor is behind an MMU
> + * @auto_boot: flag to indicate if remote processor should be auto-started
>   * @dump_segments: list of segments in the firmware
>   */
>  struct rproc {
> -- 
> 2.18.0
> 

Re: [PATCH 1/5] remoteproc: Fix unbalanced boot with sysfs for no auto-boot rprocs

[Suman Anna]

On 10/06/2018 01:13 AM, Bjorn Andersson wrote:
> On Fri 14 Sep 17:37 PDT 2018, Suman Anna wrote:
> 
>> The remoteproc core performs automatic boot and shutdown of a
>> remote processor during rproc_add() and rproc_del() for remote
>> processors supporting 'auto-boot'. The remoteproc devices not using
>> 'auto-boot' require either a remoteproc client driver or a
>> userspace client to use the sysfs 'state' variable to perform the
>> boot and shutdown. The in-kernel client drivers hold the
>> corresponding remoteproc driver module's reference count when they
>> acquire a rproc handle through the rproc_get_by_phandle() API, but
>> there is no such support for userspace applications performing the
>> boot through sysfs interface.
>> 
>> The shutdown of a remoteproc upon removing a remoteproc platform 
>> driver is automatic only with 'auto-boot' and this can cause a 
>> remoteproc with no auto-boot to stay powered on and never freed up
>> if booted using the sysfs interface without a matching stop, and
>> when the remoteproc driver module is removed or unbound from the
>> device. This will result in a memory leak as well as the 
>> corresponding remoteproc ida being never deallocated. Fix this by
>> holding a module reference count for the remoteproc's driver during
>> a sysfs 'start' and releasing it during the sysfs 'stop' 
>> operation.
>> 
> 
> This prevents you from rmmod'ing the remoteproc driver, but it does
> not prevent you from issuing an unbind of the driver - resulting in
> the same issue.

Well, the unbind is always a problem as it ignores the module usecounts,
and that's a generic issue. I have used suppress_bind_attrs in the
relevant remoteproc drivers downstream (need to actually add that for
wkup_m3) when they are being booted by other clients, otherwise more
often than not the client drivers create a kernel crash.

> 
> I would prefer if we made sure that rproc_del() always cleaned up
> any resources (and stopped the remoteproc processor), but I'm
> uncertain of how to deal with remote processors that are supposed to
> survive Linux shutting down.

This creates unbalanced paths and we definitely do not want stop a
processor from underneath the client drivers that have booted them.
Hence this patch which creates parity with auto-booted processors and
still requested by some other clients.

regards
Suman

> 
> But I'm also uncertain how we can make the remoteproc core ensure
> that no dynamic resources are leaked in such scenario.
> 
> Regards, Bjorn
> 
>> Signed-off-by: Suman Anna <s-anna@ti.com> --- 
>> drivers/remoteproc/remoteproc_sysfs.c | 16 +++++++++++++++- 1 file
>> changed, 15 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/remoteproc/remoteproc_sysfs.c
>> b/drivers/remoteproc/remoteproc_sysfs.c index
>> 47be411400e5..2142b3ea726e 100644 ---
>> a/drivers/remoteproc/remoteproc_sysfs.c +++
>> b/drivers/remoteproc/remoteproc_sysfs.c @@ -11,6 +11,7 @@ * GNU
>> General Public License for more details. */
>> 
>> +#include <linux/module.h> #include <linux/remoteproc.h>
>> 
>> #include "remoteproc_internal.h" @@ -100,14 +101,27 @@ static
>> ssize_t state_store(struct device *dev, if (rproc->state ==
>> RPROC_RUNNING) return -EBUSY;
>> 
>> +		/* +		 * prevent underlying implementation from being removed +
>> * when remoteproc does not support auto-boot +		 */ +		if
>> (!rproc->auto_boot && +
>> !try_module_get(dev->parent->driver->owner)) +			return -EINVAL; + 
>> ret = rproc_boot(rproc); -		if (ret) +		if (ret) { 
>> dev_err(&rproc->dev, "Boot failed: %d\n", ret); +			if
>> (!rproc->auto_boot) +				module_put(dev->parent->driver->owner); +
>> } } else if (sysfs_streq(buf, "stop")) { if (rproc->state !=
>> RPROC_RUNNING) return -EINVAL;
>> 
>> rproc_shutdown(rproc); +		if (!rproc->auto_boot) +
>> module_put(dev->parent->driver->owner); } else { 
>> dev_err(&rproc->dev, "Unrecognised option: %s\n", buf); ret =
>> -EINVAL; -- 2.18.0
>>