diff mbox series

[v2,7/9] bsd-user/mmap.c: Don't mmap fd == -1 independently from MAP_ANON flag

Message ID 20210922045636.25206-8-imp@bsdimp.com
State New
Headers show
Series bsd-user mmap fixes | expand

Commit Message

Warner Losh Sept. 22, 2021, 4:56 a.m. UTC 
From: Guy Yur <guyyur@gmail.com>

Switch checks for !(flags & MAP_ANONYMOUS) with checks for fd != -1.
MAP_STACK and MAP_GUARD both require fd == -1 and don't require mapping
the fd either.

Signed-off-by: Guy Yur <guyyur@gmail.com>
[ partially merged before, finishing the job and documenting origin]
Signed-off-by: Warner Losh <imp@bsdimp.com>
---
 bsd-user/mmap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Richard Henderson Sept. 23, 2021, 5:43 p.m. UTC | #1 
On 9/21/21 9:56 PM, Warner Losh wrote:
>           /* no page was there, so we allocate one */
>           void *p = mmap(host_start, qemu_host_page_size, prot,
> -                       flags | MAP_ANON, -1, 0);
> +                       flags | ((fd != -1) ? MAP_ANON : 0), -1, 0);

I don't understand this change, given that the actual fd passed is always -1.


r~
Warner Losh Sept. 26, 2021, 5:08 p.m. UTC | #2 
On Fri, Sep 24, 2021 at 6:00 AM Richard Henderson <
richard.henderson@linaro.org> wrote:

> On 9/21/21 9:56 PM, Warner Losh wrote:
> >           /* no page was there, so we allocate one */
> >           void *p = mmap(host_start, qemu_host_page_size, prot,
> > -                       flags | MAP_ANON, -1, 0);
> > +                       flags | ((fd != -1) ? MAP_ANON : 0), -1, 0);
>
> I don't understand this change, given that the actual fd passed is always
> -1.
>

That's a very good question. I'll have to trace down why that was made
because
I'm having trouble with it as well now that I'm trying to defend it.

Warner


>
> r~
>
Guy Yur Sept. 26, 2021, 7:07 p.m. UTC | #3 
On 26/9/21 20:08, Warner Losh wrote:
>
>
> On Fri, Sep 24, 2021 at 6:00 AM Richard Henderson 
> <richard.henderson@linaro.org> wrote:
>
>     On 9/21/21 9:56 PM, Warner Losh wrote:
>     >           /* no page was there, so we allocate one */
>     >           void *p = mmap(host_start, qemu_host_page_size, prot,
>     > -                       flags | MAP_ANON, -1, 0);
>     > +                       flags | ((fd != -1) ? MAP_ANON : 0), -1, 0);
>
>     I don't understand this change, given that the actual fd passed is
>     always -1.
>
>
> That's a very good question. I'll have to trace down why that was made 
> because
> I'm having trouble with it as well now that I'm trying to defend it.
>
mmap_frag can be called with a valid fd, if flags doesn't contain one of 
MAP_ANON, MAP_STACK, MAP_GUARD.
The passed fd to mmap is -1 but if flags contains MAP_GUARD then 
MAP_ANON cannot be added.

* If fd is valid (not -1) we want to map the pages with MAP_ANON.
* If flags contains MAP_GUARD we don't want to add MAP_ANON because it 
will be rejected.
https://github.com/freebsd/freebsd-src/blob/master/sys/vm/vm_mmap.c#L302
* If flags contains MAP_ANON it doesn't matter if we add it or not.
* If flags contains MAP_STACK, mmap adds MAP_ANON when called so doesn't 
matter if we add it or not either.
https://github.com/freebsd/freebsd-src/blob/master/sys/vm/vm_mmap.c#L284

The intention was to not pass MAP_ANON for the flags that use fd == -1 
without specifying the flags directly,
with the assumption that future flags that don't require fd will also 
not require MAP_ANON.
Changing to !(flags & MAP_GUARD) will also work.

