| Message ID | 201803281923.EFF26009.OFOtJSMFHQFLVO@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed 28-03-18 19:23:20, Tetsuo Handa wrote: > Michal Hocko wrote: > > On Tue 27-03-18 20:19:30, Tetsuo Handa wrote: > > > If the OOM victim is holding mm->mmap_sem held for write, and if the OOM > > > victim can interrupt operations which need mm->mmap_sem held for write, > > > we can downgrade mm->mmap_sem upon SIGKILL and the OOM reaper will be > > > able to reap the OOM victim's memory. > > > > This really begs for much better explanation. Why is it safe? > > Basic idea is > > bool downgraded = false; > down_write(mmap_sem); > for (something_1_that_might_depend_mmap_sem_held_for_write; > something_2_that_might_depend_mmap_sem_held_for_write; > something_3_that_might_depend_mmap_sem_held_for_write) { > something_4_that_might_depend_mmap_sem_held_for_write(); > if (fatal_signal_pending(current)) { > downgrade_write(mmap_sem); > downgraded = true; > break; > } > something_5_that_might_depend_mmap_sem_held_for_write(); > } > if (!downgraded) > up_write(mmap_sem); > else > up_read(mmap_sem); > > . That is, try to interrupt critical sections at locations where it is > known to be safe and consistent. Please explain why those places are safe to interrupt. > > Are you > > assuming that the killed task will not perform any changes on the > > address space? > > If somebody drops mmap_sem held for write is not safe, how can the OOM > reaper work safely? > > The OOM reaper is assuming that the thread who got mmap_sem held for write > is responsible to complete critical sections before dropping mmap_sem held > for write, isn't it? > > Then, how an attempt to perform changes on the address space can become a > problem given that the thread who got mmap_sem held for write is responsible > to complete critical sections before dropping mmap_sem held for write? ENOPARSE. How does this have anything to do with oom_reaper. Sure you want to _help_ the oom_reaper to do its job but you are dropping the lock in the downgrading the lock in the middle of dup_mmap and that is what we are dicussing here. So please explain why it is safe. It is really not straightforward. > > What about ongoing page faults or other operations deeper > > in the call chain. > > Even if there are ongoing page faults or other operations deeper in the call > chain, there should be no problem as long as the thread who got mmap_sem > held for write is responsible to complete critical sections before dropping > mmap_sem held for write. > > > Why they are safe to change things for the child > > during the copy? > > In this patch, the current thread who got mmap_sem held for write (which is > likely an OOM victim thread) downgrades mmap_sem, with an assumption that > current thread no longer accesses memory which might depend on mmap_sem held > for write. > > dup_mmap() duplicates current->mm and to-be-duplicated mm is not visible yet. > If dup_mmap() failed, to-be-duplicated incomplete mm is discarded via mmput() > in dup_mm() rather than assigned to the child. Thus, this patch should not > change things which are visible to the child during the copy. > > What we need to be careful is making changes to current->mm. > I'm assuming that current->mm->mmap_sem held for read is enough for > i_mmap_lock_write()/flush_dcache_mmap_lock()/vma_interval_tree_insert_after()/ > flush_dcache_mmap_unlock()/i_mmap_unlock_write()/is_vm_hugetlb_page()/ > reset_vma_resv_huge_pages()/__vma_link_rb(). But I'm not sure. But as soon as you downgrade the lock then all other threads can interfere and perform page faults or update respecive mappings. Does this matter? If not then why? > > I am not saying this is wrong, I would have to think about that much > > more because mmap_sem tends to be used on many surprising places and the > > write lock just hide them all. > > Then, an alternative approach which interrupts without downgrading is shown > below. But I'm not sure. Failing the whole dup_mmap might be quite reasonable, yes. I haven't checked your particular patch because this code path needs much more time than I can give this, though.
diff --git a/include/linux/fs.h b/include/linux/fs.h index bb45c48..2f11c55 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -468,6 +468,11 @@ static inline void i_mmap_lock_write(struct address_space *mapping) down_write(&mapping->i_mmap_rwsem); } +static inline int i_mmap_lock_write_killable(struct address_space *mapping) +{ + return down_write_killable(&mapping->i_mmap_rwsem); +} + static inline void i_mmap_unlock_write(struct address_space *mapping) { up_write(&mapping->i_mmap_rwsem); diff --git a/kernel/fork.c b/kernel/fork.c index 1e8c9a7..c9c141d 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -473,10 +473,21 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm, struct inode *inode = file_inode(file); struct address_space *mapping = file->f_mapping; + if (i_mmap_lock_write_killable(mapping)) { + /* + * Pretend that this is not a file mapping, for + * dup_mm() will after all discard this mm due + * to fatal_signal_pending() check below. But + * make sure not to call open()/close() hook + * which might expect tmp->vm_file != NULL. + */ + tmp->vm_file = NULL; + tmp->vm_ops = NULL; + goto skip_file; + } get_file(file); if (tmp->vm_flags & VM_DENYWRITE) atomic_dec(&inode->i_writecount); - i_mmap_lock_write(mapping); if (tmp->vm_flags & VM_SHARED) atomic_inc(&mapping->i_mmap_writable); flush_dcache_mmap_lock(mapping); @@ -486,6 +497,7 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm, flush_dcache_mmap_unlock(mapping); i_mmap_unlock_write(mapping); } +skip_file: /* * Clear hugetlb-related page reserves for children. This only @@ -508,7 +520,13 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm, rb_parent = &tmp->vm_rb; mm->map_count++; - if (!(tmp->vm_flags & VM_WIPEONFORK)) + /* + * Bail out as soon as possible, for dup_mm() will after all + * discard this mm by returning an error. + */ + if (fatal_signal_pending(current)) + retval = -EINTR; + else if (!(tmp->vm_flags & VM_WIPEONFORK)) retval = copy_page_range(mm, oldmm, mpnt); if (tmp->vm_ops && tmp->vm_ops->open)