[Bug tapsets/15219] New: syscall.exp failures on RHEL5, RHEL6, and rawhide

[Bug tapsets/15219] New: syscall.exp failures on RHEL5, RHEL6, and rawhide

[dsmith at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15219

             Bug #: 15219
           Summary: syscall.exp failures on RHEL5, RHEL6, and rawhide
           Product: systemtap
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: tapsets
        AssignedTo: systemtap@sourceware.org
        ReportedBy: dsmith@redhat.com
    Classification: Unclassified


While investigating bug #15211, I realized that we were seeing some common
failures in syscall.exp across kernel versions:

On RHEL5 (2.6.18-308.20.1.el5 x86_64) and RHEL6 (2.6.32-279.14.1.el6.x86_64), I
see the following failures:

====
FAIL: 32-bit clock syscall
FAIL: 32-bit rt_signal syscall
FAIL: 32-bit timer syscall
====

On rawhide (3.8.0-0.rc6.git1.1.fc19.x86_64), I see the following failures:

====
FAIL: 32-bit clock syscall
FAIL: 32-bit timer syscall
====

From looking at systemtap.log, the failures all happen when the 32-bit compat
version of syscalls call the "real" syscall. The systemtap tapset has trouble
decoding the arguments.  For example:

timer: timer_settime (0, 1, UNKNOWN, 0xffff880014eeff30) = -22 (EINVAL)

That "UNKNOWN" value is coming from _struct_itimerspec_u(). Inside
_struct_itimerspec_u(), we return "UNKNOWN" when _stp_copy_from_user() fails.
In this case, _stp_copy_from_user() is failing because the memory it is reading
really isn't user memory. The 32-bit compat version of timer_settime() copies
the 'struct compat_itimerspec' pointer into a real 'struct itimerspec', then
calls the real sys_timer_settime().

We may need to loosen up _stp_copy_from_user().

-- 
Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug tapsets/15219] syscall.exp failures on RHEL5, RHEL6, and rawhide

[dsmith at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15219

--- Comment #1 from David Smith <dsmith at redhat dot com> 2013-03-04 17:53:58 UTC ---
Here's compat_sys_timer_settime() from kernel/compat.c. Notice it converts the
'struct compat_itimerspec' to a 'struct itimerspec' (which is in kernel
memory), calls 'set_fs(KERENL_DS), then calls the real syscall function,
'sys_timer_settime()'.

====
long compat_sys_timer_settime(timer_t timer_id, int flags,
              struct compat_itimerspec __user *new,
              struct compat_itimerspec __user *old)
{
    long err;
    mm_segment_t oldfs;
    struct itimerspec newts, oldts;

    if (!new)
        return -EINVAL;
    if (get_compat_itimerspec(&newts, new))
        return -EFAULT;
    oldfs = get_fs();
    set_fs(KERNEL_DS);
    err = sys_timer_settime(timer_id, flags,
                (struct itimerspec __user *) &newts,
                (struct itimerspec __user *) &oldts);
    set_fs(oldfs);
    if (!err && old && put_compat_itimerspec(old, &oldts))
        return -EFAULT;
    return err;
}
====

Here's our '_stp_copy_from_user()'. Notice we explicitly call
'set_fs(USER_DS)', which just overrode compat_sys_timer_settime() setting.

====
static unsigned long _stp_copy_from_user(char *dst, const char __user *src,
unsigned long count)
{
    if (count) {
                mm_segment_t _oldfs = get_fs();
                set_fs(USER_DS);
                pagefault_disable();
        if (access_ok(VERIFY_READ, src, count))
            count = __copy_from_user_inatomic(dst, src, count);
        else
            memset(dst, 0, count);
                pagefault_enable();
                set_fs(_oldfs);
    }
    return count;
}
====

I'm thinking we should no longer change the kernel's idea of what memory space
to use in _stp_copy_from_user().

-- 
Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug tapsets/15219] syscall.exp failures on RHEL5, RHEL6, and rawhide

