test, tracing: don't use problematic bpf_usdt_readarg_p()

[0xB10C]

Instead of using the undocumented bcc helper bpf_usdt_readarg_p(), use bpf_usdt_readarg() and bpf_probe_read_user()/bpf_probe_read_user_str() as documented in the bcc USDT reference guide.

Note that the bpf_probe_read_user() documentation says the following:

For safety, all user address space memory reads must pass through bpf_probe_read_user().

It's assumed that using bpf_usdt_readarg_p() caused a lifetime issue. With bpf_usdt_readarg() and bpf_probe_read_user(), this doesn't seem to be a problem anymore.

This allows to revert faed533 and closes #27380.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31848.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	i-am-yuvi, willcl-ark

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

[0xB10C]

I did >15 re-runs of master...0xB10C:bitcoin:2024-12-dont-use-bpf_usdt_readarg_p-testing similar to the 20 runs in #27380 (comment) to make sure this won't intermittently fail again.

[willcl-ark]

Approach ACK

So if I understand the change correctly, the new (documented/recommended) behaviour is to always first read_arg() which gets the memory location of the data, then read_user[_str]() which then actually performs the copy from user memory to the BPF memory space.

If I have this correct, then using the documented, seemingly more-reliable, and more explicit approach seems fine to me.

Not sure how I could test the reliability of this myself, but #31848 (comment) looks promising.

[0xB10C]

So if I understand the change correctly, the new (documented/recommended) behaviour is to always first read_arg() which gets the memory location of the data, then read_user{_str}() which then actually performs the copy from user memory to the BPF memory space.

Correct. Here's an example from the mempool:added tracepoint test.

• (1) initialize a null-pointer phash
• (2) read the first tracepoint argument (a user-space address) into the phash pointer
• (3) copy the value behind the phash pointer into added.hash

  void *phash = NULL;                                              // (1)
  bpf_usdt_readarg(1, ctx, &phash);                                // (2)
  bpf_probe_read_user(&added.hash, sizeof(added.hash), phash);     // (3)

bpf_usdt_readarg_p() is a helper that does this, but apparently not safely enough

[laanwj]

bpf_usdt_readarg_p() is a helper that does this, but apparently not safely enough

i looked into it a bit; the implementation of bpf_usdt_readarg_p is here:
https://github.com/iovisor/bcc/blob/master/src/cc/frontends/clang/b_frontend_action.cc#L1270

It's a macro implemented in the clang frontend:

        } else if (Decl->getName() == "bpf_usdt_readarg_p") {
          text = "({ u64 __addr = 0x0; ";
          text += "_bpf_readarg_" + current_fn_ + "_" + args[0] + "(" +
                  args[1] + ", &__addr, sizeof(__addr));";

          bool overlap_addr = false;
          text += check_bpf_probe_read_user(StringRef("bpf_probe_read_user"),
                  overlap_addr);
          if (overlap_addr) {
            error(GET_BEGINLOC(Call), "bpf_probe_read_user not found. Use latest kernel");
            return false;
          }

          text += "(" + args[2] + ", " + args[3] + ", (void *)__addr);";
          text += "})";
          rewriter_.ReplaceText(expansionRange(Call->getSourceRange()), text);
        }

At first glance it generates the exact same code: a bpf_readarg folllowed by bpf_probe_read_user of that address. So it's unclear why the explicit formulation would change anything.

However, check_bpf_probe_read_user does not always return bpf_probe_read_user! If the symbol cannot be resolved for the specific kernel it returns bpf_probe_read instead.
Maybe that's it, and calling check_bpf_probe_read_user works around it? Not sure.

On the other hand there's also some #defining going on which does the same (but not sure it's under the same conditions).

[laanwj]

Using bpf_probe_read_user_str here at least makes sure that only until the zero-terminator is read and not the entire buffer. That's a clear improvement.

[i-am-yuvi]

Tested ACK a0b66b4

• I've tested all the functional tests and it passes (5/5)

  • interface_usdt_coinselection.py ✅
  • interface_usdt_mempool.py ✅
  • interface_usdt_net.py ✅
  • interface_usdt_utxo.py ✅
  • interface_usdt_validation.py ✅

• All tracing tools works fine (3/3)

• Did 5-6 runs of CI similar to this one, which can be seen here all of them succeeded.

• The issue comment looks promising to me!

Note: All of the tests has been executed in a fully synced node

[willcl-ark]

tACK a0b66b4

I also re-ran CI on this PR 10 times, of which all passed.

The code changes all look correct to me too, following the principle of the pointers being null initialized, having addressed assigned to them, and then the value being copied.

Hopefully this can finally end the intermittent failures 🤞🏼

[glozow]

@willcl-ark you acked your commit on top of the PR. It is effectively an ack, but do you mind editing your comment?

[willcl-ark]

Thanks Gloria, nice catch :)

Comment amended.

[laanwj]

Post-merge code review ACK a0b66b4