This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
[Bug tapsets/18597] New: long_arg() doesn't correctly handle negative values in 32-on-64 environment
- From: "mcermak at redhat dot com" <sourceware-bugzilla at sourceware dot org>
- To: systemtap at sourceware dot org
- Date: Thu, 25 Jun 2015 11:00:07 +0000
- Subject: [Bug tapsets/18597] New: long_arg() doesn't correctly handle negative values in 32-on-64 environment
- Auto-submitted: auto-generated
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.