11:06 p.m.
Previously, the ending identifier for parsing the task structure
member is " }, \n". However the ending identifier is not always
as expected. " },\n" can also be the ending identifier. For example,
if we have the following struct, the parsing will fail.
tasks = {\n
next = 0xffff94f8038f8838,\n
prev = 0xffff94f8036f8838\n
},\n
Before:
crash> task -R tasks ffff94f8038f4000
PID: 847 TASK: ffff94f8038f4000 CPU: 72 COMMAND: "khungtaskd"
After:
crash> task -R tasks ffff94f8038f4000
PID: 847 TASK: ffff94f8038f4000 CPU: 72 COMMAND: "khungtaskd"
tasks = {
next = 0xffff94f8038f8838,
prev = 0xffff94f8036f8838
},
Signed-off-by: Tao Liu <ltao(a)redhat.com>
---
task.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/task.c b/task.c
index 071c787..db2abc8 100644
--- a/task.c
+++ b/task.c
@@ -3436,7 +3436,8 @@ parse_task_thread(int argcnt, char *arglist[], struct task_context
*tc) {
while (fgets(buf, BUFSIZE, pc->tmpfile)) {
if (STREQ(buf, " {\n"))
randomized = TRUE;
- else if (randomized && STREQ(buf, " }, \n"))
+ else if (randomized &&
+ (STREQ(buf, " }, \n") || STREQ(buf, " },\n")))
randomized = FALSE;
if (strlen(lookfor2)) {
--
2.33.1
2:54 a.m.
On 2022/08/24 13:06, Tao Liu wrote:
Previously, the ending identifier for parsing the task structure
member is " }, \n". However the ending identifier is not always
as expected. " },\n" can also be the ending identifier. For example,
if we have the following struct, the parsing will fail.
tasks = {\n
next = 0xffff94f8038f8838,\n
prev = 0xffff94f8036f8838\n
},\n
Before:
crash> task -R tasks ffff94f8038f4000
PID: 847 TASK: ffff94f8038f4000 CPU: 72 COMMAND: "khungtaskd"
Good catch!
but it looks like the cause is that crash cannot detect the closing
brace of a union? For example,
crash-dev> task 1
PID: 1 TASK: ffff8f96c159c8c0 CPU: 15 COMMAND: "systemd"
struct task_struct {
thread_info = {
flags = 0,
status = 0
},
state = 1,
stack = 0xffff9b6dc017c000,
{
usage = {
refs = {
counter = 1
}
},
rh_kabi_hidden_616 = {
usage = {
counter = 1
}
},
{<No data fields>}
}, <<-- This
flags = 4194560,
ptrace = 0,
wake_entry = {
next = 0x0
},
on_cpu = 0,
As a result, the randomized switch is always on after this, and
lookfor1 (and 2) will become wrong.
So "task -R" can show task_struct.thread_info and cannot show
task_struct.wake_entry, because the latter is after the union.
crash-dev> task -R thread_info 1
PID: 1 TASK: ffff8f96c159c8c0 CPU: 0 COMMAND: "systemd"
thread_info = {
flags = 0,
status = 0
},
crash-dev> task -R wake_entry 1
PID: 1 TASK: ffff8f96c159c8c0 CPU: 0 COMMAND: "systemd"
And upstream kernels (not randomized) don't have a union before
the task_struct.tasks, the issue is not reproducible with it.
crash-dev> task -R tasks 1
PID: 1 TASK: ffff9d1a4081b400 CPU: 3 COMMAND: "systemd"
tasks = {
next = 0xffff9d1a4081d678,
prev = 0xffffffff9a8191b8 <init_task+2168>
},
but anyway reproducible with a member after a union.
crash-dev> struct task_struct
...
union {
refcount_t rcu_users;
struct callback_head rcu;
};
struct pipe_inode_info *splice_pipe;
crash-dev> task -R splice_pipe 1
PID: 1 TASK: ffff9d1a4081b400 CPU: 3 COMMAND: "systemd"
So please check if this is correct and fix the commit log.
After:
crash> task -R tasks ffff94f8038f4000
PID: 847 TASK: ffff94f8038f4000 CPU: 72 COMMAND: "khungtaskd"
tasks = {
next = 0xffff94f8038f8838,
prev = 0xffff94f8036f8838
},
Signed-off-by: Tao Liu <ltao(a)redhat.com>
---
task.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/task.c b/task.c
index 071c787..db2abc8 100644
--- a/task.c
+++ b/task.c
@@ -3436,7 +3436,8 @@ parse_task_thread(int argcnt, char *arglist[], struct task_context
*tc) {
while (fgets(buf, BUFSIZE, pc->tmpfile)) {
if (STREQ(buf, " {\n"))
randomized = TRUE;
- else if (randomized && STREQ(buf, " }, \n"))
+ else if (randomized &&
+ (STREQ(buf, " }, \n") || STREQ(buf, " },\n")))
randomized = FALSE;
Looks fine but it looks like gdb-10.2 does not print a space before "\n",
I think we can _replace_ the STREQ(), not add, because the current branch
only supports gdb-10.2.
Thanks,
Kazu
5:01 a.m.
Hi Kazu,
On Wed, Aug 24, 2022 at 3:54 PM HAGIO KAZUHITO(萩尾 一仁)
<k-hagio-ab(a)nec.com> wrote:
On 2022/08/24 13:06, Tao Liu wrote:
> Previously, the ending identifier for parsing the task structure
> member is " }, \n". However the ending identifier is not always
> as expected. " },\n" can also be the ending identifier. For example,
> if we have the following struct, the parsing will fail.
>
> tasks = {\n
> next = 0xffff94f8038f8838,\n
> prev = 0xffff94f8036f8838\n
> },\n
>
> Before:
> crash> task -R tasks ffff94f8038f4000
> PID: 847 TASK: ffff94f8038f4000 CPU: 72 COMMAND:
"khungtaskd"
Good catch!
but it looks like the cause is that crash cannot detect the closing
brace of a union? For example,
Yes! Thanks for your explanation, I didn't realize it was due to union...
crash-dev> task 1
PID: 1 TASK: ffff8f96c159c8c0 CPU: 15 COMMAND: "systemd"
struct task_struct {
thread_info = {
flags = 0,
status = 0
},
state = 1,
stack = 0xffff9b6dc017c000,
{
usage = {
refs = {
counter = 1
}
},
rh_kabi_hidden_616 = {
usage = {
counter = 1
}
},
{<No data fields>}
}, <<-- This
flags = 4194560,
ptrace = 0,
wake_entry = {
next = 0x0
},
on_cpu = 0,
As a result, the randomized switch is always on after this, and
lookfor1 (and 2) will become wrong.
Right, if the ending brace is not correctly identified, then lookfor
will be incorrect, and fail to extract later members...
So "task -R" can show task_struct.thread_info and cannot
show
task_struct.wake_entry, because the latter is after the union.
crash-dev> task -R thread_info 1
PID: 1 TASK: ffff8f96c159c8c0 CPU: 0 COMMAND: "systemd"
thread_info = {
flags = 0,
status = 0
},
crash-dev> task -R wake_entry 1
PID: 1 TASK: ffff8f96c159c8c0 CPU: 0 COMMAND: "systemd"
And upstream kernels (not randomized) don't have a union before
the task_struct.tasks, the issue is not reproducible with it.
crash-dev> task -R tasks 1
PID: 1 TASK: ffff9d1a4081b400 CPU: 3 COMMAND: "systemd"
tasks = {
next = 0xffff9d1a4081d678,
prev = 0xffffffff9a8191b8 <init_task+2168>
},
but anyway reproducible with a member after a union.
crash-dev> struct task_struct
...
union {
refcount_t rcu_users;
struct callback_head rcu;
};
struct pipe_inode_info *splice_pipe;
crash-dev> task -R splice_pipe 1
PID: 1 TASK: ffff9d1a4081b400 CPU: 3 COMMAND: "systemd"
So please check if this is correct and fix the commit log.
OK, I will correct the commit log and re-send the patch.
>
> After:
> crash> task -R tasks ffff94f8038f4000
> PID: 847 TASK: ffff94f8038f4000 CPU: 72 COMMAND:
"khungtaskd"
> tasks = {
> next = 0xffff94f8038f8838,
> prev = 0xffff94f8036f8838
> },
>
> Signed-off-by: Tao Liu <ltao(a)redhat.com>
> ---
> task.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/task.c b/task.c
> index 071c787..db2abc8 100644
> --- a/task.c
> +++ b/task.c
> @@ -3436,7 +3436,8 @@ parse_task_thread(int argcnt, char *arglist[], struct
task_context *tc) {
> while (fgets(buf, BUFSIZE, pc->tmpfile)) {
> if (STREQ(buf, " {\n"))
> randomized = TRUE;
> - else if (randomized && STREQ(buf, " }, \n"))
> + else if (randomized &&
> + (STREQ(buf, " }, \n") || STREQ(buf, "
},\n")))
> randomized = FALSE;
Looks fine but it looks like gdb-10.2 does not print a space before "\n",
I think we can _replace_ the STREQ(), not add, because the current branch
only supports gdb-10.2.
?>
Did you mean remove STREQ(buf, " }, \n") check, only to keep
STREQ(buf, " },\n") check? I'm not 100% sure if
no-space-before-"\n"
is the way gdb-10.2 always works. I suggest we keep the two checks,
just in case...
Thanks,
Tao Liu
Thanks,
Kazu
8:34 p.m.
On 2022/08/24 19:01, Tao Liu wrote:
>> diff --git a/task.c b/task.c
>> index 071c787..db2abc8 100644
>> --- a/task.c
>> +++ b/task.c
>> @@ -3436,7 +3436,8 @@ parse_task_thread(int argcnt, char *arglist[], struct
task_context *tc) {
>> while (fgets(buf, BUFSIZE, pc->tmpfile)) {
>> if (STREQ(buf, " {\n"))
>> randomized = TRUE;
>> - else if (randomized && STREQ(buf, " }, \n"))
>> + else if (randomized &&
>> + (STREQ(buf, " }, \n") || STREQ(buf, "
},\n")))
>> randomized = FALSE;
>
> Looks fine but it looks like gdb-10.2 does not print a space before "\n",
> I think we can _replace_ the STREQ(), not add, because the current branch
> only supports gdb-10.2.
?>
Did you mean remove STREQ(buf, " }, \n") check, only to keep
STREQ(buf, " },\n") check?
Yes.
I'm not 100% sure if no-space-before-"\n"
is the way gdb-10.2 always works. I suggest we keep the two checks,
just in case...
ok, it's a light function and I merely checked if gdb doesn't print
a space at end of lines with a few vmcores and outputs below, and
have not looked into the gdb source. It's ok to keep the two checks.
crash-dev> gdb p *(struct task_struct *)0xffff8f96c159c8c0 | grep ' $'
crash-dev>
Thanks,
Kazu
824
days inactive
825
days old
devel@lists.crash-utility.osci.io
3 comments
2 participants
participants (2)
-
HAGIO KAZUHITO(萩尾 一仁) -
Tao Liu