uprobes fixes for 3.5

__replace_page() obviously can't work with the hugetlbfs mappings, uprobe_register() will likely crash the kernel. Change valid_vma() to check VM_HUGETLB as well.

As for PageTransHuge() no need to worry, vma->vm_file != NULL.

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/events/uprobes.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

kernel/events/uprobes.c +0 -0

index 6e3b181..48d53af 100644

__copy_insn() blindly calls read_mapping_page(), this will crash the kernel if ->readpage == NULL, add the necessary check. For example, hugetlbfs_aops->readpage is NULL. Perhaps we should change read_mapping_page() instead.

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/events/uprobes.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

kernel/events/uprobes.c +0 -0

index 48d53af..9c53bc2 100644

Quoted 16 lines · Oleg Nesterov

__copy_insn() blindly calls read_mapping_page(), this will crash the kernel if ->readpage == NULL, add the necessary check. For example, hugetlbfs_aops->readpage is NULL. Perhaps we should change read_mapping_page() instead.

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/events/uprobes.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

kernel/events/uprobes.c +0 -0

index 48d53af..9c53bc2 100644
@@ -616,6 +616,8 @@ __copy_insn(struct address_space *mapping, struct vm_area_struct *vma, char *ins
 

kernel/events/uprobes.c __copy_insn(struct address_space *mapping, struct vm_area_struct *vma, char *ins · line 620

Quoted 4 lines

kernel/events/uprobes.c +2 -0

 	if (!filp)
 		return -EINVAL;
+	if (!mapping->a_ops->readpage)
+		return -EIO;

Nit: Should there be a blank line before the if. Ingo had insisted on these coding style changes.

write_opcode() gets old_page via get_user_pages() and then calls __replace_page() which assumes that this old_page is still mapped after pte_offset_map_lock().

This is not true if this old_page was already try_to_unmap()'ed, and in this case everything __replace_page() does with old_page is wrong. Just for example, put_page() is not balanced.

I think it is possible to teach __replace_page() to handle this unlikely case correctly, but this patch simply changes it to use page_check_address() and return -EAGAIN if it fails. The caller should notice this error code and retry.

Quoted 8 lines · Oleg Nesterov

write_opcode() gets old_page via get_user_pages() and then calls __replace_page() which assumes that this old_page is still mapped after pte_offset_map_lock().

This is not true if this old_page was already try_to_unmap()'ed, and in this case everything __replace_page() does with old_page is wrong. Just for example, put_page() is not balanced.

Quoted 4 lines

I think it is possible to teach __replace_page() to handle this unlikely case correctly, but this patch simply changes it to use page_check_address() and return -EAGAIN if it fails. The caller should notice this error code and retry.

Note that replace_page() was nicked from ksm, does that suffer a similar problem?

The caller
should notice this error code and retry.

Note: write_opcode() asks for the cleanups, I'll try to do this in a separate patch.

---
Subject: [PATCH] uprobes: write_opcode()->__replace_page() can race with try_to_unmap()

write_opcode() gets old_page via get_user_pages() and then calls __replace_page() which assumes that this old_page is still mapped after pte_offset_map_lock().

This is not true if this old_page was already try_to_unmap()'ed, and in this case everything __replace_page() does with old_page is wrong. Just for example, put_page() is not balanced.

I think it is possible to teach __replace_page() to handle this unlikely case correctly, but this patch simply changes it to use page_check_address() and return -EAGAIN if it fails. The caller should notice this error code and retry.

Note: write_opcode() asks for the cleanups, I'll try to do this in a separate patch.

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/events/uprobes.c |   41 +++++++++++++----------------------------
 1 files changed, 13 insertions(+), 28 deletions(-)

kernel/events/uprobes.c +6 -24

index 9c53bc2..54c8780 100644
@@ -135,33 +135,17 @@ static loff_t vma_address(struct vm_area_struct *vma, loff_t offset)
 static int __replace_page(struct vm_area_struct *vma, struct page *page, struct page *kpage)
 {
 	struct mm_struct *mm = vma->vm_mm;
-	pgd_t *pgd;
-	pud_t *pud;
-	pmd_t *pmd;
-	pte_t *ptep;
-	spinlock_t *ptl;
 	unsigned long addr;
-	int err = -EFAULT;
+	spinlock_t *ptl;
+	pte_t *ptep;
 
 	addr = page_address_in_vma(page, vma);
 	if (addr == -EFAULT)
-		goto out;
-
-	pgd = pgd_offset(mm, addr);
-	if (!pgd_present(*pgd))
-		goto out;
-
-	pud = pud_offset(pgd, addr);
-	if (!pud_present(*pud))
-		goto out;
+		return -EFAULT;
 
-	pmd = pmd_offset(pud, addr);
-	if (!pmd_present(*pmd))
-		goto out;
-
-	ptep = pte_offset_map_lock(mm, pmd, addr, &ptl);
+	ptep = page_check_address(page, mm, addr, &ptl, 0);
 	if (!ptep)
-		goto out;
+		return -EAGAIN;
 
 	get_page(kpage);
 	page_add_new_anon_rmap(kpage, vma, addr);

@@ -180,10 +164,8 @@ static int __replace_page(struct vm_area_struct *vma, struct page *page, struct
 		try_to_free_swap(page);
 	put_page(page);
 	pte_unmap_unlock(ptep, ptl);
-	err = 0;
 
-out:
-	return err;
+	return 0;
 }
 
 /**

kernel/events/uprobes.c +3 -3

 	/* Read the page with vaddr into memory */
 	ret = get_user_pages(NULL, mm, vaddr, 1, 0, 0, &old_page, &vma);
 	if (ret <= 0)

@@ -275,9 +258,9 @@ static int write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
 	memcpy(vaddr_new, vaddr_old, PAGE_SIZE);
 
 	/* poke the new insn in, ASSUMES we don't cross page boundary */
-	vaddr &= ~PAGE_MASK;
-	BUG_ON(vaddr + UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
-	memcpy(vaddr_new + vaddr, &opcode, UPROBE_SWBP_INSN_SIZE);
+	pgoff = (vaddr & ~PAGE_MASK);
+	BUG_ON(pgoff + UPROBE_SWBP_INSN_SIZE > PAGE_SIZE);
+	memcpy(vaddr_new + pgoff, &opcode, UPROBE_SWBP_INSN_SIZE);
 
 	kunmap_atomic(vaddr_new);
 	kunmap_atomic(vaddr_old);

Quoted 78 lines

---
Subject: [PATCH] uprobes: write_opcode()->__replace_page() can race with try_to_unmap()

write_opcode() gets old_page via get_user_pages() and then calls __replace_page() which assumes that this old_page is still mapped after pte_offset_map_lock().

This is not true if this old_page was already try_to_unmap()'ed, and in this case everything __replace_page() does with old_page is wrong. Just for example, put_page() is not balanced.

I think it is possible to teach __replace_page() to handle this unlikely case correctly, but this patch simply changes it to use page_check_address() and return -EAGAIN if it fails. The caller should notice this error code and retry.

Note: write_opcode() asks for the cleanups, I'll try to do this in a separate patch.

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/events/uprobes.c |   41 +++++++++++++----------------------------
 1 files changed, 13 insertions(+), 28 deletions(-)

kernel/events/uprobes.c +6 -24

index 9c53bc2..54c8780 100644
@@ -135,33 +135,17 @@ static loff_t vma_address(struct vm_area_struct *vma, loff_t offset)
 static int __replace_page(struct vm_area_struct *vma, struct page *page, struct page *kpage)
 {
 	struct mm_struct *mm = vma->vm_mm;
-	pgd_t *pgd;
-	pud_t *pud;
-	pmd_t *pmd;
-	pte_t *ptep;
-	spinlock_t *ptl;
 	unsigned long addr;
-	int err = -EFAULT;
+	spinlock_t *ptl;
+	pte_t *ptep;
 
 	addr = page_address_in_vma(page, vma);
 	if (addr == -EFAULT)
-		goto out;
-
-	pgd = pgd_offset(mm, addr);
-	if (!pgd_present(*pgd))
-		goto out;
-
-	pud = pud_offset(pgd, addr);
-	if (!pud_present(*pud))
-		goto out;
+		return -EFAULT;
 
-	pmd = pmd_offset(pud, addr);
-	if (!pmd_present(*pmd))
-		goto out;
-
-	ptep = pte_offset_map_lock(mm, pmd, addr, &ptl);
+	ptep = page_check_address(page, mm, addr, &ptl, 0);
 	if (!ptep)
-		goto out;
+		return -EAGAIN;
 
 	get_page(kpage);
 	page_add_new_anon_rmap(kpage, vma, addr);

@@ -180,10 +164,8 @@ static int __replace_page(struct vm_area_struct *vma, struct page *page, struct
 		try_to_free_swap(page);
 	put_page(page);
 	pte_unmap_unlock(ptep, ptl);
-	err = 0;
 
-out:
-	return err;
+	return 0;
 }
 

kernel/events/uprobes.c static int write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, · line 216

Quoted 10 lines

kernel/events/uprobes.c +2 -1

 /**

@@ -228,9 +210,10 @@ static int write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
 	void *vaddr_old, *vaddr_new;
 	struct vm_area_struct *vma;
 	struct uprobe *uprobe;
+	unsigned long pgoff;
 	loff_t addr;
 	int ret;
-
+retry:

Just a check on coding style: Shouldnt we have a preceeding blank line before the goto label.

Quoted 4 lines · Srikar Dronamraju

Quoted 4 lines

kernel/events/uprobes.c +0 -0

@@ -228,9 +210,10 @@ static int write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
 	void *vaddr_old, *vaddr_new;
 	struct vm_area_struct *vma;
 	struct uprobe *uprobe;

Quoted 8 lines

kernel/events/uprobes.c static int write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, · line 216

Quoted 5 lines

kernel/events/uprobes.c +2 -1

+	unsigned long pgoff;
 	loff_t addr;
 	int ret;
-
+retry:

Just a check on coding style: Shouldnt we have a preceeding blank line before the goto label.

Yeah, that's most likely helpful to readability.

Quoted 16 lines · Ingo Molnar

Quoted 12 lines · Srikar Dronamraju

kernel/events/uprobes.c static int write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm, · line 216

Quoted 9 lines

kernel/events/uprobes.c +2 -1

@@ -228,9 +210,10 @@ static int write_opcode(struct arch_uprobe *auprobe, struct mm_struct *mm,
 	void *vaddr_old, *vaddr_new;
 	struct vm_area_struct *vma;
 	struct uprobe *uprobe;
+	unsigned long pgoff;
 	loff_t addr;
 	int ret;
-
+retry:

Just a check on coding style: Shouldnt we have a preceeding blank line before the goto label.

Quoted 1 line

Yeah, that's most likely helpful to readability.

Aaah. Srikar, sorry, I didn't notice this comment and I already sent 1-15. But I added the blank line in 2/15 ;)

Ingo, please let me know if I need to re-diff and resend. Otherwise I'll add the blank line later, write_opcode() needs more changes anyway.

This doesn't depend on other uprobes patches I sent, and I think this is 3.5 material.

Quoted 3 lines · Oleg Nesterov

This doesn't depend on other uprobes patches I sent, and I think this is 3.5 material.

And during the testing I found another thing which should be fixed in 3.5 imho. I noticed that oom-killer goes crazy. In the simplest case, when there is the single and "obvious" memory hog it kills sshd daemon.

Hmm. oom_badness() does

	if (has_capability_noaudit(p, CAP_SYS_ADMIN))
		points -= 30 * totalpages / 1000;

very nice, but what if this underflows? points is unsigned long. points += p->signal->oom_score_adj... looks suspicious too.

Looks like we should remove "unsigned" from oom_badness() and its callers? Probably not, it does "return points ? points : 1".

Quoted 11 lines · Oleg Nesterov

Hmm. oom_badness() does

	if (has_capability_noaudit(p, CAP_SYS_ADMIN))
		points -= 30 * totalpages / 1000;

very nice, but what if this underflows? points is unsigned long. points += p->signal->oom_score_adj... looks suspicious too.

Looks like we should remove "unsigned" from oom_badness() and its callers? Probably not, it does "return points ? points : 1".

I've been running this from David for a week, but it still isn't right..

Dave

mm/oom_kill.c +1 -1

index ed0e196..416637f 100644
@@ -183,7 +183,7 @@ static bool oom_unkillable_task(struct task_struct *p,
 unsigned long oom_badness(struct task_struct *p, struct mem_cgroup *memcg,
                          const nodemask_t *nodemask, unsigned long totalpages)
 {
-       unsigned long points;
+       long points;
 
        if (oom_unkillable_task(p, memcg, nodemask))
                return 0;

If the privileges given to root threads (3% of allowable memory) or a negative value of /proc/pid/oom_score_adj happen to exceed the amount of rss of a thread, its badness score overflows as a result of a7f638f999ff ("mm, oom: normalize oom scores to oom_score_adj scale only for userspace").

Fix this by making the type signed and return 1, meaning the thread is still eligible for kill, if the value is negative.

Reported-by Dave Jones<[email protected]>

Acked-by Oleg Nesterov<[email protected]>

Signed-off-by David Rientjes<[email protected]>

---
  mm/oom_kill.c |    4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

mm/oom_kill.c +1 -1

@@ -183,7 +183,7 @@ static bool oom_unkillable_task(struct task_struct *p,
  unsigned long oom_badness(struct task_struct *p, struct mem_cgroup *memcg,
  			  const nodemask_t *nodemask, unsigned long totalpages)
  {
-	unsigned long points;
+	long points;
 
  	if (oom_unkillable_task(p, memcg, nodemask))
  		return 0;