This is the mail archive of the systemtap@sourceware.org mailing list for the systemtap project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[Bug tapsets/18597] New: long_arg() doesn't correctly handle negative values in 32-on-64 environment


https://sourceware.org/bugzilla/show_bug.cgi?id=18597

            Bug ID: 18597
           Summary: long_arg() doesn't correctly handle negative values in
                    32-on-64 environment
           Product: systemtap
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: tapsets
          Assignee: systemtap at sourceware dot org
          Reporter: mcermak at redhat dot com
  Target Milestone: ---

Currently (15f1b3dd85f3) long_arg() doesn't correctly handle negative values in
compat tasks:

=======
 7.1 S x86_64 # gcc -g systemtap.syscall/aio.c -m32
 7.1 S x86_64 # stap -g -e 'probe
kernel.function("compat_sys_io_submit"){printf("%d %d %d\n",
@__compat_long($nr), __int32($nr), %{ /* pure */ _stp_is_compat_task() %})}' -c
./a.out
1
1 1 1
1 1 1
-1 -1 1
1 1 1
 7.1 S x86_64 # stap -g -e 'probe
kprobe.function("compat_sys_io_submit"){printf("%d %d %d\n", long_arg(2),
int_arg(2), %{ /* pure */ _stp_is_compat_task() %})}' -c ./a.out
1
1 1 1
1 1 1
4294967295 -1 1
1 1 1
 7.1 S x86_64 #
=======

See '-1' versus '4294967295'. This happens on x86_64 and powerpc, but not on
s390x. Currently this issue is being worked around in tapsets, e.g.:

=======
# io_submit __________________________________________________
# long sys_io_submit(aio_context_t ctx_id, long nr, struct iocb __user * __user
*iocbpp)
# long compat_sys_io_submit(aio_context_t ctx_id, int nr, u32 __user *iocb)
#
probe nd_syscall.io_submit = __nd_syscall.io_submit,
        __nd_syscall.compat_io_submit ?
{
        name = "io_submit"
        asmlinkage()
        ctx_id = ulong_arg(1)
        iocbpp_uaddr = pointer_arg(3)
        argstr = sprintf("%u, %d, %p", ctx_id, nr, iocbpp_uaddr)
}
probe __nd_syscall.io_submit = kprobe.function("sys_io_submit") ?
{
        @__syscall_gate(%{ __NR_io_submit %})
        asmlinkage()
        nr = long_arg(2)
}
probe __nd_syscall.compat_io_submit = kprobe.function("compat_sys_io_submit") ?
{
        asmlinkage()
        nr = int_arg(2)
}
probe nd_syscall.io_submit.return = __nd_syscall.io_submit.return,
        kprobe.function("compat_sys_io_submit").return ?
{
        name = "io_submit"
        retstr = returnstr(1)
}
probe __nd_syscall.io_submit.return = kprobe.function("sys_io_submit").return ?
{
        @__syscall_gate(%{ __NR_io_submit %})
}

=======

Fixing this issue would allow us to do changes like following ...

=======
$ git diff tapset/linux/nd_syscalls.stp                                         
diff --git a/tapset/linux/nd_syscalls.stp b/tapset/linux/nd_syscalls.stp        
index 1a5e486..d41ec34 100644                                                   
--- a/tapset/linux/nd_syscalls.stp                                              
+++ b/tapset/linux/nd_syscalls.stp                                              
@@ -3029,24 +3029,18 @@ probe __nd_syscall.io_setup.return =
kprobe.function("sys_io_setup").return ?
 # long compat_sys_io_submit(aio_context_t ctx_id, int nr, u32 __user *iocb)    
 #                                                                              
 probe nd_syscall.io_submit = __nd_syscall.io_submit,                           
-       __nd_syscall.compat_io_submit ?                                         
+       kprobe.function("compat_sys_io_submit") ?                               
 {                                                                              
        name = "io_submit"                                                      
        asmlinkage()                                                            
        ctx_id = ulong_arg(1)                                                   
+       nr = long_arg(2)                                                        
        iocbpp_uaddr = pointer_arg(3)                                           
        argstr = sprintf("%u, %d, %p", ctx_id, nr, iocbpp_uaddr)                
 }                                                                              
 probe __nd_syscall.io_submit = kprobe.function("sys_io_submit") ?              
 {                                                                              
        @__syscall_gate(%{ __NR_io_submit %})                                   
-       asmlinkage()                                                            
-       nr = long_arg(2)                                                        
-}                                                                              
-probe __nd_syscall.compat_io_submit = kprobe.function("compat_sys_io_submit")
?
-{                                                                              
-       asmlinkage()                                                            
-       nr = int_arg(2)                                                         
 }                                                                              
 probe nd_syscall.io_submit.return = __nd_syscall.io_submit.return,             
        kprobe.function("compat_sys_io_submit").return ?                        
$  
=======

... and thus would allow us to clean up the tapset a bit. 

Following change seems to fix the issue on x86_64:

