From patchwork Wed Nov 6 09:22:15 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nam Cao X-Patchwork-Id: 13864196 Received: from galois.linutronix.de (Galois.linutronix.de [193.142.43.55]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id E7E0B1D9668; Wed, 6 Nov 2024 09:22:34 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=193.142.43.55 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730884956; cv=none; b=UdRxXBxIlrftePpsSwin0Z0Z1Fy+YtdVPY1yCBQUpj7gtmD+wp1W4ySjeHLO4jIMqgbWE0s77yBY9St5WJvi62tePUUXlCA8emErU4pVPaWQp9dBw8o8TW1r/s7lGTSMfLXbXodqvAKeysiSZ28JTpIj79NPi25F+Yp9nH0Jkcs= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730884956; c=relaxed/simple; bh=9dvyDZDuzFgzVdh1Sna0suVR+NmThMTgtfsCk+jg5HQ=; h=From:To:Cc:Subject:Date:Message-Id:In-Reply-To:References: MIME-Version; b=kRps1L8m/S3PIuemO3AsxHfy8/E008+BVosTOYTwO5juFAOooaLfvV07DpikfiGcokZv0Vx39QZiYE5IhKlqf4FHOkVkzEPBPJf/Cu4cEoEkVlRUr129pt5KH5HqKWyCDeIU+wSvhcRuyCZBlkp9nmOUswqWnnhNk+2PQGrfWuY= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linutronix.de; spf=pass smtp.mailfrom=linutronix.de; dkim=pass (2048-bit key) header.d=linutronix.de header.i=@linutronix.de header.b=MObBbPf9; dkim=permerror (0-bit key) header.d=linutronix.de header.i=@linutronix.de header.b=pWmh/UVn; arc=none smtp.client-ip=193.142.43.55 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linutronix.de Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linutronix.de Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linutronix.de header.i=@linutronix.de header.b="MObBbPf9"; dkim=permerror (0-bit key) header.d=linutronix.de header.i=@linutronix.de header.b="pWmh/UVn" From: Nam Cao DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1730884953; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=P8oBmdWzEG5uKEf7eXEXq4AParcdRSRipW4U/BCZKfQ=; b=MObBbPf9pftiMail7/CM1jJESKDTea6PRR2e/LndqzfMa0RHFrsiPg88o/YrcuQqC6jBK8 c7o4+L5WhkrAgLNl21sw40kJNI3RmF91754ZcRj3jUQ8T/8bmpVuY2cnTPcATZFoHImd7w C6x+nW/EgEMMENBC6OmeyFWlKdRfd0XtdOfXeTDowbSVyURRnWO3DjGR3xoNYRd4fi0/tI RoH/86Km33+LNy+nM4+e9mlRGkhtbrL46e8Cq1gldbXSskfdljxTKIdThhYuwXjVj7DOhK HbirB+po9XIYaH7NX1oenkRbkFtyAaIf2x3FpdC9rmxiRW+iDGaNhTefvv3qOw== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1730884953; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=P8oBmdWzEG5uKEf7eXEXq4AParcdRSRipW4U/BCZKfQ=; b=pWmh/UVnZtabwAb4lrcxkMNxl6uVJmgzXC9t/GFHd9v+Krx09n9kC5KHzm8kH6ZtTB571U RQ3Yl6N2YiRVOrBQ== To: Shuah Khan , Andrew Morton , Oleg Nesterov , Dylan Hatch , "Eric W . Biederman" , John Ogness , Kees Cook , linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-kselftest@vger.kernel.org Cc: Nam Cao , stable@vger.kernel.org Subject: [PATCH 1/2] fs/proc: do_task_stat: Fix ESP not readable during coredump Date: Wed, 6 Nov 2024 10:22:15 +0100 Message-Id: <11e1777296b7d06085c9fd341bafc4b9d82e6e4e.1730883229.git.namcao@linutronix.de> In-Reply-To: References: Precedence: bulk X-Mailing-List: linux-fsdevel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Commit 0a1eb2d474ed ("fs/proc: Stop reporting eip and esp in /proc/PID/stat") disabled stack pointer reading, because it is generally dangerous to do so. Commit fd7d56270b52 ("fs/proc: Report eip/esp in /prod/PID/stat for coredumping") made an exception for coredumping thread, because for this case it is safe. The exception was later extended to all threads in a coredumping process by commit cb8f381f1613 ("fs/proc/array.c: allow reporting eip/esp for all coredumping threads"). The above two commits determine if a task is core dumping by checking the PF_EXITING and PF_DUMPCORE flags. However, commit 92307383082d ("coredump: Don't perform any cleanups before dumping core") moved coredump to happen earlier and before PF_EXITING is set. Thus, the check of the PF_EXITING flag no longer works. Instead, use task->signal->core_state to determine if coredump is happening. This pointer is set at the beginning of coredump and is cleared once coredump is done. Thus, while this pointer is not NULL, it is safe to read ESP. Fixes: 92307383082d ("coredump: Don't perform any cleanups before dumping core") Signed-off-by: Nam Cao Cc: Cc: Eric W. Biederman Reviewed-by: John Ogness --- fs/proc/array.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/fs/proc/array.c b/fs/proc/array.c index 34a47fb0c57f..2f1dbfcf143d 100644 --- a/fs/proc/array.c +++ b/fs/proc/array.c @@ -489,25 +489,8 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns, vsize = eip = esp = 0; permitted = ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS | PTRACE_MODE_NOAUDIT); mm = get_task_mm(task); - if (mm) { + if (mm) vsize = task_vsize(mm); - /* - * esp and eip are intentionally zeroed out. There is no - * non-racy way to read them without freezing the task. - * Programs that need reliable values can use ptrace(2). - * - * The only exception is if the task is core dumping because - * a program is not able to use ptrace(2) in that case. It is - * safe because the task has stopped executing permanently. - */ - if (permitted && (task->flags & (PF_EXITING|PF_DUMPCORE))) { - if (try_get_task_stack(task)) { - eip = KSTK_EIP(task); - esp = KSTK_ESP(task); - put_task_stack(task); - } - } - } sigemptyset(&sigign); sigemptyset(&sigcatch); @@ -534,6 +517,23 @@ static int do_task_stat(struct seq_file *m, struct pid_namespace *ns, ppid = task_tgid_nr_ns(task->real_parent, ns); pgid = task_pgrp_nr_ns(task, ns); + /* + * esp and eip are intentionally zeroed out. There is no + * non-racy way to read them without freezing the task. + * Programs that need reliable values can use ptrace(2). + * + * The only exception is if the task is core dumping because + * a program is not able to use ptrace(2) in that case. It is + * safe because the task has stopped executing permanently. + */ + if (permitted && task->signal->core_state) { + if (try_get_task_stack(task)) { + eip = KSTK_EIP(task); + esp = KSTK_ESP(task); + put_task_stack(task); + } + } + unlock_task_sighand(task, &flags); } From patchwork Wed Nov 6 09:22:16 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nam Cao X-Patchwork-Id: 13864197 Received: from galois.linutronix.de (Galois.linutronix.de [193.142.43.55]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2DB6A1DA614; Wed, 6 Nov 2024 09:22:36 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=193.142.43.55 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730884959; cv=none; b=csLSshqhQ2tNRozlrZ/RAaEYBgHusoJuKxKPSeFbBdZt+/HpEBIS0LcQS8Sov3hOaulJXc5AFkHTvxCDb/Ftlt7gzPP9eS3nLcB7VT/SZN40W5UYmhu7BHispsVDLG2VFnd78S3I822Pz3vnYLlMwVosPb4TrGEF0d0SxmRa5Ck= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1730884959; c=relaxed/simple; bh=Q0IEevhvNWiwG/+ezR/1kQosqOlRJHQD7oxQy+ZmEjY=; h=From:To:Cc:Subject:Date:Message-Id:In-Reply-To:References: MIME-Version; b=I6Kkm6VWkoDg17VO+vu+bQG6xvd/ovLIuLkx4txpXcYjE0oq5FC9hPQd0J6AvMeGFkKbQS+G+VAkP7TYhqVKEH7xMFVy8BgfcrKdNa95GzvumFGhBZzMvVCVwmrnEmpgEJmBEFU9L7LrMQ+l8rXmR8SBpDpBRVkRe4+d3bYgWr0= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linutronix.de; spf=pass smtp.mailfrom=linutronix.de; dkim=pass (2048-bit key) header.d=linutronix.de header.i=@linutronix.de header.b=elMj6RuM; dkim=permerror (0-bit key) header.d=linutronix.de header.i=@linutronix.de header.b=I/RFul0L; arc=none smtp.client-ip=193.142.43.55 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linutronix.de Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linutronix.de Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linutronix.de header.i=@linutronix.de header.b="elMj6RuM"; dkim=permerror (0-bit key) header.d=linutronix.de header.i=@linutronix.de header.b="I/RFul0L" From: Nam Cao DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1730884955; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=4Puj4jtBe4cihWt1KBtKTj/Cb61WVPFZL8f3kYIJNuc=; b=elMj6RuMt+9526KWezxi4BOlFRJtAGo+v+Vmjov/GFfgSZye0n1vQQTDJPk9w3sTWKt7Nz Mx/Wa/gjK42pH5ynx16gU6i8ReUHHdvsHxqDNf1Vk9l1EuM3eq10YFkQgJxVNZfFX5V40W avaJyflctAAZWawg8NQ2VCR9E+d1FF4vpPr2apXjkA6WS54S5B5I30XKxB5t2c8H4rxLDk YvGJ1WXbPsPPOBC7QrbFU1epWLoYKUMbrGl/mxG7Wayo/1iGAx7kMDc/t/jBk0kDWUOFkT qKzJZ4rYmlApJD3r3z5Ydf2RBuV6EJck20cE3eK1a9t05pSPoL5oLHj4o2C9lw== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1730884955; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=4Puj4jtBe4cihWt1KBtKTj/Cb61WVPFZL8f3kYIJNuc=; b=I/RFul0LHz/YTul5OaaEUPimHPu5F2+b+tJeHkL82vXQJ8adNxskzh/XqBS7VMUCC6VVZm 0L6MkDut4Z4J7ZBg== To: Shuah Khan , Andrew Morton , Oleg Nesterov , Dylan Hatch , "Eric W . Biederman" , John Ogness , Kees Cook , linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-kselftest@vger.kernel.org Cc: Nam Cao Subject: [PATCH 2/2] selftests: coredump: Add stackdump test Date: Wed, 6 Nov 2024 10:22:16 +0100 Message-Id: <66d3fbd62cc1a658ce4f77eb907f8737c467fdbf.1730883229.git.namcao@linutronix.de> In-Reply-To: References: Precedence: bulk X-Mailing-List: linux-fsdevel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Add a test which checks that the kstkesp field in /proc/pid/stat can be read for all threads of a coredumping process. For full details including the motivation for this test and how it works, see the README file added by this commit. Signed-off-by: Nam Cao Reviewed-by: John Ogness --- tools/testing/selftests/coredump/Makefile | 7 + tools/testing/selftests/coredump/README.rst | 50 ++++++ tools/testing/selftests/coredump/stackdump | 14 ++ .../selftests/coredump/stackdump_test.c | 154 ++++++++++++++++++ 4 files changed, 225 insertions(+) create mode 100644 tools/testing/selftests/coredump/Makefile create mode 100644 tools/testing/selftests/coredump/README.rst create mode 100755 tools/testing/selftests/coredump/stackdump create mode 100644 tools/testing/selftests/coredump/stackdump_test.c diff --git a/tools/testing/selftests/coredump/Makefile b/tools/testing/selftests/coredump/Makefile new file mode 100644 index 000000000000..ed210037b29d --- /dev/null +++ b/tools/testing/selftests/coredump/Makefile @@ -0,0 +1,7 @@ +# SPDX-License-Identifier: GPL-2.0-only +CFLAGS = $(KHDR_INCLUDES) + +TEST_GEN_PROGS := stackdump_test +TEST_FILES := stackdump + +include ../lib.mk diff --git a/tools/testing/selftests/coredump/README.rst b/tools/testing/selftests/coredump/README.rst new file mode 100644 index 000000000000..164a7aa181c8 --- /dev/null +++ b/tools/testing/selftests/coredump/README.rst @@ -0,0 +1,50 @@ +coredump selftest +================= + +Background context +------------------ + +`coredump` is a feature which dumps a process's memory space when the process terminates +unexpectedly (e.g. due to segmentation fault), which can be useful for debugging. By default, +`coredump` dumps the memory to the file named `core`, but this behavior can be changed by writing a +different file name to `/proc/sys/kernel/core_pattern`. Furthermore, `coredump` can be piped to a +user-space program by writing the pipe symbol (`|`) followed by the command to be executed to +`/proc/sys/kernel/core_pattern`. For the full description, see `man 5 core`. + +The piped user program may be interested in reading the stack pointers of the crashed process. The +crashed process's stack pointers can be read from `procfs`: it is the `kstkesp` field in +`/proc/$PID/stat`. See `man 5 proc` for all the details. + +The problem +----------- +While a thread is active, the stack pointer is unsafe to read and therefore the `kstkesp` field +reads zero. But when the thread is dead (e.g. during a coredump), this field should have valid +value. + +However, this was broken in the past and `kstkesp` was zero even during coredump: + +* commit 0a1eb2d474ed ("fs/proc: Stop reporting eip and esp in /proc/PID/stat") changed kstkesp to + always be zero + +* commit fd7d56270b52 ("fs/proc: Report eip/esp in /prod/PID/stat for coredumping") fixed it for the + coredumping thread. However, other threads in a coredumping process still had the problem. + +* commit cb8f381f1613 ("fs/proc/array.c: allow reporting eip/esp for all coredumping threads") fixed + for all threads in a coredumping process. + +* commit 92307383082d ("coredump: Don't perform any cleanups before dumping core") broke it again + for the other threads in a coredumping process. + +The problem has been fixed now, but considering the history, it may appear again in the future. + +The goal of this test +--------------------- +This test detects problem with reading `kstkesp` during coredump by doing the following: + +#. Tell the kernel to execute the "stackdump" script when a coredump happens. This script + reads the stack pointers of all threads of crashed processes. + +#. Spawn a child process who creates some threads and then crashes. + +#. Read the output from the "stackdump" script, and make sure all stack pointer values are + non-zero. diff --git a/tools/testing/selftests/coredump/stackdump b/tools/testing/selftests/coredump/stackdump new file mode 100755 index 000000000000..96714ce42d12 --- /dev/null +++ b/tools/testing/selftests/coredump/stackdump @@ -0,0 +1,14 @@ +#!/bin/sh +# SPDX-License-Identifier: GPL-2.0 + +CRASH_PROGRAM_ID=$1 +STACKDUMP_FILE=$2 + +TMP=$(mktemp) + +for t in /proc/$CRASH_PROGRAM_ID/task/*; do + tid=$(basename $t) + cat /proc/$tid/stat | awk '{print $29}' >> $TMP +done + +mv $TMP $STACKDUMP_FILE diff --git a/tools/testing/selftests/coredump/stackdump_test.c b/tools/testing/selftests/coredump/stackdump_test.c new file mode 100644 index 000000000000..b86202aa2f04 --- /dev/null +++ b/tools/testing/selftests/coredump/stackdump_test.c @@ -0,0 +1,154 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include +#include +#include +#include +#include +#include +#include + +#include "../kselftest_harness.h" + +#define STACKDUMP_FILE "/tmp/kselftest_stackdump" +#define STACKDUMP_SCRIPT "stackdump" +#define NUM_THREAD_SPAWN 128 + +static void *do_nothing(void *) +{ + while (1) + pause(); +} + +static void crashing_child(void) +{ + pthread_t thread; + int i; + + for (i = 0; i < NUM_THREAD_SPAWN; ++i) + pthread_create(&thread, NULL, do_nothing, NULL); + + /* crash on purpose */ + i = *(int *)NULL; +} + +static void empty_function(int) {} + +FIXTURE(coredump) +{ + char original_core_pattern[256]; +}; + +FIXTURE_SETUP(coredump) +{ + char buf[PATH_MAX]; + FILE *file; + char *dir; + int ret; + + file = fopen("/proc/sys/kernel/core_pattern", "r"); + ASSERT_NE(NULL, file); + + ret = fread(self->original_core_pattern, 1, sizeof(self->original_core_pattern), file); + ASSERT_TRUE(ret || feof(file)); + ASSERT_LT(ret, sizeof(self->original_core_pattern)); + + self->original_core_pattern[ret] = '\0'; + + ret = fclose(file); + ASSERT_EQ(0, ret); +} + +FIXTURE_TEARDOWN(coredump) +{ + const char *reason; + FILE *file; + int ret; + + file = fopen("/proc/sys/kernel/core_pattern", "w"); + if (!file) { + reason = "Unable to open core_pattern"; + goto fail; + } + + ret = fprintf(file, "%s", self->original_core_pattern); + if (ret < 0) { + reason = "Unable to write to core_pattern"; + goto fail; + } + + ret = fclose(file); + if (ret) { + reason = "Unable to close core_pattern"; + goto fail; + } + + return; +fail: + /* This should never happen */ + fprintf(stderr, "Failed to cleanup stackdump test: %s\n", reason); +} + +TEST_F(coredump, stackdump) +{ + struct sigaction action = {}; + unsigned long long stack; + char *test_dir, *line; + size_t line_length; + char buf[PATH_MAX]; + int ret, i; + FILE *file; + pid_t pid; + + /* This file may be left behind by a previous test run */ + unlink(STACKDUMP_FILE); + + /* + * Step 1: Setup core_pattern so that the stackdump script is executed when the child + * process crashes + */ + ret = readlink("/proc/self/exe", buf, sizeof(buf)); + ASSERT_NE(-1, ret); + ASSERT_LT(ret, sizeof(buf)); + buf[ret] = '\0'; + + test_dir = dirname(buf); + + file = fopen("/proc/sys/kernel/core_pattern", "w"); + ASSERT_NE(NULL, file); + + ret = fprintf(file, "|%s/%s %%P %s", test_dir, STACKDUMP_SCRIPT, STACKDUMP_FILE); + ASSERT_LT(0, ret); + + ret = fclose(file); + ASSERT_EQ(0, ret); + + /* Step 2: Create a process who spawns some threads then crashes */ + pid = fork(); + ASSERT_TRUE(pid >= 0); + if (pid == 0) + crashing_child(); + + /* + * Step 3: Wait for the stackdump script to write the stack pointers to the stackdump file + */ + for (i = 0; i < 10; ++i) { + file = fopen(STACKDUMP_FILE, "r"); + if (file) + break; + sleep(1); + } + ASSERT_NE(file, NULL); + + /* Step 4: Make sure all stack pointer values are non-zero */ + for (i = 0; -1 != getline(&line, &line_length, file); ++i) { + stack = strtoull(line, NULL, 10); + ASSERT_NE(stack, 0); + } + + ASSERT_EQ(i, 1 + NUM_THREAD_SPAWN); + + fclose(file); +} + +TEST_HARNESS_MAIN