From: David Smith <dsmith@redhat.com>
To: Arkady <arkady.miasnikov@gmail.com>
Cc: systemtap@sourceware.org
Subject: Re: Failure in syscall.open probe
Date: Mon, 28 Aug 2017 17:19:00 -0000 [thread overview]
Message-ID: <CAKFOr-YPK_0=R++4Y+h4ORebRXDPkTN+POSK9Z+6kbsK3Uyd6A@mail.gmail.com> (raw)
In-Reply-To: <CANA-60q7pd0c7RdghMK_akV7q3aM9gpGVFoC7QwOjmx9r2HU6A@mail.gmail.com>
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
next prev parent reply other threads:[~2017-08-28 17:19 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-24 14:26 Arkady
2017-08-24 14:35 ` Arkady
2017-08-24 16:28 ` Arkady
2017-08-25 3:56 ` Arkady
2017-08-25 4:36 ` Arkady
2017-08-25 17:25 ` Arkady
2017-08-25 19:52 ` David Smith
2017-08-27 6:13 ` Daniel Doron
2017-08-27 12:22 ` Arkady
2017-08-27 12:38 ` Arkady
2017-08-28 5:51 ` Arkady
2017-08-28 6:28 ` Arkady
2017-08-28 17:19 ` David Smith [this message]
2017-08-29 16:06 ` David Smith
2017-08-29 16:44 ` Arkady
2017-08-30 14:18 ` David Smith
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='CAKFOr-YPK_0=R++4Y+h4ORebRXDPkTN+POSK9Z+6kbsK3Uyd6A@mail.gmail.com' \
--to=dsmith@redhat.com \
--cc=arkady.miasnikov@gmail.com \
--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).