dtrace: fix fbt regression for aarch64

[ihoro]

I was working with pf and faced the issue with dtrace on main@aarch64. The steps:

• $ kldload pf
• $ dtrace -n 'fbt:pf:hook_pf:entry'
• $ pfctl -e # it's expected to hit the probe once

As a result, dtrace was in a loop. Its output tail after interruption was like:

  4  59199                    hook_pf:entry
  4  59199                    hook_pf:entry
  4  59199                    hook_pf:entry
dtrace: 8893079 drops on CPU 4

I did some first comparisons:

• 13.2-RELEASE: works as expected -- a single probe hit only
• 14-BETA4: loops

I git bisect'ed it down to 980746e as the root cause.
The instr gets an extra increment -- a wrong instruction is targeted for patching.

This is the proposed variant of the fix.
I guess it would be good to fix it for 14.0 as well.

[emaste]

CC @markjdb

[markjdb]

Thanks, I think your analysis is correct.

I'd prefer to fix it by getting rid of found entirely. Instead of setting found = true;, just break;, then replace the if (!found) check with if (instr >= limit). That is simpler and more consistent with some other FBT implementations. The commit log message should also explain the bug that was fixed.

Would you make these changes and re-push? If not I am happy to, just let me know.

[ihoro]

Would you make these changes and re-push? If not I am happy to, just let me know.

Sure. Please, consider another commit pushed. Let me know if a bit different style or way was expected.

[markjdb]

Two more nits: first, the found label should come before the if (instr >= limit) check, not after. In practice it probably doesn't matter, but it's better not to make assumptions about the ELF symbol (i.e., that it's at least as large as a BTI and a NOP instruction). Second, the commit log message doesn't need the reproduction example, the last paragraph is enough ("The loop does an extra increment...").

With those fixed, I'll push the patch. Thanks again.

[ihoro]

In practice it probably doesn't matter, but it's better not to make assumptions about the ELF symbol (i.e., that it's at least as large as a BTI and a NOP instruction).

Agree, good point.

[markjdb]

Merged, thank you.