Re: [RESEND PATCH v5 06/11] ppc64/kexec_file: restrict memory usage of kdump kernel

2020-07-28 Thread Hari Bathini




On 28/07/20 7:14 pm, Michael Ellerman wrote:

Hari Bathini  writes:

diff --git a/arch/powerpc/kexec/file_load_64.c 
b/arch/powerpc/kexec/file_load_64.c
index 2df6f4273ddd..8df085a22fd7 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 */


Use kernel types please, u64, u32.


+   /* usable memory ranges to look up */
+   const struct crash_mem *umrngs;


"umrngs".

Given it's part of the umem_info struct could it just be "ranges"?


True. Actually, having crash_mem_range *ranges + u32 nr_ranges and 
populating them seems better. Will do that..



+   return NULL;
+   }


um_info->size = new_size;


+
+   memset(tbuf + um_info->idx, 0, MEM_RANGE_CHUNK_SZ);


Just pass __GFP_ZERO to krealloc?


There are patches submitted to stable fixing a few modules that use 
krealloc with __GFP_ZERO. Also, this zeroing is not really needed.

I will drop the memset instead..

Thanks
Hari


Re: [RESEND PATCH v5 06/11] ppc64/kexec_file: restrict memory usage of kdump kernel

2020-07-28 Thread Michael Ellerman
Hari Bathini  writes:
> diff --git a/arch/powerpc/kexec/file_load_64.c 
> b/arch/powerpc/kexec/file_load_64.c
> index 2df6f4273ddd..8df085a22fd7 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 */

Use kernel types please, u64, u32.

> + /* usable memory ranges to look up */
> + const struct crash_mem *umrngs;

"umrngs".

Given it's part of the umem_info struct could it just be "ranges"?

> +};
> +
>  const struct kexec_file_ops * const kexec_file_loaders[] = {
>   _elf64_ops,
>   NULL
> @@ -74,6 +86,42 @@ static int get_exclude_memory_ranges(struct crash_mem 
> **mem_ranges)
>   return ret;
>  }
>  
> +/**
> + * 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

I don't know what that's referring to, "prom code" is too vague.

> +  * [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
> @@ -273,6 +321,382 @@ static int locate_mem_hole_bottom_up_ppc64(struct 
> kexec_buf *kbuf,
>   return ret;
>  }
>  
> +/**
> + * 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;

This is awkward.

AFAICS you only use um_info->size here, so instead why not store the
number of u64s you have space for, as num for example.

Then the above comparison becomes:

if (um_info->num >= (um_info->idx + count))

Then you only have to calculate the size internally here for the
realloc.

> +
> + um_info->size += MEM_RANGE_CHUNK_SZ;

new_size = um_info->size + MEM_RANGE_CHUNK_SZ;
tbuf = krealloc(um_info->buf, new_size, GFP_KERNEL);

> + tbuf = krealloc(um_info->buf, um_info->size, GFP_KERNEL);
> + if (!tbuf) {
> + um_info->size -= MEM_RANGE_CHUNK_SZ;

Then you can drop this.

> + return NULL;
> + }

um_info->size = new_size;

> +
> + memset(tbuf + um_info->idx, 0, MEM_RANGE_CHUNK_SZ);

Just pass __GFP_ZERO to krealloc?

> + 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.

One caller never uses this AFAICS.

Couldn't the other caller just compare the um_info->idx before and after
the call, and avoid another pass by reference parameter.

> + *
> + * Returns 0 on success, negative errno on error.
> + */
> +static int add_usable_mem(struct umem_info *um_info, uint64_t base,
> +   uint64_t end, int *cnt)
> +{
> + uint64_t loc_base, loc_end, *buf;
> + const struct crash_mem *umrngs;
> + int i, add;

add should be bool.

> + *cnt = 0;
> + umrngs = um_info->umrngs;
> + for (i = 0; i < umrngs->nr_ranges; i++) {
> + add = 0;
> + loc_base = umrngs->ranges[i].start;
> + 

Re: [RESEND PATCH v5 06/11] ppc64/kexec_file: restrict memory usage of kdump kernel

2020-07-27 Thread Thiago Jung Bauermann


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 

I liked the new versions of get_node_path_size() and get_node_path().

Reviewed-by: Thiago Jung Bauermann 

--
Thiago Jung Bauermann
IBM Linux Technology Center