VM-related fixes

Signed-off-by Al Viro <[email protected]>

---
 mm/hugetlb.c |    2 +-

 1 files changed, 1 insertions(+), 1 deletions(-)

mm/hugetlb.c +1 -2

index 5f34bd8..a876871 100644
@@ -2277,8 +2277,8 @@ void __unmap_hugepage_range(struct vm_area_struct *vma, unsigned long start,
                        set_page_dirty(page);
                list_add(&page->lru, &page_list);
        }
-       spin_unlock(&mm->page_table_lock);
        flush_tlb_range(vma, start, end);
+       spin_unlock(&mm->page_table_lock);
        mmu_notifier_invalidate_range_end(mm, start, end);
        list_for_each_entry_safe(page, tmp, &page_list, lru) {
                page_remove_rmap(page);
--

1.7.2.5

Is this safe? And why does it need it? Please add more explanations.

Quoted 1 line · Linus Torvalds

Is this safe? And why does it need it? Please add more explanations.

a) safety - as the matter of fact, all other callers either hold either ->mmap_sem (exclusive) or ->page_table_lock. flush_tlb_range() is called under ->page_table_lock in a lot of places, e.g. page_referenced_one() -> pmdp_clear_flush_young_notify() -> -> pmdp_clear_flush_young() -> flush_tlb_range(), with

                /* go ahead even if the pmd is pmd_trans_splitting() */
                if (pmdp_clear_flush_young_notify(vma, address, pmd))
                        referenced++;
                spin_unlock(&mm->page_table_lock);

in page_referenced_one().

b) there are instances that work with page tables. See e.g. arch/powerpc/mm/tlb_hash32.c, flush_tlb_range() and flush_range() in there. The same goes for uml, with a lot more extensive playing with page tables.

Almost all callers are actually fine - flush_tlb_range() may have no need to bother playing with page tables, but it can do so safely; again, this caller is the sole exception - everything else either has exclusive ->mmap_sem on the mm in question, or mm->page_table_lock is held.

       I'd been crawling through VMA-related code for the last couple of weeks; the obvious parts of fallout are in followups and IMO they need to go into -stable as well; there's more, including some bugs I don't know what to do about and patches that need more testing...

[unsolved] binder is insane, even more than usual for drivers/staging. It stores a reference to mm at open() time,

then it stores a reference to
vma at mmap() time, then it cheerfully works with that ->vma assuming that ->mmap_sem on stored reference to ->mm would suffice.  Guess what happens if somebody opens it and then forks and does mmap in child?

 Moreover, if
we fork() and have child exit(), we get ->close() called on each VMA we'd copied into child.

 Since they have stored reference to vma invalidated by ->close(), that has unpleasant side effects, to put it mildly.

 And yes,
I realize that android userland probably doesn't do anything of that kind; fat lot of good it does us...

Quoted 2 lines

[unsolved] binder is insane, even more than usual for drivers/staging. It stores a reference to mm at open() time,

It stores a reference to the task struct, not the mm.

Quoted 4 lines

then it stores a reference to
vma at mmap() time, then it cheerfully works with that ->vma assuming that ->mmap_sem on stored reference to ->mm would suffice. ?Guess what happens if somebody opens it and then forks and does mmap in child?

mmap succeeds, but binder_update_page_range will fail when trying to create a transaction? (assuming you are talking about the current version of the driver)

Quoted 3 lines

?Moreover, if
we fork() and have child exit(), we get ->close() called on each VMA we'd copied into child.

The driver sets VM_DONTCOPY which should avoid this.

Quoted 2 lines

?Since they have stored reference to vma invalidated by ->close(), that has unpleasant side effects, to put it mildly.

The side effect of ->close() should be the same as when the process that opened the driver exits. In other words, the process that misbehaved is considered dying. It does not however send death notifications until the driver is closed, so other processes do not notice until they try to talk to it.

Quoted 4 lines

?And yes,
I realize that android userland probably doesn't do anything of that kind; fat lot of good it does us...

