change mmap_sem taken for write killable

From: Michal Hocko <[email protected]>

This is the first step in making mmap_sem write holders killable. It

focuses on the trivial ones which are taking the lock early after entering the syscall and they are not changing state before.

Therefore it is very easy to change them to use down_write_killable and immediately return with -EINTR. This will allow the waiter to pass away without blocking the mmap_sem which might be required to make a forward progress. E.g. the oom reaper will need the lock for reading to dismantle the OOM victim address space.

The only tricky function in this patch is vm_mmap_pgoff which has many call sites via vm_mmap. To reduce the risk keep vm_mmap with the original non-killable semantic for now.

vm_munmap callers do not bother checking the return value so open code it into the munmap syscall path for now for simplicity.

Cc Mel Gorman <[email protected]>

Cc "Kirill A. Shutemov" <[email protected]>

Cc Konstantin Khlebnikov <[email protected]>

Cc Hugh Dickins <[email protected]>

Cc Andrea Arcangeli <[email protected]>

Cc David Rientjes <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Johannes Weiner <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

Quoted 3 lines · Michal Hocko

From: Michal Hocko <[email protected]>

This is the first step in making mmap_sem write holders killable. It

s/holders/waiters/?

Quoted 16 lines

focuses on the trivial ones which are taking the lock early after entering the syscall and they are not changing state before.

Therefore it is very easy to change them to use down_write_killable and immediately return with -EINTR. This will allow the waiter to pass away without blocking the mmap_sem which might be required to make a forward progress. E.g. the oom reaper will need the lock for reading to dismantle the OOM victim address space.

The only tricky function in this patch is vm_mmap_pgoff which has many call sites via vm_mmap. To reduce the risk keep vm_mmap with the original non-killable semantic for now.

vm_munmap callers do not bother checking the return value so open code it into the munmap syscall path for now for simplicity.

Quoted 9 lines

Cc Mel Gorman <[email protected]>

Cc "Kirill A. Shutemov" <[email protected]>

Cc Konstantin Khlebnikov <[email protected]>

Cc Hugh Dickins <[email protected]>

Cc Andrea Arcangeli <[email protected]>

Cc David Rientjes <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Johannes Weiner <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

Acked-by Vlastimil Babka <[email protected]>

From: Michal Hocko <[email protected]>

All the callers of vm_mmap seem to check for the failure already and bail out in one way or another on the error which means that

Quoted 4 lines · Michal Hocko

From: Michal Hocko <[email protected]>

All the callers of vm_mmap seem to check for the failure already and bail out in one way or another on the error which means that

Hmm I'm not that sure about this one:
17 1071 fs/binfmt_elf.c <<load_elf_binary>>

Assigns result of vm_mmap() to "error" variable which is never checked.

Shouldn't __must_check trigger here?

Quoted 6 lines · Vlastimil Babka

Quoted 4 lines · Michal Hocko

From: Michal Hocko <[email protected]>

All the callers of vm_mmap seem to check for the failure already and bail out in one way or another on the error which means that

Quoted 4 lines

Hmm I'm not that sure about this one:
17 1071 fs/binfmt_elf.c <<load_elf_binary>>

Assigns result of vm_mmap() to "error" variable which is never checked.

Yes it is not checked but not used either. If the current got killed then it wouldn't return to the userspace so my understanding is that not checking this value is not a problem. At least that is my understanding.

Quoted 1 line

Shouldn't __must_check trigger here?

well, __must_check is a misleading name. It doesn't actually enforce the value is checked. It just has to be used and an assignment is sufficient. I was discussing this without our gcc guy and he promissed to look and try to come up with a different attribute which would actually work like __must_check.

Quoted 10 lines · Michal Hocko

Quoted 9 lines · Vlastimil Babka

Quoted 4 lines · Michal Hocko

From: Michal Hocko <[email protected]>

All the callers of vm_mmap seem to check for the failure already and bail out in one way or another on the error which means that

Hmm I'm not that sure about this one:
17 1071 fs/binfmt_elf.c <<load_elf_binary>>

Quoted 5 lines

Quoted 1 line

Assigns result of vm_mmap() to "error" variable which is never checked.

Yes it is not checked but not used either. If the current got killed then it wouldn't return to the userspace so my understanding is that not checking this value is not a problem. At least that is my understanding.

Hmm, that's true. So,

