Re: [PATCH v4 06/12] ppc64/kexec_file: restrict memory usage of kdump kernel

Hari Bathini [off-list ref] writes:

Kdump kernel, used for capturing the kernel core image, is supposed
to use only specific memory regions to avoid corrupting the image to
be captured. The regions are crashkernel range - the memory reserved
explicitly for kdump kernel, memory used for the tce-table, the OPAL
region and RTAS region as applicable. Restrict kdump kernel memory
to use only these regions by setting up usable-memory DT property.
Also, tell the kdump kernel to run at the loaded address by setting
the magic word at 0x5c.

Signed-off-by: Hari Bathini <hbathini@linux.ibm.com>
Tested-by: Pingfan Liu <redacted>
---

v3 -> v4:
* Updated get_node_path() to be an iterative function instead of a
  recursive one.
* Added comment explaining why low memory is added to kdump kernel's
  usable memory ranges though it doesn't fall in crashkernel region.
* For correctness, added fdt_add_mem_rsv() for the low memory being
  added to kdump kernel's usable memory ranges.

Good idea.

* Fixed prop pointer update in add_usable_mem_property() and changed
  duple to tuple as suggested by Thiago.

<snip>

+/**
+ * get_node_pathlen - Get the full path length of the given node.
+ * @dn:               Node.
+ *
+ * Also, counts '/' at the end of the path.
+ * For example, /memory@0 will be "/memory@0/\0" => 11 bytes.

Wouldn't this function return 10 in the case of /memory@0?
Are you saying that it should count the \0 at the end too? it's not
doing that, AFAICS.

+ *
+ * Returns the string length of the node's full path.
+ */

Maybe it's me (by analogy with strlen()), but I would expect "string
length" to not include the terminating \0. I suggest renaming the
function to something like get_node_path_size() and do s/length/size/ in
the comment above if it's supposed to count the terminating \0.

+static int get_node_pathlen(struct device_node *dn)
+{
+	int len = 0;
+
+	if (!dn)
+		return 0;
+
+	while (dn) {
+		len += strlen(dn->full_name) + 1;
+		dn = dn->parent;
+	}
+
+	return len + 1;
+}
+
+/**
+ * get_node_path - Get the full path of the given node.
+ * @node:          Device node.
+ *
+ * Allocates buffer for node path. The caller must free the buffer
+ * after use.
+ *
+ * Returns buffer with path on success, NULL otherwise.
+ */
+static char *get_node_path(struct device_node *node)
+{
+	struct device_node *dn;
+	int len, idx, nlen;
+	char *path = NULL;
+	char end_char;
+
+	if (!node)
+		goto err;
+
+	/*
+	 * Get the path length first and use it to iteratively build the path
+	 * from node to root.
+	 */
+	len = get_node_pathlen(node);
+
+	/* Allocate memory for node path */
+	path = kzalloc(ALIGN(len, 8), GFP_KERNEL);
+	if (!path)
+		goto err;
+
+	/*
+	 * Iteratively update path from node to root by decrementing
+	 * index appropriately.
+	 *
+	 * Also, add %NUL at the end of node & '/' at the end of all its
+	 * parent nodes.
+	 */
+	dn = node;
+	path[0] = '/';
+	idx = len - 1;

Here, idx is pointing to the supposed '/' at the end of the node
path ...

+	end_char = '\0';
+	while (dn->parent) {
+		path[--idx] = end_char;

.. and in the first ireation, this is writing '\0' at a place which will be
overwritten by the memcpy() below with the last character of
dn->full_name. You need to start idx with len, not len - 1.

+		end_char = '/';
+
+		nlen = strlen(dn->full_name);
+		idx -= nlen;
+		memcpy(path + idx, dn->full_name, nlen);
+
+		dn = dn->parent;
+	}
+
+	return path;
+err:
+	kfree(path);
+	return NULL;
+}

--
Thiago Jung Bauermann
IBM Linux Technology Center