Re: [PATCH v2 2/3] remoteproc: Introduce deny_sysfs_ops flag
On 11/21/20 11:33 PM, Bjorn Andersson wrote:
> On Fri 20 Nov 21:44 CST 2020, Suman Anna wrote:
>
>> On 11/20/20 9:38 PM, Bjorn Andersson wrote:
>>> On Fri 20 Nov 21:01 CST 2020, 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'. The 'recovery'
sysfs file can also be used similarly to control the error recovery
state machine of a remoteproc. 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/recovery 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.
>>>
>>> This makes sense, but can't we implement attribute_group->is_visible to
>>> simply hide these entries from userspace instead of leaving them
>>> "broken"?
>>
>> I would have to look into that, but can that be changed dynamically?
>> Also, note that the enforcement is only on the writes/stores which impact
>> the state-machine, but not the reads/shows.
>>
>> For PRU usecases, we will be setting this dynamically.
>>
>
> It looks to be dynamic, but I don't know if there's any "caching"
> involved. Please have a look and let me know.
OK, will do. I can only check the week after though.
regards
Suman
>
> Regards,
> Bjorn
>
>> regards
>> Suman
>>
>>>
>>> Regards,
>>> Bjorn
>>>
Signed-off-by: Suman Anna
---
v2: revised to account for the 'recovery' sysfs file as well, patch
description updated accordingly
v1:
https://patchwork.kernel.org/project/linux-remoteproc/patch/20180915003725.17549-5-s-a...@ti.com/
drivers/remoteproc/remoteproc_sysfs.c | 12
include/linux/remoteproc.h| 2 ++
2 files changed, 14 insertions(+)
diff --git a/drivers/remoteproc/remoteproc_sysfs.c
b/drivers/remoteproc/remoteproc_sysfs.c
index bd2950a246c9..3fd18a71c188 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -49,6 +49,10 @@ static ssize_t recovery_store(struct device *dev,
{
struct rproc *rproc = to_rproc(dev);
+ /* restrict sysfs operations if not allowed by remoteproc drivers */
+ if (rproc->deny_sysfs_ops)
+ return -EPERM;
+
if (sysfs_streq(buf, "enabled")) {
/* change the flag and begin the recovery process if needed */
rproc->recovery_disabled = false;
@@ -158,6 +162,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(>lock);
if (err) {
dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, err);
@@ -225,6 +233,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 3fa3ba6498e8..dbc3767f7d0e 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -508,6 +508,7 @@ struct rproc_dump_segment {
* @has_iommu: flag to indicate if remote processor is behind an MMU
* @auto_boot: flag to indicate if remote processor should be auto-started
* @autonomous: true if an external entity has booted the remote processor
+ * @deny_sysfs_ops: flag to not permit sysfs operations on state,
firmware and recovery
* @dump_segments: list of segments in the firmware
* @nb_vdev: number of vdev currently handled by rproc
* @char_dev: character device of the rproc
@@ -545,6 +546,7 @@ struct rproc {
bool has_iommu;
bool auto_boot;
bool autonomous;
+ bool deny_sysfs_ops;
struct list_head dump_segments;
int nb_vdev;
u8 elf_class;
--
2.28.0
>>
Re: [PATCH v2 2/3] remoteproc: Introduce deny_sysfs_ops flag
On Fri 20 Nov 21:44 CST 2020, Suman Anna wrote:
> On 11/20/20 9:38 PM, Bjorn Andersson wrote:
> > On Fri 20 Nov 21:01 CST 2020, 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'. The 'recovery'
> >> sysfs file can also be used similarly to control the error recovery
> >> state machine of a remoteproc. 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/recovery 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.
> >>
> >
> > This makes sense, but can't we implement attribute_group->is_visible to
> > simply hide these entries from userspace instead of leaving them
> > "broken"?
>
> I would have to look into that, but can that be changed dynamically?
> Also, note that the enforcement is only on the writes/stores which impact
> the state-machine, but not the reads/shows.
>
> For PRU usecases, we will be setting this dynamically.
>
It looks to be dynamic, but I don't know if there's any "caching"
involved. Please have a look and let me know.
Regards,
Bjorn
> regards
> Suman
>
> >
> > Regards,
> > Bjorn
> >
> >> Signed-off-by: Suman Anna
> >> ---
> >> v2: revised to account for the 'recovery' sysfs file as well, patch
> >> description updated accordingly
> >> v1:
> >> https://patchwork.kernel.org/project/linux-remoteproc/patch/20180915003725.17549-5-s-a...@ti.com/
> >>
> >> drivers/remoteproc/remoteproc_sysfs.c | 12
> >> include/linux/remoteproc.h| 2 ++
> >> 2 files changed, 14 insertions(+)
> >>
> >> diff --git a/drivers/remoteproc/remoteproc_sysfs.c
> >> b/drivers/remoteproc/remoteproc_sysfs.c
> >> index bd2950a246c9..3fd18a71c188 100644
> >> --- a/drivers/remoteproc/remoteproc_sysfs.c
> >> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> >> @@ -49,6 +49,10 @@ static ssize_t recovery_store(struct device *dev,
> >> {
> >>struct rproc *rproc = to_rproc(dev);
> >>
> >> + /* restrict sysfs operations if not allowed by remoteproc drivers */
> >> + if (rproc->deny_sysfs_ops)
> >> + return -EPERM;
> >> +
> >>if (sysfs_streq(buf, "enabled")) {
> >>/* change the flag and begin the recovery process if needed */
> >>rproc->recovery_disabled = false;
> >> @@ -158,6 +162,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(>lock);
> >>if (err) {
> >>dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, err);
> >> @@ -225,6 +233,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 3fa3ba6498e8..dbc3767f7d0e 100644
> >> --- a/include/linux/remoteproc.h
> >> +++ b/include/linux/remoteproc.h
> >> @@ -508,6 +508,7 @@ struct rproc_dump_segment {
> >> * @has_iommu: flag to indicate if remote processor is behind an MMU
> >> * @auto_boot: flag to indicate if remote processor should be auto-started
> >> * @autonomous: true if an external entity has booted the remote processor
> >> + * @deny_sysfs_ops: flag to not permit sysfs operations on state,
> >> firmware and recovery
> >> * @dump_segments: list of segments in the firmware
> >> * @nb_vdev: number of vdev currently handled by rproc
> >> * @char_dev: character device of the rproc
> >> @@ -545,6 +546,7 @@ struct rproc {
> >>bool has_iommu;
> >>bool auto_boot;
> >>bool autonomous;
> >> + bool deny_sysfs_ops;
> >>struct list_head dump_segments;
> >>int nb_vdev;
> >>u8 elf_class;
> >> --
> >> 2.28.0
> >>
>
Re: [PATCH v2 2/3] remoteproc: Introduce deny_sysfs_ops flag
On 11/20/20 9:38 PM, Bjorn Andersson wrote:
> On Fri 20 Nov 21:01 CST 2020, 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'. The 'recovery'
>> sysfs file can also be used similarly to control the error recovery
>> state machine of a remoteproc. 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/recovery 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.
>>
>
> This makes sense, but can't we implement attribute_group->is_visible to
> simply hide these entries from userspace instead of leaving them
> "broken"?
I would have to look into that, but can that be changed dynamically?
Also, note that the enforcement is only on the writes/stores which impact
the state-machine, but not the reads/shows.
For PRU usecases, we will be setting this dynamically.
regards
Suman
>
> Regards,
> Bjorn
>
>> Signed-off-by: Suman Anna
>> ---
>> v2: revised to account for the 'recovery' sysfs file as well, patch
>> description updated accordingly
>> v1:
>> https://patchwork.kernel.org/project/linux-remoteproc/patch/20180915003725.17549-5-s-a...@ti.com/
>>
>> drivers/remoteproc/remoteproc_sysfs.c | 12
>> include/linux/remoteproc.h| 2 ++
>> 2 files changed, 14 insertions(+)
>>
>> diff --git a/drivers/remoteproc/remoteproc_sysfs.c
>> b/drivers/remoteproc/remoteproc_sysfs.c
>> index bd2950a246c9..3fd18a71c188 100644
>> --- a/drivers/remoteproc/remoteproc_sysfs.c
>> +++ b/drivers/remoteproc/remoteproc_sysfs.c
>> @@ -49,6 +49,10 @@ static ssize_t recovery_store(struct device *dev,
>> {
>> struct rproc *rproc = to_rproc(dev);
>>
>> +/* restrict sysfs operations if not allowed by remoteproc drivers */
>> +if (rproc->deny_sysfs_ops)
>> +return -EPERM;
>> +
>> if (sysfs_streq(buf, "enabled")) {
>> /* change the flag and begin the recovery process if needed */
>> rproc->recovery_disabled = false;
>> @@ -158,6 +162,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(>lock);
>> if (err) {
>> dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, err);
>> @@ -225,6 +233,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 3fa3ba6498e8..dbc3767f7d0e 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -508,6 +508,7 @@ struct rproc_dump_segment {
>> * @has_iommu: flag to indicate if remote processor is behind an MMU
>> * @auto_boot: flag to indicate if remote processor should be auto-started
>> * @autonomous: true if an external entity has booted the remote processor
>> + * @deny_sysfs_ops: flag to not permit sysfs operations on state, firmware
>> and recovery
>> * @dump_segments: list of segments in the firmware
>> * @nb_vdev: number of vdev currently handled by rproc
>> * @char_dev: character device of the rproc
>> @@ -545,6 +546,7 @@ struct rproc {
>> bool has_iommu;
>> bool auto_boot;
>> bool autonomous;
>> +bool deny_sysfs_ops;
>> struct list_head dump_segments;
>> int nb_vdev;
>> u8 elf_class;
>> --
>> 2.28.0
>>
Re: [PATCH v2 2/3] remoteproc: Introduce deny_sysfs_ops flag
On Fri 20 Nov 21:01 CST 2020, 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'. The 'recovery'
> sysfs file can also be used similarly to control the error recovery
> state machine of a remoteproc. 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/recovery 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.
>
This makes sense, but can't we implement attribute_group->is_visible to
simply hide these entries from userspace instead of leaving them
"broken"?
Regards,
Bjorn
> Signed-off-by: Suman Anna
> ---
> v2: revised to account for the 'recovery' sysfs file as well, patch
> description updated accordingly
> v1:
> https://patchwork.kernel.org/project/linux-remoteproc/patch/20180915003725.17549-5-s-a...@ti.com/
>
> drivers/remoteproc/remoteproc_sysfs.c | 12
> include/linux/remoteproc.h| 2 ++
> 2 files changed, 14 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c
> b/drivers/remoteproc/remoteproc_sysfs.c
> index bd2950a246c9..3fd18a71c188 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -49,6 +49,10 @@ static ssize_t recovery_store(struct device *dev,
> {
> struct rproc *rproc = to_rproc(dev);
>
> + /* restrict sysfs operations if not allowed by remoteproc drivers */
> + if (rproc->deny_sysfs_ops)
> + return -EPERM;
> +
> if (sysfs_streq(buf, "enabled")) {
> /* change the flag and begin the recovery process if needed */
> rproc->recovery_disabled = false;
> @@ -158,6 +162,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(>lock);
> if (err) {
> dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, err);
> @@ -225,6 +233,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 3fa3ba6498e8..dbc3767f7d0e 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -508,6 +508,7 @@ struct rproc_dump_segment {
> * @has_iommu: flag to indicate if remote processor is behind an MMU
> * @auto_boot: flag to indicate if remote processor should be auto-started
> * @autonomous: true if an external entity has booted the remote processor
> + * @deny_sysfs_ops: flag to not permit sysfs operations on state, firmware
> and recovery
> * @dump_segments: list of segments in the firmware
> * @nb_vdev: number of vdev currently handled by rproc
> * @char_dev: character device of the rproc
> @@ -545,6 +546,7 @@ struct rproc {
> bool has_iommu;
> bool auto_boot;
> bool autonomous;
> + bool deny_sysfs_ops;
> struct list_head dump_segments;
> int nb_vdev;
> u8 elf_class;
> --
> 2.28.0
>
