| Message ID | 20240928210830.work.307-kees@kernel.org (mailing list archive) |
|---|---|
| State | In Next |
| Commit | 200f091c95bbc4b8660636bd345805c45d6eced7 |
| Headers | show |
| Series | coredump: Do not lock during 'comm' reporting | expand |
On Sat, 28 Sep 2024 14:08:31 -0700 Kees Cook <kees@kernel.org> wrote: > The 'comm' member will always be NUL terminated, Why is this? I thought this is only true if the caller holds task_lock()? > and this is not > fast-path, so we can just perform a direct memcpy during a coredump > instead of potentially deadlocking while holding the task struct lock. >
On Sat, Sep 28, 2024 at 02:35:32PM -0700, Andrew Morton wrote: > On Sat, 28 Sep 2024 14:08:31 -0700 Kees Cook <kees@kernel.org> wrote: > > > The 'comm' member will always be NUL terminated, > > Why is this? I thought this is only true if the caller holds task_lock()? Because it's always written with strscpy_pad(). The final byte will always be NUL. (And this has been true for a very long time.)
On Sat, 28 Sep 2024 14:39:45 -0700 Kees Cook <kees@kernel.org> wrote: > On Sat, Sep 28, 2024 at 02:35:32PM -0700, Andrew Morton wrote: > > On Sat, 28 Sep 2024 14:08:31 -0700 Kees Cook <kees@kernel.org> wrote: > > > > > The 'comm' member will always be NUL terminated, > > > > Why is this? I thought this is only true if the caller holds task_lock()? > > Because it's always written with strscpy_pad(). The final byte will > always be NUL. (And this has been true for a very long time.) So why does __get_task_comm() need to take task_lock()?
On Sat, Sep 28, 2024 at 02:46:36PM -0700, Andrew Morton wrote: > On Sat, 28 Sep 2024 14:39:45 -0700 Kees Cook <kees@kernel.org> wrote: > > > On Sat, Sep 28, 2024 at 02:35:32PM -0700, Andrew Morton wrote: > > > On Sat, 28 Sep 2024 14:08:31 -0700 Kees Cook <kees@kernel.org> wrote: > > > > > > > The 'comm' member will always be NUL terminated, > > > > > > Why is this? I thought this is only true if the caller holds task_lock()? > > > > Because it's always written with strscpy_pad(). The final byte will > > always be NUL. (And this has been true for a very long time.) > > So why does __get_task_comm() need to take task_lock()? That was to make sure we didn't end up with garbled results, but discussions have determined that we don't care: https://lore.kernel.org/all/20240828030321.20688-1-laoar.shao@gmail.com/ But just for safety's sake, I'll change this memcpy to: memcpy_and_pad(comm, sizeof(comm), current->comm, sizeof(comm) - 1, 0);
On 28/09/2024 23:51, Kees Cook wrote: > On Sat, Sep 28, 2024 at 02:46:36PM -0700, Andrew Morton wrote: >> On Sat, 28 Sep 2024 14:39:45 -0700 Kees Cook <kees@kernel.org> wrote: >>> On Sat, Sep 28, 2024 at 02:35:32PM -0700, Andrew Morton wrote: >> So why does __get_task_comm() need to take task_lock()? > > That was to make sure we didn't end up with garbled results, but > discussions have determined that we don't care: > https://lore.kernel.org/all/20240828030321.20688-1-laoar.shao@gmail.com/ > > But just for safety's sake, I'll change this memcpy to: > > memcpy_and_pad(comm, sizeof(comm), current->comm, sizeof(comm) - 1, 0); > Reverting Linus's revert, applying the patch in this thread, and then changing it to that memcpy_and_pad above, everything seems to work well. Tested-by: Vegard Nossum <vegard.nossum@oracle.com> (However, I have not looked at the _safety_ of omitting task_lock()...) I'm also wondering how I could successfully cat /proc/$pid/status before on one of these hung processes given that it does __get_task_comm(), which does task_lock(), which should have presumably hung as well? Finally, I'll add that the comm/pid format is different from some other messages: kernel: coredump: 31447(entry-fuzz.1): coredump has not been created, error -13 kernel: entry-fuzz.1[43396] bad frame in rt_sigreturn frame:00000000e4ec200e ip:40a6712e sp:40c25f20 orax:f Vegard
diff --git a/include/linux/coredump.h b/include/linux/coredump.h index edeb8532ce0f..a99079115a38 100644 --- a/include/linux/coredump.h +++ b/include/linux/coredump.h @@ -52,8 +52,8 @@ extern int do_coredump(const kernel_siginfo_t *siginfo); #define __COREDUMP_PRINTK(Level, Format, ...) \ do { \ char comm[TASK_COMM_LEN]; \ - \ - get_task_comm(comm, current); \ + /* This will always be NUL terminated. */ \ + memcpy(comm, current->comm, sizeof(comm)); \ printk_ratelimited(Level "coredump: %d(%*pE): " Format "\n", \ task_tgid_vnr(current), (int)strlen(comm), comm, ##__VA_ARGS__); \ } while (0) \
The 'comm' member will always be NUL terminated, and this is not fast-path, so we can just perform a direct memcpy during a coredump instead of potentially deadlocking while holding the task struct lock. Reported-by: Vegard Nossum <vegard.nossum@oracle.com> Closes: https://lore.kernel.org/all/d122ece6-3606-49de-ae4d-8da88846bef2@oracle.com Fixes: c114e9948c2b ("coredump: Standartize and fix logging") Signed-off-by: Kees Cook <kees@kernel.org> --- Cc: "Eric W. Biederman" <ebiederm@xmission.com> Cc: Allen Pais <apais@linux.microsoft.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Roman Kisel <romank@linux.microsoft.com> Cc: Xiaoming Ni <nixiaoming@huawei.com> Vegard, can you validate that this fixes the problem for you? I have been wrecked by covid, so very slow to respond here. There's a related thread about this locked, but we can just totally bypass it here. --- include/linux/coredump.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)