Re: [PATCH v4 06/12] ppc64/kexec_file: restrict memory usage of kdump kernel
Hari Bathini writes:
> On 24/07/20 5:36 am, Thiago Jung Bauermann wrote:
>>
>> Hari Bathini 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
>>> Tested-by: Pingfan Liu
>>> ---
>>>
>>> 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.
>>
>>
>>
>>> +/**
>>> + * 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?
>
> Actually, it does return 11. +1 while returning is for counting %NUL.
> On top of that we count an extra '/' for root node.. so, it ends up as 11.
> ('/'memory@0'/''\0'). Note the extra '/' before '\0'. Let me handle root node
> separately. That should avoid the confusion.
Ah, that is true. I forgot to count the iteration for the root node.
Sorry about that.
--
Thiago Jung Bauermann
IBM Linux Technology Center
Re: [PATCH v4 06/12] ppc64/kexec_file: restrict memory usage of kdump kernel
On 24/07/20 5:36 am, Thiago Jung Bauermann wrote:
>
> Hari Bathini 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
>> Tested-by: Pingfan Liu
>> ---
>>
>> 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.
>
>
>
>> +/**
>> + * 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?
Actually, it does return 11. +1 while returning is for counting %NUL.
On top of that we count an extra '/' for root node.. so, it ends up as 11.
('/'memory@0'/''\0'). Note the extra '/' before '\0'. Let me handle root node
separately. That should avoid the confusion.
>> + *
>> + * 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.
Sure, will update the function name.
Thanks
Hari
Re: [PATCH v4 06/12] ppc64/kexec_file: restrict memory usage of kdump kernel
Hari Bathini 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
> Tested-by: Pingfan Liu
> ---
>
> 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.
> +/**
> + * 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
[PATCH v4 06/12] ppc64/kexec_file: restrict memory usage of kdump kernel
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
Tested-by: Pingfan Liu
---
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.
* Fixed prop pointer update in add_usable_mem_property() and changed
duple to tuple as suggested by Thiago.
v2 -> v3:
* Unchanged. Added Tested-by tag from Pingfan.
v1 -> v2:
* Fixed off-by-one error while setting up usable-memory properties.
* Updated add_rtas_mem_range() & add_opal_mem_range() callsites based on
the new prototype for these functions.
arch/powerpc/kexec/file_load_64.c | 472 +
1 file changed, 471 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kexec/file_load_64.c
b/arch/powerpc/kexec/file_load_64.c
index 2df6f42..71c1ba7 100644
--- a/arch/powerpc/kexec/file_load_64.c
+++ b/arch/powerpc/kexec/file_load_64.c
@@ -17,9 +17,21 @@
#include
#include
#include
+#include
#include
+#include
+#include
#include
+struct umem_info {
+ uint64_t *buf; /* data buffer for usable-memory property */
+ uint32_t idx; /* current index */
+ uint32_t size; /* size allocated for the data buffer */
+
+ /* usable memory ranges to look up */
+ const struct crash_mem *umrngs;
+};
+
const struct kexec_file_ops * const kexec_file_loaders[] = {
_elf64_ops,
NULL
@@ -75,6 +87,42 @@ static int get_exclude_memory_ranges(struct crash_mem
**mem_ranges)
}
/**
+ * get_usable_memory_ranges - Get usable memory ranges. This list includes
+ *regions like crashkernel, opal/rtas & tce-table,
+ *that kdump kernel could use.
+ * @mem_ranges: Range list to add the memory ranges to.
+ *
+ * Returns 0 on success, negative errno on error.
+ */
+static int get_usable_memory_ranges(struct crash_mem **mem_ranges)
+{
+ int ret;
+
+ /*
+* prom code doesn't take kindly to missing low memory. So, add
+* [0, crashk_res.end] instead of [crashk_res.start, crashk_res.end]
+* to keep it happy.
+*/
+ ret = add_mem_range(mem_ranges, 0, crashk_res.end + 1);
+ if (ret)
+ goto out;
+
+ ret = add_rtas_mem_range(mem_ranges);
+ if (ret)
+ goto out;
+
+ ret = add_opal_mem_range(mem_ranges);
+ if (ret)
+ goto out;
+
+ ret = add_tce_mem_ranges(mem_ranges);
+out:
+ if (ret)
+ pr_err("Failed to setup usable memory ranges\n");
+ return ret;
+}
+
+/**
* __locate_mem_hole_top_down - Looks top down for a large enough memory hole
* in the memory regions between buf_min & buf_max
* for the buffer. If found, sets kbuf->mem.
@@ -274,6 +322,376 @@ static int locate_mem_hole_bottom_up_ppc64(struct
kexec_buf *kbuf,
}
/**
+ * check_realloc_usable_mem - Reallocate buffer if it can't accommodate entries
+ * @um_info: Usable memory buffer and ranges info.
+ * @cnt: No. of entries to accommodate.
+ *
+ * Frees up the old buffer if memory reallocation fails.
+ *
+ * Returns buffer on success, NULL on error.
+ */
+static uint64_t *check_realloc_usable_mem(struct umem_info *um_info, int cnt)
+{
+ void *tbuf;
+
+ if (um_info->size >=
+ ((um_info->idx + cnt) * sizeof(*(um_info->buf
+ return um_info->buf;
+
+ um_info->size += MEM_RANGE_CHUNK_SZ;
+ tbuf = krealloc(um_info->buf, um_info->size, GFP_KERNEL);
+ if (!tbuf) {
+ um_info->size -= MEM_RANGE_CHUNK_SZ;
+ return NULL;
+ }
+
+ memset(tbuf + um_info->idx, 0, MEM_RANGE_CHUNK_SZ);
+ return tbuf;
+}
+
+/**
+ * add_usable_mem - Add the usable memory ranges within the given memory range
+ * to the buffer
+ * @um_info:Usable memory buffer and ranges info.
+ * @base: Base address of memory range to look for.
+ * @end:End address of memory range to look for.
+ * @cnt:No. of usable memory ranges added to buffer.
+ *
+ * Returns 0 on success, negative errno on error.
+ */
+static int add_usable_mem(struct umem_info *um_info, uint64_t base,
+
