Add Soft/Hard RSS Limits to Scudo Standalone

[mcgrathr]

This needs a stub GetRSS for Fuchsia, at least.
Since there's no way to get a useful value for this on Fuchsia as yet, it would be preferable to conditionalize the machinery so it isn't doing no-op work.

[bsdaemon]

The feature right now is only supported on LINUX. isRssLimitExceeded() is protected by #if and inlined in any non-linux system and a generic GetRSS symbol is exported in any non-Linux systems (so even with LD_BIND_NOW the symbol exists/resolves). GetRSS() never gets called since it is only called by isRssLimitExceeded() and by a flow conditioned in the isRssLimitExceeded result.

With optimizations on, even the references to the functions and the entire conditional block get removed.

[phosek]

Can you reupload the patch with full context?

compiler-rt/lib/scudo/standalone/linux.cpp

185

The opening brace should be on the previous line.

190

The opening brace should be on the previous line.

195

The opening brace should be on the previous line.

compiler-rt/lib/scudo/standalone/report.cpp

40

The opening brace should be on the previous line.

[bsdaemon]

Fixed the few style errors pointed out (thank you!) and submitting a full context patch.

[bsdaemon]

Fixed the few style errors pointed out (thank you!) and submitting a full context patch.

[bsdaemon]

Fixed compiler warnings due to implicit type conversions, got more pedantic on using the right types (even for compatible ones).

[bsdaemon]

This comment was removed by bsdaemon.

[bsdaemon]

Added LLVM code-style fixes

[mcgrathr]

compiler-rt/lib/scudo/standalone/atomic_helpers.h

127

This should use the call above with true instead of false for the bool argument.

compiler-rt/lib/scudo/standalone/combined.h

353

excess space between parens

728

It's better not to use ALWAYS_INLINE and leave it to the compiler to decide (which it will).
Provide the stub GetRSS function even when you expect calls to it to be optimized away.

732

Use false, not 0.

1059

Put spaces around the =.

[vitalybuka]

compiler-rt/lib/scudo/standalone/atomic_helpers.h

127

please move this into a separate patch and extend compiler-rt/lib/scudo/standalone/tests/atomic_test.cpp

for context, you can organize patches into "stack" using "edit related revision" in the Phabricator menu.

compiler-rt/lib/scudo/standalone/combined.h

353

just clangformat entire patch

357

static_cast

compiler-rt/lib/scudo/standalone/flags.inc

49

limits need own tests as well

compiler-rt/lib/scudo/standalone/linux.cpp

196

How about moving GetRSS() (with a test) implementation into a separate patch?

[bsdaemon]

Bastian Kersting started splitting the patch and making the suggested changes. The first in the series is here: https://reviews.llvm.org/D138406 - I appreciate if folks here help reviewing/merging.

[vitalybuka]

Can you please rebase this one?

compiler-rt/lib/scudo/standalone/combined.h

699

can you measure a meaningful difference or real apps between weak vs strong here?

[1c3t3a]

Rebase on commit chain and latest main.

[1c3t3a]

Add requested changes and tests for Hard and Soft RSS limits

[1c3t3a]

compiler-rt/lib/scudo/standalone/combined.h

699

I benchmarked this method and found not a big difference but a difference between weak and strong here. I benchmarked this on an ARM (aarch64) machine with the following findings:

2022-12-15T09:02:48-08:00
Running third_party/llvm/llvm-project/compiler-rt/scudo_benchmarks
Run on (64 X 3000 MHz CPU s)
CPU Caches:
  L1 Data 64 KiB (x64)
  L1 Instruction 64 KiB (x64)
  L2 Unified 1024 KiB (x64)
  L3 Unified 32768 KiB (x1)
Load Average: 4.02, 9.90, 14.71
-----------------------------------------------------------------
Benchmark                       Time             CPU   Iterations
-----------------------------------------------------------------
BM_IsRssExceeded             47.6 ns         47.5 ns     14772023
BM_IsRssExceededStrong       47.8 ns         47.7 ns     14673854

I get that this is not a big difference, but I would still argue that it is enough to use weak here. We could introduce a compilation parameter with the default weak.

[vitalybuka]

compiler-rt/lib/scudo/standalone/combined.h

