Trace external pointers through maps #1285

pchaigno · 2017-08-06T06:09:31Z

The bcc rewriter currently traces external pointers using
ProbeVisitor in order to replace dereferences with calls to
bpf_probe_read. It is, however, unable to trace them through maps.

This pull request fixes this for simple (yet common) cases. Through a
first traversal of the Clang AST, MapVisitor looks for calls to
update (and insert) to find maps with an external pointer as value.
When ProbeVisitor runs, in a second time, it looks for calls to
lookup (and lookup_or_init). If the map was registered as having an
external pointer as value, the l-value of the lookup assignment is
marked as being an external pointer.

Two traversals of the Clang AST are needed because the update of a
map may happen after the lookup in the AST. Therefore, the first
traversal makes sure we inspect all updates before acting on lookups.
To implement this two-stage traversal without parsing the AST twice,
ProbeConsumer and BTypeConsumer now implement
HandleTranslationUnit, which is called after a whole translation
unit has been parsed.

This change should probably be considered a breaking API change. In
bcc, it breaks a few tools that were relying on the rewriter not
being able to replace dereferences. For instance, the
bpf_probe_read(&saddr, sizeof(saddr), &skp->__sk_common.skc_rcv_saddr)
expression in examples/tracing/tcpv4connect.py now returns an error.

Fixes #253.

/cc @drzaeus77

brendangregg · 2017-08-23T20:50:01Z

Looks like a good improvement.

yonghong-song

Thanks for fixing this. Definitely positive for user programs. A few comments are inlined with the codes.

yonghong-song · 2017-08-29T20:18:11Z

src/cc/frontends/clang/b_frontend_action.cc

+            }
+          }
+        }
+      }


I do not fully understand why we need to track map.update or map.insert here?
It is totally possible that map.update and map.insert not in the same compilation unit, e.g., populated by user space or shared with another bpf program. Then we will not able to trace map value pointers below?

Where would you track map.update and map.insert if not here?

Yes, if the updates/inserts are not in the same compilation unit, we can't trace map value pointers. I don't think there's much we can do in that case, at least without hints from developers.

yonghong-song · 2017-08-29T20:18:43Z

src/cc/frontends/clang/b_frontend_action.cc

