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

[PATCH v4 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 v3:
* Used strtobool instead of kstrtoul
* ABI Documentation includes a pointer to the discussion that
requested the sysfs property.

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 |  35 +++++
 drivers/of/base.c                                  |   7 +
 drivers/of/of_private.h                            |   9 ++
 drivers/of/overlay.c                               | 146 ++++++++++++++++++++-
 4 files changed, 195 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-firmware-devicetree-overlays

-- 
1.7.12

[PATCH v4 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       |  7 +++++++
 drivers/of/of_private.h |  9 +++++++++
 drivers/of/overlay.c    | 52 +++++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index f065026..31a4883 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -192,6 +192,7 @@ int __of_attach_node_sysfs(struct device_node *np)
 void __init of_core_init(void)
 {
 	struct device_node *np;
+	int ret;
 
 	/* Create the kset, and register existing nodes */
 	mutex_lock(&of_mutex);
@@ -208,6 +209,12 @@ void __init of_core_init(void)
 	/* Symlink in /proc as required by userspace ABI */
 	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;
 }
 
 static struct property *__of_find_property(const struct device_node *np,
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 v4 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 | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index f17f5ef..37ec858 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,35 @@ 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;
+	bool new_enable;
+
+	ret = strtobool(buf, &new_enable);
+	if (ret != 0)
+		return ret;
+	/* 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 +394,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 +634,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

[PATCH v4 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-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 drivers/of/overlay.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index f17f5ef..37ec858 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,35 @@ 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;
+	bool new_enable;
+
+	ret = strtobool(buf, &new_enable);
+	if (ret != 0)
+		return ret;
+	/* 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 +394,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 +634,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

[PATCH v4 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 37ec858..747568f 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -373,8 +373,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 v4 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-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 drivers/of/overlay.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 37ec858..747568f 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -373,8 +373,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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 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 | 35 ++++++++++++++++++++++
 1 file changed, 35 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..be2d28b
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
@@ -0,0 +1,35 @@
+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.
+
+		The discussion about this switch takes place in:
+		http://comments.gmane.org/gmane.linux.drivers.devicetree/101871
+
+		Kees Cook:
+		"Coming from the perspective of drawing a bright line between
+		kernel and the root user (which tends to start with disabling
+		kernel module loading), I would say that there at least needs
+		to be a high-level one-way "off" switch for the interface so
+		that systems that have this interface can choose to turn it off
+		during initial boot, etc."
+
+		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 v4 4/4] Documentation: ABI: /sys/firmware/devicetree/overlays

[Rob Herring]

On Fri, Jun 12, 2015 at 2:38 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Documentation ABI entry for overlays sysfs entries.
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> ---
>  .../ABI/testing/sysfs-firmware-devicetree-overlays | 35 ++++++++++++++++++++++
>  1 file changed, 35 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..be2d28b
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
> @@ -0,0 +1,35 @@
> +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.
> +
> +               The discussion about this switch takes place in:
> +               http://comments.gmane.org/gmane.linux.drivers.devicetree/101871
> +
> +               Kees Cook:
> +               "Coming from the perspective of drawing a bright line between
> +               kernel and the root user (which tends to start with disabling
> +               kernel module loading), I would say that there at least needs
> +               to be a high-level one-way "off" switch for the interface so
> +               that systems that have this interface can choose to turn it off
> +               during initial boot, etc."

Doesn't this below up above with "enable"?

> +
> +               targets: A file containing the list of targets of each overlay
> +                        with each line containing a target.

We have OF nodes in sysfs now. Would it be more useful if we created
links to the target nodes instead of having a list of names?

> +
> +               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 v4 4/4] Documentation: ABI: /sys/firmware/devicetree/overlays

[Rob Herring]

On Fri, Jun 12, 2015 at 2:38 PM, Pantelis Antoniou
<pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
> Documentation ABI entry for overlays sysfs entries.
>
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> ---
>  .../ABI/testing/sysfs-firmware-devicetree-overlays | 35 ++++++++++++++++++++++
>  1 file changed, 35 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..be2d28b
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
> @@ -0,0 +1,35 @@
> +What:          /sys/firmware/devicetree/overlays/
> +Date:          March 2015
> +Contact:       Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> +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-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> +Description:
> +               Each directory represents an applied overlay, containing
> +               the following attribute files.
> +
> +               The discussion about this switch takes place in:
> +               http://comments.gmane.org/gmane.linux.drivers.devicetree/101871
> +
> +               Kees Cook:
> +               "Coming from the perspective of drawing a bright line between
> +               kernel and the root user (which tends to start with disabling
> +               kernel module loading), I would say that there at least needs
> +               to be a high-level one-way "off" switch for the interface so
> +               that systems that have this interface can choose to turn it off
> +               during initial boot, etc."

Doesn't this below up above with "enable"?

> +
> +               targets: A file containing the list of targets of each overlay
> +                        with each line containing a target.

We have OF nodes in sysfs now. Would it be more useful if we created
links to the target nodes instead of having a list of names?

> +
> +               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 v4 4/4] Documentation: ABI: /sys/firmware/devicetree/overlays

[Pantelis Antoniou]

Hi Rob,

> On Jun 15, 2015, at 16:24 , Rob Herring <robherring2@gmail.com> wrote:
> 
> On Fri, Jun 12, 2015 at 2:38 PM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>> Documentation ABI entry for overlays sysfs entries.
>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>> ---
>> .../ABI/testing/sysfs-firmware-devicetree-overlays | 35 ++++++++++++++++++++++
>> 1 file changed, 35 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..be2d28b
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
>> @@ -0,0 +1,35 @@
>> +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.
>> +
>> +               The discussion about this switch takes place in:
>> +               http://comments.gmane.org/gmane.linux.drivers.devicetree/101871
>> +
>> +               Kees Cook:
>> +               "Coming from the perspective of drawing a bright line between
>> +               kernel and the root user (which tends to start with disabling
>> +               kernel module loading), I would say that there at least needs
>> +               to be a high-level one-way "off" switch for the interface so
>> +               that systems that have this interface can choose to turn it off
>> +               during initial boot, etc."
> 
> Doesn't this below up above with "enable”?

Ugh I guess so.

> 

>> +
>> +               targets: A file containing the list of targets of each overlay
>> +                        with each line containing a target.
> 
> We have OF nodes in sysfs now. Would it be more useful if we created
> links to the target nodes instead of having a list of names?
> 

Probably, this interface is merely informational; things get complicated by
the fact that there can be more than one target in each overlay.

>> +
>> +               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
>> 

Regards

— Pantelis

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

[Pantelis Antoniou]

Hi Rob,

> On Jun 15, 2015, at 16:24 , Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> On Fri, Jun 12, 2015 at 2:38 PM, Pantelis Antoniou
> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>> Documentation ABI entry for overlays sysfs entries.
>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>> ---
>> .../ABI/testing/sysfs-firmware-devicetree-overlays | 35 ++++++++++++++++++++++
>> 1 file changed, 35 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..be2d28b
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
>> @@ -0,0 +1,35 @@
>> +What:          /sys/firmware/devicetree/overlays/
>> +Date:          March 2015
>> +Contact:       Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>> +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-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>> +Description:
>> +               Each directory represents an applied overlay, containing
>> +               the following attribute files.
>> +
>> +               The discussion about this switch takes place in:
>> +               http://comments.gmane.org/gmane.linux.drivers.devicetree/101871
>> +
>> +               Kees Cook:
>> +               "Coming from the perspective of drawing a bright line between
>> +               kernel and the root user (which tends to start with disabling
>> +               kernel module loading), I would say that there at least needs
>> +               to be a high-level one-way "off" switch for the interface so
>> +               that systems that have this interface can choose to turn it off
>> +               during initial boot, etc."
> 
> Doesn't this below up above with "enable”?

Ugh I guess so.

> 

>> +
>> +               targets: A file containing the list of targets of each overlay
>> +                        with each line containing a target.
> 
> We have OF nodes in sysfs now. Would it be more useful if we created
> links to the target nodes instead of having a list of names?
> 

Probably, this interface is merely informational; things get complicated by
the fact that there can be more than one target in each overlay.

>> +
>> +               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
>> 

Regards

— Pantelis

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

[Rob Herring]

On Mon, Jun 15, 2015 at 8:26 AM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> Hi Rob,
>
>> On Jun 15, 2015, at 16:24 , Rob Herring <robherring2@gmail.com> wrote:
>>
>> On Fri, Jun 12, 2015 at 2:38 PM, Pantelis Antoniou
>> <pantelis.antoniou@konsulko.com> wrote:
>>> Documentation ABI entry for overlays sysfs entries.
>>>
>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>>> ---
>>> .../ABI/testing/sysfs-firmware-devicetree-overlays | 35 ++++++++++++++++++++++
>>> 1 file changed, 35 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..be2d28b
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
>>> @@ -0,0 +1,35 @@

[...]

>>> +
>>> +               targets: A file containing the list of targets of each overlay
>>> +                        with each line containing a target.
>>
>> We have OF nodes in sysfs now. Would it be more useful if we created
>> links to the target nodes instead of having a list of names?
>>
>
> Probably, this interface is merely informational; things get complicated by
> the fact that there can be more than one target in each overlay.

Right, you would need 'targetN' or perhaps '<node name>' (with a '.N'
for duplicates) as the link names.

If it is informational, then perhaps debugfs should be used instead?

What else if anything do you envision adding here?

Rob

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

[Rob Herring]

On Mon, Jun 15, 2015 at 8:26 AM, Pantelis Antoniou
<pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
> Hi Rob,
>
>> On Jun 15, 2015, at 16:24 , Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>> On Fri, Jun 12, 2015 at 2:38 PM, Pantelis Antoniou
>> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>>> Documentation ABI entry for overlays sysfs entries.
>>>
>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>>> ---
>>> .../ABI/testing/sysfs-firmware-devicetree-overlays | 35 ++++++++++++++++++++++
>>> 1 file changed, 35 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..be2d28b
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
>>> @@ -0,0 +1,35 @@

[...]

>>> +
>>> +               targets: A file containing the list of targets of each overlay
>>> +                        with each line containing a target.
>>
>> We have OF nodes in sysfs now. Would it be more useful if we created
>> links to the target nodes instead of having a list of names?
>>
>
> Probably, this interface is merely informational; things get complicated by
> the fact that there can be more than one target in each overlay.

Right, you would need 'targetN' or perhaps '<node name>' (with a '.N'
for duplicates) as the link names.

If it is informational, then perhaps debugfs should be used instead?

What else if anything do you envision adding here?

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pantelis Antoniou]

Hi Rob,

> On Jun 15, 2015, at 16:42 , Rob Herring <robherring2@gmail.com> wrote:
> 
> On Mon, Jun 15, 2015 at 8:26 AM, Pantelis Antoniou
> <pantelis.antoniou@konsulko.com> wrote:
>> Hi Rob,
>> 
>>> On Jun 15, 2015, at 16:24 , Rob Herring <robherring2@gmail.com> wrote:
>>> 
>>> On Fri, Jun 12, 2015 at 2:38 PM, Pantelis Antoniou
>>> <pantelis.antoniou@konsulko.com> wrote:
>>>> Documentation ABI entry for overlays sysfs entries.
>>>> 
>>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
>>>> ---
>>>> .../ABI/testing/sysfs-firmware-devicetree-overlays | 35 ++++++++++++++++++++++
>>>> 1 file changed, 35 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..be2d28b
>>>> --- /dev/null
>>>> +++ b/Documentation/ABI/testing/sysfs-firmware-devicetree-overlays
>>>> @@ -0,0 +1,35 @@
> 
> [...]
> 
>>>> +
>>>> +               targets: A file containing the list of targets of each overlay
>>>> +                        with each line containing a target.
>>> 
>>> We have OF nodes in sysfs now. Would it be more useful if we created
>>> links to the target nodes instead of having a list of names?
>>> 
>> 
>> Probably, this interface is merely informational; things get complicated by
>> the fact that there can be more than one target in each overlay.
> 
> Right, you would need 'targetN' or perhaps '<node name>' (with a '.N'
> for duplicates) as the link names.
> 
> If it is informational, then perhaps debugfs should be used instead?
> 

I’d rather not pull in debugfs here. It’s informational, but not only for
debugging. I’ll see what I can do with the targets.

> What else if anything do you envision adding here?
> 

As far as generic infrastructure properties, I hope nothing else.
However I do intend to use the kobj directory to make the overlay users
‘hang’ properties relevant to their use.

For instance the beaglebone capemanager could put there the compatible
and the resource declaration properties. But that’s going to come later.


> Rob
> —

Regards

— Pantelis

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

[Rob Herring]

On Fri, Jun 12, 2015 at 2:38 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:
> 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.

What will a user do with this information? Can this just be debugfs?
Don't we already have the targets in the overlay itself? If we are
going to provide some overlay info, shouldn't we provide all of it
(i.e. the FDT). For example, what do we do on a kexec?

Rob

> 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 37ec858..747568f 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -373,8 +373,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
>

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

[Rob Herring]

On Fri, Jun 12, 2015 at 2:38 PM, Pantelis Antoniou
<pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
> 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.

What will a user do with this information? Can this just be debugfs?
Don't we already have the targets in the overlay itself? If we are
going to provide some overlay info, shouldn't we provide all of it
(i.e. the FDT). For example, what do we do on a kexec?

Rob

> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/of/overlay.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 37ec858..747568f 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -373,8 +373,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
>

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

[Rob Herring]

On Fri, Jun 12, 2015 at 2:38 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> 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>

I had some questioning whether we want more states than just on/off
like "only add sub-trees", but I've convinced myself it is better to
keep the kill switch simple. So I'm okay with patches 1 and 2 (and the
relevant part of 4 if you want to split it). I think #3 needs to be
part of how we expect userspace to interact with overlays.

Please respin and I will queue at least the kill switch for 4.4 unless
there are further comments.

Rob


> ---
>  drivers/of/overlay.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index f17f5ef..37ec858 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,35 @@ 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;
> +       bool new_enable;
> +
> +       ret = strtobool(buf, &new_enable);
> +       if (ret != 0)
> +               return ret;
> +       /* 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 +394,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 +634,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 v4 2/4] of: overlay: global sysfs enable attribute

[Rob Herring]

On Fri, Jun 12, 2015 at 2:38 PM, Pantelis Antoniou
<pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> 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-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>

I had some questioning whether we want more states than just on/off
like "only add sub-trees", but I've convinced myself it is better to
keep the kill switch simple. So I'm okay with patches 1 and 2 (and the
relevant part of 4 if you want to split it). I think #3 needs to be
part of how we expect userspace to interact with overlays.

Please respin and I will queue at least the kill switch for 4.4 unless
there are further comments.

Rob


> ---
>  drivers/of/overlay.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index f17f5ef..37ec858 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,35 @@ 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;
> +       bool new_enable;
> +
> +       ret = strtobool(buf, &new_enable);
> +       if (ret != 0)
> +               return ret;
> +       /* 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 +394,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 +634,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
>