On 12/28/2024 4:40 PM, Ken Brown wrote:
Patch attached.
I'm not sure I handled the noreserve case in the best possible way, but
at least I didn't make it worse. The behavior in that case after my
patch is the same as before.
Ken
P.S. If no one has any comments in the next week or so, I might just go
ahead and push it (to main only), so it can get some testing. Corinna
has already said that's OK in https://cygwin.com/pipermail/cygwin-
developers/2024-December/012723.html
v2 is attached. The only difference is that I clarified in the log
message and the release note that this affects only the MAP_FIXED case.
Ken
From 625c77a82925185805ad57d5ef3f0d0d90dc9b57 Mon Sep 17 00:00:00 2001
From: Ken Brown <kbr...@cornell.edu>
Date: Fri, 27 Dec 2024 16:09:40 -0500
Subject: [PATCH v2] Cygwin: mmap: allow remapping part of an existing
anonymous mapping
Previously mmap with MAP_FIXED would fail with EINVAL on an attempt to
map an address range contained in the chunk of an existing mapping.
With this commit, mmap will succeed, provided the mappings are
anonymous, the MAP_SHARED/MAP_PRIVATE flags agree, and MAP_NORESERVE
is not set for either mapping.
Addresses: https://cygwin.com/pipermail/cygwin/2024-December/256901.html
Signed-off-by: Ken Brown <kbr...@cornell.edu>
---
winsup/cygwin/mm/mmap.cc | 45 ++++++++++++++++++++++---------------
winsup/cygwin/release/3.6.0 | 6 +++++
2 files changed, 33 insertions(+), 18 deletions(-)
diff --git a/winsup/cygwin/mm/mmap.cc b/winsup/cygwin/mm/mmap.cc
index fc126a87072a..0224779458ef 100644
--- a/winsup/cygwin/mm/mmap.cc
+++ b/winsup/cygwin/mm/mmap.cc
@@ -494,18 +494,24 @@ mmap_record::map_pages (caddr_t addr, SIZE_T len, int
new_prot)
off_t off = addr - get_address ();
off /= wincap.page_size ();
len = PAGE_CNT (len);
- /* First check if the area is unused right now. */
- for (SIZE_T l = 0; l < len; ++l)
- if (MAP_ISSET (off + l))
- {
- set_errno (EINVAL);
- return false;
- }
- if (!noreserve ()
- && !VirtualProtect (get_address () + off * wincap.page_size (),
- len * wincap.page_size (),
- ::gen_protect (new_prot, get_flags ()),
- &old_prot))
+ /* VirtualProtect can only be called on committed pages, so it's not
+ clear how to change protection in the noreserve case. In this
+ case we will therefore require that the pages are unmapped, in
+ order to keep the behavior the same as it was before new_prot was
+ introduced. FIXME: Is there a better way to handle this? */
+ if (noreserve ())
+ {
+ for (SIZE_T l = 0; l < len; ++l)
+ if (MAP_ISSET (off + l))
+ {
+ set_errno (EINVAL);
+ return false;
+ }
+ }
+ else if (!VirtualProtect (get_address () + off * wincap.page_size (),
+ len * wincap.page_size (),
+ ::gen_protect (new_prot, get_flags ()),
+ &old_prot))
{
__seterrno ();
return false;
@@ -625,8 +631,9 @@ mmap_list::try_map (void *addr, size_t len, int new_prot,
int flags, off_t off)
if (off == 0 && !fixed (flags))
{
- /* If MAP_FIXED isn't given, check if this mapping matches into the
- chunk of another already performed mapping. */
+ /* If MAP_FIXED isn't given, try to satisfy this mapping request
+ by recycling unmapped pages in the chunk of an existing
+ mapping. */
SIZE_T plen = PAGE_CNT (len);
LIST_FOREACH (rec, &recs, mr_next)
if (rec->find_unused_pages (plen) != (SIZE_T) -1)
@@ -640,9 +647,10 @@ mmap_list::try_map (void *addr, size_t len, int new_prot,
int flags, off_t off)
}
else if (fixed (flags))
{
- /* If MAP_FIXED is given, test if the requested area is in an
- unmapped part of an still active mapping. This can happen
- if a memory region is unmapped and remapped with MAP_FIXED. */
+ /* If MAP_FIXED is given, test if the requested area is
+ contained in the chunk of an existing mapping. If so, and if
+ the flags of that mapping are compatible with those in the
+ request, try to reset the protection on the requested area. */
caddr_t u_addr;
SIZE_T u_len;
@@ -1061,7 +1069,8 @@ go_ahead:
LIST_WRITE_LOCK ();
map_list = mmapped_areas.get_list_by_fd (fd, &st);
- /* Test if an existing anonymous mapping can be recycled. */
+ /* Try to satisfy the request by resetting the protection on part of
+ an existing anonymous mapping. */
if (map_list && anonymous (flags))
{
caddr_t tried = map_list->try_map (addr, len, prot, flags, off);
diff --git a/winsup/cygwin/release/3.6.0 b/winsup/cygwin/release/3.6.0
index 4b7604907902..171ed15e146a 100644
--- a/winsup/cygwin/release/3.6.0
+++ b/winsup/cygwin/release/3.6.0
@@ -78,3 +78,9 @@ What changed:
- Raise maximum pid from 65536 to 4194304 to account for scenarios
with lots of CPUs and lots of tasks.
Addresses: https://cygwin.com/pipermail/cygwin/2024-December/256927.html
+
+- Allow mmap with MAP_FIXED to succeed on an address range contained
+ in the chunk of an existing anonymous mapping, provided the
+ MAP_SHARED/MAP_PRIVATE flags agree and MAP_NORESERVE is not set for
+ either mapping.
+ Addresses: https://cygwin.com/pipermail/cygwin/2024-December/256901.html
--
2.45.1
