This is an archive of the discontinued LLVM Phabricator instance.

[dfsan] Add full fast8 support
ClosedPublic

Authored by gbalats on Jun 5 2021, 1:14 AM.

Details

Summary

Complete support for fast8:

  • amend shadow size and mapping in runtime
  • remove fast16 mode and -dfsan-fast-16-labels flag
  • remove legacy mode and make fast8 mode the default
  • remove dfsan-fast-8-labels flag
  • remove functions in dfsan interface only applicable to legacy
  • remove legacy-related instrumentation code and tests
  • update documentation.

Diff Detail

Event Timeline

gbalats created this revision.Jun 5 2021, 1:14 AM
gbalats requested review of this revision.Jun 5 2021, 1:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2021, 1:14 AM
Herald added subscribers: llvm-commits, Restricted Project, cfe-commits. · View Herald Transcript

The failed test cases from x64 debian > libFuzzer.libFuzzer::* seem related. They still use -dfsan-fast-16-labels.

clang/docs/DataFlowSanitizer.rst
172–178

If we swap assert(ij_label == 3) with the 3 dfsan_has_label, the two equivalent blocks are close to each other.

clang/docs/DataFlowSanitizerDesign.rst
60

integer

65

the labels, and each OR is in O(1).

68

picking

74

memory layout

99

, and

100

the label of a stored value

compiler-rt/lib/dfsan/dfsan.cpp
209

After removing legacy mode. if our code does not check l1 != l2 in IR, the comments can be updated.

How did we fix that alignment error from compiler-rt/test/dfsan/origin_ldst.c?

gbalats updated this revision to Diff 350366.Jun 7 2021, 11:13 AM
gbalats marked 7 inline comments as done.

Update docs to resolve review comments.

gbalats marked an inline comment as done.Jun 7 2021, 11:13 AM
gbalats added inline comments.
clang/docs/DataFlowSanitizer.rst
172–178

I think this way is better to demonstrate the differences:

  • the first block exposes dfsan internals (the integer representation) and makes explicit statements about label values
  • the second block uses dfsan_has_label to abstract the internals, not exposing the integer representation (or the fact that labels are OR'd).
clang/docs/DataFlowSanitizerDesign.rst
60

Done.

65

Not sure how this changes the meaning, since the two labels would need one OR instruction.

68

Done.

74

Done.

99

Done

100

Done.

compiler-rt/lib/dfsan/dfsan.cpp
209

Done.

gbalats marked an inline comment as done.Jun 7 2021, 11:14 AM
gbalats updated this revision to Diff 350395.Jun 7 2021, 12:54 PM

Update dfsan-related tests under compiler-rt/test/fuzzer/.

This revision is now accepted and ready to land.Jun 7 2021, 2:21 PM
gbalats updated this revision to Diff 350432.Jun 7 2021, 3:22 PM

Update interceptors.c test after allocator patch.

browneee accepted this revision.Jun 7 2021, 4:14 PM
browneee added inline comments.
llvm/test/Instrumentation/DataFlowSanitizer/external_mask.ll
6–9

Fix whitespace

llvm/test/Instrumentation/DataFlowSanitizer/load.ll
99

Fix whitespace

gbalats updated this revision to Diff 350445.Jun 7 2021, 4:33 PM
gbalats marked 2 inline comments as done.

Remove whitespace in empty lines.

gbalats added inline comments.Jun 7 2021, 4:36 PM
llvm/test/Instrumentation/DataFlowSanitizer/external_mask.ll
6–9

Acknowledged. Not a tab; just how phabricator indicates whitespace changes.

This revision was automatically updated to reflect the committed changes.
twoh added a subscriber: twoh.Oct 28 2021, 1:59 PM

@gbalats @stephan.yichao.zhao Hello sorry for the late comment but I wonder what was the reason behind this change (changing taint label representation from 16-bit to 8-bit-fast only). Do we have any discussion thread from llvm-dev regarding this? Thanks!

browneee added a subscriber: kcc.Oct 28 2021, 2:51 PM

@gbalats @stephan.yichao.zhao Hello sorry for the late comment but I wonder what was the reason behind this change (changing taint label representation from 16-bit to 8-bit-fast only). Do we have any discussion thread from llvm-dev regarding this? Thanks!

Hi twoh,

The reasons are:

  • simplify code (smaller codesize overhead)
  • share more code and shadow/origin memory layout with MSan (which also has a 1:1 shadow)
  • reduce memory overhead

The only disadvantage is you need 2x executions to cover the same number of taint labels, but we already need do multiple executions.

There was no discussion thread, but we discussed with @kcc internally.

twoh added a comment.Oct 28 2021, 2:57 PM

@browneee Thanks for the reply! IIUC, with non-fast mode and 16-bit shadow data, it could support 2^16 labels with a single run, so the coverage reduction is 2^16 -> 8. Do I miss something?

Sorry, I was thinking/describing fast-16 to fast-8.

The same reasons apply for legacy mode (2^16 labels) to fast-8.

  • simplify code (smaller codesize overhead)
  • share more code and shadow/origin memory layout with MSan (which also has a 1:1 shadow)
  • reduce memory overhead

In this case it also reduces runtime overhead.

Our experience was legacy mode did not scale to large programs. The choice we saw was do multiple runs with fast-8, or be unable to run with legacy.
Using fast-8 we can successfully run dfsan with large programs.

Having said this, we also fixed many other issues (e.g. memory leaks) to make fast-8 scale. So our experience comparing legacy and fast-8 mode is not quite fair.

twoh added a comment.Oct 28 2021, 3:37 PM

@browneee Thanks! Makes sense. Is there a chance that you have an idea of the ballpark overhead of the legacy mode? I'm curious if we track N-labels, the total CPU time is generally much higher with "legacy-mode single-run with N labels" than "fast-8 mode (N/8) runs". I think there are still reasons to prefer multiple runs of fast-8 mode over the legacy mode even when the total CPU times are similar, but I wonder how bad is the overhead of expensive union operations performed by the legacy mode.

llvm/test/Instrumentation/DataFlowSanitizer/struct.ll