[fche at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15219

Frank Ch. Eigler <fche at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |fche at redhat dot com

--- Comment #2 from Frank Ch. Eigler <fche at redhat dot com> 2013-03-04 18:40:06 UTC ---
I believe the USER_DS protection still has value, for contexts where the
set_fs() is not manually tweaked by the kernel.

We could track the syscall nesting, and the script code can use kernel_*()
instead of user_*() for those parameters when nesting is detected.

See also bug #6762.

-- 
Configure bugmail: http://sourceware.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

[Bug tapsets/15219] syscall.exp failures on RHEL5, RHEL6, and rawhide

[dsmith at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15219

--- Comment #3 from David Smith <dsmith at redhat dot com> ---
I wonder if we couldn't have a '_stp_copy_from_user_or_kernel()' function that
would do the correct copy based on the current set_fs() value. If we wanted to
get even more strict we could only call that one if 'current' is a
32-bits-on-64-bits process.

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

[Bug tapsets/15219] syscall.exp failures on RHEL5, RHEL6, and rawhide

[fche at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15219

Frank Ch. Eigler <fche at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jlebon at redhat dot com

--- Comment #4 from Frank Ch. Eigler <fche at redhat dot com> ---
ISTM that the places where we do _stp_copy_from_user in the
tapset, we could conditionalize those functions based on get_fs(),
dispatching to a new _stp_copy_from_kernel() or somesuch.  These
would account for the kernel's internal set_fs games that are
known to occur on those particular parameters.  I would disfavour
opening up the automation in the, as a belt&suspenders kind of
safety measure against inappropriate information disclosure.

BTW, tapset/linux/aux_syscalls.stp, _struct-itemerspec_u(), with
its NULL / UNKNOWN coding style, is implicated in bug #15044.

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

[Bug tapsets/15219] syscall.exp failures on RHEL5, RHEL6, and rawhide

[dsmith at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15219

--- Comment #5 from David Smith <dsmith at redhat dot com> ---
Created attachment 7057
  --> http://sourceware.org/bugzilla/attachment.cgi?id=7057&action=edit
beta patch

Here's a beta patch, that adds a function to copy from the user or kernel
memory (it just uses the current memory segment) and makes a couple of the
functions in aux_syscalls.stp call the new function.

This patch doesn't fix all the issues in the tapset, I'm just looking to see if
the approach looks reasonable.

One thing to think about is which functions in aux_syscalls.stp do we modify?
Do we go ahead and modify them all (that use _stp_copy_from_user()) or do we
try to only do it where we know there are compat functions that switch to using
kernel memory?

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

[Bug tapsets/15219] syscall.exp failures on RHEL5, RHEL6, and rawhide

[dsmith at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15219

--- Comment #6 from David Smith <dsmith at redhat dot com> ---
Created attachment 7063
  --> http://sourceware.org/bugzilla/attachment.cgi?id=7063&action=edit
beta 2 patch

Here's a second version of the patch, that tries to be more paranoid about when
to allow reading kernel memory, especially for Unprivileged users.

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

[Bug tapsets/15219] syscall.exp failures on RHEL5, RHEL6, and rawhide

[dsmith at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15219

--- Comment #7 from David Smith <dsmith at redhat dot com> ---
Created attachment 7067
  --> http://sourceware.org/bugzilla/attachment.cgi?id=7067&action=edit
anti-nested syscall patch

Here's a new patch that attempts (and fails) to fix this problem a different
way.

Instead of changing the way we access memory, instead this patch tries to avoid
the nested system calls. To do this, in the 64-bit syscall .call and .return
probe, I added the following line:

    if (%{ _stp_is_compat_task() %}) next

So, if we're in a 64-bit syscall with a 32-bit process, skip the probe (and let
the 32-bit syscall handle it). (That line could be prettied up by using a
macro.)

As I mentioned above, the advantage this approach has is that it doesn't change
the way we access memory. The disadvantages are:

- Knowing when and where to add that line. The problem is that some 32-bit
syscalls are wrappers around the 64-bit syscall, and some 32-bit syscalls call
the same underlying function that the 64-bit syscall does (after massaging the
arguments a bit). So, in the 1st case (the wrapper case), we need the probe
skip line, but in the 2nd case we don't.  I used the syscall.exp test case to
know where to put that line when developing the patch, but the syscall.exp test
case doesn't test every system call.

- Whether we need that line or not can change with kernels. For example, the
syscall.rt_sigprocmask probe should be skipped on RHEL[56] kernels, but not on
rawhide.

So, some more work will be required here.

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

[Bug tapsets/15219] syscall.exp failures on RHEL5, RHEL6, and rawhide

[dsmith at redhat dot com]

http://sourceware.org/bugzilla/show_bug.cgi?id=15219

--- Comment #8 from David Smith <dsmith at redhat dot com> ---
(In reply to David Smith from comment #7)
> - Whether we need that line or not can change with kernels. For example, the
> syscall.rt_sigprocmask probe should be skipped on RHEL[56] kernels, but not
> on rawhide.

I couldn't figure out why this change didn't work for rawhide, so I dug in a
bit more. Here's what I found. Over in
arch/x86/include/generated/asm/syscalls_32.h (which gets generated during the
kernel build), you'll see this:

====
__SYSCALL_I386(173, sys_rt_sigreturn, stub32_rt_sigreturn)
__SYSCALL_I386(174, sys_rt_sigaction, compat_sys_rt_sigaction)
__SYSCALL_I386(175, sys_rt_sigprocmask, sys_rt_sigprocmask)
__SYSCALL_I386(176, sys_rt_sigpending, compat_sys_rt_sigpending)
__SYSCALL_I386(177, sys_rt_sigtimedwait, compat_sys_rt_sigtimedwait)
__SYSCALL_I386(178, sys_rt_sigqueueinfo, compat_sys_rt_sigqueueinfo)
__SYSCALL_I386(179, sys_rt_sigsuspend, sys_rt_sigsuspend)
====

Notice that for syscall 175, rt_sigprocmask doesn't call
'compat_sys_rt_sigprocmask', it just calls the real 64-bit 'sys_rt_sigprocmask'
(see the 3rd argument and compare to rt_sigaction or rt_sigpending).

On RHEL[56], the compat function for rt_sigprocmask does get called.

(Also note that 'rt_sigreturn' calls 'stub32_rt_sigreturn', which the syscall
tapset isn't expecting. So, we probably don't handle a 32-bit rt_sigreturn
properly.)

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

[Bug tapsets/15219] syscall.exp failures on RHEL5, RHEL6, and rawhide

[dsmith at redhat dot com]

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

David Smith <dsmith at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #7067|0                           |1
        is obsolete|                            |

--- Comment #9 from David Smith <dsmith at redhat dot com> ---
Created attachment 7166
  --> https://sourceware.org/bugzilla/attachment.cgi?id=7166&action=edit
anti-nested syscall patch 2

Here's yet another approach to fixing this one. This one attempts to solve the
nested-syscall problem by defining 2 macros: '@__syscall_enter' (called at each
syscall entry probe) and '@__syscall_exit' (called in each syscall return
probe). It just uses a global array to figure out if we're already in a system
call in this thread. If so, the probe is skipped.

Here are some notes:

- I just modified the syscall tapset enough to get the 32-bit clock syscall
passing, I didn't modify every place that needs changes.

-There are a couple of problems with using a macro library file, listed there.
The best way of fixing these problems might be to combine syscalls.stp and
syscalls2.stp so that the macros could be defined in that file.

- Similar changes would need to be made for the nd_syscall tapset. It would
probably need its own global array.

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

[Bug tapsets/15219] syscall.exp failures on RHEL5, RHEL6, and rawhide

[dsmith at redhat dot com]

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

--- Comment #10 from David Smith <dsmith at redhat dot com> ---
Created attachment 7226
  --> https://sourceware.org/bugzilla/attachment.cgi?id=7226&action=edit
different anti-nested syscall patch

Here's a patch that attempts to fix this problem by avoiding nested syscalls.
It does this by looking at the syscall number. If we're in
syscall.clock_settime, but the syscall number isn't __NR_clock_settime, return.
This is done with macros stored in tapset/linux/syscalls.stpm, called
'__syscall_enter' and '__syscall_enter2'.

With this patch, the syscall test case passes completely on rawhide x86_64
(3.12.0-0.rc2.git0.1.fc21.x86_64).

Note that the patch isn't complete. The nd_syscall tapset files would need to
modified in a similar manner. Also, all nested syscalls weren't avoided, just
the ones needed for syscall.exp to pass.

Also note that the macros could use better names, especially since they also
get called in .return probes.

Comments welcome.

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

[Bug tapsets/15219] syscall.exp failures on RHEL5, RHEL6, and rawhide

[dsmith at redhat dot com]

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

--- Comment #11 from David Smith <dsmith at redhat dot com> ---
One more "todo" about this patch. It is very x86 centric, with the use of some
'__NR_ia32_SYSCALL' defines. We'll probably have to define our own "generic"
compat syscall numbers.

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

[Bug tapsets/15219] syscall.exp failures on RHEL5, RHEL6, and rawhide

[dsmith at redhat dot com]

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

David Smith <dsmith at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #7226|0                           |1
        is obsolete|                            |

--- Comment #12 from David Smith <dsmith at redhat dot com> ---
Created attachment 7262
  --> https://sourceware.org/bugzilla/attachment.cgi?id=7262&action=edit
simpler patch

Here's a simpler version of the last patch that just fixes one syscall.exp
failure for the 32-bit 'clock' subtest when run on rawhide x86_64. This simpler
patch should make this approach easier to review and understand.

Note that we'd have to make similar changes to nd_syscalls.stp.

Also, it might make sense to combine the syscall.clock_nanosleep and
syscall.compat_clock_settime probe aliases with their 'compat' probe aliases.

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

[Bug tapsets/15219] syscall.exp failures on RHEL5, RHEL6, and rawhide

[dsmith at redhat dot com]

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

David Smith <dsmith at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #13 from David Smith <dsmith at redhat dot com> ---
(In reply to David Smith from comment #12)
> Created attachment 7262 [details]
> simpler patch
> 
> Here's a simpler version of the last patch that just fixes one syscall.exp
> failure for the 32-bit 'clock' subtest when run on rawhide x86_64. This
> simpler patch should make this approach easier to review and understand.
> 
> Note that we'd have to make similar changes to nd_syscalls.stp.
> 
> Also, it might make sense to combine the syscall.clock_nanosleep and
> syscall.compat_clock_settime probe aliases with their 'compat' probe aliases.

I've made changes to the syscall tapsets to implement this idea. At this point,
the syscall.exp and nd_syscall.exp testcases pass on every platform I've tested
on.

(Note that these changes don't really fix *all* nested syscall problems, just
the ones tested by the testsuite. To catch more failures, we'd need to expand
the testsuite.)

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