Re: [PATCH] mremap: enforce rmap src/dst vma ordering in case ofvma_merge succeeding in copy_vma

[Nai Xia]

On Fri, Nov 18, 2011 at 10:17 AM, Andrea Arcangeli <aarcange@xxxxxxxxxx> wrote:
> On Fri, Nov 18, 2011 at 09:42:05AM +0800, Nai Xia wrote:
>> First of all, I believe that at the POSIX level, it's ok for
>> truncate_inode_page()
>> not scanning  COWed pages, since basically we does not provide any guarantee
>> for privately mapped file pages for this behavior. But missing a file
>> mapped pte after its
>> cache page is already removed from the the page cache is a
>
> I also exclude there is a case that would break, but it's safer to
> keep things as is, in case somebody depends on segfault trapping.
>
>> fundermental malfuntion for
>> a shared mapping when some threads see the file cache page is gone
>> while some thread
>> is still r/w from/to it! No matter how short the gap between
>> truncate_inode_page() and
>> the second loop, this is wrong.
>
> Truncate will destroy the info on disk too... so if somebody is
> writing to a mapping which points beyond the end of the i_size
> concurrently with truncate, the result is undefined. The write may
> well reach the page but then the page is discared. Or you may get
> SIGBUS before the write.
>
>> Second, even if the we don't care about this POSIX flaw that may
>> introduce, a pte can still
>> missed by the second loop. mremap can happen serveral times during
>> these non-atomic
>> firstpass-trunc-secondpass operations, a proper events can happily
>> make the wrong order
>> for every scan, and miss them all -- That's just what in Hugh's mind
>> in the post you just
>> replied. Without lock and proper ordering( which patial mremap cannot provide),
>> this *will* happen.
>
> There won't be more than one mremap running concurrently from the same
> process (we must enforce it by making sure anon_vma lock and
> i_mmap_lock are both taken at least once in copy_vma, they're already
> both taken in fork, they should already be taken in all common cases
> in copy_vma so for all cases it's going to be a L1 exclusive cacheline
> already). I don't exclude there may be some case that won't take the
> locks in vma_adjust though, we should check it, if we decide to relay
> on the double loop, but it'd be a simple addition if needed.

I mean it's not the concurrent mremap, it's mremap() can be done several
times between these 3-stage scans, since we don't take the mmap_sem
of the scanned VMAs, they are valid to do so. And without proper ordering
and locks/mutex it's possible for these 3-stage scans racing with these
mremap() s and a ghost PTE just jumps back and force and misses all
these scans.

>
> I'm more concerned about the pte pointing to the orphaned pagecache
> that would materialize for a little while because of
> unmap+truncate+unmap instead of unmap+unmap+truncate (but the latter
> order is needed for the COWs).
>
>> You may disagree with me and have that locking removed, and I am
>> already have that
>> one line patch prepared waiting fora bug bumpping up again, what a
>> cheap patch submission!
>
> Well I'm not yet sure it's good idea to remove the i_mmap_mutex, or if
> we should just add the anon_vma lock in mremap and add the i_mmap_lock
> in fork (to avoid the orphaned pagecache left mapped in the child
> which already may happen unless there's some i_mmap_lock belonging to
> the same inode taken after copy_page_range returns until we return to
> userland and child can run, and I don't think we can relay on the
> order of the prio tree in fork. Fork is safe for anon pages because
> there we can relay on the order of the same_anon_vma list.
>
> I think clearing up if this orphaned pagecache is dangerous would be a
> good start. If too complex we just add the i_mmap_lock around
> copy_page_range in fork if vma->vm_file is set. If you instead think
> we can deal with the orphaned pagecache we can add a dummy lock/unlock
> of i_mmap_mutex in copy_vma vma_merge succeeding case (short critical
> section and not common common case) and remove the i_mmap_mutex around
> move_page_tables (common case) overall speeding up mremap and not
> degrading fork.
>

I am actually feel comfortable either direction you take :)

But I do think orphaned pagecache is not a good idea,
don't you see there is a "BUG_ON(page_mapped(page))"
in __delete_from_page_cache()? Do you really plan to
remove this line?

Nai
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/