Re: [Libunwind-devel] mincore_validate fails sometimes

[Daniel Vassdal]

Sorry for the late reply.

> Yes, it's meant to check that the pages are mapped in.  Do you think the 
> above code is incorrect?
Yes, I think that it is incorrect, as the mincore() result array doesn't 
indicate if the page is mapped or not, the return code does.

...

> I think this means ENOMEM is returned only if *all* pages are unmapped, but 
> the vector must be checked if only some of the pages are unmapped.
This is wrong. If we consult the kernel source code, we see that for each page 
do_mincore() is called.
https://github.com/torvalds/linux/blob/master/mm/mincore.c#L251-L270

Here it is checked that the page is indeed mapped. If any page in the range 
isn't, ENOMEM is returned.
https://github.com/torvalds/linux/blob/master/mm/mincore.c#L189-L196

And thus, if the point is to check that the pages are mapped and valid, 
checking only the return value of mincore() is sufficient.
Checking the result array is wrong unless we really care if the memory is 
currently in core or not.
And even then it is dubious, as, according to the documentation, the result may 
be stale even before the function has returned, unless the memory is mlocked 
into place. And if we know it is, why are we checking?
https://github.com/torvalds/linux/blob/master/mm/mincore.c#L209-L212


- DV

-----Original Message-----
From: Dave Watson [mailto:address@hidden 
Sent: 17 January 2018 16:59
To: Daniel Vassdal <address@hidden>
Cc: address@hidden
Subject: Re: [Libunwind-devel] mincore_validate fails sometimes

On 01/17/18 09:28 AM, Daniel Vassdal wrote:
> Hi Dave,
> Thanks for your reply.
> 
> Maybe I wasn't being entirely clear; the call to mincore() itself succeeded, 
> i.e. returned 0.
> However, when the result array is checked, one or more of the pages are not 
> in memory.

Ah, ok.

> If we have a look at commit bc8698fd, part of the commit comment says:
> > we should also check that the pages are mapped, through the passed 
> > mvec array. This patch also adds this verification.
> 
> I think the code that part of the code the comment refers to is this:
> >  for (i = 0; i < (len + PAGE_SIZE - 1) / PAGE_SIZE; i++)
> >   {
> >     if (!(mvec[i] & 1)) return -1;
> >   }
> 
> I'm not sure this is correct. According to the man page for mincore:
> > mincore() returns a vector that indicates whether pages of the 
> > calling process's virtual memory are resident in core (RAM), and so  
> > will not cause a disk access (page fault) if referenced.
> 
> Is this what the author intended to check?

Yes, it's meant to check that the pages are mapped in.  Do you think the above 
code is incorrect? 

> 
> Furthermore, the man page states hat mincore() can return
> > ENOMEM addr to addr + length contained unmapped memory.
> 
> Thus it looks to me as if mincore() already returns an error code for what 
> the comment says the block above is checking for.
> This is a little out of my comfort zone so I may be talking nonsense, but 
> maybe have a look at it?

I think this means ENOMEM is returned only if *all* pages are unmapped, but the 
vector must be checked if only some of the pages are unmapped.