Re: Failure in syscall.open probe

[David Smith <dsmith at redhat dot com>]

I've checked that patch in as commit cd8837d56. Let me know if you
still see that "scheduling while atomic" bug.

On Mon, Aug 28, 2017 at 12:19 PM, David Smith <dsmith@redhat.com> wrote:
> On Mon, Aug 28, 2017 at 1:28 AM, Arkady <arkady.miasnikov@gmail.com> wrote:
>> This patch follows the API and fixes a bug - increment src/dst pointers
>
> ... patch deleted ...
>
> Your patch looked fine, but I decided to go a slightly different
> route. We had a function, called kderef_string_(), over in
> runtime/linux/loc2c-runtime.h, which did everything we needed here,
> but it was hardcoded to only work for kernel strings. I abstracted it
> slightly and renamed it '_stp_deref_string_nofault()'. Now
> kderef_string() and _stp_strncpy_from_user() both call
> _stp_deref_string_nofault(). The advantage here is that we're using
> the same function to read kernel and user strings.
>
> Assuming after a testsuite run I don't see any issues, I'll check this
> in tomorrow.
>
> ====
> diff --git a/runtime/linux/copy.c b/runtime/linux/copy.c
> index c5022b5e2..892700865 100644
> --- a/runtime/linux/copy.c
> +++ b/runtime/linux/copy.c
> @@ -39,24 +39,10 @@
>   * <i>count</i> bytes and returns <i>count</i>.
>   */
>
> -/* XXX: see also kread/uread in loc2c-runtime.h */
> -static long _stp_strncpy_from_user(char *dst, const char __user *src,
> long count)
> +static long _stp_strncpy_from_user(char *dst, const char __user *src,
> +   long count)
>  {
> - long res = -EFAULT;
> -        mm_segment_t _oldfs = get_fs();
> -        set_fs(USER_DS);
> -        pagefault_disable();
> -        /* XXX: The following preempt() manipulations should be
> -           redundant with probe entry/exit code, but for unknown
> -           reasons on RHEL5/6 conversions.exp intermittently fails
> -           without this.  */
> -        preempt_disable();
> - if (!lookup_bad_addr(VERIFY_READ, (const unsigned long)src, count))
> - res = strncpy_from_user(dst, src, count);
> -        preempt_enable_no_resched();
> -        pagefault_enable();
> -        set_fs(_oldfs);
> - return res;
> + return _stp_deref_string_nofault(dst, src, count, USER_DS);
>  }
>
>  /** Copy a block of data from user space.
> diff --git a/runtime/linux/loc2c-runtime.h b/runtime/linux/loc2c-runtime.h
> index 2920abed7..39dece07d 100644
> --- a/runtime/linux/loc2c-runtime.h
> +++ b/runtime/linux/loc2c-runtime.h
> @@ -716,16 +716,30 @@ static inline char *kderef_buffer_(char *dst,
> void *addr, size_t len)
>      _r; \
>    })
>
> -/* The following is for kernel strings, see the uconversions.stp
> -   tapset for user_string functions. */
> +/*
> + * _stp_deref_string_nofault(): safely read a string from memory
> + * without a DEREF_FAULT on error
> + *
> + * dst: read the string into this address
> + * addr: address to read from
> + * len: maximum number of bytes to read
> + * seg: memory segment to use, either kernel (KERNEL_DS) or user
> + * (USER_DS)
> + *
> + * If this function gets an error while trying to read memory, -EFAULT
> + * is returned. On success, the number of bytes copied is returned
> + * (not including the trailing NULL). Note that the kernel will not
> + * pagefault when trying to read the string.
> + */
>
> -static inline char *kderef_string_(char *dst, void *addr, size_t len)
> +static inline long _stp_deref_string_nofault(char *dst, const char *addr,
> +     size_t len, mm_segment_t seg)
>  {
>    int err = 0;
> -  size_t i;
> +  size_t i = 0;
>    mm_segment_t oldfs = get_fs();
>
> -  set_fs(KERNEL_DS);
> +  set_fs(seg);
>    pagefault_disable();
>    if (lookup_bad_addr(VERIFY_READ, (uintptr_t)addr, len))
>      err = 1;
> @@ -746,13 +760,13 @@ static inline char *kderef_string_(char *dst,
> void *addr, size_t len)
>    pagefault_enable();
>    set_fs(oldfs);
>
> -  return err ? (char *)-1 : dst;
> +  return err ? -EFAULT : i;
>  }
>
>  #define kderef_string(dst, addr, maxbytes) \
>    ({ \
> -    char *_r = kderef_string_((dst), (void *)(uintptr_t)(addr), (maxbytes)); \
> -    if (_r == (char *)-1) \
> +    long _r = _stp_deref_string_nofault((dst), (void
> *)(uintptr_t)(addr), (maxbytes), KERNEL_DS); \
> +    if (_r < 0) \
>        DEREF_FAULT(addr); \
>      _r; \
>    })
> ====
> --
> David Smith
> Principal Software Engineer
> Red Hat



-- 
David Smith
Principal Software Engineer
Red Hat