Bug 191260 - [patch] dtrace fbt entry function gets the wrong values from arg5 to arg9 on amd64 platform
Summary: [patch] dtrace fbt entry function gets the wrong values from arg5 to arg9 on ...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Mark Johnston
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-22 03:17 UTC by luke.tw
Modified: 2014-08-05 01:57 UTC (History)
1 user (show)

See Also:


Attachments
patch for dtrace_getarg() (1.71 KB, patch)
2014-06-22 03:17 UTC, luke.tw
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description luke.tw 2014-06-22 03:17:30 UTC
Created attachment 144012 [details]
patch for dtrace_getarg()

There are two problems in the dtrace_getarg() implementation on amd64 platform.
In sys/cddl/dev/dtrace/amd64/dtrace_isa.c, 
    
1. dtrace_getarg() cannot find the dtrace_invop stack frame
    The return address of function dtrace_invop() may be different than dtrace_invop_callsite, because the later is aligned on 16-byte boundary on amd64 platform. As shown in the following disassembly code, there is 14 bytes nop between them. 
 
00000000000249f0 <dtrace_invop_start>:
   249f0:   48 8b bc 24 98 00 00    mov    0x98(%rsp),%rdi
   249f7:   00
   249f8:   48 ff cf                dec    %rdi
   249fb:   48 8b b4 24 b0 00 00    mov    0xb0(%rsp),%rsi
   24a02:   00
   24a03:   48 8b 54 24 30          mov    0x30(%rsp),%rdx
   24a08:   ff 36                   pushq  (%rsi)
   24a0a:   48 89 e6                mov    %rsp,%rsi
   24a0d:   e8 00 00 00 00          callq  24a12 <dtrace_invop_start+0x22>
   24a12:   66 66 66 66 66 2e 0f    nopw   %cs:0x0(%rax,%rax,1)
   24a19:   1f 84 00 00 00 00 00

0000000000024a20 <dtrace_invop_callsite>:
   24a20:   48 83 c4 08             add    $0x8,%rsp
   24a24:   83 f8 01                cmp    $0x1,%eax

2. struct trapframe should be used to match the struct regs used in illumos.

* experiment:
   I write a simple kernel module with a function traceme to print its ten arguments:

void traceme(long arg0, long arg1, long arg2, long arg3, long arg4, 
                    long arg5, long arg6, long arg7, long arg8, long arg9) {
        printf("test:%ld %ld %ld %ld %ld %ld %ld %ld %ld %ld\n",
                        arg0, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8, arg9);
}

   And the calling the function like this:

traceme(0,1,2,3,4,5,6,7,8,9);
     
 
* before patch
 # dtrace -n 'fbt:example:traceme:entry {printf("%d %d %d %d %d %d %d %d %d %d\n", arg0, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8, arg9);}'
dtrace: description 'fbt:example:traceme:entry ' matched 1 probe
CPU     ID                    FUNCTION:NAME
  0  48648                    traceme:entry 0 1 2 3 4 -2118041099 0 1 2 3

* after patch
 # dtrace -n 'fbt:example:traceme:entry {printf("%d %d %d %d %d %d %d %d %d %d\n", arg0, arg1, arg2, arg3, arg4, arg5, arg6, arg7, arg8, arg9);}'
dtrace: description 'fbt:example:traceme:entry ' matched 1 probe
CPU     ID                    FUNCTION:NAME
  1  48648                    traceme:entry 0 1 2 3 4 5 6 7 8 9
Comment 1 commit-hook freebsd_committer freebsd_triage 2014-06-23 01:11:39 UTC
A commit references this bug:

Author: markj
Date: Mon Jun 23 01:10:56 UTC 2014
New revision: 267759
URL: http://svnweb.freebsd.org/changeset/base/267759

Log:
  Fix a couple of bugs on amd64 when fetching probe arguments beyond the
  first five for probes entered through a UD fault (i.e. FBT probes).

  Specifically, handle the fact that dtrace_invop_callsite must be
  16 byte-aligned and thus may not immediately follow the call to
  dtrace_invop() in dtrace_invop_start(). Also fetch register arguments and
  the stack pointer through a struct trapframe instead of a struct reg.

  PR:		191260
  Submitted by:	luke.tw@gmail.com
  MFC after:	3 weeks