167

zero initialized value will work the same

691

Probably nicer to put this method and related fields into another class, e.g

class RssLimitChecker {
 atomic_u64 RssLastCheckedAtNS = {};
 atomic_u8 RssLimitExceeded = {};

  // Slow part in cpp file
  bool Check();

public:
 // Inline fast path.
 bool isExceeded() {
    u64 LastCheck = atomic_load_relaxed(&RssLastCheckedAtNS);
    const u64 CurrentCheck = getMonotonicTime();
     if (UNLIKELY(CurrentCheck >= LastCheck + (250ULL * 1000000ULL)))
       Check();
     return atomic_load_relaxed(&RssLimitExceeded);

 }
 
}

697

instead of adding interval here,
add it when you store it in compare_exchange
then it should be named LastCheck -> NextCheck

CurrentCheck could be named e.g. "Now".

699

Then please just switch to strong

715–722

compiler-rt/lib/scudo/standalone/tests/combined_test.cpp

744 ↗

(On Diff #484194)

This relies on the order of the test execution
everything after this will see scudo::getFlags()->soft_rss_limit_mb = 1

for avoid this the test should backup and restore flags

760 ↗

(On Diff #484194)

this code does not use {} for one line loop bodies

767 ↗

(On Diff #484194)

restore flags

[1c3t3a]

Add requested changes

[vitalybuka]

This looks good to me with nits

compiler-rt/lib/scudo/standalone/combined.h

165

remove {}

352–353

redundant ()

compiler-rt/lib/scudo/standalone/common.h

222

atomic_compare_exchange_strong is in UNLIKELY branch already
sink it into the Check() to avoid unnecessary inlining it

240

Remove redundant ULL as result fits into int32
Remove redundant ()

compiler-rt/lib/scudo/standalone/linux.cpp

230

remove {}

231

maybe nicer to RssLimitExceeded -> enum {Neither, Soft, Hard}
and move reportRSSCheck() same place where you report soft limit

[vitalybuka]

you don't need getOptionsForTesting if you can run init after updating flags

compiler-rt/lib/scudo/standalone/combined.h

165

get from RssChecker

357

please add and use RssChecker.getSoftRssLimit() RssChecker.getHardRssLimit()
So we have not concerns if IsExceeded and this message use the same value.

compiler-rt/lib/scudo/standalone/linux.cpp

217

Return here if they are both zeroes and eliminate CheckRssLimit?

[1c3t3a]

Add requested changes and reorganize tests

[1c3t3a]

I also restructured the tests to get rid of getOptionsForTesting and the flags since this is now read at the creation of the RssChecker.

compiler-rt/lib/scudo/standalone/common.h

222

That would mean Check needs the NextCheck variable for context and that would become a parameter of Check right?

compiler-rt/lib/scudo/standalone/linux.cpp

217

Done. What do you mean by "eliminate CheckRssLimit"?

[1c3t3a]

Remove unused headers

[1c3t3a]

Fix init of limits

[vitalybuka]

update

[vitalybuka]

rebase

compiler-rt/lib/scudo/standalone/atomic_helpers.h

                                    __ATOMIC_RELAXED);
 }
 
+template <typename T>
+inline bool atomic_compare_exchange_weak(volatile T *A, typename T::Type *Cmp,
+                                           typename T::Type Xchg,
+                                           memory_order MO) {
+  return atomic_compare_exchange_strong(A, Cmp, Xchg, MO);
+}
+
+
+
 // Clutter-reducing helpers.
 
 template <typename T>

compiler-rt/lib/scudo/standalone/combined.h

         systemSupportsMemoryTagging())
       Primary.Options.set(OptionBit::UseMemoryTagging);
     Primary.Options.set(OptionBit::UseOddEvenTags);
+    if (getFlags()->hard_rss_limit_mb || getFlags()->soft_rss_limit_mb) {
+      Primary.Options.set(OptionBit::CheckRssLimit);
+      atomic_store_relaxed(&RssLastCheckedAtNS, getMonotonicTime());
+    }
 
     QuarantineMaxChunkSize =
         static_cast<u32>(getFlags()->quarantine_max_chunk_size);

       reportAllocationSizeTooBig(Size, NeededSize, MaxAllowedMallocSize);
     }
     DCHECK_LE(Size, NeededSize);