Acked-by Vlastimil Babka <[email protected]>

From: Michal Hocko <[email protected]>

Almost all current users of vm_munmap are ignoring the return value and so they do not handle potential error. This means that some VMAs

Quoted 4 lines · Michal Hocko

From: Michal Hocko <[email protected]>

Almost all current users of vm_munmap are ignoring the return value and so they do not handle potential error. This means that some VMAs

   1   7834  arch/x86/kvm/x86.c <<__x86_set_memory_region>>

             r = vm_munmap(old.userspace_addr, old.npages * PAGE_SIZE);
             WARN_ON(r < 0);

This warning will potentially add noise to OOM output?

From: Michal Hocko <[email protected]>

vm_brk is allowed to fail but load_aout_binary simply ignores the error and happily continues. I haven't noticed any problem from that in real life but later patches will make the failure more likely because vm_brk will become killable (resp. mmap_sem for write waiting will become killable) so we should be more careful now.

The error handling should be quite straightforward because there are calls to vm_mmap which check the error properly already. The only notable exception is set_brk which is called after beyond_if label. But nothing indicates that we cannot move it above set_binfmt as the two do not depend on each other and fail before we do set_binfmt and alter reference counting.

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Alexander Viro <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

(quoted diff) +0 -0

  		read_code(file, start_addr, N_TXTOFF(ex),
  			  ex.a_text + ex.a_data);

Quoted 15 lines · Michal Hocko

From: Michal Hocko <[email protected]>

vm_brk is allowed to fail but load_aout_binary simply ignores the error and happily continues. I haven't noticed any problem from that in real life but later patches will make the failure more likely because vm_brk will become killable (resp. mmap_sem for write waiting will become killable) so we should be more careful now.

The error handling should be quite straightforward because there are calls to vm_mmap which check the error properly already. The only notable exception is set_brk which is called after beyond_if label. But nothing indicates that we cannot move it above set_binfmt as the two do not depend on each other and fail before we do set_binfmt and alter reference counting.

Quoted 5 lines

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Alexander Viro <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

Acked--by Vlastimil Babka <[email protected]>

[...]

fs/binfmt_aout.c static int load_aout_library(struct file *file) · line 387

Quoted 9 lines

fs/binfmt_aout.c +3 -1

@@ -378,7 +381,9 @@ static int load_aout_library(struct file *file)
 			       "N_TXTOFF is not page aligned. Please convert library: %pD\n",
 			       file);
 		}
-		vm_brk(start_addr, ex.a_text + ex.a_data + ex.a_bss);
+		retval = vm_brk(start_addr, ex.a_text + ex.a_data + ex.a_bss);
+		if (IS_ERR_VALUE(retval))
+			goto out;
 

You could have removed the extra whitespace on the line above, which my vim so prominently highlights :)

Quoted 4 lines

(quoted diff) +0 -0

 		read_code(file, start_addr, N_TXTOFF(ex),
 			  ex.a_text + ex.a_data);

From: Michal Hocko <[email protected]>

load_elf_library doesn't handle vm_brk failure although nothing really indicates it cannot do that because the function is allowed to fail due to vm_mmap failures already. This might be not a problem now but later patch will make vm_brk killable (resp. mmap_sem for write waiting will become killable) and so the failure will be more probable.

Cc Alexander Viro <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

From: Michal Hocko <[email protected]>

Now that all the callers handle vm_brk failure we can change it wait for mmap_sem killable to help oom_reaper to not get blocked just because vm_brk gets blocked behind mmap_sem readers.

Cc "Kirill A. Shutemov" <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Andrea Arcangeli <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

fs/proc/task_mmu.c +5 -1