Changes:
  head/sys/cddl/dev/dtrace/amd64/dtrace_isa.c
Comment 2 Mark Johnston freebsd_committer freebsd_triage 2014-06-23 01:12:36 UTC
Applied the patch with some slight modifications as r267759. Thanks for the report!
Comment 3 Mark Johnston freebsd_committer freebsd_triage 2014-06-23 02:02:02 UTC
Just a note that i386 had some of the same bugs (plus some others). They've been fixed in r267761.
Comment 4 luke.tw 2014-06-23 02:16:34 UTC
Dear markj, 

Thank you for such a quick response and your effort to make dtrace better in freebsd.
Comment 5 commit-hook freebsd_committer freebsd_triage 2014-08-05 01:54:16 UTC
A commit references this bug:

Author: markj
Date: Tue Aug  5 01:53:15 UTC 2014
New revision: 269556
URL: http://svnweb.freebsd.org/changeset/base/269556

Log:
  MFC r267759, r267761

  r267759:
  Fix a couple of bugs on amd64 when fetching probe arguments beyond the
  first five for probes entered through a UD fault (i.e. FBT probes).

  Specifically, handle the fact that dtrace_invop_callsite must be
  16 byte-aligned and thus may not immediately follow the call to
  dtrace_invop() in dtrace_invop_start(). Also fetch register arguments and
  the stack pointer through a struct trapframe instead of a struct reg.

  r267761:
  Fix some bugs when fetching probe arguments in i386. Firstly ensure that
  the 4 byte-aligned dtrace_invop_callsite can be found and that it
  immediately follows the call to dtrace_invop(). Secondly, fix some pointer
  arithmetic to account for differences between struct i386_frame and illumos'
  struct frame. Finally, ensure that dtrace_getarg() isn't inlined. It works
  by following a fixed number of frame pointers to the probe site, so inlining
  breaks it.

  PR:		191260

Changes:
_U  stable/9/sys/
_U  stable/9/sys/cddl/contrib/opensolaris/
  stable/9/sys/cddl/contrib/opensolaris/uts/common/sys/dtrace_impl.h
  stable/9/sys/cddl/dev/dtrace/amd64/dtrace_isa.c
  stable/9/sys/cddl/dev/dtrace/i386/dtrace_asm.S
  stable/9/sys/cddl/dev/dtrace/i386/dtrace_isa.c
Comment 6 commit-hook freebsd_committer freebsd_triage 2014-08-05 01:54:17 UTC
A commit references this bug:

Author: markj
Date: Tue Aug  5 01:53:16 UTC 2014
New revision: 269557
URL: http://svnweb.freebsd.org/changeset/base/269557

Log:
  MFC r267759, r267761

  r267759:
  Fix a couple of bugs on amd64 when fetching probe arguments beyond the
  first five for probes entered through a UD fault (i.e. FBT probes).

  Specifically, handle the fact that dtrace_invop_callsite must be
  16 byte-aligned and thus may not immediately follow the call to
  dtrace_invop() in dtrace_invop_start(). Also fetch register arguments and
  the stack pointer through a struct trapframe instead of a struct reg.

  r267761:
  Fix some bugs when fetching probe arguments in i386. Firstly ensure that
  the 4 byte-aligned dtrace_invop_callsite can be found and that it
  immediately follows the call to dtrace_invop(). Secondly, fix some pointer
  arithmetic to account for differences between struct i386_frame and illumos'
  struct frame. Finally, ensure that dtrace_getarg() isn't inlined. It works
  by following a fixed number of frame pointers to the probe site, so inlining
  breaks it.

  PR:		191260

Changes:
_U  stable/10/
  stable/10/sys/cddl/contrib/opensolaris/uts/common/sys/dtrace_impl.h
  stable/10/sys/cddl/dev/dtrace/amd64/dtrace_isa.c
  stable/10/sys/cddl/dev/dtrace/i386/dtrace_asm.S
  stable/10/sys/cddl/dev/dtrace/i386/dtrace_isa.c
Comment 7 Mark Johnston freebsd_committer freebsd_triage 2014-08-05 01:57:38 UTC
This has been MFCed to stable/9 and stable/10. Thanks for the PR!