+    
+
+    if ( (Options.get(OptionBit::CheckRssLimit)) && UNLIKELY(isRssLimitExceeded())) {
+      if (Options.get(OptionBit::MayReturnNull))
+        return nullptr;
+            const uptr CurrentRssMb = GetRSS() >> 20;
+            uptr SoftRssLimitMb = getFlags()->soft_rss_limit_mb;
+            reportRSSCheck("Soft RSS limit exhausted (Limit x Current): ",
+                   SoftRssLimitMb, CurrentRssMb);
+    }
 
     void *Block = nullptr;
     uptr ClassId = 0;

     return NewPtr;
   }
 
+// Opportunistic RSS limit check. This will update the RSS limit status, if
+// it can, every 250ms, otherwise it will just return the current one.
+  NOINLINE bool isRssLimitExceeded() {
+    u64 LastCheck = atomic_load_relaxed(&RssLastCheckedAtNS);
+    const u64 CurrentCheck = getMonotonicTime();
+    uptr HardRssLimitMb = getFlags()->hard_rss_limit_mb;
+    uptr SoftRssLimitMb = getFlags()->soft_rss_limit_mb;
+
+    if (LIKELY(CurrentCheck < LastCheck + (250ULL * 1000000ULL)))
+      return atomic_load_relaxed(&RssLimitExceeded);
+
+    if (!atomic_compare_exchange_weak(&RssLastCheckedAtNS, &LastCheck,
+                                    CurrentCheck, memory_order_relaxed))
+      return atomic_load_relaxed(&RssLimitExceeded);
+
+    // TODO:          We currently use sanitizer_common's GetRSS which reads the
+    //                RSS from /proc/self/statm by default. We might want to
+    //                call getrusage directly, even if it's less accurate.
+    const uptr CurrentRssMb = GetRSS() >> 20;
+
+    if (HardRssLimitMb && UNLIKELY(HardRssLimitMb < CurrentRssMb)) {
+      reportRSSCheck("Hard RSS limit exhausted (Limit x Current): ",
+                   HardRssLimitMb, CurrentRssMb);
+ 
+    }
+
+    if (SoftRssLimitMb) {
+      if (atomic_load_relaxed(&RssLimitExceeded)) {
+        if (CurrentRssMb <= SoftRssLimitMb)
+          atomic_store_relaxed(&RssLimitExceeded, false);
+      } else {
+        if (CurrentRssMb > SoftRssLimitMb) {
+          atomic_store_relaxed(&RssLimitExceeded, true);
+        }
+      }
+    }
+
+    return atomic_load_relaxed(&RssLimitExceeded);
+
+  }
+  
+
   // TODO(kostyak): disable() is currently best-effort. There are some small
   //                windows of time when an allocation could still succeed after
   //                this function finishes. We will revisit that later.

   QuarantineT Quarantine;
   TSDRegistryT TSDRegistry;
   pthread_once_t PostInitNonce = PTHREAD_ONCE_INIT;
+  atomic_u64 RssLastCheckedAtNS={};
+  atomic_u8 RssLimitExceeded={};
 
 #ifdef GWP_ASAN_HOOKS
   gwp_asan::GuardedPoolAllocator GuardedAlloc;

compiler-rt/lib/scudo/standalone/common.h

 
 const char *getEnv(const char *Name);
 
+uptr GetRSS();
+
 u64 getMonotonicTime();
 
 u32 getThreadID();

compiler-rt/lib/scudo/standalone/flags.cpp

 //
 //===----------------------------------------------------------------------===//
 
+#include "internal_defs.h"
 #include "flags.h"
 #include "common.h"
 #include "flags_parser.h"

compiler-rt/lib/scudo/standalone/flags.inc

 SCUDO_FLAG(int, release_to_os_interval_ms, SCUDO_ANDROID ? INT32_MIN : 5000,
            "Interval (in milliseconds) at which to attempt release of unused "
            "memory to the OS. Negative values disable the feature.")
