Re: [PATCH 5/5] Use seek+read instead of pread to read from /dev/$$/mem files.

Re: [PATCH 5/5] Use seek+read instead of pread to read from /dev/$$/mem files.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

On Fri, 2015-10-09 at 12:35 +0200, Jose E. Marchesi wrote:
> Here you go.
> 
> commit 43f68c295e60ca716bc5d1a3876c62f949d4f889
> Author: Jose E. Marchesi <jose.marchesi@oracle.com>
> Date:   Fri Oct 9 06:38:39 2015 -0400
> 
>     Use seek+read instead of pread to read from /dev/$$/mem files.
>     
>     pread[64] always returns EINVAL when negative offsets are used.
>     read+seek allows us to read in-memory vdso objects mapped high in the
>     address space.
>     
>     Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com>

Looks good. Thanks. Pushed to master.

Re: [PATCH 5/5] Use seek+read instead of pread to read from /dev/$$/mem files.

[Jose E. Marchesi]

[-- Attachment #1: Type: text/plain, Size: 2560 bytes --]


    This subtly changes the code to always ignore errors and turns them into
    "short reads" instead. Looking at how this code is used I think it is
    actually better to instead change the code to report an error with errno.
    So return -1 from lseek and just fall through to returning -1 after the
    read. It doesn't matter too much, but seems more consistent and might
    provide slightly nicer error messages if something fails (actually currently
    the error message is swallowed later in the code, but if we are changing
    this anyway, then changing it to report a real errno always seems better).

Here you go.

commit 43f68c295e60ca716bc5d1a3876c62f949d4f889
Author: Jose E. Marchesi <jose.marchesi@oracle.com>
Date:   Fri Oct 9 06:38:39 2015 -0400

    Use seek+read instead of pread to read from /dev/$$/mem files.
    
    pread[64] always returns EINVAL when negative offsets are used.
    read+seek allows us to read in-memory vdso objects mapped high in the
    address space.
    
    Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com>

diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index ee41405..211f9ec 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,8 @@
+2015-10-09  Jose E. Marchesi  <jose.marchesi@oracle.com>
+
+	* linux-proc-maps.c (read_proc_memory): Use seek+read instead of
+	pread, as the later doesn't accept negative offsets.
+
 2015-10-07  Mark Wielaard  <mjw@redhat.com>
 
 	* dwfl_module_getdwarf.c (MAX): Removed.
diff --git a/libdwfl/linux-proc-maps.c b/libdwfl/linux-proc-maps.c
index d085834..2a65db2 100644
--- a/libdwfl/linux-proc-maps.c
+++ b/libdwfl/linux-proc-maps.c
@@ -315,10 +315,17 @@ read_proc_memory (void *arg, void *data, GElf_Addr address,
 		  size_t minread, size_t maxread)
 {
   const int fd = *(const int *) arg;
-  ssize_t nread = pread64 (fd, data, maxread, (off64_t) address);
-  /* Some kernels don't actually let us do this read, ignore those errors.  */
-  if (nread < 0 && (errno == EINVAL || errno == EPERM))
-    return 0;
+
+  /* This code relies on the fact the Linux kernel accepts negative
+     offsets when seeking /dev/$$/mem files, as a special case. In
+     particular pread[64] cannot be used here, because it will always
+     return EINVAL when passed a negative offset.  */
+
+  if (lseek (fd, (off64_t) address, SEEK_SET) == -1)
+    return -1;
+
+  ssize_t nread = read (fd, data, maxread);
+
   if (nread > 0 && (size_t) nread < minread)
     nread = 0;
   return nread;

Re: [PATCH 5/5] Use seek+read instead of pread to read from /dev/$$/mem files.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1640 bytes --]

On Mon, Oct 05, 2015 at 05:36:34PM +0200, Jose E. Marchesi wrote:
> pread[64] always returns EINVAL when negative offsets are used.
> read+seek allows us to read in-memory vdso objects mapped high in the
> address space.

Nice find.
 
>  {
>    const int fd = *(const int *) arg;
> -  ssize_t nread = pread64 (fd, data, maxread, (off64_t) address);
> +
> +  /* This code relies on the fact the Linux kernel accepts negative
> +     offsets when seeking /dev/$$/mem files, as a special case. In
> +     particular pread[64] cannot be used here, because it will always
> +     return EINVAL when passed a negative offset.  */
> +
> +  if (lseek (fd, (off64_t) address, SEEK_SET) == -1)
> +    return 0;
> +
> +  ssize_t nread = read (fd, data, maxread);
> +
>    /* Some kernels don't actually let us do this read, ignore those errors.  */
> -  if (nread < 0 && (errno == EINVAL || errno == EPERM))
> +  if (nread < 0)
>      return 0;
>    if (nread > 0 && (size_t) nread < minread)
>      nread = 0;

This subtly changes the code to always ignore errors and turns them into
"short reads" instead. Looking at how this code is used I think it is
actually better to instead change the code to report an error with errno.
So return -1 from lseek and just fall through to returning -1 after the
read. It doesn't matter too much, but seems more consistent and might
provide slightly nicer error messages if something fails (actually currently
the error message is swallowed later in the code, but if we are changing
this anyway, then changing it to report a real errno always seems better).

Thanks,

Mark

[PATCH 5/5] Use seek+read instead of pread to read from /dev/$$/mem files.

[Jose E. Marchesi]

[-- Attachment #1: Type: text/plain, Size: 1862 bytes --]

pread[64] always returns EINVAL when negative offsets are used.
read+seek allows us to read in-memory vdso objects mapped high in the
address space.

Signed-off-by: Jose E. Marchesi <jose.marchesi@oracle.com>
---
 libdwfl/ChangeLog         |  5 +++++
 libdwfl/linux-proc-maps.c | 14 ++++++++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/libdwfl/ChangeLog b/libdwfl/ChangeLog
index ed1156e..355c750 100644
--- a/libdwfl/ChangeLog
+++ b/libdwfl/ChangeLog
@@ -1,3 +1,8 @@
+2015-10-05  Jose E. Marchesi  <jose.marchesi@oracle.com>
+
+	* linux-proc-maps.c (read_proc_memory): Use seek+read instead of
+	pread, as the later doesn't accept negative offsets.
+
 2015-09-18  Chih-Hung Hsieh  <chh@google.com>
 
 	* relocate.c (relocate_section): Move nested function "relocate"
diff --git a/libdwfl/linux-proc-maps.c b/libdwfl/linux-proc-maps.c
index d085834..83b90db 100644
--- a/libdwfl/linux-proc-maps.c
+++ b/libdwfl/linux-proc-maps.c
@@ -315,9 +315,19 @@ read_proc_memory (void *arg, void *data, GElf_Addr address,
 		  size_t minread, size_t maxread)
 {
   const int fd = *(const int *) arg;
-  ssize_t nread = pread64 (fd, data, maxread, (off64_t) address);
+
+  /* This code relies on the fact the Linux kernel accepts negative
+     offsets when seeking /dev/$$/mem files, as a special case. In
+     particular pread[64] cannot be used here, because it will always
+     return EINVAL when passed a negative offset.  */
+
+  if (lseek (fd, (off64_t) address, SEEK_SET) == -1)
+    return 0;
+
+  ssize_t nread = read (fd, data, maxread);
+
   /* Some kernels don't actually let us do this read, ignore those errors.  */
-  if (nread < 0 && (errno == EINVAL || errno == EPERM))
+  if (nread < 0)
     return 0;
   if (nread > 0 && (size_t) nread < minread)
     nread = 0;
-- 
2.3.4