[PATCH v2 0/2] *-user: Handle short reads in mmap_h_gt_g

[PATCH v2 0/2] *-user: Handle short reads in mmap_h_gt_g

[Richard Henderson]

Changes for v2:
  - Handle short reads from the mmap_frag subroutine.
  - Update bsd-user as well.


r~


Richard Henderson (2):
  linux-user: Handle short reads in mmap_h_gt_g
  bsd-user: Handle short reads in mmap_h_gt_g

 bsd-user/mmap.c   | 38 ++++++++++++++++++++++++++++++++++++--
 linux-user/mmap.c | 44 ++++++++++++++++++++++++++++++++++++++------
 2 files changed, 74 insertions(+), 8 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] linux-user: Handle short reads in mmap_h_gt_g

[Richard Henderson]

In particular, if an image has a large bss, we can hit
EOF before reading all host_len bytes of the mapping.

Create a helper, mmap_pread to handle the job for both
the larger block in mmap_h_gt_g itself, as well as the
smaller block in mmap_frag.

Cc: qemu-stable@nongnu.org
Fixes: eb5027ac618 ("linux-user: Split out mmap_h_gt_g")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2504
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/mmap.c | 44 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 6 deletions(-)

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 6418e811f6..e4bf5d5f39 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -283,6 +283,40 @@ static int do_munmap(void *addr, size_t len)
     return munmap(addr, len);
 }
 
+/*
+ * Perform a pread on behalf of target_mmap.  We can reach EOF, we can be
+ * interrupted by signals, and in general there's no good error return path.
+ * If @zero, zero the rest of the block at EOF.
+ * Return true on success.
+ */
+static bool mmap_pread(int fd, void *p, size_t len, off_t offset, bool zero)
+{
+    while (1) {
+        ssize_t r = pread(fd, p, len, offset);
+
+        if (likely(r == len)) {
+            /* Complete */
+            return true;
+        }
+        if (r == 0) {
+            /* EOF */
+            if (zero) {
+                memset(p, 0, len);
+            }
+            return true;
+        }
+        if (r > 0) {
+            /* Short read */
+            p += r;
+            len -= r;
+            offset += r;
+        } else if (errno != EINTR) {
+            /* Error */
+            return false;
+        }
+    }
+}
+
 /*
  * Map an incomplete host page.
  *
@@ -357,10 +391,9 @@ static bool mmap_frag(abi_ulong real_start, abi_ulong start, abi_ulong last,
     /* Read or zero the new guest pages. */
     if (flags & MAP_ANONYMOUS) {
         memset(g2h_untagged(start), 0, last - start + 1);
-    } else {
-        if (pread(fd, g2h_untagged(start), last - start + 1, offset) == -1) {
-            return false;
-        }
+    } else if (!mmap_pread(fd, g2h_untagged(start), last - start + 1,
+                           offset, true)) {
+        return false;
     }
 
     /* Put final protection */
@@ -853,8 +886,7 @@ static abi_long mmap_h_gt_g(abi_ulong start, abi_ulong len,
     }
 
     if (misaligned_offset) {
-        /* TODO: The read could be short. */
-        if (pread(fd, p, host_len, offset + real_start - start) != host_len) {
+        if (!mmap_pread(fd, p, host_len, offset + real_start - start, false)) {
             do_munmap(p, host_len);
             return -1;
         }
-- 
2.43.0

[PATCH v2 2/2] bsd-user: Handle short reads in mmap_h_gt_g

[Richard Henderson]

In particular, if an image has a large bss, we can hit EOF before reading
all bytes of the mapping.  Mirror the similar change to linux-user.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 bsd-user/mmap.c | 38 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
index f3a4f1712d..775e905960 100644
--- a/bsd-user/mmap.c
+++ b/bsd-user/mmap.c
@@ -128,6 +128,40 @@ error:
     return ret;
 }
 
+/*
+ * Perform a pread on behalf of target_mmap.  We can reach EOF, we can be
+ * interrupted by signals, and in general there's no good error return path.
+ * If @zero, zero the rest of the block at EOF.
+ * Return true on success.
+ */
+static bool mmap_pread(int fd, void *p, size_t len, off_t offset, bool zero)
+{
+    while (1) {
+        ssize_t r = pread(fd, p, len, offset);
+
+        if (likely(r == len)) {
+            /* Complete */
+            return true;
+        }
+        if (r == 0) {
+            /* EOF */
+            if (zero) {
+                memset(p, 0, len);
+            }
+            return true;
+        }
+        if (r > 0) {
+            /* Short read */
+            p += r;
+            len -= r;
+            offset += r;
+        } else if (errno != EINTR) {
+            /* Error */
+            return false;
+        }
+    }
+}
+
 /*
  * map an incomplete host page
  *
@@ -190,7 +224,7 @@ static int mmap_frag(abi_ulong real_start,
             mprotect(host_start, qemu_host_page_size, prot1 | PROT_WRITE);
 
         /* read the corresponding file data */
-        if (pread(fd, g2h_untagged(start), end - start, offset) == -1) {
+        if (!mmap_pread(fd, g2h_untagged(start), end - start, offset, true)) {
             return -1;
         }
 
@@ -565,7 +599,7 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
                                   -1, 0);
             if (retaddr == -1)
                 goto fail;
-            if (pread(fd, g2h_untagged(start), len, offset) == -1) {
+            if (!mmap_pread(fd, g2h_untagged(start), len, offset, false)) {
                 goto fail;
             }
             if (!(prot & PROT_WRITE)) {
-- 
2.43.0

Re: [PATCH v2 0/2] *-user: Handle short reads in mmap_h_gt_g

[Philippe Mathieu-Daudé]

On 20/8/24 07:08, Richard Henderson wrote:

> Richard Henderson (2):
>    linux-user: Handle short reads in mmap_h_gt_g
>    bsd-user: Handle short reads in mmap_h_gt_g

Series:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 0/2] *-user: Handle short reads in mmap_h_gt_g

[Warner Losh]

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

On Tue, Aug 20, 2024 at 4:34 AM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> On 20/8/24 07:08, Richard Henderson wrote:
>
> > Richard Henderson (2):
> >    linux-user: Handle short reads in mmap_h_gt_g
> >    bsd-user: Handle short reads in mmap_h_gt_g
>
> Series:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>

Series:

Reviewed-by: Warner Losh <imp@bsdimp.com>

[-- Attachment #2: Type: text/html, Size: 892 bytes --]