Hi Kazu,
On Wed, Aug 24, 2022 at 3:54 PM HAGIO KAZUHITO(萩尾 一仁)
<k-hagio-ab@xxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
