On Tue, 25 Nov 2014 16:50:15 +0200
, Pantelis Antoniou <panto@xxxxxxxxxxxxxxxxxxxxxxx>
wrote:
> Hi Grant,
>
> > On Nov 25, 2014, at 12:28 , Grant Likely <grant.likely@xxxxxxxxxxxx> wrote:
> >
> > Hi Pantelis,
> >
> > Comments below...
> >
> > On Tue, 28 Oct 2014 22:36:00 +0200
> > , Pantelis Antoniou <pantelis.antoniou@xxxxxxxxxxxx>
> > wrote:
> >> Add a runtime interface to using configfs for generic device tree overlay
> >> usage.
> >>
> >> A device-tree configfs entry is created in /config/device-tree/overlays
> >>
> >> * To create an overlay you mkdir the directory:
> >>
> >> # mkdir /config/device-tree/overlays/foo
> >>
> >> * Either you echo the overlay firmware file to the path property file.
> >>
> >> # echo foo.dtbo >/config/device-tree/overlays/foo/path
> >>
> >> * Or you cat the contents of the overlay to the dtbo file
> >>
> >> # cat foo.dtbo >/config/device-tree/overlays/foo/dtbo
> >>
> >> The overlay file will be applied.
> >>
> >> To remove it simply rmdir the directory.
> >>
> >> # rmdir /config/device-tree/overlays/foo
> >
> > The above is documentation on how to use it, but it isn't a good commit
> > message. The above should be moved into a documentation file (if it
> > isn't already and I've missed it) and the commit message should give
> > detail about what was needed, what was changed to make it happen, and
> > what the result of the new code is.
> >
>
> Hmm, okie dokie.
>
> >>
> >> Changes since v1:
> >> * of_resolve() -> of_resolve_phandles().
> >>
> >> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@xxxxxxxxxxxx>
> >> ---
> >> drivers/of/Kconfig | 7 ++
> >> drivers/of/Makefile | 1 +
> >> drivers/of/configfs.c | 340 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 348 insertions(+)
> >> create mode 100644 drivers/of/configfs.c
> >>
> >> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> >> index aa315c4..d59ba40 100644
> >> --- a/drivers/of/Kconfig
> >> +++ b/drivers/of/Kconfig
> >> @@ -90,4 +90,11 @@ config OF_OVERLAY
> >> select OF_DEVICE
> >> select OF_RESOLVE
> >>
> >> +config OF_CONFIGFS
> >> + bool "OpenFirmware Overlay ConfigFS interface"
> >
> > Despite all the APIs being prefixed with OpenFirmware, this isn't an
> > OpenFirmware interface, it is a device tree interface.
> >
>
> OK, so device tree config fs interface?
Yes
>
> >> + select CONFIGFS_FS
> >> + select OF_OVERLAY
> >> + help
> >> + Enable a simple user-space driver DT overlay interface.
> >> +
> >> endmenu # OF
> >> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> >> index 1bfe462..6d12949 100644
> >> --- a/drivers/of/Makefile
> >> +++ b/drivers/of/Makefile
> >> @@ -15,6 +15,7 @@ obj-$(CONFIG_OF_MTD) += of_mtd.o
> >> obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
> >> obj-$(CONFIG_OF_RESOLVE) += resolver.o
> >> obj-$(CONFIG_OF_OVERLAY) += overlay.o
> >> +obj-$(CONFIG_OF_CONFIGFS) += configfs.o
> >
> > Tip: Insert lines in the middle of the block, roughly alphabetically
> > sorted. It cuts down on merge conflicts that way.
> >
>
> k
>
> >>
> >> CFLAGS_fdt.o = -I$(src)/../../scripts/dtc/libfdt
> >> CFLAGS_fdt_address.o = -I$(src)/../../scripts/dtc/libfdt
> >> diff --git a/drivers/of/configfs.c b/drivers/of/configfs.c
> >> new file mode 100644
> >> index 0000000..932f572
> >> --- /dev/null
> >> +++ b/drivers/of/configfs.c
> >> @@ -0,0 +1,340 @@
> >> +/*
> >> + * Configfs entries for device-tree
> >> + *
> >> + * Copyright (C) 2013 - Pantelis Antoniou <panto@xxxxxxxxxxxxxxxxxxxxxxx>
> >> + *
> >> + * This program is free software; you can redistribute it and/or
> >> + * modify it under the terms of the GNU General Public License
> >> + * as published by the Free Software Foundation; either version
> >> + * 2 of the License, or (at your option) any later version.
> >> + */
> >> +#include <linux/ctype.h>
> >> +#include <linux/cpu.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_fdt.h>
> >> +#include <linux/spinlock.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/proc_fs.h>
> >> +#include <linux/configfs.h>
> >> +#include <linux/types.h>
> >> +#include <linux/stat.h>
> >> +#include <linux/limits.h>
> >> +#include <linux/file.h>
> >> +#include <linux/vmalloc.h>
> >> +#include <linux/firmware.h>
> >> +
> >> +#include "of_private.h"
> >> +
> >> +#ifdef CONFIG_OF_OVERLAY
> >
> > ???
> >
> > This file shouldn't even be compiled if CONFIG_OF_OVERLAY isn't
> > selected. Why is this here?
> >
>
> Err, good question.
>
> >> +
> >> +struct cfs_overlay_item {
> >> + struct config_item item;
> >> +
> >> + char path[PATH_MAX];
> >> +
> >> + const struct firmware *fw;
> >> + struct device_node *overlay;
> >> + int ov_id;
> >> +
> >> + void *dtbo;
> >> + int dtbo_size;
> >> +};
> >> +
> >> +static int create_overlay(struct cfs_overlay_item *overlay, void *blob)
> >> +{
> >> + int err;
> >> +
> >> + /* unflatten the tree */
> >> + of_fdt_unflatten_tree((void *)blob, &overlay->overlay);
> >
> > blob is already void*
> >
>
> ok
>
> >> + if (overlay->overlay == NULL) {
> >> + pr_err("%s: failed to unflatten tree\n", __func__);
> >> + err = -EINVAL;
> >> + goto out_err;
> >> + }
> >> + pr_debug("%s: unflattened OK\n", __func__);
> >> +
> >> + /* mark it as detached */
> >> + of_node_set_flag(overlay->overlay, OF_DETACHED);
> >> +
> >> + /* perform resolution */
> >> + err = of_resolve_phandles(overlay->overlay);
> >> + if (err != 0) {
> >> + pr_err("%s: Failed to resolve tree\n", __func__);
> >> + goto out_err;
> >> + }
> >> + pr_debug("%s: resolved OK\n", __func__);
> >> +
> >> + err = of_overlay_create(overlay->overlay);
> >> + if (err < 0) {
> >> + pr_err("%s: Failed to create overlay (err=%d)\n",
> >> + __func__, err);
> >> + goto out_err;
> >> + }
> >> + overlay->ov_id = err;
> >> +
> >> +out_err:
> >> + return err;
> >> +}
> >> +
> >> +static inline struct cfs_overlay_item *to_cfs_overlay_item(
> >> + struct config_item *item)
> >> +{
> >> + return item ? container_of(item, struct cfs_overlay_item, item) : NULL;
> >> +}
> >> +
> >> +CONFIGFS_ATTR_STRUCT(cfs_overlay_item);
> >> +#define CFS_OVERLAY_ITEM_ATTR(_name, _mode, _show, _store) \
> >> +struct cfs_overlay_item_attribute cfs_overlay_item_attr_##_name = \
> >> + __CONFIGFS_ATTR(_name, _mode, _show, _store)
> >> +#define CFS_OVERLAY_ITEM_ATTR_RO(_name, _show) \
> >> +struct cfs_overlay_item_attribute cfs_overlay_item_attr_##_name = \
> >> + __CONFIGFS_ATTR_RO(_name, _show)
> >> +
> >> +CONFIGFS_BIN_ATTR_STRUCT(cfs_overlay_item);
> >> +#define CFS_OVERLAY_ITEM_BIN_ATTR(_name, _mode, _read, _write, _priv, _max) \
> >> +struct cfs_overlay_item_bin_attribute cfs_overlay_item_bin_attr_##_name = \
> >> + __CONFIGFS_BIN_ATTR(_name, _mode, _read, _write, _priv, _max)
> >> +#define CFS_OVERLAY_ITEM_BIN_ATTR_RO(_name, _read, _priv, _max) \
> >> +struct cfs_overlay_item_bin_attribute cfs_overlay_item_bin_attr_##_name = \
> >> + __CONFIGFS_BIN_ATTR_RO(_name, _read, _priv, _max)
> >> +
> >> +static ssize_t cfs_overlay_item_path_show(struct cfs_overlay_item *overlay,
> >> + char *page)
> >> +{
> >> + return sprintf(page, "%s\n", overlay->path);
> >> +}
> >> +
> >> +static ssize_t cfs_overlay_item_path_store(struct cfs_overlay_item *overlay,
> >> + const char *page, size_t count)
> >> +{
> >> + const char *p = page;
> >> + char *s;
> >> + int err;
> >> +
> >> + /* if it's set do not allow changes */
> >> + if (overlay->path[0] != '\0' || overlay->dtbo_size > 0)
> >> + return -EPERM;
> >> +
> >> + /* copy to path buffer (and make sure it's always zero terminated */
> >> + count = snprintf(overlay->path, sizeof(overlay->path) - 1, "%s", p);
> >> + overlay->path[sizeof(overlay->path) - 1] = '\0';
> >> +
> >> + /* strip trailing newlines */
> >> + s = overlay->path + strlen(overlay->path);
> >> + while (s > overlay->path && *--s == '\n')
> >> + *s = '\0';
> >> +
> >> + pr_debug("%s: path is '%s'\n", __func__, overlay->path);
> >> +
> >> + err = request_firmware(&overlay->fw, overlay->path, NULL);
> >> + if (err != 0)
> >> + goto out_err;
> >> +
> >> + err = create_overlay(overlay, (void *)overlay->fw->data);
> >> + if (err != 0)
> >> + goto out_err;
> >> +
> >> + return count;
> >> +
> >> +out_err:
> >> +
> >> + release_firmware(overlay->fw);
> >> + overlay->fw = NULL;
> >> +
> >> + overlay->path[0] = '\0';
> >> + return err;
> >> +}
> >> +
> >> +static ssize_t cfs_overlay_item_status_show(struct cfs_overlay_item *overlay,
> >> + char *page)
> >> +{
> >> + return sprintf(page, "%s\n",
> >> + overlay->ov_id >= 0 ? "applied" : "unapplied");
> >> +}
> >> +
> >> +CFS_OVERLAY_ITEM_ATTR(path, S_IRUGO | S_IWUSR,
> >> + cfs_overlay_item_path_show, cfs_overlay_item_path_store);
> >
> > World writable? Am I reading this correctly?
> >
>
> Err, user writeable, world readable. â??-rw-r--râ??"
Oops, I did read it wrong. Sorry for the noise
> > DT modifications are privileged. A user can potentially get arbitrary
> > access to i2c, spi, gpio or other things that shouldn't be allowed. This
> > feature give access right into the Linux driver model.
> >
>
> Yes, it does.
>
> > Before this can be merged, it needs to be locked down, and there needs
> > to be documentation about how it is locked down. Owned by root is only
> > the first step, there also needs to be some rules about which nodes can
> > be modified by the configfs interface. By default think no modifications
> > should be allowed on a tree unless there are properties somewhere in the
> > tree that explicitly allow modifications to be performed.
> >
>
> TBH this is more of a debug level interface. The way I see it a different
> overlay manager, thatâ??s tuned to the platform, should handle the overlay
> request and application, in a manner thatâ??s not directly open to user
> control. For example in a beaglebone youâ??d have a â??capeâ?? manager reading
> the i2c eeproms and request the overlays they match without any user-space
> implication.
Right.
> However, this is an interface that developers will use daily, so I agree
> that we need to look into the security model now before itâ??s too late.
>
> How do you feel for something like this in chosen:
>
> overlay-targets = â??/ocpâ??, â??/pinctrlâ??;
It's a reasonable proposal. I'd like to have some time to think on it
though. The other way to do it would be to add properties throughout the
tree that mask/unmask modifications by overlay. I think the overlays are
more a property of the board than a configurable boot time argument (so
not something that would normally go in /chosen)
g.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
