| Message ID | 1539406418-18162-13-git-send-email-frowand.list@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | of: overlay: validation checks, subsequent fixes | expand |
On Fri, 2018-10-12 at 21:53 -0700, frowand.list@gmail.com wrote: > From: Frank Rowand <frank.rowand@sony.com> > > Multiple overlay fragments adding or deleting the same node is not > supported. Replace code comment of such, with check to detect the > attempt and fail the overlay apply. > > Devicetree unittest where multiple fragments added the same node was > added in the previous patch in the series. After applying this patch > the unittest messages will no longer include: > > Duplicate name in motor-1, renamed to "controller#1" > OF: overlay: of_overlay_apply() err=0 > ### dt-test ### of_overlay_fdt_apply() expected -22, ret=0, overlay_bad_add_dup_node > ### dt-test ### FAIL of_unittest_overlay_high_level():2419 Adding overlay 'overlay_bad_add_dup_node' failed > > ... > > ### dt-test ### end of unittest - 210 passed, 1 failed > > but will instead include: > > OF: overlay: ERROR: multiple overlay fragments add and/or delete node /testcase-data-2/substation@100/motor-1/controller > > ... > > ### dt-test ### end of unittest - 211 passed, 0 failed [] > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c [] > @@ -523,6 +515,54 @@ static int build_changeset_symbols_node(struct overlay_changeset *ovcs, > } > > /** > + * check_changeset_dup_add_node() - changeset validation: duplicate add node > + * @ovcs: Overlay changeset > + * > + * Check changeset @ovcs->cset for multiple add node entries for the same > + * node. > + * > + * Returns 0 on success, -ENOMEM if memory allocation failure, or -EINVAL if > + * invalid overlay in @ovcs->fragments[]. > + */ > +static int check_changeset_dup_add_node(struct overlay_changeset *ovcs) > +{ > + struct of_changeset_entry *ce_1, *ce_2; > + char *fn_1, *fn_2; > + int name_match; > + > + list_for_each_entry(ce_1, &ovcs->cset.entries, node) { > + > + if (ce_1->action == OF_RECONFIG_ATTACH_NODE || > + ce_1->action == OF_RECONFIG_DETACH_NODE) { > + > + ce_2 = ce_1; > + list_for_each_entry_continue(ce_2, &ovcs->cset.entries, node) { > + if (ce_2->action == OF_RECONFIG_ATTACH_NODE || > + ce_2->action == OF_RECONFIG_DETACH_NODE) { > + /* inexpensive name compare */ > + if (!of_node_cmp(ce_1->np->full_name, > + ce_2->np->full_name)) { A bit of odd indentation here. This line is normally aligned to the second ( on the line above. > + /* expensive full path name compare */ > + fn_1 = kasprintf(GFP_KERNEL, "%pOF", ce_1->np); > + fn_2 = kasprintf(GFP_KERNEL, "%pOF", ce_2->np); > + name_match = !strcmp(fn_1, fn_2); > + kfree(fn_1); > + kfree(fn_2); > + if (name_match) { > + pr_err("ERROR: multiple overlay fragments add and/or delete node %pOF\n", > + ce_1->np); > + return -EINVAL; > + } > + } > + } > + } > + } > + } > + > + return 0; > +} Style trivia: Using inverted tests and continue would reduce indentation. list_for_each_entry(ce_1, &ovcs->cset.entries, node) { if (ce_1->action != OF_RECONFIG_ATTACH_NODE && ce_1->action != OF_RECONFIG_DETACH_NODE) continue; ce_2 = ce_1; list_for_each_entry_continue(ce_2, &ovcs->cset.entries, node) { if (ce_2->action != OF_RECONFIG_ATTACH_NODE && ce_2->action != OF_RECONFIG_DETACH_NODE) continue; /* inexpensive name compare */ if (of_node_cmp(ce_1->np->full_name, ce_2->np->full_name)) continue; /* expensive full path name compare */ fn_1 = kasprintf(GFP_KERNEL, "%pOF", ce_1->np); fn_2 = kasprintf(GFP_KERNEL, "%pOF", ce_2->np); name_match = !strcmp(fn_1, fn_2); kfree(fn_1); kfree(fn_2); if (name_match) { pr_err("ERROR: multiple overlay fragments add and/or delete node %pOF\n", ce_1->np); return -EINVAL; } } }
On 10/13/18 05:51, Joe Perches wrote: > On Fri, 2018-10-12 at 21:53 -0700, frowand.list@gmail.com wrote: >> From: Frank Rowand <frank.rowand@sony.com> >> >> Multiple overlay fragments adding or deleting the same node is not >> supported. Replace code comment of such, with check to detect the >> attempt and fail the overlay apply. >> >> Devicetree unittest where multiple fragments added the same node was >> added in the previous patch in the series. After applying this patch >> the unittest messages will no longer include: >> >> Duplicate name in motor-1, renamed to "controller#1" >> OF: overlay: of_overlay_apply() err=0 >> ### dt-test ### of_overlay_fdt_apply() expected -22, ret=0, overlay_bad_add_dup_node >> ### dt-test ### FAIL of_unittest_overlay_high_level():2419 Adding overlay 'overlay_bad_add_dup_node' failed >> >> ... >> >> ### dt-test ### end of unittest - 210 passed, 1 failed >> >> but will instead include: >> >> OF: overlay: ERROR: multiple overlay fragments add and/or delete node /testcase-data-2/substation@100/motor-1/controller >> >> ... >> >> ### dt-test ### end of unittest - 211 passed, 0 failed > [] >> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c > [] >> @@ -523,6 +515,54 @@ static int build_changeset_symbols_node(struct overlay_changeset *ovcs, >> } >> >> /** >> + * check_changeset_dup_add_node() - changeset validation: duplicate add node >> + * @ovcs: Overlay changeset >> + * >> + * Check changeset @ovcs->cset for multiple add node entries for the same >> + * node. >> + * >> + * Returns 0 on success, -ENOMEM if memory allocation failure, or -EINVAL if >> + * invalid overlay in @ovcs->fragments[]. >> + */ >> +static int check_changeset_dup_add_node(struct overlay_changeset *ovcs) >> +{ >> + struct of_changeset_entry *ce_1, *ce_2; >> + char *fn_1, *fn_2; >> + int name_match; >> + >> + list_for_each_entry(ce_1, &ovcs->cset.entries, node) { >> + >> + if (ce_1->action == OF_RECONFIG_ATTACH_NODE || >> + ce_1->action == OF_RECONFIG_DETACH_NODE) { >> + >> + ce_2 = ce_1; >> + list_for_each_entry_continue(ce_2, &ovcs->cset.entries, node) { >> + if (ce_2->action == OF_RECONFIG_ATTACH_NODE || >> + ce_2->action == OF_RECONFIG_DETACH_NODE) { >> + /* inexpensive name compare */ >> + if (!of_node_cmp(ce_1->np->full_name, >> + ce_2->np->full_name)) { > > A bit of odd indentation here. > This line is normally aligned to the second ( on the line above. Yes, thanks. > >> + /* expensive full path name compare */ >> + fn_1 = kasprintf(GFP_KERNEL, "%pOF", ce_1->np); >> + fn_2 = kasprintf(GFP_KERNEL, "%pOF", ce_2->np); >> + name_match = !strcmp(fn_1, fn_2); >> + kfree(fn_1); >> + kfree(fn_2); >> + if (name_match) { >> + pr_err("ERROR: multiple overlay fragments add and/or delete node %pOF\n", >> + ce_1->np); >> + return -EINVAL; >> + } >> + } >> + } >> + } >> + } >> + } >> + >> + return 0; >> +} > > Style trivia: > > Using inverted tests and continue would reduce indentation. Yes, thanks. -Frank > > list_for_each_entry(ce_1, &ovcs->cset.entries, node) { > if (ce_1->action != OF_RECONFIG_ATTACH_NODE && > ce_1->action != OF_RECONFIG_DETACH_NODE) > continue; > > ce_2 = ce_1; > list_for_each_entry_continue(ce_2, &ovcs->cset.entries, node) { > if (ce_2->action != OF_RECONFIG_ATTACH_NODE && > ce_2->action != OF_RECONFIG_DETACH_NODE) > continue; > > /* inexpensive name compare */ > if (of_node_cmp(ce_1->np->full_name, ce_2->np->full_name)) > continue; > > /* expensive full path name compare */ > fn_1 = kasprintf(GFP_KERNEL, "%pOF", ce_1->np); > fn_2 = kasprintf(GFP_KERNEL, "%pOF", ce_2->np); > name_match = !strcmp(fn_1, fn_2); > kfree(fn_1); > kfree(fn_2); > if (name_match) { > pr_err("ERROR: multiple overlay fragments add and/or delete node %pOF\n", > ce_1->np); > return -EINVAL; > } > } > } > > >
On 10/13/18 11:21, Frank Rowand wrote: > On 10/13/18 05:51, Joe Perches wrote: >> On Fri, 2018-10-12 at 21:53 -0700, frowand.list@gmail.com wrote: >>> From: Frank Rowand <frank.rowand@sony.com> >>> >>> Multiple overlay fragments adding or deleting the same node is not >>> supported. Replace code comment of such, with check to detect the >>> attempt and fail the overlay apply. >>> >>> Devicetree unittest where multiple fragments added the same node was >>> added in the previous patch in the series. After applying this patch >>> the unittest messages will no longer include: >>> >>> Duplicate name in motor-1, renamed to "controller#1" >>> OF: overlay: of_overlay_apply() err=0 >>> ### dt-test ### of_overlay_fdt_apply() expected -22, ret=0, overlay_bad_add_dup_node >>> ### dt-test ### FAIL of_unittest_overlay_high_level():2419 Adding overlay 'overlay_bad_add_dup_node' failed >>> >>> ... >>> >>> ### dt-test ### end of unittest - 210 passed, 1 failed >>> >>> but will instead include: >>> >>> OF: overlay: ERROR: multiple overlay fragments add and/or delete node /testcase-data-2/substation@100/motor-1/controller >>> >>> ... >>> >>> ### dt-test ### end of unittest - 211 passed, 0 failed >> [] >>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c >> [] >>> @@ -523,6 +515,54 @@ static int build_changeset_symbols_node(struct overlay_changeset *ovcs, >>> } >>> >>> /** >>> + * check_changeset_dup_add_node() - changeset validation: duplicate add node >>> + * @ovcs: Overlay changeset >>> + * >>> + * Check changeset @ovcs->cset for multiple add node entries for the same >>> + * node. >>> + * >>> + * Returns 0 on success, -ENOMEM if memory allocation failure, or -EINVAL if >>> + * invalid overlay in @ovcs->fragments[]. >>> + */ >>> +static int check_changeset_dup_add_node(struct overlay_changeset *ovcs) >>> +{ >>> + struct of_changeset_entry *ce_1, *ce_2; >>> + char *fn_1, *fn_2; >>> + int name_match; >>> + >>> + list_for_each_entry(ce_1, &ovcs->cset.entries, node) { >>> + >>> + if (ce_1->action == OF_RECONFIG_ATTACH_NODE || >>> + ce_1->action == OF_RECONFIG_DETACH_NODE) { >>> + >>> + ce_2 = ce_1; >>> + list_for_each_entry_continue(ce_2, &ovcs->cset.entries, node) { >>> + if (ce_2->action == OF_RECONFIG_ATTACH_NODE || >>> + ce_2->action == OF_RECONFIG_DETACH_NODE) { >>> + /* inexpensive name compare */ >>> + if (!of_node_cmp(ce_1->np->full_name, >>> + ce_2->np->full_name)) { >> >> A bit of odd indentation here. >> This line is normally aligned to the second ( on the line above. > > Yes, thanks. This line gets joined into a single line in version 3, so I will leave the bad formatting in patch 12 to make my life easier when moving to version 3. >> >>> + /* expensive full path name compare */ >>> + fn_1 = kasprintf(GFP_KERNEL, "%pOF", ce_1->np); >>> + fn_2 = kasprintf(GFP_KERNEL, "%pOF", ce_2->np); >>> + name_match = !strcmp(fn_1, fn_2); >>> + kfree(fn_1); >>> + kfree(fn_2); >>> + if (name_match) { >>> + pr_err("ERROR: multiple overlay fragments add and/or delete node %pOF\n", >>> + ce_1->np); >>> + return -EINVAL; >>> + } >>> + } >>> + } >>> + } >>> + } >>> + } >>> + >>> + return 0; >>> +} >> >> Style trivia: >> >> Using inverted tests and continue would reduce indentation. > > Yes, thanks. In version 3, fixed in patch 13/18 instead of 12/18, where this pattern has been split into two functions. -Frank > > -Frank > > >> >> list_for_each_entry(ce_1, &ovcs->cset.entries, node) { >> if (ce_1->action != OF_RECONFIG_ATTACH_NODE && >> ce_1->action != OF_RECONFIG_DETACH_NODE) >> continue; >> >> ce_2 = ce_1; >> list_for_each_entry_continue(ce_2, &ovcs->cset.entries, node) { >> if (ce_2->action != OF_RECONFIG_ATTACH_NODE && >> ce_2->action != OF_RECONFIG_DETACH_NODE) >> continue; >> >> /* inexpensive name compare */ >> if (of_node_cmp(ce_1->np->full_name, ce_2->np->full_name)) >> continue; >> >> /* expensive full path name compare */ >> fn_1 = kasprintf(GFP_KERNEL, "%pOF", ce_1->np); >> fn_2 = kasprintf(GFP_KERNEL, "%pOF", ce_2->np); >> name_match = !strcmp(fn_1, fn_2); >> kfree(fn_1); >> kfree(fn_2); >> if (name_match) { >> pr_err("ERROR: multiple overlay fragments add and/or delete node %pOF\n", >> ce_1->np); >> return -EINVAL; >> } >> } >> } >> >> >> > >
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c index a3990c20e210..b0a0dafb6a13 100644 --- a/drivers/of/overlay.c +++ b/drivers/of/overlay.c @@ -398,14 +398,6 @@ static int add_changeset_property(struct overlay_changeset *ovcs, * a live devicetree created from Open Firmware. * * NOTE_2: Multiple mods of created nodes not supported. - * If more than one fragment contains a node that does not already exist - * in the live tree, then for each fragment of_changeset_attach_node() - * will add a changeset entry to add the node. When the changeset is - * applied, __of_attach_node() will attach the node twice (once for - * each fragment). At this point the device tree will be corrupted. - * - * TODO: add integrity check to ensure that multiple fragments do not - * create the same node. * * Returns 0 on success, -ENOMEM if memory allocation failure, or -EINVAL if * invalid @overlay. @@ -523,6 +515,54 @@ static int build_changeset_symbols_node(struct overlay_changeset *ovcs, } /** + * check_changeset_dup_add_node() - changeset validation: duplicate add node + * @ovcs: Overlay changeset + * + * Check changeset @ovcs->cset for multiple add node entries for the same + * node. + * + * Returns 0 on success, -ENOMEM if memory allocation failure, or -EINVAL if + * invalid overlay in @ovcs->fragments[]. + */ +static int check_changeset_dup_add_node(struct overlay_changeset *ovcs) +{ + struct of_changeset_entry *ce_1, *ce_2; + char *fn_1, *fn_2; + int name_match; + + list_for_each_entry(ce_1, &ovcs->cset.entries, node) { + + if (ce_1->action == OF_RECONFIG_ATTACH_NODE || + ce_1->action == OF_RECONFIG_DETACH_NODE) { + + ce_2 = ce_1; + list_for_each_entry_continue(ce_2, &ovcs->cset.entries, node) { + if (ce_2->action == OF_RECONFIG_ATTACH_NODE || + ce_2->action == OF_RECONFIG_DETACH_NODE) { + /* inexpensive name compare */ + if (!of_node_cmp(ce_1->np->full_name, + ce_2->np->full_name)) { + /* expensive full path name compare */ + fn_1 = kasprintf(GFP_KERNEL, "%pOF", ce_1->np); + fn_2 = kasprintf(GFP_KERNEL, "%pOF", ce_2->np); + name_match = !strcmp(fn_1, fn_2); + kfree(fn_1); + kfree(fn_2); + if (name_match) { + pr_err("ERROR: multiple overlay fragments add and/or delete node %pOF\n", + ce_1->np); + return -EINVAL; + } + } + } + } + } + } + + return 0; +} + +/** * build_changeset() - populate overlay changeset in @ovcs from @ovcs->fragments * @ovcs: Overlay changeset * @@ -577,7 +617,7 @@ static int build_changeset(struct overlay_changeset *ovcs) } } - return 0; + return check_changeset_dup_add_node(ovcs); } /*