[lkml]   [2010]   [Mar]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH] proc: pagemap: Hold mmap_sem during page walk

On Wed, 31 Mar 2010, San Mehat wrote:
> If the mmap_sem is not held while we walk_page_range(), then
> it is possible for find_vma() to race with a remove_vma_list()
> caused by do_munmap() (or others).

I think you've found a bug, but I also look at that code and say "that's
just totally insane".

Why does it do that initial "get_user_pages()" at all? It never _uses_
that 'pages' array except to mark the pages dirty, but that's insane,
since as far as I can see the way it actually dirties the pages in
question is by doing a regular "put_user(pfn, pm->out);". And that will
dirty the pages in hardware (or put_user).

Also, I get the feeling that the _reason_ it is not doing that down_read()
is that it would dead-lock the whole system, exactly on that "put_user()",
if somebody else did a down_write() in another thread. In that case you

thread#1 thread#2
-------- --------

down_write() - blocks
.. page fault ..
down_read(); **DEADLOCK **

because our down_read() tries to be fair to the down_write().

So I think your patch would just create _different_ trouble.

I get the _feeling_ that the whole point of that 'pages' array was to not
do that put_user() at all, but write to the physical pages through that
array. But the code looks totally buggy.

I would seriously suggest that we consider removing the 'pagemap'
interface. The way that code looks, it's just broken.

Matt - give me a reason (which includes either a patch to fix this sh*t up
or telling me why I'm wrong, but _also_ includes a real independent reason
to keep that thing around regardless) to not remove it all.

The whole notion seems to be utterly misdesigned.


 \ /
  Last update: 2010-03-31 20:01    [W:0.044 / U:6.928 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site