Skip to content

[LLVM][rtsan] Add nosanitize_realtime instrumentation #106125

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

[LLVM][rtsan] Add nosanitize_realtime instrumentation #106125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions llvm/lib/Transforms/Instrumentation/RealtimeSanitizer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@

#include "llvm/Transforms/Instrumentation/RealtimeSanitizer.h"

#include <cassert>

using namespace llvm;

static void insertCallBeforeInstruction(Function &Fn, Instruction &Instruction,
Expand Down Expand Up @@ -51,6 +53,7 @@ RealtimeSanitizerPass::RealtimeSanitizerPass(
PreservedAnalyses RealtimeSanitizerPass::run(Function &F,
AnalysisManager<Function> &AM) {
if (F.hasFnAttribute(Attribute::SanitizeRealtime)) {
assert(!F.hasFnAttribute(Attribute::NoSanitizeRealtime));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need NoSanitizeRealtime, when Address and Memory sanitizers can exist without them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main reason this is necessary is we need to have a way to turn off the rtsan stack for all function calls below the function with the nosanitize attribute.

If you mark a function as needing the real-time sanitizer, all functions invoked by that function are subject to it's constraints (no allocations, no locks, no system calls). This is enforced via a stack, which is popped or pushed via __rtsan_realtime_enter and __rtsan_realtime_exit, inserted in this file.

When you mark a function as not needing to be inspected by the realtime sanitizer, it must change the stack such so these constraints are no longer in place. The nosanitize function AS WELL AS functions invoked by it can allocate, take locks, or call system calls.

Address and Memory sanitizer don't have this hierarchical model.

To make the example concrete...
Realtime sanitizer

int ChildWithViolation() {
  int* v = new int[100]; // SHOULD BE VIOLATION, can't alloc in nonblocking context
}

__attribute__((no_sanitize("realtime")))
int Parent() {
  return ChildWithViolation(); // does not throw an error, because we are within a no_sanitize("realtime") context
}

int main() [[clang::nonblocking]]{
  Parent();
}

Marking parent with no_sanitize("realtime") disables the realtime sanitizer for this function and all functions this function calls.

Address sanitizer, does not work like this, if you try to do something similar:

int ChildWithViolation() {
  int* v = new int[100];
  return v[100];
}

__attribute__((no_sanitize("address")))
int Parent() {
  return ChildWithViolation();
}

int main() {
  Parent();

 	return 0;
}
> ./build/helloWorld
=================================================================
==24510==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x000107915bd0 at pc 0x000104ea3e14 bp 0x00016af5e950 sp 0x00016af5e948
READ of size 4 at 0x000107915bd0 thread T0
    #0 0x104ea3e10 in ChildWithViolation() main.cpp:17
    #1 0x104ea3e34 in Parent() main.cpp:22
    #2 0x104ea3e58 in main main.cpp:26
    #3 0x1958960dc  (<unknown module>)
...

The nosanitize attribute only turns it off for that particular function, not "down the stack"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess another way to say the same thing as the previous comment:

ASan instruments and inspects every function in a binary. Asan disables on a per function basis.

RTSan instruments and inspects only in a specific context. RTSan disables on a per context basis.

This is the root of the difference and why we have to disable in this way, we are keeping state that tracks if we are in the context our sanitizer cares about.

Copy link
Collaborator

@vitalybuka vitalybuka Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why it needs to be attribute?

I think it's unusual, not sure if there is example at all, when fn attribute affects callees.

__lsan::ScopedDisabler achieves the same without this confusion using language and even can be more precise, by limiting itself to a code block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out, I wasn't aware it existed!

Pros of ScopedDisabler:

  • Enables finer-grained disabling within a function.
  • Avoids modifying LLVM attributes or Clang, simplifying the implementation.
  • Precedent exists with lsan.

Cons of ScopedDisabler:

  • Doesn't follow existing patterns for disabling interceptors, unlike asan and others.
  • Users need to recreate build system infrastructure for conditional compilation with -fsanitize=realtime.

The biggest concern is that second "con" -- users would need to manually include out-of-project headers and manage build conditions, leading to some potential issues:

  1. Duplication of effort across users.
  2. Difficulty in locating the required header files. (and conditionally including them if the sanitizer is enabled, leading to more complex build systems)
  3. Risk of users re-implementing ScopedDisabler in their codebases.

The nosanitize attribute is comparatively simpler, which "just works" without extra steps. I would prefer whatever solution we go forward with, it uses a construct that would be as non-invasive as the nosanitize approach to avoid burdening users with additional setup. Are there other solutions that provide this level of ease to the end user?

For finer-grained interception, we've advised extracting code into new functions and applying the nosanitize attribute. This approach is a little more invasive but clearly indicates that the code is treated differently.

Copy link
Collaborator

@vitalybuka vitalybuka Aug 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out, I wasn't aware it existed!

Pros of ScopedDisabler:

  • Enables finer-grained disabling within a function.
  • Avoids modifying LLVM attributes or Clang, simplifying the implementation.
  • Precedent exists with lsan.

Cons of ScopedDisabler:

  • Doesn't follow existing patterns for disabling interceptors, unlike asan and others.
  • Users need to recreate build system infrastructure for conditional compilation with -fsanitize=realtime.

The biggest concern is that second "con" -- users would need to manually include out-of-project headers and manage build conditions, leading to some potential issues:

conditions should not be a problem if you check __feature in rtsan header.
So it could be included unconditionally, and just compile into no-op without RTSan
we should do the same for all sanitizers, we have some inconsistency there.

  1. Duplication of effort across users.

Code modification is needed. It should be either attribute or C++ object either way, so I don't see much difference in duplication.

  1. Difficulty in locating the required header files. (and conditionally including them if the sanitizer is enabled, leading to more complex build systems)

Should be unconditional. Header installed with clang.
However with g++ it may need a condition.

But I don't think it's a big issue. We used to preprocessor in C++.

  1. Risk of users re-implementing ScopedDisabler in their codebases.

Why it's a problem? They can do so even you go with existing attribute. I am 100% sure that ScopedDisabler like will be needed, as function scope may affect to much.

Also inliner and other passes can be affected by attribute. Regular C++ code will be more predictable for end user.

The nosanitize attribute is comparatively simpler, which "just works" without extra steps. I would prefer whatever solution we go forward with, it uses a construct that would be as non-invasive as the nosanitize approach to avoid burdening users with additional setup. Are there other solutions that provide this level of ease to the end user?

lsan has larger user base then rtsan, at least for now, and it was never an issue. Attribute approach can work for lsan as well.

For finer-grained interception, we've advised extracting code into new functions and applying the nosanitize attribute. This approach is a little more invasive but clearly indicates that the code is treated differently.

I am not aware of context for this, can you point me there? How attribute is better here then ScopedDisabler?

So unless we have strong reason, I would prefer we solve the same problem in all component consistently.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

conditions should not be a problem if you check __feature in rtsan head.
So it could be included unconditionally, and just compile into no-op without RTSan
we should do the same for all sanitizers, we have some inconsistency there.

Could you elaborate on this, perhaps with a small code example? I think if this were possible my concern would be alleviated!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(actually, I think I have it figured out, going to test this for a little bit and get back to you. Thanks for the guidance)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works great for us @vitalybuka ! Thanks for explaining this to me. You will see some follow up reviews today that accomplish this goal.

Closing this review.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That review is posted here: #106736

insertCallAtFunctionEntryPoint(F, "__rtsan_realtime_enter");
insertCallAtAllFunctionExitPoints(F, "__rtsan_realtime_exit");

Expand All @@ -59,5 +62,15 @@ PreservedAnalyses RealtimeSanitizerPass::run(Function &F,
return PA;
}

if (F.hasFnAttribute(Attribute::NoSanitizeRealtime)) {
assert(!F.hasFnAttribute(Attribute::SanitizeRealtime));
insertCallAtFunctionEntryPoint(F, "__rtsan_off");
insertCallAtAllFunctionExitPoints(F, "__rtsan_on");

PreservedAnalyses PA;
PA.preserveSet<CFGAnalyses>();
return PA;
}

return PreservedAnalyses::all();
}
25 changes: 25 additions & 0 deletions llvm/test/Instrumentation/RealtimeSanitizer/rtsan_nosanitize.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
; RUN: opt < %s -passes=rtsan -S | FileCheck %s

define void @nosanitized_function() #0 {
%1 = alloca ptr, align 8
%2 = call ptr @malloc(i64 noundef 2) #3
store ptr %2, ptr %1, align 8
ret void
}

declare ptr @malloc(i64 noundef) #1

define noundef i32 @main() #2 {
%1 = alloca i32, align 4
store i32 0, ptr %1, align 4
call void @nosanitized_function() #4
ret i32 0
}

attributes #0 = { nosanitize_realtime }

; CHECK-LABEL: @nosanitized_function()
; CHECK-NEXT: call{{.*}}@__rtsan_off

; CHECK: call{{.*}}@__rtsan_on
; CHECK-NEXT: ret{{.*}}void
Loading