[PATCH] Cygwin: mmap: allow remapping part of an existing anonymous, mapping

[Ken Brown]

Hi Corinna,

On 1/7/2025 3:18 PM, Corinna Vinschen wrote:
> On Jan  2 16:42, Ken Brown wrote:
>> 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;
>> +	  }
>> +    }
> 
> I think this is ok for the time being.  But since you're asking...

OK, I'll go ahead and push this, and then...

> When we talked about this last month, I already felt that the
> implementation is lacking.  Actually, it's missing two things which
> would improve MAP_NORESERVE mappings considerably:
> 
> 
> - mmap_record::prot flag, should be an array of protection bits per page
>    (POSIX page i e., 64K, not Windows page).  Right now we only store the
>    first protection set at mmap time for the entire map, rather than the
>    requested protection of every single page.  Consider this scenario:
> 
>      addr = mmap (NULL, 4 * PAGESIZE, PROT_READ | PROT_WRITE,
> 		 MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE, -1, 0L);
> 
>      /* At this point, mmap_record::prot is PROT_READ | PROT_WRITE */
> 
>      mprotect (addr + 2 * PAGESIZE, PAGESIZE, PROT_READ);
> 
>      /* At this point, mmap_record::prot *still* is PROT_READ | PROT_WRITE */
> 
>      /* This write to the memory region will commit page 3... */
>      addr[2 * PAGE_SIZE + 42] = 1;
> 
>      /* ...but should have raised a SIGSEGV because the page is supposedly
>         non-writable! */
> 
>    The reason is obvious: We only store the start protection PROT_READ |
>    PROT_WRITE.  So if we access the page, mmap_is_attached_or_noreserve()
>    calls VirtualAlloc() with the start protection bits.
>    If we store the protection per page, mmap_is_attached_or_noreserve()
>    can call VirtualAlloc with the correct page protection and we receive
>    the deserved SIGSEGV.
> 
> 
> - For mprotect() it doesn't in fact matter if a page is MAP_NORESERVE or
>    not.  It only matters if the page has been written to (that is, it has
>    been committed) or not (that is, it's still reserved).
> 
>    If the page is still only reserved map_pages() can just change the
>    protection bits in mmap_record::prot[page].  If the page was already
>    commited, it can additionally call VirtualProtect() with the new
>    per-page protection.
> 
>    We don't even need to keep track of reserved vs. commited, because
>    VirtualQuery() already does that for us.
I'll try to implement this idea.  Thanks for the suggestion.

Ken