2:09 a.m.
From: Kazuhito Hagio <k-hagio-ab(a)nec.com>
The current comparison macros for kernel version shift minor number only
8 bits. This can cause an unexpected result on kernels with revision
number over 255, e.g. Linux 4.14.314.
In fact, on Linux 4.14.314 for x86_64 without CONFIG_RANDOMIZE_BASE=y
(KASLR), the following condition became false in x86_64_init().
((THIS_KERNEL_VERSION >= LINUX(4,14,84)) &&
(THIS_KERNEL_VERSION < LINUX(4,15,0)))
As a result, crash used a wrong hard-coded value for PAGE_OFFSET and
failed to start a session with the following seek error.
crash: seek error: physical address: 200e000 type: "pud page"
Shift the major and minor number by 24 and 16 bits respectively to fix
this issue.
Reported-by: Luiz Capitulino <luizcap(a)amazon.com>
Signed-off-by: Kazuhito Hagio <k-hagio-ab(a)nec.com>
---
defs.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/defs.h b/defs.h
index 12ad6aaa0998..211fc9d55d33 100644
--- a/defs.h
+++ b/defs.h
@@ -807,10 +807,10 @@ struct kernel_table { /* kernel data */
} \
}
-#define THIS_KERNEL_VERSION ((kt->kernel_version[0] << 16) + \
- (kt->kernel_version[1] << 8) + \
+#define THIS_KERNEL_VERSION ((kt->kernel_version[0] << 24) + \
+ (kt->kernel_version[1] << 16) + \
(kt->kernel_version[2]))
-#define LINUX(x,y,z) (((uint)(x) << 16) + ((uint)(y) << 8) + (uint)(z))
+#define LINUX(x,y,z) (((uint)(x) << 24) + ((uint)(y) << 16) + (uint)(z))
#define THIS_GCC_VERSION ((kt->gcc_version[0] << 16) + \
(kt->gcc_version[1] << 8) + \
--
2.31.1
4:03 a.m.
Hi, Kazu
Thank you for the patch.
On Wed, May 10, 2023 at 3:09 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com>
wrote:
From: Kazuhito Hagio <k-hagio-ab(a)nec.com>
The current comparison macros for kernel version shift minor number only
8 bits. This can cause an unexpected result on kernels with revision
number over 255, e.g. Linux 4.14.314.
For this case, I saw kernel deal with it as below:
#define KERNEL_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + ((c) > 255 ?
255 : (c)))
Can you try to imitate the above macro definition and help to confirm if it
can also work for your case? If yes, it should be good to follow up kernel
change.
Thanks
Lianbo
In fact, on Linux 4.14.314 for x86_64 without CONFIG_RANDOMIZE_BASE=y
(KASLR), the following condition became false in x86_64_init().
((THIS_KERNEL_VERSION >= LINUX(4,14,84)) &&
(THIS_KERNEL_VERSION < LINUX(4,15,0)))
As a result, crash used a wrong hard-coded value for PAGE_OFFSET and
failed to start a session with the following seek error.
crash: seek error: physical address: 200e000 type: "pud page"
Shift the major and minor number by 24 and 16 bits respectively to fix
this issue.
Reported-by: Luiz Capitulino <luizcap(a)amazon.com>
Signed-off-by: Kazuhito Hagio <k-hagio-ab(a)nec.com>
---
defs.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/defs.h b/defs.h
index 12ad6aaa0998..211fc9d55d33 100644
--- a/defs.h
+++ b/defs.h
@@ -807,10 +807,10 @@ struct kernel_table { /* kernel
data */
} \
}
-#define THIS_KERNEL_VERSION ((kt->kernel_version[0] << 16) + \
- (kt->kernel_version[1] << 8) + \
+#define THIS_KERNEL_VERSION ((kt->kernel_version[0] << 24) + \
+ (kt->kernel_version[1] << 16) + \
(kt->kernel_version[2]))
-#define LINUX(x,y,z) (((uint)(x) << 16) + ((uint)(y) << 8) + (uint)(z))
+#define LINUX(x,y,z) (((uint)(x) << 24) + ((uint)(y) << 16) + (uint)(z))
#define THIS_GCC_VERSION ((kt->gcc_version[0] << 16) + \
(kt->gcc_version[1] << 8) + \
--
2.31.1
7:38 p.m.
On 2023/05/10 18:03, lijiang wrote:
Hi, Kazu
Thank you for the patch.
On Wed, May 10, 2023 at 3:09 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com>
wrote:
> From: Kazuhito Hagio <k-hagio-ab(a)nec.com>
>
> The current comparison macros for kernel version shift minor number only
> 8 bits. This can cause an unexpected result on kernels with revision
> number over 255, e.g. Linux 4.14.314.
>
>
For this case, I saw kernel deal with it as below:
#define KERNEL_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + ((c) > 255 ?
255 : (c)))
Can you try to imitate the above macro definition and help to confirm if it
can also work for your case? If yes, it should be good to follow up kernel
change.
um, if we need to check a revision number over 255, the KERNEL_VERSION
macro will not work. For example,
if (THIS_KERNEL_VERSION < LINUX(4,14,300))
and yet, why do you think it is good to follow the kernel one?
(Although I don't know the purpose and usage of the macro in kernel,
probably it's enough for kernel or tools.)
btw, makedumpfile has the same shift values as the patch since 2007.
#define KVER_MAJ_SHIFT 24
#define KVER_MIN_SHIFT 16
#define KERNEL_VERSION(x,y,z) (((x) << KVER_MAJ_SHIFT) | ((y) <<
KVER_MIN_SHIFT) | (z))
Thanks,
Kazu
3:49 a.m.
On Thu, May 11, 2023 at 8:38 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com>
wrote:
On 2023/05/10 18:03, lijiang wrote:
> Hi, Kazu
> Thank you for the patch.
> On Wed, May 10, 2023 at 3:09 PM HAGIO KAZUHITO(萩尾 一仁) <
k-hagio-ab(a)nec.com>
> wrote:
>
>> From: Kazuhito Hagio <k-hagio-ab(a)nec.com>
>>
>> The current comparison macros for kernel version shift minor number only
>> 8 bits. This can cause an unexpected result on kernels with revision
>> number over 255, e.g. Linux 4.14.314.
>>
>>
> For this case, I saw kernel deal with it as below:
>
> #define KERNEL_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + ((c) >
255 ?
> 255 : (c)))
>
> Can you try to imitate the above macro definition and help to confirm if
it
> can also work for your case? If yes, it should be good to follow up
kernel
> change.
um, if we need to check a revision number over 255, the KERNEL_VERSION
macro will not work. For example,
Do you mean the following changes can not work?
#define THIS_KERNEL_VERSION ((kt->kernel_version[0] << 16) + \
(kt->kernel_version[1] << 8) + \
((kt->kernel_version[2] > 255) ? 255 :
(kt->kernel_version[2])))
#define LINUX(x,y,z) (((uint)(x) << 16) + ((uint)(y) << 8) + ((uint)(z) >
255 ? 255 : (uint)(z)))
if (THIS_KERNEL_VERSION < LINUX(4,14,300))
and yet, why do you think it is good to follow the kernel one?
I saw that similar cases were handled in the kernel, and the kernel did not
change the bit shift operations.
...
Makefile:1334: echo '#define KERNEL_VERSION(a,b,c) (((a) << 16) + ((b) <<
8) + \
((c) > 255 ? 255 : (c)))'; \
tools/lib/bpf/bpf_helpers.h:74:#define KERNEL_VERSION(a, b, c) (((a) << 16)
+ ((b) << 8) + ((c) > 255 ? 255 : (c)))
...
Thanks.
Lianbo
(Although I don't know the purpose and usage of the macro in kernel,
probably it's enough for kernel or tools.)
btw, makedumpfile has the same shift values as the patch since 2007.
#define KVER_MAJ_SHIFT 24
#define KVER_MIN_SHIFT 16
#define KERNEL_VERSION(x,y,z) (((x) << KVER_MAJ_SHIFT) | ((y) <<
KVER_MIN_SHIFT) | (z))
Thanks,
Kazu
8:51 p.m.
On 2023/05/11 17:49, lijiang wrote:
On Thu, May 11, 2023 at 8:38 AM HAGIO KAZUHITO(萩尾 一仁)
<k-hagio-ab(a)nec.com>
wrote:
> On 2023/05/10 18:03, lijiang wrote:
>> Hi, Kazu
>> Thank you for the patch.
>> On Wed, May 10, 2023 at 3:09 PM HAGIO KAZUHITO(萩尾 一仁) <
> k-hagio-ab(a)nec.com>
>> wrote:
>>
>>> From: Kazuhito Hagio <k-hagio-ab(a)nec.com>
>>>
>>> The current comparison macros for kernel version shift minor number only
>>> 8 bits. This can cause an unexpected result on kernels with revision
>>> number over 255, e.g. Linux 4.14.314.
>>>
>>>
>> For this case, I saw kernel deal with it as below:
>>
>> #define KERNEL_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + ((c) >
255 ?
>> 255 : (c)))
>>
>> Can you try to imitate the above macro definition and help to confirm if
> it
>> can also work for your case? If yes, it should be good to follow up
> kernel
>> change.
>
> um, if we need to check a revision number over 255, the KERNEL_VERSION
> macro will not work. For example,
>
>
Do you mean the following changes can not work?
#define THIS_KERNEL_VERSION ((kt->kernel_version[0] << 16) + \
(kt->kernel_version[1] << 8) + \
((kt->kernel_version[2] > 255) ? 255 :
(kt->kernel_version[2])))
#define LINUX(x,y,z) (((uint)(x) << 16) + ((uint)(y) << 8) + ((uint)(z)
>
255 ? 255 : (uint)(z)))
No, in the current crash, this will work. Because there is no
if-conditional that checks a revision value over 255.
if (THIS_KERNEL_VERSION < LINUX(4,14,300))
But yes, when we need to check like this, the above cannot work. For
example, THIS_KERNEL_VERSION on 4.14.310 equals to LINUX(4,14,300).
So, my thought is that it would be better to change the shift amounts
now for future changes with the small effort, rather than change it
again in the future.
And the version checks are closed in crash, we don't need to emulate
the kernel macros.
Thanks,
Kazu
10:07 p.m.
On Mon, May 15, 2023 at 9:51 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com>
wrote:
> Do you mean the following changes can not work?
>
> #define THIS_KERNEL_VERSION ((kt->kernel_version[0] << 16) + \
> (kt->kernel_version[1] << 8) + \
> ((kt->kernel_version[2] > 255) ? 255 :
> (kt->kernel_version[2])))
> #define LINUX(x,y,z) (((uint)(x) << 16) + ((uint)(y) << 8) + ((uint)(z)
>
> 255 ? 255 : (uint)(z)))
No, in the current crash, this will work. Because there is no
if-conditional that checks a revision value over 255.
> if (THIS_KERNEL_VERSION < LINUX(4,14,300))
But yes, when we need to check like this, the above cannot work. For
example, THIS_KERNEL_VERSION on 4.14.310 equals to LINUX(4,14,300).
Thank you for the explanation, Kazu.
So, my thought is that it would be better to change the shift
amounts
now for future changes with the small effort, rather than change it
again in the future.
There is a similar discussion in the kernel here, but not sure why it was
not accepted.
https://lore.kernel.org/lkml/20210206035033.2036180-3-sashal@kernel.org/
But anyway, it is just a small change, and also fine to me. So: Ack.
Thanks.
Lianbo
12:32 a.m.
On 2023/05/15 12:07, lijiang wrote:
On Mon, May 15, 2023 at 9:51 AM HAGIO KAZUHITO(萩尾 一仁)
<k-hagio-ab(a)nec.com>
wrote:
>> Do you mean the following changes can not work?
>>
>> #define THIS_KERNEL_VERSION ((kt->kernel_version[0] << 16) + \
>> (kt->kernel_version[1] << 8) + \
>> ((kt->kernel_version[2] > 255) ? 255 :
>> (kt->kernel_version[2])))
>> #define LINUX(x,y,z) (((uint)(x) << 16) + ((uint)(y) << 8) +
((uint)(z) >
>> 255 ? 255 : (uint)(z)))
>
> No, in the current crash, this will work. Because there is no
> if-conditional that checks a revision value over 255.
>
>> if (THIS_KERNEL_VERSION < LINUX(4,14,300))
>
> But yes, when we need to check like this, the above cannot work. For
> example, THIS_KERNEL_VERSION on 4.14.310 equals to LINUX(4,14,300).
>
>
Thank you for the explanation, Kazu.
> So, my thought is that it would be better to change the shift amounts
> now for future changes with the small effort, rather than change it
> again in the future.
>
>
There is a similar discussion in the kernel here, but not sure why it was
not accepted.
https://lore.kernel.org/lkml/20210206035033.2036180-3-sashal@kernel.org/
Thanks for the info. To me, it looks like (1) they changed the shift
values but broke some userspace, so reverted. (2) the patch above was
posted but not accepted because there was no user at that time.
But anyway, it is just a small change, and also fine to me. So: Ack.
Thanks, applied.
https://github.com/crash-utility/crash/commit/040a56e9f9d0df15a2f8161ed3a...
Kazu
>
> Thanks.
> Lianbo
>
>
> On Mon, May 15, 2023 at 9:51 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com
<mailto:k-hagio-ab@nec.com>> wrote:
>
> > Do you mean the following changes can not work?
> >
> > #define THIS_KERNEL_VERSION ((kt->kernel_version[0] << 16) + \
> > (kt->kernel_version[1] << 8) + \
> > ((kt->kernel_version[2] > 255) ? 255
:
> > (kt->kernel_version[2])))
> > #define LINUX(x,y,z) (((uint)(x) << 16) + ((uint)(y) << 8) +
((uint)(z) >
> > 255 ? 255 : (uint)(z)))
>
> No, in the current crash, this will work. Because there is no
> if-conditional that checks a revision value over 255.
>
> > if (THIS_KERNEL_VERSION < LINUX(4,14,300))
>
> But yes, when we need to check like this, the above cannot work. For
> example, THIS_KERNEL_VERSION on 4.14.310 equals to LINUX(4,14,300).
>
>
> Thank you for the explanation, Kazu.
>
> So, my thought is that it would be better to change the shift amounts
> now for future changes with the small effort, rather than change it
> again in the future.
>
> There is a similar discussion in the kernel here, but not sure why it was not
accepted.
> https://lore.kernel.org/lkml/20210206035033.2036180-3-sashal@kernel.org/
<https://lore.kernel.org/lkml/20210206035033.2036180-3-sashal@kernel.org/>
But anyway, it is just a small change, and also fine to me. So: Ack.
>
> Thanks.
> Lianbo
1:02 p.m.
On 2023-05-10 03:09, HAGIO KAZUHITO(萩尾 一仁) wrote:
CAUTION: This email originated from outside of the organization. Do
not click links or open attachments unless you can confirm the sender and know the content
is safe.
From: Kazuhito Hagio <k-hagio-ab(a)nec.com>
The current comparison macros for kernel version shift minor number only
8 bits. This can cause an unexpected result on kernels with revision
number over 255, e.g. Linux 4.14.314.
In fact, on Linux 4.14.314 for x86_64 without CONFIG_RANDOMIZE_BASE=y
(KASLR), the following condition became false in x86_64_init().
((THIS_KERNEL_VERSION >= LINUX(4,14,84)) &&
(THIS_KERNEL_VERSION < LINUX(4,15,0)))
As a result, crash used a wrong hard-coded value for PAGE_OFFSET and
failed to start a session with the following seek error.
crash: seek error: physical address: 200e000 type: "pud page"
Shift the major and minor number by 24 and 16 bits respectively to fix
this issue.
This fixes the issue for me. The only detail is that I couldn't apply
this patch to latest crash HEAD 2505a65ff5471 so I applied it manually.
Thank you very much, Kazu!
Tested-by: Luiz Capitulino <luizcap(a)amazon.com>
- Luiz
Reported-by: Luiz Capitulino <luizcap(a)amazon.com>
Signed-off-by: Kazuhito Hagio <k-hagio-ab(a)nec.com>
---
defs.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/defs.h b/defs.h
index 12ad6aaa0998..211fc9d55d33 100644
--- a/defs.h
+++ b/defs.h
@@ -807,10 +807,10 @@ struct kernel_table { /* kernel data */
} \
}
-#define THIS_KERNEL_VERSION ((kt->kernel_version[0] << 16) + \
- (kt->kernel_version[1] << 8) + \
+#define THIS_KERNEL_VERSION ((kt->kernel_version[0] << 24) + \
+ (kt->kernel_version[1] << 16) + \
(kt->kernel_version[2]))
-#define LINUX(x,y,z) (((uint)(x) << 16) + ((uint)(y) << 8) + (uint)(z))
+#define LINUX(x,y,z) (((uint)(x) << 24) + ((uint)(y) << 16) + (uint)(z))
#define THIS_GCC_VERSION ((kt->gcc_version[0] << 16) + \
(kt->gcc_version[1] << 8) + \
--
2.31.1
7:55 p.m.
On 2023/05/11 3:02, Luiz Capitulino wrote:
On 2023-05-10 03:09, HAGIO KAZUHITO(萩尾 一仁) wrote:
> CAUTION: This email originated from outside of the organization. Do not click links
or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> From: Kazuhito Hagio <k-hagio-ab(a)nec.com>
>
> The current comparison macros for kernel version shift minor number only
> 8 bits. This can cause an unexpected result on kernels with revision
> number over 255, e.g. Linux 4.14.314.
>
> In fact, on Linux 4.14.314 for x86_64 without CONFIG_RANDOMIZE_BASE=y
> (KASLR), the following condition became false in x86_64_init().
>
> ((THIS_KERNEL_VERSION >= LINUX(4,14,84)) &&
> (THIS_KERNEL_VERSION < LINUX(4,15,0)))
>
> As a result, crash used a wrong hard-coded value for PAGE_OFFSET and
> failed to start a session with the following seek error.
>
> crash: seek error: physical address: 200e000 type: "pud page"
>
> Shift the major and minor number by 24 and 16 bits respectively to fix
> this issue.
This fixes the issue for me. The only detail is that I couldn't apply
this patch to latest crash HEAD 2505a65ff5471 so I applied it manually.
oh, what happened. at my end, the email could be applied cleanly.
Thank you very much, Kazu!
Tested-by: Luiz Capitulino <luizcap(a)amazon.com>
Anyway, thank you for the report and test!
Kazu
8:06 a.m.
On 2023-05-10 20:55, HAGIO KAZUHITO(萩尾 一仁) wrote:
On 2023/05/11 3:02, Luiz Capitulino wrote:
>
>
> On 2023-05-10 03:09, HAGIO KAZUHITO(萩尾 一仁) wrote:
>> CAUTION: This email originated from outside of the organization. Do not click
links or open attachments unless you can confirm the sender and know the content is safe.
>>
>>
>>
>> From: Kazuhito Hagio <k-hagio-ab(a)nec.com>
>>
>> The current comparison macros for kernel version shift minor number only
>> 8 bits. This can cause an unexpected result on kernels with revision
>> number over 255, e.g. Linux 4.14.314.
>>
>> In fact, on Linux 4.14.314 for x86_64 without CONFIG_RANDOMIZE_BASE=y
>> (KASLR), the following condition became false in x86_64_init().
>>
>> ((THIS_KERNEL_VERSION >= LINUX(4,14,84)) &&
>> (THIS_KERNEL_VERSION < LINUX(4,15,0)))
>>
>> As a result, crash used a wrong hard-coded value for PAGE_OFFSET and
>> failed to start a session with the following seek error.
>>
>> crash: seek error: physical address: 200e000 type: "pud page"
>>
>> Shift the major and minor number by 24 and 16 bits respectively to fix
>> this issue.
>
> This fixes the issue for me. The only detail is that I couldn't apply
> this patch to latest crash HEAD 2505a65ff5471 so I applied it manually.
oh, what happened. at my end, the email could be applied cleanly.
Yeah, the problem is on my end. Tried again and could get it applied
cleanly. Tested again just to be sure and it works :)
- Luiz
>
> Thank you very much, Kazu!
>
> Tested-by: Luiz Capitulino <luizcap(a)amazon.com>
Anyway, thank you for the report and test!
Kazu
561
days inactive
566
days old
devel@lists.crash-utility.osci.io
9 comments
3 participants
participants (3)
-
HAGIO KAZUHITO(萩尾 一仁) -
lijiang -
Luiz Capitulino