From: "mcermak at redhat dot com" <sourceware-bugzilla@sourceware.org>
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 [thread overview]
Message-ID: <bug-18597-6586@http.sourceware.org/bugzilla/> (raw)
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.
next reply other threads:[~2015-06-25 11:00 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-25 11:00 mcermak at redhat dot com [this message]
2015-06-25 11:03 ` [Bug tapsets/18597] " mcermak at redhat dot com
2015-06-25 11:04 ` mcermak at redhat dot com
2015-06-25 15:03 ` mcermak at redhat dot com
2015-06-26 12:13 ` dsmith at redhat dot com
2015-06-26 12:20 ` dsmith at redhat dot com
2015-06-30 15:56 ` dsmith at redhat dot com
2015-06-30 15:59 ` dsmith at redhat dot com
2015-06-30 17:21 ` jistone at redhat dot com
2015-06-30 17:36 ` dsmith at redhat dot com
2015-06-30 20:21 ` dsmith at redhat dot com
2015-06-30 20:22 ` dsmith at redhat dot com
2015-06-30 20:32 ` jistone at redhat dot com
2015-06-30 20:46 ` dsmith at redhat dot com
2015-07-01 15:21 ` mcermak at redhat dot com
2015-07-01 16:14 ` dsmith at redhat dot com
2015-07-01 17:10 ` mcermak at redhat dot com
2015-07-01 17:29 ` dsmith at redhat dot com
2015-07-03 14:18 ` mcermak at redhat dot com
2015-07-06 13:45 ` dsmith at redhat dot com
2015-07-07 7:02 ` mcermak at redhat dot com
2015-07-08 6:09 ` mcermak at redhat dot com
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bug-18597-6586@http.sourceware.org/bugzilla/ \
--to=sourceware-bugzilla@sourceware.org \
--cc=systemtap@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).