@@ -1027,11 +1027,15 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
 		};
 
 		if (type == CLEAR_REFS_MM_HIWATER_RSS) {
+			if (down_write_killable(&mm->mmap_sem)) {
+				put_task_struct(task);
+				return -EINTR;
+			}
+
 			/*
 			 * Writing 5 to /proc/pid/clear_refs resets the peak
 			 * resident set size to this mm's current rss value.
 			 */
-			down_write(&mm->mmap_sem);
 			reset_mm_hiwater_rss(mm);
 			up_write(&mm->mmap_sem);
 			goto out_mm;

Quoted 23 lines · Michal Hocko

fs/proc/task_mmu.c +5 -1

@@ -1027,11 +1027,15 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
 		};
 
 		if (type == CLEAR_REFS_MM_HIWATER_RSS) {
+			if (down_write_killable(&mm->mmap_sem)) {
+				put_task_struct(task);
+				return -EINTR;
+			}
+
 			/*
 			 * Writing 5 to /proc/pid/clear_refs resets the peak
 			 * resident set size to this mm's current rss value.
 			 */
-			down_write(&mm->mmap_sem);
 			reset_mm_hiwater_rss(mm);
 			up_write(&mm->mmap_sem);
 			goto out_mm;

@@ -1043,7 +1047,10 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
 				if (!(vma->vm_flags & VM_SOFTDIRTY))
 					continue;

fs/proc/task_mmu.c static ssize_t clear_refs_write(struct file *file, const char __user *buf, · line 1053

Quoted 6 lines

fs/proc/task_mmu.c +4 -1

 				up_read(&mm->mmap_sem);
-				down_write(&mm->mmap_sem);
+				if (down_write_killable(&mm->mmap_sem)) {
+					put_task_struct(task);
+					return -EINTR;
+				}

Both lack mmput() afaics. Don't you need "goto out_mm" rather then "return" ?

In this case you do not need put_task_struct().

Quoted 1 line · Oleg Nesterov

In this case you do not need put_task_struct().

Why not? Both are after get_proc_task which takes a reference to the task...

Quoted 7 lines · Michal Hocko

Quoted 1 line · Oleg Nesterov

In this case you do not need put_task_struct().

Why not? Both are after get_proc_task which takes a reference to the task...

Yes, but we already have put_task_struct(task) in the "out_mm" path, so "goto out_mm" should work just fine?

From: Michal Hocko <[email protected]>

CLEAR_REFS_MM_HIWATER_RSS and CLEAR_REFS_SOFT_DIRTY are relying on mmap_sem for write. If the waiting task gets killed by the oom killer and it would operate on the current's mm it would block oom_reaper from asynchronous address space reclaim and reduce the chances of timely OOM resolving. Wait for the lock in the killable mode and return with EINTR if the task got killed while waiting. This will also expedite the return to the userspace and do_exit even if the mm is remote.

Cc Oleg Nesterov <[email protected]>

Cc Petr Cermak <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

From: Michal Hocko <[email protected]>

dup_mmap needs to lock current's mm mmap_sem for write. If the waiting task gets killed by the oom killer it would block oom_reaper from asynchronous address space reclaim and reduce the chances of timely OOM resolving. Wait for the lock in the killable mode and return with EINTR if the task got killed while waiting.

Cc Ingo Molnar <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Konstantin Khlebnikov <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
  kernel/fork.c | 6 +++++-
  1 file changed, 5 insertions(+), 1 deletion(-)

kernel/fork.c +4 -1

index d277e83ed3e0..139968026b76 100644
@@ -413,7 +413,10 @@ static int dup_mmap(struct mm_struct *mm, struct mm_struct *oldmm)
  	unsigned long charge;
 
  	uprobe_start_dup_mmap();
-	down_write(&oldmm->mmap_sem);
+	if (down_write_killable(&oldmm->mmap_sem)) {
+		retval = -EINTR;
+		goto fail_uprobe_end;
+	}
  	flush_cache_dup_mm(oldmm);
  	uprobe_dup_mmap(oldmm, mm);
  	/*

From: Michal Hocko <[email protected]>

shmat and shmdt rely on mmap_sem for write. If the waiting task gets killed by the oom killer it would block oom_reaper from asynchronous address space reclaim and reduce the chances of timely OOM resolving. Wait for the lock in the killable mode and return with EINTR if the task got killed while waiting.

Cc Davidlohr Bueso <[email protected]>

Cc Hugh Dickins <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
ipc/shm.c | 9 +++++++--

1 file changed, 7 insertions(+), 2 deletions(-)

ipc/shm.c +0 -0

index 331fc1b0b3c7..b8cfa05940d2 100644

ipc/shm.c +3 -1

+
 	if (addr && !(shmflg & SHM_REMAP)) {
 		err = -EINVAL;
 		if (addr + size < addr)

@@ -1271,7 +1275,8 @@ SYSCALL_DEFINE1(shmdt, char __user *, shmaddr)
 	if (addr & ~PAGE_MASK)
 		return retval;
 
-	down_write(&mm->mmap_sem);
+	if (down_write_killable(&mm->mmap_sem))
+		return -EINTR;
 
 	/*
 	 * This function tries to be smart and unmap shm segments that

Signature · 1 line

2.7.0

Quoted 8 lines · Michal Hocko

From: Michal Hocko <[email protected]>

shmat and shmdt rely on mmap_sem for write. If the waiting task gets killed by the oom killer it would block oom_reaper from asynchronous address space reclaim and reduce the chances of timely OOM resolving. Wait for the lock in the killable mode and return with EINTR if the task got killed while waiting.

Quoted 3 lines

Cc Davidlohr Bueso <[email protected]>

Cc Hugh Dickins <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

I have no objection to this perse, just one comment below.

Acked-by Davidlohr Bueso <[email protected]>

ipc/shm.c long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr, · line 1206

Quoted 5 lines

ipc/shm.c +4 -1

-	down_write(&current->mm->mmap_sem);
+	if (down_write_killable(&current->mm->mmap_sem)) {
+		err = -EINVAL;
+		goto out_fput;
+	}

This should be EINTR, no?

From: Michal Hocko <[email protected]>

shmat and shmdt rely on mmap_sem for write. If the waiting task gets killed by the oom killer it would block oom_reaper from asynchronous address space reclaim and reduce the chances of timely OOM resolving. Wait for the lock in the killable mode and return with EINTR if the task got killed while waiting.

Cc Hugh Dickins <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

Acked-by Davidlohr Bueso <[email protected]>

---
  ipc/shm.c | 9 +++++++--
  1 file changed, 7 insertions(+), 2 deletions(-)

ipc/shm.c +5 -1

index 331fc1b0b3c7..13282510bc0d 100644
@@ -1200,7 +1200,11 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, ulong *raddr,
  	if (err)
  		goto out_fput;
 
-	down_write(&current->mm->mmap_sem);
+	if (down_write_killable(&current->mm->mmap_sem)) {
+		err = -EINTR;
+		goto out_fput;
+	}
+
  	if (addr && !(shmflg & SHM_REMAP)) {
  		err = -EINVAL;
  		if (addr + size < addr)

From: Michal Hocko <[email protected]>

most architectures are relying on mmap_sem for write in their arch_setup_additional_pages. If the waiting task gets killed by the oom killer it would block oom_reaper from asynchronous address space reclaim and reduce the chances of timely OOM resolving. Wait for the lock in the killable mode and return with EINTR if the task got killed while waiting.

From: Michal Hocko <[email protected]>

most architectures are relying on mmap_sem for write in their arch_setup_additional_pages. If the waiting task gets killed by the oom killer it would block oom_reaper from asynchronous address space reclaim and reduce the chances of timely OOM resolving. Wait for the lock in the killable mode and return with EINTR if the task got killed while waiting.

Cc [email protected]

Cc Andy Lutomirski <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

From: Michal Hocko <[email protected]>

coredump_wait waits for mmap_sem for write currently which can prevent oom_reaper to reclaim the oom victims address space asynchronously because that requires mmap_sem for read. This might happen if the oom victim is multi threaded and some thread(s) is holding mmap_sem for read (e.g. page fault) and it is stuck in the page allocator while other thread(s) reached coredump_wait already.

This patch simply uses down_write_killable and bails out with EINTR if the lock got interrupted by the fatal signal. do_coredump will return right away and do_group_exit will take care to zap the whole thread group.

Cc Oleg Nesterov <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
  fs/coredump.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

fs/coredump.c +0 -0

index 9ea87e9fdccf..6b8aa1629891 100644

Quoted 15 lines · Michal Hocko

From: Michal Hocko <[email protected]>

coredump_wait waits for mmap_sem for write currently which can prevent oom_reaper to reclaim the oom victims address space asynchronously because that requires mmap_sem for read. This might happen if the oom victim is multi threaded and some thread(s) is holding mmap_sem for read (e.g. page fault) and it is stuck in the page allocator while other thread(s) reached coredump_wait already.

This patch simply uses down_write_killable and bails out with EINTR if the lock got interrupted by the fatal signal. do_coredump will return right away and do_group_exit will take care to zap the whole thread group.

Quoted 2 lines

Cc Oleg Nesterov <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

Acked-by Vlastimil Babka <[email protected]>

Quoted 16 lines · Vlastimil Babka

Quoted 15 lines · Michal Hocko

From: Michal Hocko <[email protected]>

coredump_wait waits for mmap_sem for write currently which can prevent oom_reaper to reclaim the oom victims address space asynchronously because that requires mmap_sem for read. This might happen if the oom victim is multi threaded and some thread(s) is holding mmap_sem for read (e.g. page fault) and it is stuck in the page allocator while other thread(s) reached coredump_wait already.

This patch simply uses down_write_killable and bails out with EINTR if the lock got interrupted by the fatal signal. do_coredump will return right away and do_group_exit will take care to zap the whole thread group.

Quoted 4 lines

Quoted 2 lines

Cc Oleg Nesterov <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

Acked-by Vlastimil Babka <[email protected]>

Forgot to point out typo in Subject which makes it hard to grep for mmap_sem

From: Michal Hocko <[email protected]>

aio_setup_ring waits for mmap_sem in writable mode. If the waiting task gets killed by the oom killer it would block oom_reaper from asynchronous address space reclaim and reduce the chances of timely OOM resolving. Wait for the lock in the killable mode and return with EINTR if the task got killed while waiting. This will also expedite the return to the userspace and do_exit.

Cc Benjamin LaHaise <[email protected]>

Cc Alexander Viro <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
 fs/aio.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

fs/aio.c +0 -0

index 56bcdf4105f4..1c2e7e2c1b2b 100644
@@ -520,7 +520,12 @@ static int aio_setup_ring(struct kioctx *ctx)
 	ctx->mmap_size = nr_pages * PAGE_SIZE;
 	pr_debug("attempting mmap of %lu bytes\n", ctx->mmap_size);
 

From: Michal Hocko <[email protected]>

aio_setup_ring waits for mmap_sem in writable mode. If the waiting task gets killed by the oom killer it would block oom_reaper from asynchronous address space reclaim and reduce the chances of timely OOM resolving. Wait for the lock in the killable mode and return with EINTR if the task got killed while waiting. This will also expedite the return to the userspace and do_exit.

Cc Benjamin LaHaise <[email protected]>

Cc Alexander Viro <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
  fs/aio.c | 7 ++++++-
  1 file changed, 6 insertions(+), 1 deletion(-)

fs/aio.c +6 -1

index 56bcdf4105f4..1c2e7e2c1b2b 100644
@@ -520,7 +520,12 @@ static int aio_setup_ring(struct kioctx *ctx)
  	ctx->mmap_size = nr_pages * PAGE_SIZE;
  	pr_debug("attempting mmap of %lu bytes\n", ctx->mmap_size);
 
-	down_write(&mm->mmap_sem);
+	if (down_write_killable(&mm->mmap_sem)) {
+		ctx->mmap_size = 0;
+		aio_free_ring(ctx);
+		return -EINTR;
+	}
+
  	ctx->mmap_base = do_mmap_pgoff(ctx->aio_ring_file, 0, ctx->mmap_size,
  				       PROT_READ | PROT_WRITE,
  				       MAP_SHARED, 0, &unused);

fs/exec.c static int __bprm_mm_init(struct linux_binprm *bprm) · line 274

Quoted 11 lines · Michal Hocko

fs/exec.c +4 -1

@@ -267,7 +267,10 @@ static int __bprm_mm_init(struct linux_binprm *bprm)
 	if (!vma)
 		return -ENOMEM;
 
-	down_write(&mm->mmap_sem);
+	if (down_write_killable(&mm->mmap_sem)) {
+		err = -EINTR;
+		goto err_free;
+	}
 	vma->vm_mm = mm;

I won't argue, but this looks unnecessary. Nobody else can see this new mm, down_write() can't block.

In fact I think we can just remove down_write/up_write here. Except perhaps there is lockdep_assert_held() somewhere in these paths.

Quoted 16 lines · Oleg Nesterov

fs/exec.c static int __bprm_mm_init(struct linux_binprm *bprm) · line 274

Quoted 11 lines · Michal Hocko

fs/exec.c +4 -1

@@ -267,7 +267,10 @@ static int __bprm_mm_init(struct linux_binprm *bprm)
 	if (!vma)
 		return -ENOMEM;
 
-	down_write(&mm->mmap_sem);
+	if (down_write_killable(&mm->mmap_sem)) {
+		err = -EINTR;
+		goto err_free;
+	}
 	vma->vm_mm = mm;

I won't argue, but this looks unnecessary. Nobody else can see this new mm, down_write() can't block.

Quoted 2 lines

In fact I think we can just remove down_write/up_write here. Except perhaps there is lockdep_assert_held() somewhere in these paths.

This is what I had initially but then I've noticed that mm_alloc() does mm_init(current)->init_new_context(current)

code doesn't seem much harder to follow, the callers are already handling all error paths so I guess it would be better to simply move on this.

From: Michal Hocko <[email protected]>

setup_arg_pages requires mmap_sem for write. If the waiting task gets killed by the oom killer it would block oom_reaper from asynchronous address space reclaim and reduce the chances of timely OOM resolving. Wait for the lock in the killable mode and return with EINTR if the task got killed while waiting. All the callers are already handling error path and the fatal signal doesn't need any additional treatment.

The same applies to __bprm_mm_init.

Cc Alexander Viro <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

From: Michal Hocko <[email protected]>

PR_SET_THP_DISABLE requires mmap_sem for write. If the waiting task gets killed by the oom killer it would block oom_reaper from asynchronous address space reclaim and reduce the chances of timely OOM resolving. Wait for the lock in the killable mode and return with EINTR if the task got killed while waiting.

Cc Alex Thorlton <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
  kernel/sys.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

kernel/sys.c +2 -1

index cf8ba545c7d3..89d5be418157 100644
@@ -2246,7 +2246,8 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
  	case PR_SET_THP_DISABLE:
  		if (arg3 || arg4 || arg5)
  			return -EINVAL;
-		down_write(&me->mm->mmap_sem);
+		if (down_write_killable(&me->mm->mmap_sem))
+			return -EINTR;
  		if (arg2)
  			me->mm->def_flags |= VM_NOHUGEPAGE;
  		else

Quoted 7 lines · Michal Hocko

kernel/events/uprobes.c +0 -0

@@ -1130,7 +1130,9 @@ static int xol_add_vma(struct mm_struct *mm, struct xol_area *area)
 	struct vm_area_struct *vma;
 	int ret;
 

kernel/events/uprobes.c static int xol_add_vma(struct mm_struct *mm, struct xol_area *area) · line 1135

Quoted 4 lines

kernel/events/uprobes.c +3 -1

-	down_write(&mm->mmap_sem);
+	if (down_write_killable(&mm->mmap_sem))
+		return -EINTR;
+

Yes, but then dup_xol_work() should probably check fatal_signal_pending() to suppress uprobe_warn(), the warning looks like a kernel problem.

Ahh, I see. I didn't understand what is the purpose of the warning. Does the following work for you?
---

kernel/events/uprobes.c +0 -0

index a79315d0f711..fb4a6bcc88ce 100644

kernel/events/uprobes.c +3 -1

@@ -1130,7 +1130,9 @@ static int xol_add_vma(struct mm_struct *mm, struct xol_area *area)
 	struct vm_area_struct *vma;
 	int ret;
 
-	down_write(&mm->mmap_sem);
+	if (down_write_killable(&mm->mmap_sem))
+		return -EINTR;
+
 	if (mm->uprobes_state.xol_area) {
 		ret = -EALREADY;
 		goto fail;

Quoted 18 lines · Michal Hocko

kernel/events/uprobes.c +3 -1

@@ -1130,7 +1130,9 @@ static int xol_add_vma(struct mm_struct *mm, struct xol_area *area)
 	struct vm_area_struct *vma;
 	int ret;
 
-	down_write(&mm->mmap_sem);
+	if (down_write_killable(&mm->mmap_sem))
+		return -EINTR;
+
 	if (mm->uprobes_state.xol_area) {
 		ret = -EALREADY;
 		goto fail;

@@ -1468,7 +1470,8 @@ static void dup_xol_work(struct callback_head *work)
 	if (current->flags & PF_EXITING)
 		return;
 

kernel/events/uprobes.c static void dup_xol_work(struct callback_head *work) · line 1476

Quoted 5 lines

kernel/events/uprobes.c +2 -1

-	if (!__create_xol_area(current->utask->dup_xol_addr))
+	if (!__create_xol_area(current->utask->dup_xol_addr) &&
+			!fatal_signal_pending(current)
 		uprobe_warn(current, "dup xol area");
 }

Looks good, thanks.

Quoted 19 lines · Oleg Nesterov

Quoted 18 lines · Michal Hocko

kernel/events/uprobes.c +3 -1

@@ -1130,7 +1130,9 @@ static int xol_add_vma(struct mm_struct *mm, struct xol_area *area)
 	struct vm_area_struct *vma;
 	int ret;
 
-	down_write(&mm->mmap_sem);
+	if (down_write_killable(&mm->mmap_sem))
+		return -EINTR;
+
 	if (mm->uprobes_state.xol_area) {
 		ret = -EALREADY;
 		goto fail;

@@ -1468,7 +1470,8 @@ static void dup_xol_work(struct callback_head *work)
 	if (current->flags & PF_EXITING)
 		return;
 

Quoted 7 lines

kernel/events/uprobes.c static void dup_xol_work(struct callback_head *work) · line 1476

Quoted 5 lines

kernel/events/uprobes.c +2 -1

-	if (!__create_xol_area(current->utask->dup_xol_addr))
+	if (!__create_xol_area(current->utask->dup_xol_addr) &&
+			!fatal_signal_pending(current)
 		uprobe_warn(current, "dup xol area");
 }

Looks good, thanks.

Can I consider this your Acked-by?

From: Michal Hocko <[email protected]>

xol_add_vma needs mmap_sem for write. If the waiting task gets killed by the oom killer it would block oom_reaper from asynchronous address space reclaim and reduce the chances of timely OOM resolving. Wait for the lock in the killable mode and return with EINTR if the task got killed while waiting.

Do not warn in dup_xol_work if __create_xol_area failed due to fatal signal pending because this is usually considered a kernel issue.

Cc Oleg Nesterov <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
  kernel/events/uprobes.c | 7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)

kernel/events/uprobes.c +3 -1

index 8eef5f55d3f0..fb4a6bcc88ce 100644
@@ -1130,7 +1130,9 @@ static int xol_add_vma(struct mm_struct *mm, struct xol_area *area)
  	struct vm_area_struct *vma;
  	int ret;
 
-	down_write(&mm->mmap_sem);
+	if (down_write_killable(&mm->mmap_sem))
+		return -EINTR;
+
  	if (mm->uprobes_state.xol_area) {
  		ret = -EALREADY;
  		goto fail;

On 02/29/2016 06:42 PM, Michal Hocko wrote:

kernel/events/uprobes.c static void dup_xol_work(struct callback_head *work) · line 1474

Quoted 7 lines

kernel/events/uprobes.c +2 -1

@@ -1468,7 +1470,8 @@ static void dup_xol_work(struct callback_head *work)
 	if (current->flags & PF_EXITING)
 		return;
 
-	if (!__create_xol_area(current->utask->dup_xol_addr))
+	if (!__create_xol_area(current->utask->dup_xol_addr) &&
+			!fatal_signal_pending(current)

                                                      ^ missing ")"

Other than that,

Acked-by Vlastimil Babka <[email protected]>

Quoted 5 lines

kernel/events/uprobes.c +0 -0

 		uprobe_warn(current, "dup xol area");
 }
 

From: Michal Hocko <[email protected]>

i915_gem_mmap_ioctl relies on mmap_sem for write. If the waiting task gets killed by the oom killer it would block oom_reaper from asynchronous address space reclaim and reduce the chances of timely OOM resolving. Wait for the lock in the killable mode and return with EINTR if the task got killed while waiting.

Cc Daniel Vetter <[email protected]>

Cc David Airlie <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
  drivers/gpu/drm/i915/i915_gem.c | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/i915_gem.c +4 -1

index f68f34606f2f..a50136cbd332 100644
@@ -1754,7 +1754,10 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
  		struct mm_struct *mm = current->mm;
  		struct vm_area_struct *vma;
 
-		down_write(&mm->mmap_sem);
+		if (down_write_killable(&mm->mmap_sem)) {
+			drm_gem_object_unreference_unlocked(obj);
+			return -EINTR;
+		}
  		vma = find_vma(mm, addr);
  		if (vma)
  			vma->vm_page_prot =

From: Michal Hocko <[email protected]>

radeon_mn_get which is called during ioct path relies on mmap_sem for write. If the waiting task gets killed by the oom killer it would block oom_reaper from asynchronous address space reclaim and reduce the chances of timely OOM resolving. Wait for the lock in the killable mode and return with EINTR if the task got killed while waiting.

Cc Alex Deucher <[email protected]>

Cc "Christian König" <[email protected]>

Cc David Airlie <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
  drivers/gpu/drm/radeon/radeon_mn.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

drivers/gpu/drm/radeon/radeon_mn.c +0 -0

index eef006c48584..896f2cf51e4e 100644

From: Michal Hocko <[email protected]>

radeon_mn_get which is called during ioct path relies on mmap_sem for write. If the waiting task gets killed by the oom killer it would block oom_reaper from asynchronous address space reclaim and reduce the chances of timely OOM resolving. Wait for the lock in the killable mode and return with EINTR if the task got killed while waiting.

Cc Alex Deucher <[email protected]>

Cc "Christian KA?nig" <[email protected]>

Cc David Airlie <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
  drivers/gpu/drm/radeon/radeon_mn.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

drivers/gpu/drm/radeon/radeon_mn.c +0 -0

index eef006c48584..896f2cf51e4e 100644

From: Michal Hocko <[email protected]>

amdgpu_mn_get which is called during ioct path relies on mmap_sem for write. If the waiting task gets killed by the oom killer it would block oom_reaper from asynchronous address space reclaim and reduce the chances of timely OOM resolving. Wait for the lock in the killable mode and return with EINTR if the task got killed while waiting.

Cc David Airlie <[email protected]>

Cc Alex Deucher <[email protected]>

Signed-off-by Michal Hocko <[email protected]>

---
  drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)

drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c +0 -0

index d7ec9bd6755f..6f44f1c23be3 100644
@@ -181,7 +181,10 @@ static struct amdgpu_mn *amdgpu_mn_get(struct amdgpu_device *adev)
  	int r;

As this work is touching more areas which are not directly connected I have tried to keep the CC list as small as possible and people who I believed would be familiar are CCed only to the specific patches (all should have received the cover though).

Hi,
this is a follow up work for oom_reaper [1]. As the async OOM killing depends on oom_sem for read we would really appreciate if a holder for write stood in the way. This patchset is changing many of down_write calls to be killable to help those cases when the writer is blocked and waiting for readers to release the lock and so help __oom_reap_task to process the oom victim.

Most of the patches are really trivial because the lock is help from a shallow syscall paths where we can return EINTR trivially. Others seem to be easy as well as the callers are already handling fatal errors and bail and return to userspace which should be sufficient to handle the failure gracefully. I am not familiar with all those code paths so a deeper review is really appreciated.

As this work is touching more areas which are not directly connected I have tried to keep the CC list as small as possible and people who I believed would be familiar are CCed only to the specific patches (all should have received the cover though).

This patchset is based on linux-next and it depends on down_write_killable for rw_semaphores posted recently [2].

I haven't covered all the mmap_write(mm->mmap_sem) instances here

$ git grep "down_write(.*\<mmap_sem\>)" next/master | wc -l
102
$ git grep "down_write(.*\<mmap_sem\>)" | wc -l
66

I have tried to cover those which should be relatively easy to review in this series because this alone should be a nice improvement. Other places can be changed on top.

Any feedback is highly appreciated.

---
[1] http://lkml.kernel.org/r/[email protected]
[2] http://lkml.kernel.org/r/[email protected]

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Quoted 6 lines

Most of the patches are really trivial because the lock is help from a shallow syscall paths where we can return EINTR trivially. Others seem to be easy as well as the callers are already handling fatal errors and bail and return to userspace which should be sufficient to handle the failure gracefully. I am not familiar with all those code paths so a deeper review is really appreciated.

What about effect on userspace? IIUC, we would have now EINTR returned from bunch of syscall, which haven't had this errno on the table before. Should we care?

On Mon 29-02-16 17:04:16, Kirill A. Shutemov wrote:
[...]

Quoted 7 lines

Quoted 6 lines

Most of the patches are really trivial because the lock is help from a shallow syscall paths where we can return EINTR trivially. Others seem to be easy as well as the callers are already handling fatal errors and bail and return to userspace which should be sufficient to handle the failure gracefully. I am not familiar with all those code paths so a deeper review is really appreciated.

Quoted 3 lines

What about effect on userspace? IIUC, we would have now EINTR returned from bunch of syscall, which haven't had this errno on the table before. Should we care?

Those function will return EINTR only when the current was _killed_ when we do not return to the userspace. So there shouldn't be any visible effect.