Skip to content

[Support] Fix Process::PreventCoreFiles() when coredumps are piped #83703

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

arichardson
Copy link
Member

@arichardson arichardson commented Mar 3, 2024

On many current Linux systems, coredumps are no longer dumped in the CWD
but instead piped to a utility such as systemd-coredumpd that stores
them in a deterministic location. This can be done by setting the
kernel.core_pattern sysctl to start with a '|'. However, when using such
a setup the kernel ignores a coredump limit of 0 (since there is no file
being written) and we can end up piping many gigabytes of data to
systemd-coredumpd which causes the test suite to freeze for a long time.
While most piped coredump handlers do respect the crashing processes'
RLIMIT_CORE, this is notable not the case for Debian's systemd-coredump
due to a local patch that changes sysctl.d/50-coredump.conf to ignore
the specified limit and instead use RLIM_INFINITY
(https://salsa.debian.org/systemd-team/systemd/-/commit/64599ffe44f0d).

Fortunately there is a workaround: the kernel recognizes the magic value
of 1 for RLIMIT_CORE to disable coredumps when piping. One byte is also
too small to generate any coredump, so it effectively behaves as if we
had set the value to zero.

The alternative to using RLIMIT_CORE=1 would be to use prctl() with the
PR_SET_DUMPABLE flag, however that also prevents ptrace(), so makes it
impossible to attach a debugger.

See #83701 and
#45797

Created using spr 1.3.4
@llvmbot
Copy link
Member

llvmbot commented Mar 3, 2024

@llvm/pr-subscribers-llvm-support

Author: Alexander Richardson (arichardson)

Changes

On Linux, if the kernel.core_pattern sysctl starts with a '|' (i.e. it
is being piped to a coredump handler such as systemd-coredumpd), the
kernel ignores RLIMIT_CORE (since we aren't creating a file in the file
system) except for the magic value of 1, that disables coredumps when
piping. 1 byte is also too small for any kind of valid core dump, so it
also disables coredumps if kernel.core_pattern creates files directly.

See #83701 and
#45797


Full diff: https://github.com/llvm/llvm-project/pull/83703.diff

1 Files Affected:

  • (modified) llvm/lib/Support/Unix/Process.inc (+12-1)
diff --git a/llvm/lib/Support/Unix/Process.inc b/llvm/lib/Support/Unix/Process.inc
index f94eec6963c18e..ac80c199d028c9 100644
--- a/llvm/lib/Support/Unix/Process.inc
+++ b/llvm/lib/Support/Unix/Process.inc
@@ -143,7 +143,18 @@ void Process::GetTimeUsage(TimePoint<> &elapsed,
 void Process::PreventCoreFiles() {
 #if HAVE_SETRLIMIT
   struct rlimit rlim;
-  rlim.rlim_cur = rlim.rlim_max = 0;
+  getrlimit(RLIMIT_CORE, &rlim);
+#ifdef __linux__
+  // On Linux, if the kernel.core_pattern sysctl starts with a '|' (i.e. it
+  // is being piped to a coredump handler such as systemd-coredumpd), the
+  // kernel ignores RLIMIT_CORE (since we aren't creating a file in the file
+  // system) except for the magic value of 1, that disables coredumps when
+  // piping. 1 byte is also too small for any kind of valid core dump, so it
+  // also disables coredumps if kernel.core_pattern creates files directly.
+  rlim.rlim_cur = std::min<rlim_t>(1, rlim.rlim_max);
+#else
+  rlim.rlim_cur = 0;
+#endif
   setrlimit(RLIMIT_CORE, &rlim);
 #endif
 

@arichardson arichardson requested a review from aganea March 3, 2024 03:19
@aganea aganea requested a review from MaskRay March 3, 2024 19:50
@MaskRay
Copy link
Member

MaskRay commented Mar 4, 2024

Does 1 leave a kernel warning when kernel.core_pattern specifies a pipe? Then how about PR_SET_DUMPABLE?

@arichardson
Copy link
Member Author

I considered that, but according to the manpage it means we can't ptrace the process afterwards:

Processes that are not dumpable can not be attached via ptrace(2) PTRACE_ATTACH; see ptrace(2) for further details.

If a process is not dumpable, the ownership of files in the process's /proc/pid directory is affected as described in proc(5).

It does look like it will add to dmesg (but it already seems flooded with lots of other messages):

[  +0.218577] traps: bounds.cpp.tmp[244068] trap invalid opcode ip:561028d05dc9 sp:7ffddaacc640 error:0 in bounds.cpp.tmp[561028c32000+d4000]
[  +0.013130] Process 244068(bounds.cpp.tmp) has RLIMIT_CORE set to 1
[  +0.006593] Aborting core

Is this a dealbreak? I can see if another value avoids the coredump without spamming dmesg.

@MaskRay
Copy link
Member

MaskRay commented Mar 5, 2024

I considered that, but according to the manpage it means we can't ptrace the process afterwards:

Processes that are not dumpable can not be attached via ptrace(2) PTRACE_ATTACH; see ptrace(2) for further details.
If a process is not dumpable, the ownership of files in the process's /proc/pid directory is affected as described in proc(5).

It does look like it will add to dmesg (but it already seems flooded with lots of other messages):

[  +0.218577] traps: bounds.cpp.tmp[244068] trap invalid opcode ip:561028d05dc9 sp:7ffddaacc640 error:0 in bounds.cpp.tmp[561028c32000+d4000]
[  +0.013130] Process 244068(bounds.cpp.tmp) has RLIMIT_CORE set to 1
[  +0.006593] Aborting core

Is this a dealbreak? I can see if another value avoids the coredump without spamming dmesg.

It isn't a dealbreaker. Since PR_SET_DUMPABLE 0 would disable PTRACE_ATTACH, a feature which I consider more important, it seems that we should just tolerate it... This seems a Linux API issue that doesn't offer desired granularity....

@arichardson
Copy link
Member Author

I considered that, but according to the manpage it means we can't ptrace the process afterwards:

Processes that are not dumpable can not be attached via ptrace(2) PTRACE_ATTACH; see ptrace(2) for further details.
If a process is not dumpable, the ownership of files in the process's /proc/pid directory is affected as described in proc(5).

It does look like it will add to dmesg (but it already seems flooded with lots of other messages):

[  +0.218577] traps: bounds.cpp.tmp[244068] trap invalid opcode ip:561028d05dc9 sp:7ffddaacc640 error:0 in bounds.cpp.tmp[561028c32000+d4000]
[  +0.013130] Process 244068(bounds.cpp.tmp) has RLIMIT_CORE set to 1
[  +0.006593] Aborting core

Is this a dealbreak? I can see if another value avoids the coredump without spamming dmesg.

It isn't a dealbreaker. Since PR_SET_DUMPABLE 0 would disable PTRACE_ATTACH, a feature which I consider more important, it seems that we should just tolerate it... This seems a Linux API issue that doesn't offer desired granularity....

Sounds good! I just tried reading through the systemd coredump code and it seems like they don't write the coredump if the rlimit is set to a value smaller than one PAGE: https://github.com/systemd/systemd/blob/1500b656cd6e4500dd0514d90c27fe0e16b14c77/src/coredump/coredump.c#L438

However, it seems like the kernel is still streaming many gigabytes of data to the file descriptor. I'm not sure if that is an issue on the systemd side or the kernel side.

@arichardson
Copy link
Member Author

Further debugging shows that the systemd-coredumpd config file has a hardcoded RLIMIT_INFINITY value:

❯ sysctl kernel.core_pattern                            
kernel.core_pattern = |/lib/systemd/systemd-coredump %P %u %g %s %t 9223372036854775808 %h

After changing the value to use %c as it should be in the default config file (https://github.com/systemd/systemd/blob/main/sysctl.d/50-coredump.conf.in) coredumpctl list -r now lists the failed tests, but no longer blocks streaming the gigabytes of data trying to generate a coredump.

TIME                           PID    UID   GID SIG     COREFILE  EXE                  SIZE
Mon 2024-03-04 22:45:51 PST 393894 928191 89939 SIGABRT none      .../stack-uas.c.tmp  -
Mon 2024-03-04 22:45:10 PST 393458 928191 89939 SIGABRT none      .../stack-uas.c.tmp  -

I will file a bug with the people configuring the system, so that it gets fixed for all my coworkers but even though this is a misconfigured system, I think setting the value to 1 is still beneficial since these are expected coredumps that don't need to show up coredumpctl.

@arichardson
Copy link
Member Author

It turns out all of this is caused by the following Debian patch https://salsa.debian.org/systemd-team/systemd/-/commit/64599ffe44f0d.

Created as part of https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=815020

arichardson added a commit to arichardson/upstream-llvm-project that referenced this pull request Mar 7, 2024
On Linux, if the kernel.core_pattern sysctl starts with a '|' (i.e. it
is being piped to a coredump handler such as systemd-coredumpd), the
kernel ignores RLIMIT_CORE (since we aren't creating a file in the file
system) except for the magic value of 1, that disables coredumps when
piping. 1 byte is also too small for any kind of valid core dump, so it
also disables coredumps if kernel.core_pattern creates files directly.

See llvm#83701 and
llvm#45797

Pull Request: llvm#83703
Created using spr 1.3.6-beta.1
@arichardson arichardson merged commit abbf1f1 into main Mar 9, 2024
@arichardson arichardson deleted the users/arichardson/spr/support-fix-processpreventcorefiles-when-coredumps-are-piped branch March 9, 2024 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants