On Dec 19 18:36, Ken Brown wrote:
> Hi Corinna,
>
> I plan to keep working on mmap issues while you're on vacation. A few
> questions have come up while I've been looking at the code.
>
> 1. The function is_mmapped_region() doesn't seem to be used anywhere. Is it
> OK to remove it?
Yes. Commit 29a126322783 ("Simplify stack allocation code in child
after fork") dropped its only usage. I just forgot to remove
is_mmapped_region() as well.
> 2. I can't find any uses of filler pages, in spite of the comment preceding
> the definition of __PROT_FILLER. Is it OK to remove all references to
> __PROT_FILLER and the associated methods?
Yes. The filler pages were only used on 32 bit Windows. This code should
have been removed when we we're going 64 bit-only.
> 3. I'm still puzzled about mmap_is_attached_or_noreserve(). See my email
> from yesterday on the cygwin ML. I think it's broken but I may be wrong.
> If I'm right, I don't see an easy way to fix it. As I said before, the only
> thing I can think of is to sort the mmap_list of anonymous mappings. But
> I'm afraid that would be too expensive (in terms of time).
I copy/pasted from the cygwin ML:
===================
> I think I'm seeing a similar confusion in
> mmap_is_attached_or_noreserve(). I'm tired now and am having trouble
> sorting out exactly what that function is doing.
noreserve() pages are MAP_RESERVE pages. Trying to read or write on them
raises a STATUS_ACCESS_VIOLATION. The idea is basically to commit the
requested read/write region (in case of the signal: just 1 byte, which in
turn requires to commit a full page.
> But the definition of
> commit_len looks suspicious to me. We know from above that start_addr <=
> u_addr.
We do? match() returns the intersection, independently of addr being
lower or higher than get_address(). Same goes for the length.
In case of a signal:
start_addr is addr rounded down to the page address and len is rounded
up to the pagesize. So we have a single page. rec->match() either
returns false, or if a noreserve() record matches, it will return the
single page in u_addr, u_len.
commit_len = 4096 - (0x100000000 - 0x100000000) = 4096
In case we're called via fhandler_base::raw_read():
Assuming the region to read is enclosing the noreserve() region...
- addr = 0x100000000, len = 256K
- rec->get_address() = 0x100010000, len = 64K
--> start_addr = addr = 0x100000000
len = 256K
rec->match() returns
u_addr = 0x100010000, u_len = 64K
commit_len = 64K - (0x100000000 - 0x100010000)
= 64K - 0x0xFFFFFFFFFFFF0000
= 128K
Note: commit_len is size_t, so it's unsigned!
if (commit_len (128K) > len (256K))? Nope!
--> VirtualAlloc (0x100000000, 128K, MEM_COMMIT, rec->gen_protect ());
If this VirtualAlloc fails, the process is screwed and gets a
well-deserved SIGBUS.
I'm not saying that there's a chance I'm missing something, but
as of now, it looks ok to me.
> The only thing I can come up with immediately is that we should sort the list
> of mmap_records in order of their starting addresses. Then if start_addr ==
> u_addr, we commit u_len bytes, otherwise we fail.
I don't understand this one. The code loops over all records,
so where's the problem?
Thanks,
Corinna
