diff mbox series

[v2,12/18] of: overlay: check prevents multiple fragments add or delete same node

Message ID 1539406418-18162-13-git-send-email-frowand.list@gmail.com
State Superseded, archived
Headers show
Series of: overlay: validation checks, subsequent fixes | expand

Commit Message

Frank Rowand Oct. 13, 2018, 4:53 a.m. UTC 
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

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---

checkpatch errors "line over 80 characters" are ok, they will be
fixed two patches later in this series

 drivers/of/overlay.c | 58 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 49 insertions(+), 9 deletions(-)

Comments

Joe Perches Oct. 13, 2018, 12:51 p.m. UTC | #1 
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;
			}
		}
	}
Frank Rowand Oct. 13, 2018, 6:21 p.m. UTC | #2 
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;
> 			}
> 		}
> 	}
> 
> 
>
Frank Rowand Oct. 15, 2018, 12:29 a.m. UTC | #3 
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 mbox series

Patch

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);
 }
 
 /*