=======
$ git diff tapset/x86_64/registers.stp | nl
     1  diff --git a/tapset/x86_64/registers.stp b/tapset/x86_64/registers.stp
     2  index d7035e4..e662f9e 100644
     3  --- a/tapset/x86_64/registers.stp
     4  +++ b/tapset/x86_64/registers.stp
     5  @@ -167,7 +167,7 @@ function _stp_arg:long (argnum:long,
sign_extend:long, truncate:long) %{ /* pure
     6          default:
     7                  goto bad_argnum;
     8          }
     9  -       if (STAP_ARG_truncate || argsz == sizeof(int)) {
    10  +       if (STAP_ARG_truncate || _stp_is_compat_task()) {
    11                  if (STAP_ARG_sign_extend)
    12                          STAP_RETVALUE = (int64_t)
__stp_sign_extend32(val);
    13                  else
    14  @@ -215,12 +215,7 @@ function ulong_arg:long (argnum:long) {
    15   }
    16   
    17   function longlong_arg:long (argnum:long) {
    18  -       if (probing_32bit_app()) {
    19  -               lowbits = _stp_arg(argnum, 0, 1)
    20  -               highbits = _stp_arg(argnum+1, 0, 1)
    21  -               return ((highbits << 32) | lowbits)
    22  -       } else
    23  -               return _stp_arg(argnum, 0, 0)
    24  +       return _stp_arg(argnum, 0, 0)
    25   }
    26   
    27   function ulonglong_arg:long (argnum:long) {
$ 
=======

The actual fix is on lines 9-10. Lines 18-23 are just removal of redundant code
that, based on my testing, appears to never be used. Note, that in case of
probing compat functions, we usually build longlong value out of two 32 bit
values passed to the function as separate parameters. An x86_64 example is
sys32_sync_file_range(int fd, unsigned off_low, unsigned off_hi, unsigned
n_low, unsigned n_hi,  int flags), in which case we fabricate offset =
((uint_arg(3) << 32) | uint_arg(2)). Based on my testing lines 18-23 are of no
use and this analogically applies to respective powerpc and s390x snippets. So
I'm removing this later on im my patch.

But back to the actual fix. For powerpc, following update seems to work:

=======
$ git diff tapset/powerpc/registers.stp | nl
     1  diff --git a/tapset/powerpc/registers.stp
b/tapset/powerpc/registers.stp
     2  index 03609f8..eb80018 100644
     3  --- a/tapset/powerpc/registers.stp
     4  +++ b/tapset/powerpc/registers.stp
     5  @@ -149,7 +149,7 @@ function _stp_arg:long (argnum:long,
sign_extend:long, truncate:long) {
     6          else if (argnum == 8)
     7                  val = u_register("r10")
     8   
     9  -       if (truncate) {
    10  +       if (truncate || @__compat_task) {
    11                  if (sign_extend)
    12                          val = _stp_sign_extend32(val)
    13                  else
    14  @@ -178,12 +178,7 @@ function ulong_arg:long (argnum:long) {
    15   }
    16   
    17   function longlong_arg:long (argnum:long) {
    18  -       if (probing_32bit_app()) {
    19  -               lowbits = _stp_arg(argnum, 0, 1)
    20  -               highbits = _stp_arg(argnum+1, 0, 1)
    21  -               return ((highbits << 32) | lowbits)
    22  -       } else
    23  -               return _stp_arg(argnum, 0, 0)
    24  +       return _stp_arg(argnum, 0, 0)
    25   }
    26   
    27   function ulonglong_arg:long (argnum:long) {
$ 
=======

As I said, this issue is not present on s390, because a change in this sense is
already there. So related part of my patch reads:

=======
$ git diff tapset/s390/registers.stp | nl
     1  diff --git a/tapset/s390/registers.stp b/tapset/s390/registers.stp
     2  index c8cb304..c2ced72 100644
     3  --- a/tapset/s390/registers.stp
     4  +++ b/tapset/s390/registers.stp
     5  @@ -237,7 +237,7 @@ function _stp_arg:long (argnum:long,
sign_extend:long, truncate:long)
     6          else if (argnum >= 6)
     7                  val = _stp_get_kernel_stack_param(argnum - 6)
     8   
     9  -       if (truncate || %{ /* pure */ _stp_is_compat_task() %}) {
    10  +       if (truncate || @__compat_task) {
    11                  /* High bits may be garbage. */
    12                  val = (val & 0xffffffff)
    13                  if (sign_extend)
    14  @@ -265,13 +265,7 @@ function ulong_arg:long (argnum:long) {
    15   }
    16   
    17   function longlong_arg:long (argnum:long) {
    18  -       if (probing_32bit_app()) {
    19  -               /* TODO verify if this is correct for 31bit apps */
    20  -               highbits = _stp_arg(argnum, 0, 1)
    21  -               lowbits = _stp_arg(argnum+1, 0, 1)
    22  -               return ((highbits << 32) | lowbits)
    23  -       } else
    24  -               return _stp_arg(argnum, 0, 0)
    25  +       return _stp_arg(argnum, 0, 0)
    26   }
    27   
    28   function ulonglong_arg:long (argnum:long) {
$ 
=======

Which actually is a cosmetical update only.

In general, applying aforementioned updates seems to cause regressions in
pread, pwrite and sync_file_range. But after looking closer I think the problem
is just that these probes rely on aforementioned misbehavior. Their fixes make
significant part of patch I'm going to attach.

-- 
You are receiving this mail because:
You are the assignee for the bug.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]