On Thu, Feb 29, 2024 at 6:08 PM Tao Liu <ltao@xxxxxxxxxx> wrote:
>
> Hi Yulong,
>
> Thanks for your patch!
>
> On Mon, Feb 26, 2024 at 3:20 PM Yulong TANG 汤玉龙 <yulong.tang@xxxxxxx> wrote:
> >
> > In Linux 5.1, the ZRAM block driver has changed its default compressor from "lzo" to "lzo-rle" to enhance LZO compression support. However, crash does not support the improved LZO algorithm, resulting in failure when reading memory.
> >
> > change default compressor : ce82f19fd5809f0cf87ea9f753c5cc65ca0673d6
> >
> >
> > The issue was discovered when using the extension 'gcore' to generate a process coredump, which was found to be incomplete and unable to be opened properly with gdb.
> >
> >
> > This patch is for Crash-utility tool, it enables the Crash-utility to support decompression of the "lzo-rle" compression algorithm used in zram. The patch has been tested with vmcore files from kernel version 5.4, and successfully allows reading of memory compressed with the zram compression algorithm.
>
> I have no objection to the lzo-rle decompression feature for crash.
> However I have some concern of your patch:
>
> The patch you attached is a "lzorle_decompress_safe" implementation
> which is copied from kernel source code. One of the drawbacks of
> copying kernel source code is, kernel is constantly evolving, the code
> you copied here today maybe updated someday later, and in support of
> different kernel versions, we need to keep a bunch of
> switch(kernel_version) and case code to keep the compatibility, which
> is what we are trying to avoid.
>
> In addition, the code you copied has deliberately deleted the "if
> defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)" part, which may also
> cause some problem, and as far as I know, there is no good way in
> crash to determine the kernel config status, please feel free to
> correct me if I'm wrong.
>
> I'm thinking of another way to implement this, by copying the related
> kernel function's binary to crash and execute it in crash, of course
> the kernel function needs to meet some limitations, but at least it
> can work for some simple functions as my test. So could you please
> give the following trial patch some modification and try?
>
> diff --git a/memory.c b/memory.c
> index b84e974..998ccbd 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1555,6 +1555,14 @@ cmd_rd(void)
> }
>
> display_memory(addr, count, flag, memtype, outputfile);
> +
> + char (*code)(char *, char *) = (char (*)(char *, char
> *))copy_kernel_function("strcat");
> + if (code) {
> + char buf[64] = "ABCD";
> + char src[] = "abcd";
> + fprintf(fp, ">>>>>>>>> %p %s\n", strcat(buf, src), buf);
sorry, there is a error, the line should be:
fprintf(fp, ">>>>>>>>> %p %s\n", code(buf, src), buf);
> + free(code);
> + }
> }
>
> /*
> diff --git a/tools.c b/tools.c
> index 1f8448c..e57bf87 100644
> --- a/tools.c
> +++ b/tools.c
> @@ -7006,3 +7006,48 @@ get_subsys_private(char *kset_name, char *target_name)
>
> return private;
> }
> +
> +void *copy_kernel_code(ulong kvaddr_start, ulong kvaddr_end)
> +{
> + void *code_buf = NULL;
> + int res;
> +
> + res = posix_memalign(&code_buf, machdep->pagesize,
> + kvaddr_end - kvaddr_start);
> + if (res)
> + goto fail;
> + res = mprotect(code_buf, kvaddr_end - kvaddr_start,
> + PROT_READ|PROT_WRITE|PROT_EXEC);
> + if (res)
> + goto fail;
> + memset(code_buf, 0, kvaddr_end - kvaddr_start);
> + readmem(kvaddr_start, KVADDR, code_buf, kvaddr_end - kvaddr_start,
> + "read kernel code", FAULT_ON_ERROR);
> +
> + return code_buf;
> +fail:
> + if (code_buf)
> + free(code_buf);
> + return NULL;
> +}
> +
> +void *copy_kernel_function(char *func_name)
> +{
> + struct syment *sp_start, *sp_end;
> + void *code;
> +
> + if (!symbol_exists(func_name))
> + error(FATAL, "kernel function %s not exist!\n", func_name);
> +
> + sp_start = symbol_search(func_name);
> + sp_end = next_symbol(NULL, sp_start);
> + if (!sp_start || !sp_end)
> + goto fail;
> +
> + code = copy_kernel_code(sp_start->value, sp_end->value);
> + if (!code)
> + goto fail;
> + return code;
> +fail:
> + return NULL;
> +}
>
> You can modify "char (*code)(char *, char *) = (char (*)(char *, char
> *))copy_kernel_function("strcat");" part and use it in
> diskdump.c:try_zram_decompress() as something like:
>
> int (*code)(unsigned char *, size_t, unsigned char *, size_t *) = (int
> (*)(unsigned char *, size_t, unsigned char *, size_t
> *))copy_kernel_function("lzo1x_decompress_safe");
>
> code(arg1, arg2, arg3, arg4);
> ...
>
> So we don't need to maintain lzo1x_decompress_safe() source code, and
> always get the lzo1x_decompress_safe() function in binary form at
> runtime, which is compatible with the current kernel.
>
> Thanks,
> Tao Liu
>
> >
> > Thanks and regards,
> > Yulong
> >
> >
> > --
> > Crash-utility mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx
> > To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxxxxxx
> > https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
> > Contribution Guidelines: https://github.com/crash-utility/crash/wiki
--
Crash-utility mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxxxxxx
https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