+            return true;
+
+          if (memb_name == "lookup" || memb_name == "lookup_or_init") {
+            if (m_.find(Ref->getDecl()) != m_.end()) {


Again not sure whether we need this m_ check or not. What if we do not check?

The m_ check makes sure the map has an external pointer as value. AFAIK, the only way to know that is to track them from the function arguments (what's already implemented) to the map.inserts/updates, remember that, and use it when we meet the map.lookups (what this pull request implements).

yonghong-song · 2017-08-29T20:26:25Z

src/cc/frontends/clang/b_frontend_action.cc

@@ -773,7 +852,8 @@ void BFrontendAction::EndSourceFileAction() {
 unique_ptr<ASTConsumer> BFrontendAction::CreateASTConsumer(CompilerInstance &Compiler, llvm::StringRef InFile) {
  rewriter_->setSourceMgr(Compiler.getSourceManager(), Compiler.getLangOpts());
  vector<unique_ptr<ASTConsumer>> consumers;
-  consumers.push_back(unique_ptr<ASTConsumer>(new ProbeConsumer(Compiler.getASTContext(), *rewriter_)));
+  consumers.push_back(unique_ptr<ASTConsumer>(new MapConsumer(m_)));
+  consumers.push_back(unique_ptr<ASTConsumer>(new ProbeConsumer(Compiler.getASTContext(), *rewriter_, m_)));


If we use handleTranslationUnit to post process ProbeConsumer and BTypeConsumer, I am wondering whether we need ProbeConsumer and BTypeConsumer registered as the BFrontendAction consumer. We can do post process at MapConsumer handleTranslationUnit?

Note that this implementation changes how each construct is processed by ProbeConsumer and BTypeConsumer. The old implementation will call the processing function for each consumer for each construct at a time before moving to the next construct. The new implementation will process all constructs for one consumer before moving to the next consumer.

Look at the ProbeConsumer and BTypeConsumer code, they are largely independent so it should be okay.

If we use handleTranslationUnit to post process ProbeConsumer and BTypeConsumer, I am wondering whether we need ProbeConsumer and BTypeConsumer registered as the BFrontendAction consumer. We can do post process at MapConsumer handleTranslationUnit?

So we'd have a single Consumer for all visitors? Sure 👍

Done in the last commit.

pchaigno · 2017-09-16T18:13:51Z

@yonghong-song Thanks for the review and sorry for the delay to answer.

4ast · 2017-10-26T03:40:45Z

Looks great. Probably needs rebase before merge?

The bcc rewriter currently traces external pointers using ProbeVisitor in order to replace dereferences with calls to bpf_probe_read. It is, however, unable to trace them through maps. This commit remedy this for simple (yet common) cases. Through a first traversal of the Clang AST, MapVisitor looks for calls to update (and insert) to find maps with an external pointer as value. When ProbeVisitor runs, in a second time, it looks for calls to lookup (and lookup_or_init). If the map was registered as having an external pointer as value, the l-value of the lookup assignment is marked as being an external pointer. Two traversals of the Clang AST are needed because the update of a map may happen after the lookup in the AST. Therefore, the first traversal makes sure we inspect all updates before acting on lookups. To implement this two-stage traversal without parsing the AST twice, ProbeConsumer and BTypeConsumer now implement HandleTranslationUnit, which is called after a whole translation unit has been parsed.

pchaigno · 2017-10-26T06:50:19Z

Probably needs rebase before merge?

@4ast Done (with a new commit to address @yonghong-song's comment above).

yonghong-song

LGTM

pchaigno force-pushed the track-external-pointers-maps branch 4 times, most recently from 294a0e9 to 91ccf40 Compare August 6, 2017 14:11

drzaeus77 self-requested a review August 11, 2017 16:15

pchaigno mentioned this pull request Aug 24, 2017

Add support to Map-in-Map in BCC #1318

Closed

yonghong-song reviewed Aug 29, 2017

View reviewed changes

pchaigno force-pushed the track-external-pointers-maps branch from 91ccf40 to 114a5ee Compare September 16, 2017 18:15

pchaigno requested review from 4ast, brendangregg and goldshtn as code owners September 16, 2017 18:15

pchaigno force-pushed the track-external-pointers-maps branch from 114a5ee to 3c75763 Compare October 14, 2017 12:20

pchaigno force-pushed the track-external-pointers-maps branch from 3c75763 to 55cf837 Compare October 26, 2017 06:49

Merge BType, Map, and Probe Consumers

e67cb56

pchaigno force-pushed the track-external-pointers-maps branch from 55cf837 to e67cb56 Compare October 26, 2017 09:47

yonghong-song approved these changes Oct 27, 2017

View reviewed changes

yonghong-song merged commit 6e45749 into iovisor:master Oct 27, 2017

pchaigno deleted the track-external-pointers-maps branch October 27, 2017 16:35

pchaigno mentioned this pull request May 7, 2018

Trace all external pointers passed through a first map #1737

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trace external pointers through maps #1285

Trace external pointers through maps #1285

pchaigno commented Aug 6, 2017 •

edited

Loading

brendangregg commented Aug 23, 2017

yonghong-song left a comment

yonghong-song Aug 29, 2017

pchaigno Sep 16, 2017

yonghong-song Aug 29, 2017

pchaigno Sep 16, 2017

yonghong-song Aug 29, 2017

pchaigno Sep 16, 2017

pchaigno Oct 26, 2017

pchaigno commented Sep 16, 2017

4ast commented Oct 26, 2017

pchaigno commented Oct 26, 2017 •

edited

Loading

yonghong-song left a comment

Trace external pointers through maps #1285

Trace external pointers through maps #1285

Conversation

pchaigno commented Aug 6, 2017 • edited Loading

brendangregg commented Aug 23, 2017

yonghong-song left a comment

Choose a reason for hiding this comment

yonghong-song Aug 29, 2017

Choose a reason for hiding this comment

pchaigno Sep 16, 2017

Choose a reason for hiding this comment

yonghong-song Aug 29, 2017

Choose a reason for hiding this comment

pchaigno Sep 16, 2017

Choose a reason for hiding this comment

yonghong-song Aug 29, 2017

Choose a reason for hiding this comment

pchaigno Sep 16, 2017

Choose a reason for hiding this comment

pchaigno Oct 26, 2017

Choose a reason for hiding this comment

pchaigno commented Sep 16, 2017

4ast commented Oct 26, 2017

pchaigno commented Oct 26, 2017 • edited Loading

yonghong-song left a comment

Choose a reason for hiding this comment

pchaigno commented Aug 6, 2017 •

edited

Loading

pchaigno commented Oct 26, 2017 •

edited

Loading