Date: Mon, 12 May 2025 10:22:43 +1200 From: Tao Liu <ltao(a)redhat.com&gt; Subject: [Crash-utility] Re: [PATCH] Fix incorrect task state during exit To: Stephen Brennan <stephen.s.brennan(a)oracle.com&gt; Cc: devel(a)lists.crash-utility.osci.io Message-ID: <CAO7dBbWHa5x+BdiVF3y_QL-JgQhDLgEqKzaQS+YfBrL= jY6N_w(a)mail.gmail.com&gt; Content-Type: text/plain; charset="UTF-8" Hi Stephen, Thanks for your fix and detailed explanation! On Sat, May 3, 2025 at 8:19 AM Stephen Brennan <stephen.s.brennan(a)oracle.com&gt; wrote: > > task_state() assumes that exit_state is a unsigned long, when in > reality, it has been declared as an int since 97dc32cdb1b53 ("reduce > size of task_struct on 64-bit machines"), in Linux 2.6.22. So on 64-bit > machines, task_state() reads 8 bytes rather than 4, and gets the wrong > exit_state value by including the next field. > > This has gone unnoticed because directly after exit_state comes > exit_code, which is generally zero while the task is alive. When the > exit_code is set, exit_state is usually set not long after. Since > task_state_string() only checks whether exit_state bits are set, it > never notices the presence of the exit code inside of the state. > > But this leaves open a window during the process exit, when the > exit_code has been set (in do_exit()), but the exit_state has not (in > exit_notify()). In this case, crash reports a state of "??", but in > reality, the task is still running -- it's just running the exit() > system call. This race window can be long enough to be observed in core > dumps, for example if the mmput() takes a long time. > > This should be considered a bug. A task state of "??" or "(unknown)" is > frequently of concern when debugging, as it could indicate that the > state fields had some sort of corruption, and draw the attention of the > debugger. To handle it properly, record the size of exit_state, and read > it conditionally as a UINT or ULONG, just like the state. This ensures > we retain compatibility with kernel before v2.6.22. Whether that is > actually desirable is anybody's guess. > > Reported-by: Jeffery Yoder <jeffery.yoder(a)oracle.com&gt; > Signed-off-by: Stephen Brennan <stephen.s.brennan(a)oracle.com&gt; > --- > defs.h | 1 + > task.c | 11 +++++++++-- > 2 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/defs.h b/defs.h > index 4cf169c..58362d0 100644 > --- a/defs.h > +++ b/defs.h > @@ -2435,6 +2435,7 @@ struct size_table { /* stash of commonly-used sizes */ > long prb_desc; > long wait_queue_entry; > long task_struct_state; > + long task_struct_exit_state; > long printk_safe_seq_buf_buffer; > long sbitmap_word; > long sbitmap; The patch looks good to me, except for the above code. Any newly added

members for size/offset_table should go to the end of the struct, rather than the middle of it. See the 2nd item of https://github.com/crash-utility/crash/wiki#writing-patches. But this is a slight error, I can get it corrected when merging. Other than that, Ack for the patch. Thanks, Tao Liu > diff --git a/task.c b/task.c > index 3bafe79..e07b479 100644 > --- a/task.c > +++ b/task.c > @@ -306,6 +306,7 @@ task_init(void) > MEMBER_SIZE_INIT(task_struct_state, "task_struct", "__state"); > } > MEMBER_OFFSET_INIT(task_struct_exit_state, "task_struct", "exit_state"); > + MEMBER_SIZE_INIT(task_struct_exit_state, "task_struct", "exit_state"); > MEMBER_OFFSET_INIT(task_struct_pid, "task_struct", "pid"); > MEMBER_OFFSET_INIT(task_struct_comm, "task_struct", "comm"); > MEMBER_OFFSET_INIT(task_struct_next_task, "task_struct", "next_task"); > @@ -5965,8 +5966,14 @@ task_state(ulong task) > state = ULONG(tt->task_struct + OFFSET(task_struct_state)); > else > state = UINT(tt->task_struct + OFFSET(task_struct_state)); > - exit_state = VALID_MEMBER(task_struct_exit_state) ? > - ULONG(tt->task_struct + OFFSET(task_struct_exit_state)) : 0; > + > + if (VALID_MEMBER(task_struct_exit_state) > + && SIZE(task_struct_exit_state) == sizeof(ulong)) > + exit_state = ULONG(tt->task_struct + OFFSET(task_struct_exit_state)); > + else if (VALID_MEMBER(task_struct_exit_state)) > + exit_state = UINT(tt->task_struct + OFFSET(task_struct_exit_state)); > + else > + exit_state = 0; > > return (state | exit_state); > } > --