On Fri, Mar 16, 2018 at 06:37:55PM +0530, Srivatsa Vaddagiri wrote:
> When merging two overlay blobs, fragment nodes whose target node can't be found
> in base node would need to be retained as-is (including the fragment names) in
> the combined blob. Such unresolved symbols will also need to be listed in
> __fixups__ section of combined blob. This could lead to name comflicts in
> combined blob (two nodes with same name/path such as /fragment@0).
>
> To avoid such name conflicts in combined blob, rename all fragment@xyz in
> overlay blob as fragment@xyz+delta, where delta is the maximum count of fragment
> nodes found in base blob
>
> Signed-off-by: Srivatsa Vaddagiri <vatsa@xxxxxxxxxxxxxx>
>
> ---
> libfdt/fdt_overlay.c | 390 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 390 insertions(+)
>
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> index 81b9feb..556551e 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -53,6 +53,8 @@
>
> #include <fdt.h>
> #include <libfdt.h>
> +#include <stdio.h>
> +#include <assert.h>
>
> #include "libfdt_internal.h"
>
> @@ -67,6 +69,114 @@ void fdt_overlay_set_verbose(int val)
> verbose_overlay = val;
> }
>
> +int base_has_fixups;
> +int base_fixup_search_done;
> +unsigned long max_base_fragments;
> +
> +#define PATH_MAX 512
> +#define ULONG_MAX (~0UL)
> +
> +#define ALIGN(x) (((x) + (FDT_TAGSIZE) - 1) & ~((FDT_TAGSIZE) - 1))
> +
> +static inline void *xrealloc(void *p, size_t len)
As noted on 1/6 libfdt does not require an allocator, as a deliberate
design decision. I especially don't want to rely on an allocator
which supplies realloc().
> +{
> + void *new = realloc(p, len);
> +
> + if (!new) {
> + dprintf("FATAL ERROR: realloc() failed for %lu bytes\n", len);
> + exit(1);
exit() is also a new dependency which would not make sense in a number
of environments which aren't like regular userspace.
> + }
> +
> + return new;
> +}
> +
> +static inline void *xmalloc(size_t len)
> +{
> + void *new = malloc(len);
> +
> + if (!new) {
> + dprintf("FATAL ERROR: malloc() failed for %lu bytes\n", len);
> + exit(1);
> + }
> +
> + return new;
> +}
> +
> +static char *realloc_fdt(char *fdt, int delta)
> +{
> + int new_sz;
> +
> + delta = ALIGN(delta);
> + assert(delta > 0);
> +
> + new_sz = fdt_totalsize(fdt) + delta;
> +
> + fdt = xrealloc(fdt, new_sz);
> + fdt_set_totalsize(fdt, new_sz);
> +
> + return fdt;
> +}
> +/**
> + * fdt_set_name_memcheck - Change name of a node, increasing the size of the
> + * blob in the process if required
> + *
> + * @fdt - Pointer to pointer that points to device-tree blob
> + * @offset - Offset to the node
> + * @orig_len - Length of node's current name
> + * @name - new name of the node
> + */
> +static int fdt_set_name_memcheck(void **fdt, int offset,
> + int orig_len, char *name)
> +{
> + int err, lendiff;
> +
> + err = fdt_set_name(*fdt, offset, name);
> + if (!err)
> + return 0;
> +
> + if (err != -FDT_ERR_NOSPACE)
> + return err;
> +
> + lendiff = strlen(name) + 1 - orig_len;
> +
> + *fdt = realloc_fdt(*fdt, lendiff);
> +
> + return fdt_set_name(*fdt, offset, name);
> +}
> +
> +/**
> + * fdt_setprop_placeholder_memcheck - Set property value
> + *
> + * This is a wrapper around fdt_setprop_placeholder(). It increases the size of
> + * DT blob in case fdt_setprop_placeholder() fails because of lack of space
> + *
> + * @fdt - pointer to pointer that point to a device-tree blob
> + * @offset - Offset of property whose length needs to be increased to @new_len
> + * @name - name of the property
> + * @orig_len - Current length of the property value
> + * @new_len - New length of the property value
> + * @prop_data - Used to store pointer to property value of length @new_len
> + */
> +static int fdt_setprop_placeholder_memcheck(void **fdt, int offset,
> + const char *name, int orig_len, int new_len, void **prop_data)
> +{
> + int err;
> +
> + assert(new_len >= orig_len);
> +
> + err = fdt_setprop_placeholder(*fdt, offset, name, new_len, prop_data);
> + if (!err)
> + return 0;
> +
> + if (err != -FDT_ERR_NOSPACE)
> + return err;
> +
> + *fdt = realloc_fdt(*fdt, new_len - orig_len);
> +
> + return fdt_setprop_placeholder(*fdt, offset, name, new_len, prop_data);
> +}
> +
> /**
> * overlay_get_target_phandle - retrieves the target phandle of a fragment
> * @fdto: pointer to the device tree overlay blob
> @@ -874,15 +984,295 @@ static int overlay_symbol_update(void *fdt, void *fdto)
> return 0;
> }
>
> +/* Return maximum count of nodes that are named fragment@xyz */
> +static unsigned long count_fragments(void *fdt)
> +{
> + int offset = -1;
> + const char *name, *idx;
> + char *stop;
> + unsigned long index, max = 0;
> + int len;
> +
> + do {
> + offset = fdt_next_node(fdt, offset, NULL);
> + if (offset < 0)
> + break;
> +
> + name = fdt_get_name(fdt, offset, &len);
> + if (len > 9 && !strncmp(name, "fragment@", 9)) {
> + idx = name + 9;
> + index = strtoul(idx, &stop, 10);
> + if (index > max)
> + max = index;
> + }
This is wrong on multiple counts. 1) You should only check top level
nodes, not all of them, 2) fragments aren't necessarily named
"fragment@XX", that's just a convention - several people have
suggested using "fragment-XX" to avoid some other problems, for
example. The correct way to identify fragments is to look for the
__overlay__ subnode. 3) You have no error handling if the unit
address doesn't parse as a decimal integer.
Come to that, I'm not sure it's ever been properly specified whether
the number on a fragment is in decimal or not. Usually unit names are
in hex, but fragments are weird in a number of ways.
> + } while (1);
> +
> + return max;
> +}
> +
> +static void copy_buf(const char **start, int len, char **pp,
> + int *rembufsize, char **bufp, int *bufsize)
> +{
> + int delta;
> +
> + if (*rembufsize < len) {
> + delta = (len - *rembufsize) + 1024;
> + *bufp = xrealloc(*bufp, *bufsize + delta);
> + *bufsize += delta;
> + *rembufsize += delta;
> + }
> + memcpy(*pp, *start, len);
> + *start += len;
> + *pp += len;
> + *rembufsize -= len;
> +}
> +
> +/**
> + * rename_fragments_in_section - Rename fragment@xyz instances in a section
What does "section" mean? It's not a standard term in connection to
fdts.
> + *
> + * @fdto - pointer to pointer that points to a device-tree blob
> + * @secname - Section in which fragments need to be renamed
> + * @delta - Increment to be applied to fragment index
> + */
> +static int rename_fragments_in_section(void **fdto, const char *secname,
> + unsigned long delta)
> +{
> + int offset, len, new_len, property;
> + unsigned long index, new_index;
> + int bufsize, rembufsize, err;
> +
> + offset = fdt_path_offset(*fdto, secname);
> + if (offset < 0) {
> + dprintf("%s section not found\n", secname);
> + return -FDT_ERR_NOTFOUND;
> + }
> +
> + fdt_for_each_property_offset(property, *fdto, offset) {
> + const char *value, *end, *sep, *start, *copy_start;
> + const char *label;
> + int modified = 0, needed;
> + char *buf, *p;
> + void *q;
> +
> + value = fdt_getprop_by_offset(*fdto, property,
> + &label, &len);
> + if (!value) {
> + dprintf("Failed to get value of property at offset %d"
> + " in section %s (err %d)\n", property,
> + secname, len);
> + return -1;
That's equivalent to -FDT_ERR_NOTFOUND, which I don't think is what
you want.
> + }
> +
> + /*
> + * Property value could be in this format
> + * fragment@M ...fragment@N....fragment@O..
> + *
> + * This need to be converted to
> + * fragment@M+delta...fragment@N+delta....fragment@O+delta
> + *
> + * which could require more space than what is currently
> + * allocated to the property. 'buf' will be used to create the
> + * new value for the property, which can then be used to
> + * modify property's value
> + */
> +
> + /*
> + * 1024 bytes of additional space for holding extra characters
> + * if required
> + */
> + buf = xmalloc(len + 1024);
> + bufsize = len + 1024;
> + rembufsize = len + 1024;
> +
> + start = value;
> + end = value + len;
> + p = buf;
> + copy_start = start;
> +
> + while (start < end) {
> + char *stop;
> +
> + sep = memchr(start, '@', (end - start));
> + if (!sep)
> + break;
> +
> + /* Check if string "fragment" exists */
> + sep -= 8;
> + if (sep < start || strncmp(sep, "fragment", 8)) {
> + /* Start scan again after '@' */
> + start = sep + 9;
> + continue;
> + }
> +
> + sep += 9;
> +
> + /*
> + * copy characters from @copy_start to @sep into @p
> + * increasing the size of @buf if available space is
> + * less.
> + */
> + copy_buf(©_start, sep - copy_start, &p, &rembufsize,
> + &buf, &bufsize);
> + stop = NULL;
> + index = strtoul(sep, &stop, 10);
> + if (!stop || ULONG_MAX - index < delta) {
> + free(buf);
> + return -FDT_ERR_BADSTATE;
BADSTATE has a specific meaning, and this isn't it.
> + }
> +
> + new_index = index + delta;
> + needed = snprintf(NULL, 0, "%lu", new_index);
> + if (needed > rembufsize) {
> + long diff = needed - rembufsize;
> + if (diff < 1024)
> + diff = 1024;
> + buf = xrealloc(buf, bufsize + diff);
> + bufsize += diff;
> + rembufsize += diff;
> + }
> + modified = 1;
> + p += snprintf(p, rembufsize, "%lu", new_index);
> + start = stop;
> + copy_start = start;
> + }
> +
> + if (end > copy_start)
> + copy_buf(©_start, end - copy_start, &p,
> + &rembufsize, &buf, &bufsize);
> +
> + if (modified) {
> + new_len = p - buf;
> + err = fdt_setprop_placeholder_memcheck(fdto, offset,
> + label, len, new_len, &q);
> + if (err) {
> + free(buf);
> + return err;
> + }
> +
> + memcpy(q, buf, new_len);
> + }
> + free(buf);
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * rename_nodes - Rename all fragement@xyz nodes
> + *
> + * @fdto - pointer to pointer that points to device-tree blob
> + * @delta - increment to be added to fragment number
> + */
> +static int rename_nodes(void **fdto, unsigned long delta)
> +{
> + int offset = -1, err;
> + const char *name, *idx;
> + char *stop = NULL;
> + int len;
> + char new_name[PATH_MAX];
PATH_MAX is another new dependency that doesn't make sense for
arbitrary environments.
> + unsigned long index, new_index;
> +
> + do {
> + offset = fdt_next_node(*fdto, offset, NULL);
> + if (offset < 0)
> + break;
Again, you're mangling all nodes, not just top-level ones.
> + name = fdt_get_name(*fdto, offset, &len);
> + if (len > 9 && !strncmp(name, "fragment@", 9)) {
> + idx = name + 9;
> + stop = NULL;
> + index = strtoul(idx, &stop, 10);
> + if (ULONG_MAX - delta < index)
> + return -FDT_ERR_BADSTATE;
> + new_index = index + delta;
> + snprintf(new_name, sizeof(new_name),
> + "fragment@%lu", new_index);
> +
> + err = fdt_set_name_memcheck(fdto, offset,
> + len, new_name);
> + if (err < 0)
> + return err;
> + }
> + } while (1);
> +
> + return 0;
> +}
> +
> +/*
> + * In case of overlaying onto a base blob which itself has unresolved external
> + * phandle references, we will need to merge overlay fragments found in overlay
> + * blob into root node of base blob, which can lead to naming conflicts for
> + * nodes. To prevent such conflicts, rename all occurence of fragment@xyz in
> + * overlay blob with fragment@abc (abc = xyz + delta)
> + */
> +static int rename_fragments(void **fdto, unsigned long delta)
> +{
> + int rc;
> +
> + rc = rename_nodes(fdto, delta);
> + if (rc < 0) {
> + dprintf("rename_nodes failed %d\n", rc);
> + return rc;
> + }
> +
> + rc = rename_fragments_in_section(fdto, "/__symbols__", delta);
> + if (rc < 0 && rc != -FDT_ERR_NOTFOUND) {
> + dprintf("rename of fragments in __symbols__ section "
> + "failed %d\n", rc);
> + return rc;
> + }
> +
> + rc = rename_fragments_in_section(fdto, "/__fixups__", delta);
> + if (rc < 0) {
> + dprintf("rename of fragments in __fixups__ section "
> + "failed %d\n", rc);
> + return rc;
> + }
> +
> + /*
> + * renaming fragments in __local_fixups__ section should be covered by
> + * rename_nodes()
> + */
Ugh, I guess it will, but not safely. It's possible that some other
node further down in the tree could have the name "fragment@" in which
case it will get mangled improperly by this code.
> +
> + return 0;
> +}
> +
> int fdt_overlay_apply(void **fdt, void **fdto)
> {
> uint32_t delta = fdt_get_max_phandle(*fdt);
> int ret;
> + unsigned long max_fdto_fragments;
>
> FDT_CHECK_HEADER(*fdt);
> FDT_CHECK_HEADER(*fdto);
>
> dprintf("Adjusting local phandles\n");
> + /* Does base blob have unresolved external references? */
> + if (!base_fixup_search_done) {
> + if (fdt_path_offset(*fdt, "/__fixups__") >= 0)
> + base_has_fixups = 1;
> + base_fixup_search_done = 1;
> + }
Yeah, this is gross, just use a different entry point for the two cases.
> +
> + if (base_has_fixups) {
> + if (!max_base_fragments)
> + max_base_fragments = count_fragments(*fdt) + 1;
> + if ((long)max_base_fragments < 0)
> + return -FDT_ERR_BADSTATE;
This is not a safe overflow check.
> + max_fdto_fragments = count_fragments(*fdto) + 1;
> + if ((long)max_fdto_fragments < 0)
> + return -FDT_ERR_BADSTATE;
> + dprintf("max_base fragments %lu, max_overlay fragments %lu\n",
> + max_base_fragments, max_fdto_fragments);
> + ret = rename_fragments(fdto, max_base_fragments);
> + if (ret < 0) {
> + dprintf("rename_fragments failed %d\n", ret);
> + return ret;
> + }
> + max_base_fragments += max_fdto_fragments;
You adjust this value, then never use it again.
> + }
> +
> ret = overlay_adjust_local_phandles(*fdto, delta);
> if (ret)
> goto err;
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
