The first patch puts the overlays as objects in the sysfs in
/sys/firmware/devicetree/overlays.
The next adds a master overlay enable switch (that once is set to
disabled can't be re-enabled), while the one after that
introduces a number of default per overlay attributes.
The patchset is against linus's tree as of today.
The last patch updates the ABI docs for the sysfs entries.
Changes since v2:
* Removed the unittest patch.
* Split the sysfs attribute patch to a global and a per-overlay
patch.
* Dropped binary attributes using textual kobj_attributes instead.
Changes since v1:
* Maintainer requested changes.
* Documented the sysfs entries
* Per overlay sysfs attributes.
Pantelis Antoniou (4):
of: overlay: kobjectify overlay objects
of: overlay: global sysfs enable attribute
of: overlay: add per overlay sysfs attributes
Documentation: ABI: /sys/firmware/devicetree/overlays
.../ABI/testing/sysfs-firmware-devicetree-overlays | 23 ++++
drivers/of/base.c | 5 +
drivers/of/of_private.h | 9 ++
drivers/of/overlay.c | 148 ++++++++++++++++++++-
4 files changed, 183 insertions(+), 2 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
--
1.7.12
We are going to need the overlays to appear on sysfs with runtime
global properties (like master enable) so turn them into kobjects.
Signed-off-by: Pantelis Antoniou <[email protected]>
---
drivers/of/base.c | 5 +++++
drivers/of/of_private.h | 9 +++++++++
drivers/of/overlay.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++--
3 files changed, 64 insertions(+), 2 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index a1aa0c7..7a2b46c 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -192,6 +192,7 @@ int __of_attach_node_sysfs(struct device_node *np)
static int __init of_init(void)
{
struct device_node *np;
+ int ret;
/* Create the kset, and register existing nodes */
mutex_lock(&of_mutex);
@@ -208,6 +209,10 @@ static int __init of_init(void)
if (of_root)
proc_symlink("device-tree", NULL, "/sys/firmware/devicetree/base");
+ ret = of_overlay_init();
+ if (ret != 0)
+ pr_warn("of_init: of_overlay_init failed!\n");
+
return 0;
}
core_initcall(of_init);
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 8e882e7..120eb44 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -90,4 +90,13 @@ extern void __of_detach_node_sysfs(struct device_node *np);
#define for_each_transaction_entry_reverse(_oft, _te) \
list_for_each_entry_reverse(_te, &(_oft)->te_list, node)
+#if defined(CONFIG_OF_OVERLAY)
+extern int of_overlay_init(void);
+#else
+static inline int of_overlay_init(void)
+{
+ return 0;
+}
+#endif
+
#endif /* _LINUX_OF_PRIVATE_H */
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index dee9270..f17f5ef 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -20,6 +20,7 @@
#include <linux/slab.h>
#include <linux/err.h>
#include <linux/idr.h>
+#include <linux/sysfs.h>
#include "of_private.h"
@@ -51,6 +52,7 @@ struct of_overlay {
int count;
struct of_overlay_info *ovinfo_tab;
struct of_changeset cset;
+ struct kobject kobj;
};
static int of_overlay_apply_one(struct of_overlay *ov,
@@ -325,6 +327,24 @@ static int of_free_overlay_info(struct of_overlay *ov)
static LIST_HEAD(ov_list);
static DEFINE_IDR(ov_idr);
+static inline struct of_overlay *kobj_to_overlay(struct kobject *kobj)
+{
+ return container_of(kobj, struct of_overlay, kobj);
+}
+
+void of_overlay_release(struct kobject *kobj)
+{
+ struct of_overlay *ov = kobj_to_overlay(kobj);
+
+ kfree(ov);
+}
+
+static struct kobj_type of_overlay_ktype = {
+ .release = of_overlay_release,
+};
+
+static struct kset *ov_kset;
+
/**
* of_overlay_create() - Create and apply an overlay
* @tree: Device node containing all the overlays
@@ -350,6 +370,9 @@ int of_overlay_create(struct device_node *tree)
of_changeset_init(&ov->cset);
+ /* initialize kobject */
+ kobject_init(&ov->kobj, &of_overlay_ktype);
+
mutex_lock(&of_mutex);
id = idr_alloc(&ov_idr, ov, 0, 0, GFP_KERNEL);
@@ -385,6 +408,14 @@ int of_overlay_create(struct device_node *tree)
goto err_revert_overlay;
}
+ ov->kobj.kset = ov_kset;
+ err = kobject_add(&ov->kobj, NULL, "%d", id);
+ if (err != 0) {
+ pr_err("%s: kobject_add() failed for tree@%s\n",
+ __func__, tree->full_name);
+ goto err_cancel_overlay;
+ }
+
/* add to the tail of the overlay list */
list_add_tail(&ov->node, &ov_list);
@@ -392,6 +423,8 @@ int of_overlay_create(struct device_node *tree)
return id;
+err_cancel_overlay:
+ of_changeset_revert(&ov->cset);
err_revert_overlay:
err_abort_trans:
of_free_overlay_info(ov);
@@ -512,7 +545,9 @@ int of_overlay_destroy(int id)
of_free_overlay_info(ov);
idr_remove(&ov_idr, id);
of_changeset_destroy(&ov->cset);
- kfree(ov);
+
+ kobject_del(&ov->kobj);
+ kobject_put(&ov->kobj);
err = 0;
@@ -542,7 +577,8 @@ int of_overlay_destroy_all(void)
of_changeset_revert(&ov->cset);
of_free_overlay_info(ov);
idr_remove(&ov_idr, ov->id);
- kfree(ov);
+ kobject_del(&ov->kobj);
+ kobject_put(&ov->kobj);
}
mutex_unlock(&of_mutex);
@@ -550,3 +586,15 @@ int of_overlay_destroy_all(void)
return 0;
}
EXPORT_SYMBOL_GPL(of_overlay_destroy_all);
+
+/* called from of_init() */
+int of_overlay_init(void)
+{
+ int rc;
+
+ ov_kset = kset_create_and_add("overlays", NULL, &of_kset->kobj);
+ if (!ov_kset)
+ return -ENOMEM;
+
+ return 0;
+}
--
1.7.12
A throw once master enable switch to protect against any
further overlay applications if the administrator desires so.
Signed-off-by: Pantelis Antoniou <[email protected]>
---
drivers/of/overlay.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 44 insertions(+), 1 deletion(-)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index f17f5ef..c335809 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -21,6 +21,7 @@
#include <linux/err.h>
#include <linux/idr.h>
#include <linux/sysfs.h>
+#include <linux/atomic.h>
#include "of_private.h"
@@ -55,8 +56,12 @@ struct of_overlay {
struct kobject kobj;
};
+/* master enable switch; once set to 0 can't be re-enabled */
+static atomic_t ov_enable = ATOMIC_INIT(1);
+
static int of_overlay_apply_one(struct of_overlay *ov,
struct device_node *target, const struct device_node *overlay);
+static int overlay_removal_is_ok(struct of_overlay *ov);
static int of_overlay_apply_single_property(struct of_overlay *ov,
struct device_node *target, struct property *prop)
@@ -339,6 +344,37 @@ void of_overlay_release(struct kobject *kobj)
kfree(ov);
}
+static ssize_t enable_show(struct kobject *kobj,
+ struct kobj_attribute *attr, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, "%d\n", atomic_read(&ov_enable));
+}
+
+static ssize_t enable_store(struct kobject *kobj,
+ struct kobj_attribute *attr, const char *buf, size_t count)
+{
+ int ret;
+ long new_enable;
+
+ ret = kstrtol(buf, 10, &new_enable);
+ if (ret != 0)
+ return ret;
+ if ((unsigned long)new_enable > 1)
+ return -EINVAL;
+ /* if we've disabled it, no going back */
+ if (atomic_read(&ov_enable) == 0)
+ return -EPERM;
+ atomic_set(&ov_enable, (int)new_enable);
+ return count;
+}
+
+static struct kobj_attribute enable_attr = __ATTR_RW(enable);
+
+static const struct attribute *overlay_global_attrs[] = {
+ &enable_attr.attr,
+ NULL
+};
+
static struct kobj_type of_overlay_ktype = {
.release = of_overlay_release,
};
@@ -360,6 +396,10 @@ int of_overlay_create(struct device_node *tree)
struct of_overlay *ov;
int err, id;
+ /* administratively disabled */
+ if (!atomic_read(&ov_enable))
+ return -EPERM;
+
/* allocate the overlay structure */
ov = kzalloc(sizeof(*ov), GFP_KERNEL);
if (ov == NULL)
@@ -596,5 +636,8 @@ int of_overlay_init(void)
if (!ov_kset)
return -ENOMEM;
- return 0;
+ rc = sysfs_create_files(&ov_kset->kobj, overlay_global_attrs);
+ WARN(rc, "%s: error adding global attributes\n", __func__);
+
+ return rc;
}
--
1.7.12
The two default overlay attributes are:
* A targets sysfs attribute listing the targets of the installed
overlay. The targets list the path on the kernel's device tree
where each overlay fragment is applied to
* A per overlay can_remove sysfs attribute that reports whether
the overlay can be removed or not due to another overlapping overlay.
Signed-off-by: Pantelis Antoniou <[email protected]>
---
drivers/of/overlay.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 53 insertions(+)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index c335809..9af4d8d 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -375,8 +375,61 @@ static const struct attribute *overlay_global_attrs[] = {
NULL
};
+static ssize_t can_remove_show(struct kobject *kobj,
+ struct kobj_attribute *attr, char *buf)
+{
+ struct of_overlay *ov = kobj_to_overlay(kobj);
+
+ return snprintf(buf, PAGE_SIZE, "%d\n", overlay_removal_is_ok(ov));
+}
+
+static ssize_t targets_show(struct kobject *kobj,
+ struct kobj_attribute *attr, char *buf)
+{
+ struct of_overlay *ov = kobj_to_overlay(kobj);
+ struct of_overlay_info *ovinfo;
+ char *s, *e;
+ ssize_t ret;
+ int i, len;
+
+ s = buf;
+ e = buf + PAGE_SIZE;
+
+ mutex_lock(&of_mutex);
+
+ /* targets */
+ for (i = 0; i < ov->count; i++) {
+ ovinfo = &ov->ovinfo_tab[i];
+
+ len = snprintf(s, e - s, "%s\n",
+ of_node_full_name(ovinfo->target));
+ if (len == 0) {
+ ret = -ENOSPC;
+ goto err;
+ }
+ s += len;
+ }
+
+ /* the buffer is zero terminated */
+ ret = s - buf;
+err:
+ mutex_unlock(&of_mutex);
+ return ret;
+}
+
+static struct kobj_attribute can_remove_attr = __ATTR_RO(can_remove);
+static struct kobj_attribute targets_attr = __ATTR_RO(targets);
+
+static struct attribute *overlay_attrs[] = {
+ &can_remove_attr.attr,
+ &targets_attr.attr,
+ NULL
+};
+
static struct kobj_type of_overlay_ktype = {
.release = of_overlay_release,
+ .sysfs_ops = &kobj_sysfs_ops, /* default kobj sysfs ops */
+ .default_attrs = overlay_attrs,
};
static struct kset *ov_kset;
--
1.7.12
Documentation ABI entry for overlays sysfs entries.
Signed-off-by: Pantelis Antoniou <[email protected]>
---
.../ABI/testing/sysfs-firmware-devicetree-overlays | 23 ++++++++++++++++++++++
1 file changed, 23 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
diff --git a/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays b/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
new file mode 100644
index 0000000..6b81f1c
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
@@ -0,0 +1,23 @@
+What: /sys/firmware/devicetree/overlays/
+Date: March 2015
+Contact: Pantelis Antoniou <[email protected]>
+Description:
+ This directory contains the applied device tree overlays of
+ the running system, as directories of the overlay id.
+
+ enable: The master enable switch, by default is 1, and when
+ set to 0 it cannot be re-enabled for security reasons.
+
+What: /sys/firmware/devicetree/overlays/<id>
+Date: March 2015
+Contact: Pantelis Antoniou <[email protected]>
+Description:
+ Each directory represents an applied overlay, containing
+ the following attribute files.
+
+ targets: A file containing the list of targets of each overlay
+ with each line containing a target.
+
+ can_remove: The attribute set to 1 means that the overlay can
+ be removed, while 0 means that the overlay is being
+ overlapped therefore removal is prohibited.
--
1.7.12
On Fri, Apr 24, 2015 at 12:45:42PM +0300, Pantelis Antoniou wrote:
> A throw once master enable switch to protect against any
> further overlay applications if the administrator desires so.
>
> Signed-off-by: Pantelis Antoniou <[email protected]>
> ---
> drivers/of/overlay.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index f17f5ef..c335809 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -21,6 +21,7 @@
> #include <linux/err.h>
> #include <linux/idr.h>
> #include <linux/sysfs.h>
> +#include <linux/atomic.h>
>
> #include "of_private.h"
>
> @@ -55,8 +56,12 @@ struct of_overlay {
> struct kobject kobj;
> };
>
> +/* master enable switch; once set to 0 can't be re-enabled */
> +static atomic_t ov_enable = ATOMIC_INIT(1);
> +
> static int of_overlay_apply_one(struct of_overlay *ov,
> struct device_node *target, const struct device_node *overlay);
> +static int overlay_removal_is_ok(struct of_overlay *ov);
>
> static int of_overlay_apply_single_property(struct of_overlay *ov,
> struct device_node *target, struct property *prop)
> @@ -339,6 +344,37 @@ void of_overlay_release(struct kobject *kobj)
> kfree(ov);
> }
>
> +static ssize_t enable_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + return snprintf(buf, PAGE_SIZE, "%d\n", atomic_read(&ov_enable));
> +}
> +
> +static ssize_t enable_store(struct kobject *kobj,
> + struct kobj_attribute *attr, const char *buf, size_t count)
> +{
> + int ret;
> + long new_enable;
> +
> + ret = kstrtol(buf, 10, &new_enable);
> + if (ret != 0)
> + return ret;
> + if ((unsigned long)new_enable > 1)
> + return -EINVAL;
> + /* if we've disabled it, no going back */
> + if (atomic_read(&ov_enable) == 0)
> + return -EPERM;
> + atomic_set(&ov_enable, (int)new_enable);
> + return count;
> +}
> +
> +static struct kobj_attribute enable_attr = __ATTR_RW(enable);
> +
> +static const struct attribute *overlay_global_attrs[] = {
> + &enable_attr.attr,
> + NULL
> +};
Why not make this an attribute group and then attach it to the kobj_type
to create the files in a race-free manner?
> +
> static struct kobj_type of_overlay_ktype = {
> .release = of_overlay_release,
> };
> @@ -360,6 +396,10 @@ int of_overlay_create(struct device_node *tree)
> struct of_overlay *ov;
> int err, id;
>
> + /* administratively disabled */
> + if (!atomic_read(&ov_enable))
> + return -EPERM;
> +
> /* allocate the overlay structure */
> ov = kzalloc(sizeof(*ov), GFP_KERNEL);
> if (ov == NULL)
> @@ -596,5 +636,8 @@ int of_overlay_init(void)
> if (!ov_kset)
> return -ENOMEM;
>
> - return 0;
> + rc = sysfs_create_files(&ov_kset->kobj, overlay_global_attrs);
> + WARN(rc, "%s: error adding global attributes\n", __func__);
What can a user do with this warning message? If nothing, then don't
print it out, right?
You are creating sysfs files _after_ the kobject has been announced to
userspace, causing nasty race conditions. Please don't do that.
thanks,
greg k-h
On Fri, Apr 24, 2015 at 12:45:44PM +0300, Pantelis Antoniou wrote:
> Documentation ABI entry for overlays sysfs entries.
>
> Signed-off-by: Pantelis Antoniou <[email protected]>
> ---
> .../ABI/testing/sysfs-firmware-devicetree-overlays | 23 ++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
>
> diff --git a/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays b/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
> new file mode 100644
> index 0000000..6b81f1c
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
> @@ -0,0 +1,23 @@
> +What: /sys/firmware/devicetree/overlays/
> +Date: March 2015
> +Contact: Pantelis Antoniou <[email protected]>
> +Description:
> + This directory contains the applied device tree overlays of
> + the running system, as directories of the overlay id.
"as"?
> +
> + enable: The master enable switch, by default is 1, and when
> + set to 0 it cannot be re-enabled for security reasons.
What are those reasons?
thanks,
greg k-h
On Fri, Apr 24, 2015 at 12:45:42PM +0300, Pantelis Antoniou wrote:
> A throw once master enable switch to protect against any
> further overlay applications if the administrator desires so.
>
> Signed-off-by: Pantelis Antoniou <[email protected]>
> ---
> drivers/of/overlay.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index f17f5ef..c335809 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -21,6 +21,7 @@
> #include <linux/err.h>
> #include <linux/idr.h>
> #include <linux/sysfs.h>
> +#include <linux/atomic.h>
>
> #include "of_private.h"
>
> @@ -55,8 +56,12 @@ struct of_overlay {
> struct kobject kobj;
> };
>
> +/* master enable switch; once set to 0 can't be re-enabled */
> +static atomic_t ov_enable = ATOMIC_INIT(1);
> +
> static int of_overlay_apply_one(struct of_overlay *ov,
> struct device_node *target, const struct device_node *overlay);
> +static int overlay_removal_is_ok(struct of_overlay *ov);
>
> static int of_overlay_apply_single_property(struct of_overlay *ov,
> struct device_node *target, struct property *prop)
> @@ -339,6 +344,37 @@ void of_overlay_release(struct kobject *kobj)
> kfree(ov);
> }
>
> +static ssize_t enable_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + return snprintf(buf, PAGE_SIZE, "%d\n", atomic_read(&ov_enable));
> +}
> +
> +static ssize_t enable_store(struct kobject *kobj,
> + struct kobj_attribute *attr, const char *buf, size_t count)
> +{
> + int ret;
> + long new_enable;
> +
> + ret = kstrtol(buf, 10, &new_enable);
strtobool()?
Hi Greg,
> On Apr 24, 2015, at 23:31 , Greg KH <[email protected]> wrote:
>
> On Fri, Apr 24, 2015 at 12:45:44PM +0300, Pantelis Antoniou wrote:
>> Documentation ABI entry for overlays sysfs entries.
>>
>> Signed-off-by: Pantelis Antoniou <[email protected]>
>> ---
>> .../ABI/testing/sysfs-firmware-devicetree-overlays | 23 ++++++++++++++++++++++
>> 1 file changed, 23 insertions(+)
>> create mode 100644 Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
>>
>> diff --git a/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays b/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
>> new file mode 100644
>> index 0000000..6b81f1c
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
>> @@ -0,0 +1,23 @@
>> +What: /sys/firmware/devicetree/overlays/
>> +Date: March 2015
>> +Contact: Pantelis Antoniou <[email protected]>
>> +Description:
>> + This directory contains the applied device tree overlays of
>> + the running system, as directories of the overlay id.
>
> "as"?
>
>> +
>> + enable: The master enable switch, by default is 1, and when
>> + set to 0 it cannot be re-enabled for security reasons.
>
> What are those reasons?
>
There’s a whole subthread with Grant & Kees going over security concerns.
http://comments.gmane.org/gmane.linux.drivers.devicetree/101871
> thanks,
>
> greg k-h
Regards
— Pantelis
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 27, 2015 at 08:51:20PM +0300, Pantelis Antoniou wrote:
> Hi Greg,
>
> > On Apr 24, 2015, at 23:31 , Greg KH <[email protected]> wrote:
> >
> > On Fri, Apr 24, 2015 at 12:45:44PM +0300, Pantelis Antoniou wrote:
> >> Documentation ABI entry for overlays sysfs entries.
> >>
> >> Signed-off-by: Pantelis Antoniou <[email protected]>
> >> ---
> >> .../ABI/testing/sysfs-firmware-devicetree-overlays | 23 ++++++++++++++++++++++
> >> 1 file changed, 23 insertions(+)
> >> create mode 100644 Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
> >>
> >> diff --git a/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays b/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
> >> new file mode 100644
> >> index 0000000..6b81f1c
> >> --- /dev/null
> >> +++ b/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
> >> @@ -0,0 +1,23 @@
> >> +What: /sys/firmware/devicetree/overlays/
> >> +Date: March 2015
> >> +Contact: Pantelis Antoniou <[email protected]>
> >> +Description:
> >> + This directory contains the applied device tree overlays of
> >> + the running system, as directories of the overlay id.
> >
> > "as"?
> >
> >> +
> >> + enable: The master enable switch, by default is 1, and when
> >> + set to 0 it cannot be re-enabled for security reasons.
> >
> > What are those reasons?
> >
>
> There’s a whole subthread with Grant & Kees going over security concerns.
>
> http://comments.gmane.org/gmane.linux.drivers.devicetree/101871
Great, document it here please, so that others know this :)
thanks,
greg k-h
Hi Greg,
> On Apr 24, 2015, at 23:29 , Greg KH <[email protected]> wrote:
>
> On Fri, Apr 24, 2015 at 12:45:42PM +0300, Pantelis Antoniou wrote:
>> A throw once master enable switch to protect against any
>> further overlay applications if the administrator desires so.
>>
>> Signed-off-by: Pantelis Antoniou <[email protected]>
>> ---
>> drivers/of/overlay.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 44 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index f17f5ef..c335809 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -21,6 +21,7 @@
>> #include <linux/err.h>
>> #include <linux/idr.h>
>> #include <linux/sysfs.h>
>> +#include <linux/atomic.h>
>>
>> #include "of_private.h"
>>
>> @@ -55,8 +56,12 @@ struct of_overlay {
>> struct kobject kobj;
>> };
>>
>> +/* master enable switch; once set to 0 can't be re-enabled */
>> +static atomic_t ov_enable = ATOMIC_INIT(1);
>> +
>> static int of_overlay_apply_one(struct of_overlay *ov,
>> struct device_node *target, const struct device_node *overlay);
>> +static int overlay_removal_is_ok(struct of_overlay *ov);
>>
>> static int of_overlay_apply_single_property(struct of_overlay *ov,
>> struct device_node *target, struct property *prop)
>> @@ -339,6 +344,37 @@ void of_overlay_release(struct kobject *kobj)
>> kfree(ov);
>> }
>>
>> +static ssize_t enable_show(struct kobject *kobj,
>> + struct kobj_attribute *attr, char *buf)
>> +{
>> + return snprintf(buf, PAGE_SIZE, "%d\n", atomic_read(&ov_enable));
>> +}
>> +
>> +static ssize_t enable_store(struct kobject *kobj,
>> + struct kobj_attribute *attr, const char *buf, size_t count)
>> +{
>> + int ret;
>> + long new_enable;
>> +
>> + ret = kstrtol(buf, 10, &new_enable);
>> + if (ret != 0)
>> + return ret;
>> + if ((unsigned long)new_enable > 1)
>> + return -EINVAL;
>> + /* if we've disabled it, no going back */
>> + if (atomic_read(&ov_enable) == 0)
>> + return -EPERM;
>> + atomic_set(&ov_enable, (int)new_enable);
>> + return count;
>> +}
>> +
>> +static struct kobj_attribute enable_attr = __ATTR_RW(enable);
>> +
>> +static const struct attribute *overlay_global_attrs[] = {
>> + &enable_attr.attr,
>> + NULL
>> +};
>
> Why not make this an attribute group and then attach it to the kobj_type
> to create the files in a race-free manner?
>
Err, these are the global attributes. They are attached to the
parent of the overlay objects which is a kset object itself.
There’s no way which I can attach the attribute to it as far
as I can tell.
>> +
>> static struct kobj_type of_overlay_ktype = {
>> .release = of_overlay_release,
>> };
>> @@ -360,6 +396,10 @@ int of_overlay_create(struct device_node *tree)
>> struct of_overlay *ov;
>> int err, id;
>>
>> + /* administratively disabled */
>> + if (!atomic_read(&ov_enable))
>> + return -EPERM;
>> +
>> /* allocate the overlay structure */
>> ov = kzalloc(sizeof(*ov), GFP_KERNEL);
>> if (ov == NULL)
>> @@ -596,5 +636,8 @@ int of_overlay_init(void)
>> if (!ov_kset)
>> return -ENOMEM;
>>
>> - return 0;
>> + rc = sysfs_create_files(&ov_kset->kobj, overlay_global_attrs);
>> + WARN(rc, "%s: error adding global attributes\n", __func__);
>
> What can a user do with this warning message? If nothing, then don't
> print it out, right?
>
> You are creating sysfs files _after_ the kobject has been announced to
> userspace, causing nasty race conditions. Please don't do that.
>
This is at overlay_init() time, which is way way before userspace ever
has a chance to start. If there’s a different way to attach a attribute
group to a kset object I’d like to find out how :)
> thanks,
>
> greg k-h
Regards
— Pantelis-
Hi Greg,
> On Apr 24, 2015, at 23:36 , Greg KH <[email protected]> wrote:
>
> On Fri, Apr 24, 2015 at 12:45:42PM +0300, Pantelis Antoniou wrote:
>> A throw once master enable switch to protect against any
>> further overlay applications if the administrator desires so.
>>
>> Signed-off-by: Pantelis Antoniou <[email protected]>
>> ---
>> drivers/of/overlay.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 44 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index f17f5ef..c335809 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -21,6 +21,7 @@
>> #include <linux/err.h>
>> #include <linux/idr.h>
>> #include <linux/sysfs.h>
>> +#include <linux/atomic.h>
>>
>> #include "of_private.h"
>>
>> @@ -55,8 +56,12 @@ struct of_overlay {
>> struct kobject kobj;
>> };
>>
>> +/* master enable switch; once set to 0 can't be re-enabled */
>> +static atomic_t ov_enable = ATOMIC_INIT(1);
>> +
>> static int of_overlay_apply_one(struct of_overlay *ov,
>> struct device_node *target, const struct device_node *overlay);
>> +static int overlay_removal_is_ok(struct of_overlay *ov);
>>
>> static int of_overlay_apply_single_property(struct of_overlay *ov,
>> struct device_node *target, struct property *prop)
>> @@ -339,6 +344,37 @@ void of_overlay_release(struct kobject *kobj)
>> kfree(ov);
>> }
>>
>> +static ssize_t enable_show(struct kobject *kobj,
>> + struct kobj_attribute *attr, char *buf)
>> +{
>> + return snprintf(buf, PAGE_SIZE, "%d\n", atomic_read(&ov_enable));
>> +}
>> +
>> +static ssize_t enable_store(struct kobject *kobj,
>> + struct kobj_attribute *attr, const char *buf, size_t count)
>> +{
>> + int ret;
>> + long new_enable;
>> +
>> + ret = kstrtol(buf, 10, &new_enable);
>
> strtobool()?
OK
Regards
— Pantelis
On Mon, Apr 27, 2015 at 09:13:56PM +0300, Pantelis Antoniou wrote:
> Hi Greg,
>
> > On Apr 24, 2015, at 23:29 , Greg KH <[email protected]> wrote:
> >
> > On Fri, Apr 24, 2015 at 12:45:42PM +0300, Pantelis Antoniou wrote:
> >> A throw once master enable switch to protect against any
> >> further overlay applications if the administrator desires so.
> >>
> >> Signed-off-by: Pantelis Antoniou <[email protected]>
> >> ---
> >> drivers/of/overlay.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
> >> 1 file changed, 44 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> >> index f17f5ef..c335809 100644
> >> --- a/drivers/of/overlay.c
> >> +++ b/drivers/of/overlay.c
> >> @@ -21,6 +21,7 @@
> >> #include <linux/err.h>
> >> #include <linux/idr.h>
> >> #include <linux/sysfs.h>
> >> +#include <linux/atomic.h>
> >>
> >> #include "of_private.h"
> >>
> >> @@ -55,8 +56,12 @@ struct of_overlay {
> >> struct kobject kobj;
> >> };
> >>
> >> +/* master enable switch; once set to 0 can't be re-enabled */
> >> +static atomic_t ov_enable = ATOMIC_INIT(1);
> >> +
> >> static int of_overlay_apply_one(struct of_overlay *ov,
> >> struct device_node *target, const struct device_node *overlay);
> >> +static int overlay_removal_is_ok(struct of_overlay *ov);
> >>
> >> static int of_overlay_apply_single_property(struct of_overlay *ov,
> >> struct device_node *target, struct property *prop)
> >> @@ -339,6 +344,37 @@ void of_overlay_release(struct kobject *kobj)
> >> kfree(ov);
> >> }
> >>
> >> +static ssize_t enable_show(struct kobject *kobj,
> >> + struct kobj_attribute *attr, char *buf)
> >> +{
> >> + return snprintf(buf, PAGE_SIZE, "%d\n", atomic_read(&ov_enable));
> >> +}
> >> +
> >> +static ssize_t enable_store(struct kobject *kobj,
> >> + struct kobj_attribute *attr, const char *buf, size_t count)
> >> +{
> >> + int ret;
> >> + long new_enable;
> >> +
> >> + ret = kstrtol(buf, 10, &new_enable);
> >> + if (ret != 0)
> >> + return ret;
> >> + if ((unsigned long)new_enable > 1)
> >> + return -EINVAL;
> >> + /* if we've disabled it, no going back */
> >> + if (atomic_read(&ov_enable) == 0)
> >> + return -EPERM;
> >> + atomic_set(&ov_enable, (int)new_enable);
> >> + return count;
> >> +}
> >> +
> >> +static struct kobj_attribute enable_attr = __ATTR_RW(enable);
> >> +
> >> +static const struct attribute *overlay_global_attrs[] = {
> >> + &enable_attr.attr,
> >> + NULL
> >> +};
> >
> > Why not make this an attribute group and then attach it to the kobj_type
> > to create the files in a race-free manner?
> >
>
> Err, these are the global attributes. They are attached to the
> parent of the overlay objects which is a kset object itself.
Ick, no, never attach attributes to a kobject that you don't create
yourself, otherwise it's a race that you lost.
> >> +
> >> static struct kobj_type of_overlay_ktype = {
> >> .release = of_overlay_release,
> >> };
> >> @@ -360,6 +396,10 @@ int of_overlay_create(struct device_node *tree)
> >> struct of_overlay *ov;
> >> int err, id;
> >>
> >> + /* administratively disabled */
> >> + if (!atomic_read(&ov_enable))
> >> + return -EPERM;
> >> +
> >> /* allocate the overlay structure */
> >> ov = kzalloc(sizeof(*ov), GFP_KERNEL);
> >> if (ov == NULL)
> >> @@ -596,5 +636,8 @@ int of_overlay_init(void)
> >> if (!ov_kset)
> >> return -ENOMEM;
> >>
> >> - return 0;
> >> + rc = sysfs_create_files(&ov_kset->kobj, overlay_global_attrs);
> >> + WARN(rc, "%s: error adding global attributes\n", __func__);
> >
> > What can a user do with this warning message? If nothing, then don't
> > print it out, right?
> >
> > You are creating sysfs files _after_ the kobject has been announced to
> > userspace, causing nasty race conditions. Please don't do that.
> >
>
> This is at overlay_init() time, which is way way before userspace ever
> has a chance to start. If there’s a different way to attach a attribute
> group to a kset object I’d like to find out how :)
You can't load this as a module? Where exactly in sysfs is this
kobject, and who creates it?
thanks,
greg k-h
Hi Greg,
> On Apr 27, 2015, at 21:39 , Greg KH <[email protected]> wrote:
>
> On Mon, Apr 27, 2015 at 09:13:56PM +0300, Pantelis Antoniou wrote:
>> Hi Greg,
>>
>>> On Apr 24, 2015, at 23:29 , Greg KH <[email protected]> wrote:
>>>
>>> On Fri, Apr 24, 2015 at 12:45:42PM +0300, Pantelis Antoniou wrote:
>>>> A throw once master enable switch to protect against any
>>>> further overlay applications if the administrator desires so.
>>>>
>>>> Signed-off-by: Pantelis Antoniou <[email protected]>
>>>> ---
>>>> drivers/of/overlay.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>>>> 1 file changed, 44 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>>>> index f17f5ef..c335809 100644
>>>> --- a/drivers/of/overlay.c
>>>> +++ b/drivers/of/overlay.c
>>>> @@ -21,6 +21,7 @@
>>>> #include <linux/err.h>
>>>> #include <linux/idr.h>
>>>> #include <linux/sysfs.h>
>>>> +#include <linux/atomic.h>
>>>>
>>>> #include "of_private.h"
>>>>
>>>> @@ -55,8 +56,12 @@ struct of_overlay {
>>>> struct kobject kobj;
>>>> };
>>>>
>>>> +/* master enable switch; once set to 0 can't be re-enabled */
>>>> +static atomic_t ov_enable = ATOMIC_INIT(1);
>>>> +
>>>> static int of_overlay_apply_one(struct of_overlay *ov,
>>>> struct device_node *target, const struct device_node *overlay);
>>>> +static int overlay_removal_is_ok(struct of_overlay *ov);
>>>>
>>>> static int of_overlay_apply_single_property(struct of_overlay *ov,
>>>> struct device_node *target, struct property *prop)
>>>> @@ -339,6 +344,37 @@ void of_overlay_release(struct kobject *kobj)
>>>> kfree(ov);
>>>> }
>>>>
>>>> +static ssize_t enable_show(struct kobject *kobj,
>>>> + struct kobj_attribute *attr, char *buf)
>>>> +{
>>>> + return snprintf(buf, PAGE_SIZE, "%d\n", atomic_read(&ov_enable));
>>>> +}
>>>> +
>>>> +static ssize_t enable_store(struct kobject *kobj,
>>>> + struct kobj_attribute *attr, const char *buf, size_t count)
>>>> +{
>>>> + int ret;
>>>> + long new_enable;
>>>> +
>>>> + ret = kstrtol(buf, 10, &new_enable);
>>>> + if (ret != 0)
>>>> + return ret;
>>>> + if ((unsigned long)new_enable > 1)
>>>> + return -EINVAL;
>>>> + /* if we've disabled it, no going back */
>>>> + if (atomic_read(&ov_enable) == 0)
>>>> + return -EPERM;
>>>> + atomic_set(&ov_enable, (int)new_enable);
>>>> + return count;
>>>> +}
>>>> +
>>>> +static struct kobj_attribute enable_attr = __ATTR_RW(enable);
>>>> +
>>>> +static const struct attribute *overlay_global_attrs[] = {
>>>> + &enable_attr.attr,
>>>> + NULL
>>>> +};
>>>
>>> Why not make this an attribute group and then attach it to the kobj_type
>>> to create the files in a race-free manner?
>>>
>>
>> Err, these are the global attributes. They are attached to the
>> parent of the overlay objects which is a kset object itself.
>
> Ick, no, never attach attributes to a kobject that you don't create
> yourself, otherwise it's a race that you lost.
>
Err, it is created by me. It’s a kset by the following call:
> ov_kset = kset_create_and_add("overlays", NULL, &of_kset->kobj);
>
What is the problem with this?
>>>> +
>>>> static struct kobj_type of_overlay_ktype = {
>>>> .release = of_overlay_release,
>>>> };
>>>> @@ -360,6 +396,10 @@ int of_overlay_create(struct device_node *tree)
>>>> struct of_overlay *ov;
>>>> int err, id;
>>>>
>>>> + /* administratively disabled */
>>>> + if (!atomic_read(&ov_enable))
>>>> + return -EPERM;
>>>> +
>>>> /* allocate the overlay structure */
>>>> ov = kzalloc(sizeof(*ov), GFP_KERNEL);
>>>> if (ov == NULL)
>>>> @@ -596,5 +636,8 @@ int of_overlay_init(void)
>>>> if (!ov_kset)
>>>> return -ENOMEM;
>>>>
>>>> - return 0;
>>>> + rc = sysfs_create_files(&ov_kset->kobj, overlay_global_attrs);
>>>> + WARN(rc, "%s: error adding global attributes\n", __func__);
>>>
>>> What can a user do with this warning message? If nothing, then don't
>>> print it out, right?
>>>
>>> You are creating sysfs files _after_ the kobject has been announced to
>>> userspace, causing nasty race conditions. Please don't do that.
>>>
>>
>> This is at overlay_init() time, which is way way before userspace ever
>> has a chance to start. If there’s a different way to attach a attribute
>> group to a kset object I’d like to find out how :)
>
> You can't load this as a module? Where exactly in sysfs is this
> kobject, and who creates it?
>
No, there’s no way to load this as a module, it’s in /sys/firmware/devicetree
And it’s created by the call to kset_create_and_add()
> thanks,
>
> greg k-h
Regards
— Pantelis