From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22370 invoked by alias); 25 Jun 2015 11:00:17 -0000 Mailing-List: contact systemtap-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: systemtap-owner@sourceware.org Received: (qmail 22263 invoked by uid 48); 25 Jun 2015 11:00:10 -0000 From: "mcermak at redhat dot com" To: systemtap@sourceware.org Subject: [Bug tapsets/18597] New: long_arg() doesn't correctly handle negative values in 32-on-64 environment Date: Thu, 25 Jun 2015 11:00:00 -0000 X-Bugzilla-Reason: AssignedTo X-Bugzilla-Type: new X-Bugzilla-Watch-Reason: None X-Bugzilla-Product: systemtap X-Bugzilla-Component: tapsets X-Bugzilla-Version: unspecified X-Bugzilla-Keywords: X-Bugzilla-Severity: normal X-Bugzilla-Who: mcermak at redhat dot com X-Bugzilla-Status: NEW X-Bugzilla-Resolution: X-Bugzilla-Priority: P2 X-Bugzilla-Assigned-To: systemtap at sourceware dot org X-Bugzilla-Target-Milestone: --- X-Bugzilla-Flags: X-Bugzilla-Changed-Fields: bug_id short_desc product version bug_status bug_severity priority component assigned_to reporter target_milestone Message-ID: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Bugzilla-URL: http://sourceware.org/bugzilla/ Auto-Submitted: auto-generated MIME-Version: 1.0 X-SW-Source: 2015-q2/txt/msg00205.txt.bz2 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.