+
+SCUDO_FLAG(int, hard_rss_limit_mb, 0,
+     "Hard RSS Limit in Mb"
+     "If non-zero, once the limit is achieved, abort the process")
+
+SCUDO_FLAG(int, soft_rss_limit_mb, 0,
+     "Soft RSS Limit in Mb"
+     "If non-zero, once the limit is reached, all subsequent calls will fail or return NULL"
+     "until the RSS goes below the soft limit")
+
+

compiler-rt/lib/scudo/standalone/internal_defs.h

 typedef int16_t s16;
 typedef int32_t s32;
 typedef int64_t s64;
+typedef int fd_t;
+
+enum FileAccessMode {
+  RdOnly,
+  WrOnly,
+  RdWr
+};
+
+#define kInvalidFd ((fd_t) -1)
 
 // The following two functions have platform specific implementations.
 void outputRaw(const char *Buffer);

 
 #define RAW_CHECK(Expr) RAW_CHECK_MSG(Expr, #Expr)
 
+void NORETURN reportRSSCheck(const char *Message, uptr RssLimit, uptr RssCurrent);
+
 void NORETURN reportCheckFailed(const char *File, int Line,
                                 const char *Condition, u64 Value1, u64 Value2);
 #define CHECK_IMPL(C1, Op, C2)                                                 \

compiler-rt/lib/scudo/standalone/linux.cpp

 #include <sys/time.h>
 #include <time.h>
 #include <unistd.h>
+#include <stdio.h>
 
 #if SCUDO_ANDROID
 #include <sys/prctl.h>

 #endif // defined(SYS_getrandom)
   // Up to 256 bytes, a read off /dev/urandom will not be interrupted.
   // Blocking is moot here, O_NONBLOCK has no effect when opening /dev/urandom.
-  const int FileDesc = open("/dev/urandom", O_RDONLY);
+  const int FileDesc = open("/dev/urandom", RdOnly);
   if (FileDesc == -1)
     return false;
   ReadBytes = read(FileDesc, Buffer, Length);

 extern "C" WEAK int async_safe_write_log(int pri, const char *tag,
                                          const char *msg);
 
+uptr internal_read(fd_t fd, char *buf, size_t size)
+{
+  return read(fd, buf, size);
+}
+
+void internal_close(fd_t fd)
+{
+  close(fd);
+}
+
+fd_t OpenFile(const char *FileName, mode_t mode)
+{
+  return open(FileName, mode);
+}
+
+uptr GetRSS() {
+  fd_t fd = OpenFile("/proc/self/statm", RdOnly);
+  char buf[64];
+  uptr len = internal_read(fd, buf, sizeof(buf) - 1);
+  internal_close(fd);
+  if ((sptr)len <= 0)
+    return 0;
+  buf[len] = 0;
+
+  // The format of the file is:
+  // 1084 89 69 11 0 79 0
+  // We need the second number which is RSS in pages.
+  char *pos = buf;
+  // Skip the first number.
+  while (*pos >= '0' && *pos <= '9')
+    pos++;
+  // Skip whitespaces.
+  while (!(*pos >= '0' && *pos <= '9') && *pos != 0)
+    pos++;
+  // Read the number.
+  uptr rss = 0;
+  while (*pos >= '0' && *pos <= '9')
+    rss = rss * 10 + *pos++ - '0';
+  return rss * getPageSizeCached();
+}
+
+
 void outputRaw(const char *Buffer) {
   if (&async_safe_write_log) {
     constexpr s32 AndroidLogInfo = 4;

compiler-rt/lib/scudo/standalone/options.h

   UseOddEvenTags,
   UseMemoryTagging,
   AddLargeAllocationSlack,
+  CheckRssLimit,
 };
 
 struct Options {

compiler-rt/lib/scudo/standalone/report.cpp

 
 inline void NORETURN trap() { __builtin_trap(); }
 
+void NORETURN reportRSSCheck(const char *Message, uptr RssLimit, uptr RssCurrent)
+{
+  ScopedErrorReport Report;
+  Report.append("%s %d x %d\n", Message, RssLimit, RssCurrent);
+}
+
 // This could potentially be called recursively if a CHECK fails in the reports.
 void NORETURN reportCheckFailed(const char *File, int Line,
                                 const char *Condition, u64 Value1, u64 Value2) {