Guy Yur

> Warner
>
>
>     r~
>
Warner Losh Sept. 27, 2021, 12:17 a.m. UTC | #4 
On Sun, Sep 26, 2021 at 1:07 PM Guy Yur <guyyur@gmail.com> wrote:

> On 26/9/21 20:08, Warner Losh wrote:
> >
> >
> > On Fri, Sep 24, 2021 at 6:00 AM Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >
> >     On 9/21/21 9:56 PM, Warner Losh wrote:
> >     >           /* no page was there, so we allocate one */
> >     >           void *p = mmap(host_start, qemu_host_page_size, prot,
> >     > -                       flags | MAP_ANON, -1, 0);
> >     > +                       flags | ((fd != -1) ? MAP_ANON : 0), -1,
> 0);
> >
> >     I don't understand this change, given that the actual fd passed is
> >     always -1.
> >
> >
> > That's a very good question. I'll have to trace down why that was made
> > because
> > I'm having trouble with it as well now that I'm trying to defend it.
> >
> mmap_frag can be called with a valid fd, if flags doesn't contain one of
> MAP_ANON, MAP_STACK, MAP_GUARD.
> The passed fd to mmap is -1 but if flags contains MAP_GUARD then
> MAP_ANON cannot be added.
>
> * If fd is valid (not -1) we want to map the pages with MAP_ANON.
> * If flags contains MAP_GUARD we don't want to add MAP_ANON because it
> will be rejected.
> https://github.com/freebsd/freebsd-src/blob/master/sys/vm/vm_mmap.c#L302
> * If flags contains MAP_ANON it doesn't matter if we add it or not.
> * If flags contains MAP_STACK, mmap adds MAP_ANON when called so doesn't
> matter if we add it or not either.
> https://github.com/freebsd/freebsd-src/blob/master/sys/vm/vm_mmap.c#L284
>
> The intention was to not pass MAP_ANON for the flags that use fd == -1
> without specifying the flags directly,
> with the assumption that future flags that don't require fd will also
> not require MAP_ANON.
> Changing to !(flags & MAP_GUARD) will also work.
>

Thanks Guy. that fills in the missing pieces for me of the range of
possibilities
for calling. I've added it as a comment since it's tricky enough I've had
to ask
twice and Richard asked as well :). It will be in the next spin of the mmap
series.


> Guy Yur
>
> > Warner
> >
> >
> >     r~
> >
>
diff mbox series

Patch

diff --git a/bsd-user/mmap.c b/bsd-user/mmap.c
index 8b763fffc3..347d314aa9 100644
--- a/bsd-user/mmap.c
+++ b/bsd-user/mmap.c
@@ -154,7 +154,7 @@  static int mmap_frag(abi_ulong real_start,
     if (prot1 == 0) {
         /* no page was there, so we allocate one */
         void *p = mmap(host_start, qemu_host_page_size, prot,
-                       flags | MAP_ANON, -1, 0);
+                       flags | ((fd != -1) ? MAP_ANON : 0), -1, 0);
         if (p == MAP_FAILED)
             return -1;
         prot1 = prot;
@@ -162,7 +162,7 @@  static int mmap_frag(abi_ulong real_start,
     prot1 &= PAGE_BITS;
 
     prot_new = prot | prot1;
-    if (!(flags & MAP_ANON)) {
+    if (fd != -1) {
         /* msync() won't work here, so we return an error if write is
            possible while it is a shared mapping */
         if ((flags & TARGET_BSD_MAP_FLAGMASK) == MAP_SHARED &&
@@ -571,7 +571,7 @@  abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
          * worst case: we cannot map the file because the offset is not
          * aligned, so we read it
          */
-        if (!(flags & MAP_ANON) &&
+        if (fd != -1 &&
             (offset & ~qemu_host_page_mask) != (start & ~qemu_host_page_mask)) {
             /*
              * msync() won't work here, so we return an error if write is