If we're mlocking something, it's because it's sensitive information. Therefore, don't include it in core dump files, and unmap it from forked processes.
The return value is not checked because the madvise calls might fail on older kernels as a rule (unsure).
No apparent reviewer interest, and it seems to break -daemon with no obvious solution.
Reopened due to renewed interest, and removed the DONTFORK part that was broken
Concept ACK
<!--a722867cd34abeea1fadc8d60700f111-->
| File | commit a71c34742c24245a0d642e68f9e61f46cc7573fb<br>(master) | commit 2d320372aa2aba3512512d30e2e6b5c4483cd5c0<br>(master and this pull) |
|---|---|---|
| bitcoin-0.19.99-aarch64-linux-gnu-debug.tar.gz | 263dd4df65588db4... |
87a836ebe2640c24... |
| bitcoin-0.19.99-aarch64-linux-gnu.tar.gz | 3f779204013ffde0... |
b25121535f4b3e4e... |
| bitcoin-0.19.99-arm-linux-gnueabihf-debug.tar.gz | f6cc7d17fdf2b4f0... |
9bf22b2ea053fe9f... |
| bitcoin-0.19.99-arm-linux-gnueabihf.tar.gz | b08643b04f8d69c0... |
0811dc229d30b135... |
| bitcoin-0.19.99-osx-unsigned.dmg | 82112ea8d8b8dfb7... |
3f15eeb9c8995188... |
| bitcoin-0.19.99-osx64.tar.gz | e7408d167771af4e... |
452145dfff313f76... |
| bitcoin-0.19.99-riscv64-linux-gnu-debug.tar.gz | af924ded182d7648... |
98cdc0cf6d4dcb61... |
| bitcoin-0.19.99-riscv64-linux-gnu.tar.gz | d6a4f981ad9fa31f... |
f71881fa68838b30... |
| bitcoin-0.19.99-win64-debug.zip | b0796bf49b421fa0... |
8b6b299372408b9b... |
| bitcoin-0.19.99-win64-setup-unsigned.exe | 3a473d863012b1fe... |
340157f6bb540f2b... |
| bitcoin-0.19.99-win64.zip | f658d68b14d640c3... |
5a0e6165b71cc67f... |
| bitcoin-0.19.99-x86_64-linux-gnu-debug.tar.gz | dbab3fa3cc95a255... |
554ef07154623a5d... |
| bitcoin-0.19.99-x86_64-linux-gnu.tar.gz | fa419172b1f67f66... |
de1b54355262591a... |
| bitcoin-0.19.99.tar.gz | 38d9e79747ecfb3b... |
558c333681cb5459... |
| bitcoin-core-linux-0.20-res.yml | 0bb8e45015337601... |
4e8a46fb1c0d3f43... |
| bitcoin-core-osx-0.20-res.yml | 587b85f6de99b01c... |
14718c9a1dc15f0c... |
| bitcoin-core-win-0.20-res.yml | 4cb36ab35ba216ed... |
903f3ad4472eea5e... |
| linux-build.log | 17b4963debeaff4c... |
cb1bc7e73f69d362... |
| osx-build.log | de1d4010171bee32... |
30eed6610c634837... |
| win-build.log | a7428da768940c64... |
6a23faa140bab3c4... |
| bitcoin-core-linux-0.20-res.yml.diff | 88743d508c492414... |
|
| bitcoin-core-osx-0.20-res.yml.diff | a6e23042337e3f57... |
|
| bitcoin-core-win-0.20-res.yml.diff | c308f24f99ffc760... |
|
| linux-build.log.diff | 50481d877cfcb295... |
|
| osx-build.log.diff | 1125ea1d898b8949... |
|
| win-build.log.diff | 7e982717b61ae018... |
Concept ACK
Code review ACK d831831822885717e9841f1ff67c19add566fa45 -- patch looks correct
Please note that I have not verified that the desired end result is achieved.
That should be done before merge :)
249 | @@ -250,6 +250,9 @@ void *PosixLockedPageAllocator::AllocateLocked(size_t len, bool *lockingSuccess) 250 | addr = mmap(nullptr, len, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0); 251 | if (addr) { 252 | *lockingSuccess = mlock(addr, len) == 0; 253 | +#ifdef MADV_DONTDUMP 254 | + madvise(addr, len, MADV_DONTDUMP); 255 | +#endif
FreeBSD has the same functionality under a different name MADV_NOCORE:
#if defined(MADV_DONTDUMP) // Linux
madvise(addr, len, MADV_DONTDUMP);
#elif defined(MADV_NOCORE) // FreeBSD
madvise(addr, len, MADV_NOCORE);
#endif
249 | @@ -250,6 +250,9 @@ void *PosixLockedPageAllocator::AllocateLocked(size_t len, bool *lockingSuccess) 250 | addr = mmap(nullptr, len, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0); 251 | if (addr) { 252 | *lockingSuccess = mlock(addr, len) == 0; 253 | +#ifdef MADV_DONTDUMP 254 | + madvise(addr, len, MADV_DONTDUMP);
should the return value be used to signal success/failure to the user?
ACK d831831822885717e9841f1ff67c19add566fa45
Code review and rebased on master/built/tests/ran bitcoind. The concept appears correct according to man core
There are various circumstances in which a core dump file is not produced:
... / ...
In addition, a core dump may exclude part of the address space of the
process if the madvise(2) MADV_DONTDUMP flag was employed.
and implementation correct as per man madvise
MADV_DONTDUMP (since Linux 3.4)
Exclude from a core dump those pages in the range specified by
addr and length. This is useful in applications that have
large areas of memory that are known not to be useful in a
core dump. The effect of MADV_DONTDUMP takes precedence over
the bit mask that is set via the /proc/[pid]/coredump_filter
file (see core(5)).
... / ...
RETURN VALUE
On success, madvise() returns zero.
On error, it returns -1 and errno is set appropriately.
EDIT: tested as described in #15600 (comment) below
I checked the manual page for madvise and I think the usage is correct.
Not checking the return value is okay in this case because if it fails, there's no problem. The function is not marked as "warn_unused_result" so this won't result in compiler warnings either.
/usr/include/x86_64-linux-gnu/sys/mman.h:
extern int madvise (void *__addr, size_t __len, int __advice) __THROW;
ACK d831831822885717e9841f1ff67c19add566fa45
Agree.
/usr/include/x86_64-linux-gnu/sys/mman.h:
#ifdef __USE_MISC
/* Advise the system about particular usage patterns the program follows
for the region starting at ADDR and extending LEN bytes. */
extern int madvise (void *__addr, size_t __len, int __advice) __THROW;
#endif
ACK d831831822885717e9841f1ff67c19add566fa45
But notice support for FreeBSD is easy to hook, so I would suggest to add it.
I tested this on FreeBSD by writing some specific string to the allocated memory and calling abort();. Then ./src/bitcoind crashes immediately. It works as expected: without madvise() the string is present in the core file. With madvise() the string is absent.
I also verified that the effects of the new call need not be explicitly undone when freeing up the memory. For example we undo the mlock() call in FreeLocked() by doing munlock(). I was worried that if we don't do madvise(MADV_DODUMP) when we free the page to the OS, then maybe later, if we allocate it again then MADV_DONTDUMP would be in effect without us asking for it. But this is not the case.
For reference here are the changes I did to test it:
<details> <summary>basic test</summary>
--- i/src/support/lockedpool.cpp
+++ w/src/support/lockedpool.cpp
@@ -250,15 +250,23 @@ void *PosixLockedPageAllocator::AllocateLocked(size_t len, bool *lockingSuccess)
addr = mmap(nullptr, len, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
if (addr == MAP_FAILED) {
return nullptr;
}
if (addr) {
*lockingSuccess = mlock(addr, len) == 0;
-#ifdef MADV_DONTDUMP
+#if 1
+#if defined(MADV_DONTDUMP) // Linux
madvise(addr, len, MADV_DONTDUMP);
+#elif defined(MADV_NOCORE) // FreeBSD
+ madvise(addr, len, MADV_NOCORE);
#endif
+#endif
+ memset(addr, 'A', 16);
+ memset((char*)addr + 16, 'B', 16);
+ memset((char*)addr + 32, 'C', 16);
+ abort();
}
return addr;
}
void PosixLockedPageAllocator::FreeLocked(void* addr, size_t len)
{
len = align_up(len, page_size);
</details>
<details> <summary>test that undo is not necessary</summary>
--- i/src/support/lockedpool.cpp
+++ w/src/support/lockedpool.cpp
@@ -250,15 +250,36 @@ void *PosixLockedPageAllocator::AllocateLocked(size_t len, bool *lockingSuccess)
addr = mmap(nullptr, len, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
if (addr == MAP_FAILED) {
return nullptr;
}
if (addr) {
*lockingSuccess = mlock(addr, len) == 0;
-#ifdef MADV_DONTDUMP
+#if 1
+#if defined(MADV_DONTDUMP) // Linux
madvise(addr, len, MADV_DONTDUMP);
+#elif defined(MADV_NOCORE) // FreeBSD
+ madvise(addr, len, MADV_NOCORE);
#endif
+#endif
+ FreeLocked(addr, len);
+
+ // Allocate again to confirm that the effect of MADV_DONTDUMP/MADV_NOCORE
+ // has vanished. The string must be in the core file.
+ void* new_addr = mmap(addr, len, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
+
+ if (new_addr == MAP_FAILED) {
+ perror("mmap");
+ exit(1);
+ }
+
+ assert(new_addr == addr);
+
+ memset(addr, 'A', 16);
+ memset((char*)addr + 16, 'B', 16);
+ memset((char*)addr + 32, 'C', 16);
+ abort();
}
return addr;
}
void PosixLockedPageAllocator::FreeLocked(void* addr, size_t len)
{
len = align_up(len, page_size);
</details>
@vasild thanks for the interesting tests! Good idea. Here are similar steps I took to reproduce them:
<details> <summary>test that without madvise string is present in core dump</summary> <p>
(pr/15600)$ uname -a
Linux 4.19.0-5-amd64 [#1](/bitcoin-bitcoin/1/) SMP Debian 4.19.37-5+deb10u2 (2019-08-08) x86_64 GNU/Linux
(pr/15600)$ ulimit -c unlimited && git diff && make
--- a/src/support/lockedpool.cpp
+++ b/src/support/lockedpool.cpp
@@ -23,6 +23,7 @@
#include <algorithm>
+#include <cstring>
#ifdef ARENA_DEBUG
@@ -253,9 +254,14 @@ void *PosixLockedPageAllocator::AllocateLocked(size_t len, bool *lockingSuccess)
if (addr) {
*lockingSuccess = mlock(addr, len) == 0;
-#ifdef MADV_DONTDUMP
- madvise(addr, len, MADV_DONTDUMP);
-#endif
+ memset(addr, 'A', 16);
+ memset((char*)addr + 16, 'B', 16);
+ memset((char*)addr + 32, 'C', 16);
+ printf("\n");
+ printf("Process is aborting\n");
+ abort();
+ printf("Control not reaching here\n");
+ return 0;
}
return addr;
}
(pr/15600)$ bitcoind
Process is aborting
Aborted (core dumped)
(pr/15600)$ grep AAAAAAAAAAAAAAAABBBBBBBBBBBBBBBBCCCCCCCCCCCCCCCC core
Binary file core matches
(pr/15600)$
</p> </details>
<details> <summary>test that with madvise string is absent from core dump</summary> <p>
(pr/15600)$ uname -a
Linux 4.19.0-5-amd64 [#1](/bitcoin-bitcoin/1/) SMP Debian 4.19.37-5+deb10u2 (2019-08-08) x86_64 GNU/Linux
(pr/15600)$ ulimit -c unlimited && git diff && make
--- a/src/support/lockedpool.cpp
+++ b/src/support/lockedpool.cpp
@@ -23,6 +23,7 @@
#include <algorithm>
+#include <cstring>
#ifdef ARENA_DEBUG
#include <iomanip>
#include <iostream>
@@ -253,9 +254,21 @@ void *PosixLockedPageAllocator::AllocateLocked(size_t len, bool *lockingSuccess)
if (addr) {
*lockingSuccess = mlock(addr, len) == 0;
-#ifdef MADV_DONTDUMP
+#if defined(MADV_DONTDUMP) // Linux
madvise(addr, len, MADV_DONTDUMP);
+#elif defined(MADV_NOCORE) // FreeBSD
+ madvise(addr, len, MADV_NOCORE);
#endif
+ memset(addr, 'A', 16);
+ memset((char*)addr + 16, 'B', 16);
+ memset((char*)addr + 32, 'C', 16);
+ printf("\n");
+ printf("Process is aborting\n");
+ abort();
+ printf("Control not reaching here\n");
+ return 0;
}
return addr;
}
(pr/15600)$ bitcoind
Process is aborting
Aborted (core dumped)
(pr/15600)$ grep AAAAAAAAAAAAAAAABBBBBBBBBBBBBBBBCCCCCCCCCCCCCCCC core
(pr/15600)$
</p> </details>
I'm going ahead to merge this. Let's add FreeBSD support in a new PR.
Milestone
0.20.0