4:08 a.m.
Hi Yulong,
Thanks for your patch!
On Mon, Feb 26, 2024 at 3:20 PM Yulong TANG 汤玉龙 <yulong.tang(a)nio.com> 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);
+ 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(a)lists.crash-utility.osci.io
To unsubscribe send an email to devel-leave(a)lists.crash-utility.osci.io
https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
4:15 a.m.
New subject: Adding the zram decompression algorithm "lzo-rle" to support kernel versions >= 5.1
On Thu, Feb 29, 2024 at 6:08 PM Tao Liu <ltao(a)redhat.com> wrote:
Hi Yulong,
Thanks for your patch!
On Mon, Feb 26, 2024 at 3:20 PM Yulong TANG 汤玉龙 <yulong.tang(a)nio.com> 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(a)lists.crash-utility.osci.io
> To unsubscribe send an email to devel-leave(a)lists.crash-utility.osci.io
> https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
> Contribution Guidelines: https://github.com/crash-utility/crash/wiki
6:29 p.m.
New subject: Adding the zram decompression algorithm "lzo-rle" to support kernel versions >= 5.1
On 2024/02/29 19:08, Tao Liu wrote:
Hi Yulong,
Thanks for your patch!
On Mon, Feb 26, 2024 at 3:20 PM Yulong TANG 汤玉龙 <yulong.tang(a)nio.com> 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?
Hi Tao,
That's an interesting idea, but I'm very afraid of copying a function
from data (vmcore) and executing it, for security. Maybe it's not
impossible that someone sends a crafted vmcore to your support team..
Also, with that, we cannot handle a vmcore of another architecture, e.g.
arm64 vmcores with x86_64 crash with target=ARM64.
sorry I still haven't check Yulong's patch, but if lzo-rle library is
not available, maybe we need to import the code..?
Thanks,
Kazu
7:33 p.m.
New subject: Adding the zram decompression algorithm "lzo-rle" to support kernel versions >= 5.1
On 2024/03/01 9:29, HAGIO KAZUHITO(萩尾 一仁) wrote:
On 2024/02/29 19:08, Tao Liu wrote:
> Hi Yulong,
>
> Thanks for your patch!
>
> On Mon, Feb 26, 2024 at 3:20 PM Yulong TANG 汤玉龙 <yulong.tang(a)nio.com> 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?
Hi Tao,
That's an interesting idea, but I'm very afraid of copying a function
from data (vmcore) and executing it, for security. Maybe it's not
impossible that someone sends a crafted vmcore to your support team..
Also, with that, we cannot handle a vmcore of another architecture, e.g.
arm64 vmcores with x86_64 crash with target=ARM64.
sorry I still haven't check Yulong's patch, but if lzo-rle library is
not available, maybe we need to import the code..?
seeing lib/lzo/ in the kernel source, lzo-rle is not far from lzo, the
lzo library will not support lzo-rle?
Thanks,
Kazu
9:56 p.m.
New subject: Adding the zram decompression algorithm "lzo-rle" to support kernel versions >= 5.1
Hi Kazu and Yulong,
On Fri, Mar 1, 2024 at 9:33 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com> wrote:
On 2024/03/01 9:29, HAGIO KAZUHITO(萩尾 一仁) wrote:
> On 2024/02/29 19:08, Tao Liu wrote:
>> Hi Yulong,
>>
>> Thanks for your patch!
>>
>> On Mon, Feb 26, 2024 at 3:20 PM Yulong TANG 汤玉龙 <yulong.tang(a)nio.com>
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?
>
> Hi Tao,
>
> That's an interesting idea, but I'm very afraid of copying a function
> from data (vmcore) and executing it, for security. Maybe it's not
> impossible that someone sends a crafted vmcore to your support team..
>
Yeah, I haven't thought of the security issue...
> Also, with that, we cannot handle a vmcore of another
architecture, e.g.
> arm64 vmcores with x86_64 crash with target=ARM64.
I have thought about this. I'm exploring the possibility of the
feature about "allowing crash to execute some kernel code".
One advantage of this, is we can skip the work of maintaining kernel
version specific code, and can always get the current kernel
compatible functions for use.
And it does have architecture and security issues as you mentioned.
I'm thinking of adding a code emulator to solve this, kind of like the
following pseudocode:
in crash:
char share_mem_buffer[4096];
char code[4096];
copy_kernel_function(code);
pid t = fork();
if (t > 0) {
wait(pid);
} else if (t == 0) {
move_child_process_into_cgroup_or_namespace_to_limit_access();
execute("qemu-aarch64", code, share_mem_buffer);
exit(0)
}
read_mem(share_mem_buffer);
By cgroup and namespace control, we limit the kernel code access to
the outer resources.
By qemu-aarch64 and similar emulators, we simulate the arch dependent code.
Also, the kernel function itself needs to have some limitations, such
as 1) a stand alone function, 2) no sub function calling or global
variable access etc...
Just my immature idea of solving copy/paste of some pure kernel code.
It is more complex and maybe hard to implement, not sure if there will
be more kernel code being copied to crash in the future. But for this
case, I agree just copying the kernel code is the simplest way.
>
> sorry I still haven't check Yulong's patch, but if lzo-rle library is
> not available, maybe we need to import the code..?
seeing lib/lzo/ in the kernel source, lzo-rle is not far from lzo, the
lzo library will not support lzo-rle?
Yeah, I have the same question. However by comparing lzo library
function [1] and kernel lib/lzo/, there are differences. I suspect the
current lzo library won't work for lzo-rle, but I agree it's worth a
try of confirmation.
[1]:
https://github.com/nemequ/lzo/blob/0083878c235a89ef96a009d1ff0b500f3a364e...
Thanks,
Tao Liu
Thanks,
Kazu
2:24 a.m.
New subject: Adding the zram decompression algorithm "lzo-rle" to support kernel versions >= 5.1
Hi Yulong,
Thanks for your patch!
On Mon, Feb 26, 2024 at 3:20 PM Yulong TANG 汤玉龙 <yulong.tang(a)nio.com> wrote:
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.
Hi Tao,
I discovered that there seems to be an array called ikconfig_all in kernel.c which records
the kernel config status, and there is an API called get_kernel_config(char *conf_name,
char **str) for obtaining kernel configuration. This might help solve the second problem
you raised.
For example, adding a line:
#define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
get_kernel_config("CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS", NULL);
Thanks,
Yulong
2:48 a.m.
New subject: Adding the zram decompression algorithm "lzo-rle" to support kernel versions >= 5.1
Hi Yulong,
On Fri, Mar 1, 2024 at 4:25 PM tang yulong <1637403063(a)qq.com> wrote:
> Hi Yulong,
>
> Thanks for your patch!
>
> On Mon, Feb 26, 2024 at 3:20 PM Yulong TANG 汤玉龙 <yulong.tang(a)nio.com>
wrote:
>
> 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.
Hi Tao,
I discovered that there seems to be an array called ikconfig_all in kernel.c which
records the kernel config status, and there is an API called get_kernel_config(char
*conf_name, char **str) for obtaining kernel configuration. This might help solve the
second problem you raised.
For example, adding a line:
#define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
get_kernel_config("CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS", NULL);
OK, thanks for the clarification, sounds workable. Then what do you
think if we keep the "if
defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)" related code of the
copied kernel code, but change the "if defined" macro into a runtime
check like "if get_kernel_config("CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS",
NULL)) == IKCONFIG_Y". So the code can be chosen whether to execute
depending on the config status.
Thanks,
Tao Liu
Thanks,
Yulong
--
Crash-utility mailing list -- devel(a)lists.crash-utility.osci.io
To unsubscribe send an email to devel-leave(a)lists.crash-utility.osci.io
https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
3:45 a.m.
New subject: Adding the zram decompression algorithm "lzo-rle" to support kernel versions >= 5.1
OK, I have made the modifications as per your suggestions. Should I proceed with
resubmitting the patch? And how should I acknowledge your contribution? I'm not quite
clear on that for the first submission. Thank you for your guidance.
Thanks,
Yulong
diff --git a/lzorle_decompress.c b/lzorle_decompress.c
index bd7ee3f..5e83430 100644
--- a/lzorle_decompress.c
+++ b/lzorle_decompress.c
@@ -20,6 +20,8 @@
#include <stdint.h>
#include <string.h>
+#include "defs.h"
+
/* This MAX_255_COUNT is the maximum number of times we can add 255 to a base
* count without overflowing an integer. The multiply will overflow when
* multiplying 255 by more than MAXINT/255. The sum will overflow earlier
@@ -91,7 +93,28 @@ int lzorle_decompress_safe(const unsigned char *in, unsigned long
in_len,
}
t += 3;
copy_literal_run:
- {
+ if (get_kernel_config("CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS", NULL) ==
IKCONFIG_Y) {
+ if (HAVE_IP(t + 15) && HAVE_OP(t + 15)) {
+ const unsigned char *ie = ip + t;
+ unsigned char *oe = op + t;
+ do {
+ COPY8(op, ip);
+ op += 8;
+ ip += 8;
+ COPY8(op, ip);
+ op += 8;
+ ip += 8;
+ } while (ip < ie);
+ ip = ie;
+ op = oe;
+ } else {
+ NEED_OP(t);
+ NEED_IP(t + 3);
+ do {
+ *op++ = *ip++;
+ } while (--t > 0);
+ }
+ } else {
NEED_OP(t);
NEED_IP(t + 3);
do {
@@ -194,7 +217,45 @@ copy_literal_run:
}
}
TEST_LB(m_pos);
- {
+
+ if (get_kernel_config("CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS", NULL) ==
IKCONFIG_Y) {
+ if (op - m_pos >= 8) {
+ unsigned char *oe = op + t;
+ if (HAVE_OP(t + 15)) {
+ do {
+ COPY8(op, m_pos);
+ op += 8;
+ m_pos += 8;
+ COPY8(op, m_pos);
+ op += 8;
+ m_pos += 8;
+ } while (op < oe);
+ op = oe;
+ if (HAVE_IP(6)) {
+ state = next;
+ COPY4(op, ip);
+ op += next;
+ ip += next;
+ continue;
+ }
+ } else {
+ NEED_OP(t);
+ do {
+ *op++ = *m_pos++;
+ } while (op < oe);
+ }
+ } else {
+ unsigned char *oe = op + t;
+ NEED_OP(t);
+ op[0] = m_pos[0];
+ op[1] = m_pos[1];
+ op += 2;
+ m_pos += 2;
+ do {
+ *op++ = *m_pos++;
+ } while (op < oe);
+ }
+ } else {
unsigned char *oe = op + t;
NEED_OP(t);
op[0] = m_pos[0];
@@ -208,7 +269,20 @@ copy_literal_run:
match_next:
state = next;
t = next;
- {
+ if (get_kernel_config("CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS", NULL) ==
IKCONFIG_Y) {
+ if (HAVE_IP(6) && HAVE_OP(4)) {
+ COPY4(op, ip);
+ op += t;
+ ip += t;
+ } else {
+ NEED_IP(t + 3);
+ NEED_OP(t);
+ while (t > 0) {
+ *op++ = *ip++;
+ t--;
+ }
+ }
+ } else {
NEED_IP(t + 3);
NEED_OP(t);
while (t > 0) {
4:29 a.m.
New subject: Adding the zram decompression algorithm "lzo-rle" to support kernel versions >= 5.1
Hi Yulong,
On Fri, Mar 1, 2024 at 5:46 PM Tang Yulong <1637403063(a)qq.com> wrote:
OK, I have made the modifications as per your suggestions. Should I proceed with
resubmitting the patch? And how should I acknowledge your contribution? I'm not quite
clear on that for the first submission. Thank you for your guidance.
Please merge the following patch with your original patch(let's say
patch v1), and we will get an updated patch (let's say patch v2). So
you can post your v2 patch directly upstream like what you did for v1.
If there is no problem then I will ack it. The other
maintainers/developers will also review your v2 patch and may give ack
or comments to it.
Please have no worries about submitting patches. You can also add
Reviewed-by: Tao Liu <ltao(a)redhat.com>
after your signed-off-by signature if you like.
Thanks,
Yulong
diff --git a/lzorle_decompress.c b/lzorle_decompress.c
index bd7ee3f..5e83430 100644
--- a/lzorle_decompress.c
+++ b/lzorle_decompress.c
@@ -20,6 +20,8 @@
#include <stdint.h>
#include <string.h>
+#include "defs.h"
+
/* This MAX_255_COUNT is the maximum number of times we can add 255 to a base
* count without overflowing an integer. The multiply will overflow when
* multiplying 255 by more than MAXINT/255. The sum will overflow earlier
@@ -91,7 +93,28 @@ int lzorle_decompress_safe(const unsigned char *in, unsigned long
in_len,
}
t += 3;
copy_literal_run:
- {
+ if
(get_kernel_config("CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS", NULL) ==
IKCONFIG_Y) {
+ if (HAVE_IP(t + 15) && HAVE_OP(t + 15))
{
+ const unsigned char *ie = ip + t;
+ unsigned char *oe = op + t;
+ do {
+ COPY8(op, ip);
+ op += 8;
+ ip += 8;
+ COPY8(op, ip);
+ op += 8;
+ ip += 8;
+ } while (ip < ie);
+ ip = ie;
+ op = oe;
+ } else {
+ NEED_OP(t);
+ NEED_IP(t + 3);
+ do {
+ *op++ = *ip++;
+ } while (--t > 0);
+ }
+ } else {
NEED_OP(t);
NEED_IP(t + 3);
do {
@@ -194,7 +217,45 @@ copy_literal_run:
}
}
TEST_LB(m_pos);
- {
+
+ if (get_kernel_config("CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS",
NULL) == IKCONFIG_Y) {
+ if (op - m_pos >= 8) {
+ unsigned char *oe = op + t;
+ if (HAVE_OP(t + 15)) {
+ do {
+ COPY8(op, m_pos);
+ op += 8;
+ m_pos += 8;
+ COPY8(op, m_pos);
+ op += 8;
+ m_pos += 8;
+ } while (op < oe);
+ op = oe;
+ if (HAVE_IP(6)) {
+ state = next;
+ COPY4(op, ip);
+ op += next;
+ ip += next;
+ continue;
+ }
+ } else {
+ NEED_OP(t);
+ do {
+ *op++ = *m_pos++;
+ } while (op < oe);
+ }
+ } else {
+ unsigned char *oe = op + t;
+ NEED_OP(t);
+ op[0] = m_pos[0];
+ op[1] = m_pos[1];
+ op += 2;
+ m_pos += 2;
+ do {
+ *op++ = *m_pos++;
+ } while (op < oe);
+ }
+ } else {
unsigned char *oe = op + t;
NEED_OP(t);
op[0] = m_pos[0];
@@ -208,7 +269,20 @@ copy_literal_run:
match_next:
state = next;
t = next;
- {
+ if (get_kernel_config("CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS",
NULL) == IKCONFIG_Y) {
+ if (HAVE_IP(6) && HAVE_OP(4)) {
+ COPY4(op, ip);
+ op += t;
+ ip += t;
+ } else {
+ NEED_IP(t + 3);
+ NEED_OP(t);
+ while (t > 0) {
+ *op++ = *ip++;
+ t--;
+ }
+ }
+ } else {
NEED_IP(t + 3);
NEED_OP(t);
while (t > 0) {
--
Since I didn't successfully set up the test environment, please also
have a test on your v2 patch, to see if it really works as expected.
In addition, there are 3 identical
CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS checks within the code, can you
shrink it so we only run the check one time?
Thanks,
Tao Liu
Crash-utility mailing list -- devel(a)lists.crash-utility.osci.io
To unsubscribe send an email to devel-leave(a)lists.crash-utility.osci.io
https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
8:40 a.m.
New subject: Adding the zram decompression algorithm "lzo-rle" to support kernel versions >= 5.1
Thank you for your detailed comments.
Thanks,
Yulong
2:40 a.m.
New subject: Adding the zram decompression algorithm "lzo-rle" to support kernel versions >= 5.1
On 2024/03/01 17:48, Tao Liu wrote:
Hi Yulong,
On Fri, Mar 1, 2024 at 4:25 PM tang yulong <1637403063(a)qq.com> wrote:
>
>> Hi Yulong,
>>
>> Thanks for your patch!
>>
>> On Mon, Feb 26, 2024 at 3:20 PM Yulong TANG 汤玉龙 <yulong.tang(a)nio.com>
wrote:
>>
>> 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.
>
> Hi Tao,
>
> I discovered that there seems to be an array called ikconfig_all in kernel.c which
records the kernel config status, and there is an API called get_kernel_config(char
*conf_name, char **str) for obtaining kernel configuration. This might help solve the
second problem you raised.
> For example, adding a line:
> #define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
get_kernel_config("CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS", NULL);
>
OK, thanks for the clarification, sounds workable. Then what do you
think if we keep the "if
defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)" related code of the
copied kernel code, but change the "if defined" macro into a runtime
check like "if
get_kernel_config("CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS",
NULL)) == IKCONFIG_Y". So the code can be chosen whether to execute
depending on the config status.
hmm, as far as I know, not many kernels are configured with
CONFIG_IKCONFIG=y, e.g. RHEL kernels don't have it. so if it depends
only on ikconfig, it will not work on many kernels.
It's good to use ikconfig, but isn't there any other way?
For example, it looks like it's always set to y on x86. so maybe we can
have a default value depending on architecture, for no ikconfig kernels.
config X86
...
select HAVE_EFFICIENT_UNALIGNED_ACCESS
Thanks,
Kazu
4:08 a.m.
New subject: Adding the zram decompression algorithm "lzo-rle" to support kernel versions >= 5.1
Hi, Kazu:
On 2024/03/01 17:48, Tao Liu wrote:
hmm, as far as I know, not many kernels are configured with
CONFIG_IKCONFIG=y, e.g. RHEL kernels don't have it. so if it depends
only on ikconfig, it will not work on many kernels.
Thank you for the correction.
It's good to use ikconfig, but isn't there any other way?
For example, it looks like it's always set to y on x86. so maybe we can
have a default value depending on architecture, for no ikconfig kernels.
config X86
...
select HAVE_EFFICIENT_UNALIGNED_ACCESS
Thanks,
Kazu
Yes, it seems that some architectures have this configuration enabled by default, such as
ARM, ARM64, and X86. These are also commonly used architectures.
So setting the default to CONFIG_Y for these architectures and CONFIG_N for others. Then,
we can check whether IKCONFIG is enabled to retrieve the actual value. Do you think this
is acceptable?
like this:
- bool efficient_unaligned_access =
(get_kernel_config("CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS", NULL) ==
IKCONFIG_Y);
+ bool efficient_unaligned_access;
+
+#if defined(ARM) || defined(ARM64) || defined(X86)
+ efficient_unaligned_access = true;
+#else
+ efficient_unaligned_access = false;
+#endif
+
+ if (kt->ikconfig_flags & IKCONFIG_AVAIL)
+ efficient_unaligned_access =
(get_kernel_config("CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS", NULL) ==
IKCONFIG_Y);
Thanks,
Yulong
1:38 a.m.
New subject: Adding the zram decompression algorithm "lzo-rle" to support kernel versions >= 5.1
On 2024/03/11 18:08, Tang Yulong wrote:
Hi, Kazu:
> On 2024/03/01 17:48, Tao Liu wrote:
>
> hmm, as far as I know, not many kernels are configured with
> CONFIG_IKCONFIG=y, e.g. RHEL kernels don't have it. so if it depends
> only on ikconfig, it will not work on many kernels.
>
Thank you for the correction.
> It's good to use ikconfig, but isn't there any other way?
>
> For example, it looks like it's always set to y on x86. so maybe we can
> have a default value depending on architecture, for no ikconfig kernels.
>
> config X86
> ...
> select HAVE_EFFICIENT_UNALIGNED_ACCESS
>
> Thanks,
> Kazu
Yes, it seems that some architectures have this configuration enabled by default, such as
ARM, ARM64, and X86. These are also commonly used architectures.
So setting the default to CONFIG_Y for these architectures and CONFIG_N for others. Then,
we can check whether IKCONFIG is enabled to retrieve the actual value. Do you think this
is acceptable?
like this:
- bool efficient_unaligned_access =
(get_kernel_config("CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS", NULL) ==
IKCONFIG_Y);
+ bool efficient_unaligned_access;
+
+#if defined(ARM) || defined(ARM64) || defined(X86)
+ efficient_unaligned_access = true;
Yes. Looking at arch/*/Kconfig, it looks like the following
architectures will have 'y' IIUC, please use these.
X86, X86_64, ARM, ARM64, PPC, PPC64, S390, S390X
(LOONGARCH64 and RISCV64 can have 'n' depending on configs.)
With respect to lzo-rle user library, Tao is asking the kernel
developers. (Thanks Tao)
Thanks,
Kazu
> +#else
> + efficient_unaligned_access = false;
> +#endif
> +
> + if (kt->ikconfig_flags & IKCONFIG_AVAIL)
> + efficient_unaligned_access =
(get_kernel_config("CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS", NULL) ==
IKCONFIG_Y);
>
> Thanks,
> Yulong
> --
> Crash-utility mailing list -- devel(a)lists.crash-utility.osci.io
> To unsubscribe send an email to devel-leave(a)lists.crash-utility.osci.io
> https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
> Contribution Guidelines: https://github.com/crash-utility/crash/wiki
3:41 a.m.
New subject: Adding the zram decompression algorithm "lzo-rle" to support kernel versions >= 5.1
Thank you for your review and help,
I have made the modifications and post v3 patch.
Thanks,
Yulong
109
days inactive
121
days old
devel@lists.crash-utility.osci.io
13 comments
4 participants
participants (4)
-
HAGIO KAZUHITO(萩尾 一仁) -
tang yulong -
Tang Yulong
-
Tao Liu