What usage case do you have in mind? The binder driver uses the file pointer as the binder process identifier, so you need to open and mmap the driver in every process that want to use the driver. While it is not common to fork a process in android, the driver should work as expected if you do (assuming each process opens and mmaps the driver on its own and don't share the file).

Quoted 4 lines · Arve Hj?nnev?g

Quoted 2 lines

[unsolved] binder is insane, even more than usual for drivers/staging. It stores a reference to mm at open() time,

It stores a reference to the task struct, not the mm.

Equivalent, for our purposes.  OK, almost - strictly speaking that avoids some extra nasty scenarios with open/exec/mmap, but the real PITA comes from much simpler open/fork/mmap in child anyway...

Quoted 8 lines

Quoted 4 lines

then it stores a reference to
vma at mmap() time, then it cheerfully works with that ->vma assuming that ->mmap_sem on stored reference to ->mm would suffice. ?Guess what happens if somebody opens it and then forks and does mmap in child?

mmap succeeds, but binder_update_page_range will fail when trying to create a transaction? (assuming you are talking about the current version of the driver)

Sorry, no.
               down_write(&mm->mmap_sem);
               vma = proc->vma;
               if (vma && mm != vma->vm_mm) {
does *not* do what you seem to describe; there's nothing to protect you from proc->vma getting freed under you right between load from proc->vma and check of vma->mm.  ->mmap_sem on the right mm would prevent that, but this one doesn't guarantee anything.  Get preempted after the second line quoted above and by the time you get the timeslice back, you might have had munmap() done by another thread, with vma freed, its memory recycled, etc.

Quoted 5 lines

Quoted 3 lines

?Moreover, if
we fork() and have child exit(), we get ->close() called on each VMA we'd copied into child.

The driver sets VM_DONTCOPY which should avoid this.

Umm...  Point taken - missed that.

Quoted 14 lines

Quoted 2 lines

?Since they have stored reference to vma invalidated by ->close(), that has unpleasant side effects, to put it mildly.

The side effect of ->close() should be the same as when the process that opened the driver exits. In other words, the process that misbehaved is considered dying. It does not however send death notifications until the driver is closed, so other processes do not notice until they try to talk to it.

Quoted 4 lines

?And yes,
I realize that android userland probably doesn't do anything of that kind; fat lot of good it does us...

Quoted 6 lines

What usage case do you have in mind? The binder driver uses the file pointer as the binder process identifier, so you need to open and mmap the driver in every process that want to use the driver. While it is not common to fork a process in android, the driver should work as expected if you do (assuming each process opens and mmaps the driver on its own and don't share the file).

Except that we can't make that assumption...

Quoted 12 lines

Sorry, no.
? ? ? ? ? ? ? ?down_write(&mm->mmap_sem);
? ? ? ? ? ? ? ?vma = proc->vma;
? ? ? ? ? ? ? ?if (vma && mm != vma->vm_mm) {
does *not* do what you seem to describe; there's nothing to protect you from proc->vma getting freed under you right between load from proc->vma and check of vma->mm. ?->mmap_sem on the right mm would prevent that, but this one doesn't guarantee anything. ?Get preempted after the second line quoted above and by the time you get the timeslice back, you might have had munmap() done by another thread, with vma freed, its memory recycled, etc.

OK, if the memory got freed and then re-used by someone who stored a value that matched a pointer to the mm struct that was just locked, this check will fail to catch it. I can check against a cached vm_mm member from mmap instead, assuming this will not change before ->close() is called. Does that sound reasonable, or is there a better way to check this?

Quoted 13 lines · Arve Hj?nnev?g

Quoted 12 lines

Sorry, no.
? ? ? ? ? ? ? ?down_write(&mm->mmap_sem);
? ? ? ? ? ? ? ?vma = proc->vma;
? ? ? ? ? ? ? ?if (vma && mm != vma->vm_mm) {
does *not* do what you seem to describe; there's nothing to protect you from proc->vma getting freed under you right between load from proc->vma and check of vma->mm. ?->mmap_sem on the right mm would prevent that, but this one doesn't guarantee anything. ?Get preempted after the second line quoted above and by the time you get the timeslice back, you might have had munmap() done by another thread, with vma freed, its memory recycled, etc.

Quoted 6 lines

OK, if the memory got freed and then re-used by someone who stored a value that matched a pointer to the mm struct that was just locked, this check will fail to catch it. I can check against a cached vm_mm member from mmap instead, assuming this will not change before ->close() is called. Does that sound reasonable, or is there a better way to check this?

Huh?  Sorry, I hadn't been able to parse that - what do you want to cache,

where and what do you want to check?  Again, at that point *(proc->vma) may very well be random garbage, so looking at it would be pointless;

the value you had at ->mmap() time would be simply current->mm of mmap(2) caller; if you want to check that it matches that of opener, fine, but then why not do just that in ->mmap()?