[PATCH v3 0/4] of: overlay: kobject & sysfs'ation

[PATCH v3 0/4] of: overlay: kobject & sysfs'ation

[Pantelis Antoniou]

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

[PATCH v3 1/4] of: overlay: kobjectify overlay objects

[Pantelis Antoniou]

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 <pantelis.antoniou@konsulko.com>
---
 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

[PATCH v3 2/4] of: overlay: global sysfs enable attribute

[Pantelis Antoniou]

A throw once master enable switch to protect against any
further overlay applications if the administrator desires so.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 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

Re: [PATCH v3 2/4] of: overlay: global sysfs enable attribute

[Greg KH]

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 <pantelis.antoniou@konsulko.com>
> ---
>  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

Re: [PATCH v3 2/4] of: overlay: global sysfs enable attribute

[Pantelis Antoniou]

Hi Greg,

> On Apr 24, 2015, at 23:29 , Greg KH <greg@kroah.com> 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 <pantelis.antoniou@konsulko.com>
>> ---
>> 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

Re: [PATCH v3 2/4] of: overlay: global sysfs enable attribute

[Greg KH]

On Mon, Apr 27, 2015 at 09:13:56PM +0300, Pantelis Antoniou wrote:
> Hi Greg,
> 
> > On Apr 24, 2015, at 23:29 , Greg KH <greg@kroah.com> 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 <pantelis.antoniou@konsulko.com>
> >> ---
> >> 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

Re: [PATCH v3 2/4] of: overlay: global sysfs enable attribute

[Pantelis Antoniou]

Hi Greg,

> On Apr 27, 2015, at 21:39 , Greg KH <greg@kroah.com> 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 <greg@kroah.com> 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 <pantelis.antoniou@konsulko.com>
>>>> ---
>>>> 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

Re: [PATCH v3 2/4] of: overlay: global sysfs enable attribute

[Greg KH]

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 <pantelis.antoniou@konsulko.com>
> ---
>  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()?

Re: [PATCH v3 2/4] of: overlay: global sysfs enable attribute

[Pantelis Antoniou]

Hi Greg,

> On Apr 24, 2015, at 23:36 , Greg KH <greg@kroah.com> 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 <pantelis.antoniou@konsulko.com>
>> ---
>> 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

[PATCH v3 3/4] of: overlay: add per overlay sysfs attributes

[Pantelis Antoniou]

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 <pantelis.antoniou@konsulko.com>
---
 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

[PATCH v3 4/4] Documentation: ABI: /sys/firmware/devicetree/overlays

[Pantelis Antoniou]

Documentation ABI entry for overlays sysfs entries.

Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
---
 .../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 <pantelis.antoniou@konsulko.com>
+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 <pantelis.antoniou@konsulko.com>
+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

Re: [PATCH v3 4/4] Documentation: ABI: /sys/firmware/devicetree/overlays

[Greg KH]

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 <pantelis.antoniou@konsulko.com>
> ---
>  .../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 <pantelis.antoniou@konsulko.com>
> +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

Re: [PATCH v3 4/4] Documentation: ABI: /sys/firmware/devicetree/overlays

[Pantelis Antoniou]

Hi Greg,

> On Apr 24, 2015, at 23:31 , Greg KH <greg@kroah.com> 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 <pantelis.antoniou@konsulko.com>
>> ---
>> .../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 <pantelis.antoniou@konsulko.com>
>> +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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/4] Documentation: ABI: /sys/firmware/devicetree/overlays

[Greg KH]

On Mon, Apr 27, 2015 at 08:51:20PM +0300, Pantelis Antoniou wrote:
> Hi Greg,
> 
> > On Apr 24, 2015, at 23:31 , Greg KH <greg@kroah.com> 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 <pantelis.antoniou@konsulko.com>
> >> ---
> >> .../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 <pantelis.antoniou@konsulko.com>
> >> +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