[RFC 0/3] Slight improvements for OOM/Futex

[RFC 0/3] Slight improvements for OOM/Futex

[Nico Pache]

The following 3 patches were developed while working on the OOM/Futex
fix. 

Patch 1: This is a nonfunctional change. The vma_is_anonymous function
originally led to some confusion about what the oom reaper is checking
for in __oom_reap_task_mm. This patch makes that function name more 
verbose.

Patch 2: Futex cleanup was designed with silent failures. Printing this
failure would have led to more quickly finding the issue. This
introduces a pr_info if the exit path has any issues walking the
userspace list.

Patch 3: During the debug process I noticed that the oom_reap_task_mm
function was always running twice for the same mm_struct; Once in the
exit path and once in the oom_reaper. By checking the MMF_OOM_SKIP we
can prevent these unnecissary double calls. I'd like some input from
David Rientjes here with regards to CVE-2018-1000200, I want to make
sure we are not reintroducing that CVE.

Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Logan Gunthorpe <logang@deltatee.com>
Cc: Hari Bathini <hbathini@linux.ibm.com>
Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
Cc: "Matthew Wilcox (Oracle)" <willy@infradead.org>
Cc: Yang Shi <shy828301@gmail.com>
Cc: Miaohe Lin <linmiaohe@huawei.com>
Cc: William Kucharski <william.kucharski@oracle.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Darren Hart <dvhart@infradead.org>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: "André Almeida" <andrealmeid@collabora.com>
Cc: Arjan van de Ven <arjan@infradead.org>
Cc: Ulrich Drepper <drepper@redhat.com>
Cc: Michal Hocko <mhocko@suse.com>
Signed-off-by: Nico Pache <npache@redhat.com>

Nico Pache (3):
  mm: change vma_is_anonymous to vma_is_private_anon
  futex: exit: Print a warning when futex_cleanup fails
  exit: Check for MMF_OOM_SKIP in exit_mmap

 arch/powerpc/mm/book3s64/pgtable.c |  2 +-
 fs/userfaultfd.c                   |  2 +-
 include/linux/huge_mm.h            |  2 +-
 include/linux/mm.h                 |  2 +-
 kernel/futex/core.c                | 44 ++++++++++++++++++------------
 mm/gup.c                           |  4 +--
 mm/huge_memory.c                   | 10 +++----
 mm/madvise.c                       |  4 +--
 mm/memory.c                        | 10 +++----
 mm/migrate_device.c                |  6 ++--
 mm/mincore.c                       |  2 +-
 mm/mmap.c                          |  7 +++--
 mm/oom_kill.c                      |  2 +-
 mm/userfaultfd.c                   |  8 +++---
 14 files changed, 57 insertions(+), 48 deletions(-)

-- 
2.35.1

[RFC 1/3] mm: change vma_is_anonymous to vma_is_private_anon

[Nico Pache]

The vma_is_anonymous function isn't fully indicative of what it checks.

Without having full knowledge of the mmap process, one may incorrectly
assume this covers all types of anonymous memory; which is not the case.

No functional changes.

Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Nicholas Piggin <npiggin@gmail.com>
Cc: Logan Gunthorpe <logang@deltatee.com>
Cc: Hari Bathini <hbathini@linux.ibm.com>
Cc: "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>
Cc: "Matthew Wilcox (Oracle)" <willy@infradead.org>
Cc: Yang Shi <shy828301@gmail.com>
Cc: Miaohe Lin <linmiaohe@huawei.com>
Cc: William Kucharski <william.kucharski@oracle.com>
Cc: Hugh Dickins <hughd@google.com>
Signed-off-by: Nico Pache <npache@redhat.com>
---
 arch/powerpc/mm/book3s64/pgtable.c |  2 +-
 fs/userfaultfd.c                   |  2 +-
 include/linux/huge_mm.h            |  2 +-
 include/linux/mm.h                 |  2 +-
 mm/gup.c                           |  4 ++--
 mm/huge_memory.c                   | 10 +++++-----
 mm/madvise.c                       |  4 ++--
 mm/memory.c                        | 10 +++++-----
 mm/migrate_device.c                |  6 +++---
 mm/mincore.c                       |  2 +-
 mm/mmap.c                          |  4 ++--
 mm/oom_kill.c                      |  2 +-
 mm/userfaultfd.c                   |  8 ++++----
 13 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/arch/powerpc/mm/book3s64/pgtable.c b/arch/powerpc/mm/book3s64/pgtable.c
index 052e6590f84f..c8fd68a7965d 100644
--- a/arch/powerpc/mm/book3s64/pgtable.c
+++ b/arch/powerpc/mm/book3s64/pgtable.c
@@ -479,7 +479,7 @@ int pmd_move_must_withdraw(struct spinlock *new_pmd_ptl,
 			   struct vm_area_struct *vma)
 {
 	if (radix_enabled())
-		return (new_pmd_ptl != old_pmd_ptl) && vma_is_anonymous(vma);
+		return (new_pmd_ptl != old_pmd_ptl) && vma_is_private_anon(vma);
 
 	return true;
 }
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index aa0c47cb0d16..d3fe2c26874a 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -1269,7 +1269,7 @@ static inline bool vma_can_userfault(struct vm_area_struct *vma,
 			return false;
 	}
 
-	return vma_is_anonymous(vma) || is_vm_hugetlb_page(vma) ||
+	return vma_is_private_anon(vma) || is_vm_hugetlb_page(vma) ||
 	       vma_is_shmem(vma);
 }
 
diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index 2999190adc22..40493f53562c 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -119,7 +119,7 @@ static inline bool transhuge_vma_suitable(struct vm_area_struct *vma,
 		unsigned long haddr)
 {
 	/* Don't have to check pgoff for anonymous vma */
-	if (!vma_is_anonymous(vma)) {
+	if (!vma_is_private_anon(vma)) {
 		if (!IS_ALIGNED((vma->vm_start >> PAGE_SHIFT) - vma->vm_pgoff,
 				HPAGE_PMD_NR))
 			return false;
diff --git a/include/linux/mm.h b/include/linux/mm.h
index e34edb775334..9a01d053853b 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -623,7 +623,7 @@ static inline void vma_set_anonymous(struct vm_area_struct *vma)
 	vma->vm_ops = NULL;
 }
 
-static inline bool vma_is_anonymous(struct vm_area_struct *vma)
+static inline bool vma_is_private_anon(struct vm_area_struct *vma)
 {
 	return !vma->vm_ops;
 }
diff --git a/mm/gup.c b/mm/gup.c
index f598a037eb04..7cd3ef329ea2 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -398,7 +398,7 @@ static struct page *no_page_table(struct vm_area_struct *vma,
 	 * be zero-filled if handle_mm_fault() actually did handle it.
 	 */
 	if ((flags & FOLL_DUMP) &&
-			(vma_is_anonymous(vma) || !vma->vm_ops->fault))
+			(vma_is_private_anon(vma) || !vma->vm_ops->fault))
 		return ERR_PTR(-EFAULT);
 	return NULL;
 }
@@ -913,7 +913,7 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags)
 	if (vm_flags & (VM_IO | VM_PFNMAP))
 		return -EFAULT;
 
-	if (gup_flags & FOLL_ANON && !vma_is_anonymous(vma))
+	if (gup_flags & FOLL_ANON && !vma_is_private_anon(vma))
 		return -EFAULT;
 
 	if ((gup_flags & FOLL_LONGTERM) && vma_is_fsdax(vma))
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index c468fee595ff..81c2123aeffe 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -82,7 +82,7 @@ bool transparent_hugepage_active(struct vm_area_struct *vma)
 
 	if (!transhuge_vma_suitable(vma, addr))
 		return false;
-	if (vma_is_anonymous(vma))
+	if (vma_is_private_anon(vma))
 		return __transparent_hugepage_enabled(vma);
 	if (vma_is_shmem(vma))
 		return shmem_huge_enabled(vma);
@@ -1035,7 +1035,7 @@ int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 	int ret = -ENOMEM;
 
 	/* Skip if can be re-fill on fault */
-	if (!vma_is_anonymous(dst_vma))
+	if (!vma_is_private_anon(dst_vma))
 		return 0;
 
 	pgtable = pte_alloc_one(dst_mm);
@@ -1622,7 +1622,7 @@ static inline int pmd_move_must_withdraw(spinlock_t *new_pmd_ptl,
 	 *
 	 * We also don't deposit and withdraw tables for file pages.
 	 */
-	return (new_pmd_ptl != old_pmd_ptl) && vma_is_anonymous(vma);
+	return (new_pmd_ptl != old_pmd_ptl) && vma_is_private_anon(vma);
 }
 #endif
 
@@ -1799,7 +1799,7 @@ int change_huge_pmd(struct vm_area_struct *vma, pmd_t *pmd,
 	}
 	ret = HPAGE_PMD_NR;
 	set_pmd_at(mm, addr, pmd, entry);
-	BUG_ON(vma_is_anonymous(vma) && !preserve_write && pmd_write(entry));
+	BUG_ON(vma_is_private_anon(vma) && !preserve_write && pmd_write(entry));
 unlock:
 	spin_unlock(ptl);
 	return ret;
@@ -1957,7 +1957,7 @@ static void __split_huge_pmd_locked(struct vm_area_struct *vma, pmd_t *pmd,
 
 	count_vm_event(THP_SPLIT_PMD);
 
-	if (!vma_is_anonymous(vma)) {
+	if (!vma_is_private_anon(vma)) {
 		old_pmd = pmdp_huge_clear_flush_notify(vma, haddr, pmd);
 		/*
 		 * We are going to unmap this huge page. So
diff --git a/mm/madvise.c b/mm/madvise.c
index 1873616a37d2..23771875ae9d 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -543,7 +543,7 @@ static void madvise_pageout_page_range(struct mmu_gather *tlb,
 
 static inline bool can_do_pageout(struct vm_area_struct *vma)
 {
-	if (vma_is_anonymous(vma))
+	if (vma_is_private_anon(vma))
 		return true;
 	if (!vma->vm_file)
 		return false;
@@ -725,7 +725,7 @@ static int madvise_free_single_vma(struct vm_area_struct *vma,
 	struct mmu_gather tlb;
 
 	/* MADV_FREE works for only anon vma at the moment */
-	if (!vma_is_anonymous(vma))
+	if (!vma_is_private_anon(vma))
 		return -EINVAL;
 
 	range.start = max(vma->vm_start, start_addr);
diff --git a/mm/memory.c b/mm/memory.c
index 76e3af9639d9..fcb599bc5c33 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4494,7 +4494,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
 
 static inline vm_fault_t create_huge_pmd(struct vm_fault *vmf)
 {
-	if (vma_is_anonymous(vmf->vma))
+	if (vma_is_private_anon(vmf->vma))
 		return do_huge_pmd_anonymous_page(vmf);
 	if (vmf->vma->vm_ops->huge_fault)
 		return vmf->vma->vm_ops->huge_fault(vmf, PE_SIZE_PMD);
@@ -4504,7 +4504,7 @@ static inline vm_fault_t create_huge_pmd(struct vm_fault *vmf)
 /* `inline' is required to avoid gcc 4.1.2 build error */
 static inline vm_fault_t wp_huge_pmd(struct vm_fault *vmf)
 {
-	if (vma_is_anonymous(vmf->vma)) {
+	if (vma_is_private_anon(vmf->vma)) {
 		if (userfaultfd_huge_pmd_wp(vmf->vma, vmf->orig_pmd))
 			return handle_userfault(vmf, VM_UFFD_WP);
 		return do_huge_pmd_wp_page(vmf);
@@ -4527,7 +4527,7 @@ static vm_fault_t create_huge_pud(struct vm_fault *vmf)
 #if defined(CONFIG_TRANSPARENT_HUGEPAGE) &&			\
 	defined(CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD)
 	/* No support for anonymous transparent PUD pages yet */
-	if (vma_is_anonymous(vmf->vma))
+	if (vma_is_private_anon(vmf->vma))
 		goto split;
 	if (vmf->vma->vm_ops->huge_fault) {
 		vm_fault_t ret = vmf->vma->vm_ops->huge_fault(vmf, PE_SIZE_PUD);
@@ -4546,7 +4546,7 @@ static vm_fault_t wp_huge_pud(struct vm_fault *vmf, pud_t orig_pud)
 {
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
 	/* No support for anonymous transparent PUD pages yet */
-	if (vma_is_anonymous(vmf->vma))
+	if (vma_is_private_anon(vmf->vma))
 		return VM_FAULT_FALLBACK;
 	if (vmf->vma->vm_ops->huge_fault)
 		return vmf->vma->vm_ops->huge_fault(vmf, PE_SIZE_PUD);
@@ -4621,7 +4621,7 @@ static vm_fault_t handle_pte_fault(struct vm_fault *vmf)
 	}
 
 	if (!vmf->pte) {
-		if (vma_is_anonymous(vmf->vma))
+		if (vma_is_private_anon(vmf->vma))
 			return do_anonymous_page(vmf);
 		else
 			return do_fault(vmf);
diff --git a/mm/migrate_device.c b/mm/migrate_device.c
index 70c7dc05bbfc..4a4068c410f7 100644
--- a/mm/migrate_device.c
+++ b/mm/migrate_device.c
@@ -40,7 +40,7 @@ static int migrate_vma_collect_hole(unsigned long start,
 	unsigned long addr;
 
 	/* Only allow populating anonymous memory. */
-	if (!vma_is_anonymous(walk->vma))
+	if (!vma_is_private_anon(walk->vma))
 		return migrate_vma_collect_skip(start, end, walk);
 
 	for (addr = start; addr < end; addr += PAGE_SIZE) {
@@ -120,7 +120,7 @@ static int migrate_vma_collect_pmd(pmd_t *pmdp,
 		pte = *ptep;
 
 		if (pte_none(pte)) {
-			if (vma_is_anonymous(vma)) {
+			if (vma_is_private_anon(vma)) {
 				mpfn = MIGRATE_PFN_MIGRATE;
 				migrate->cpages++;
 			}
@@ -518,7 +518,7 @@ static void migrate_vma_insert_page(struct migrate_vma *migrate,
 	pte_t *ptep;
 
 	/* Only allow populating anonymous memory */
-	if (!vma_is_anonymous(vma))
+	if (!vma_is_private_anon(vma))
 		goto abort;
 
 	pgdp = pgd_offset(mm, addr);
diff --git a/mm/mincore.c b/mm/mincore.c
index 9122676b54d6..5aa4a1358ac1 100644
--- a/mm/mincore.c
+++ b/mm/mincore.c
@@ -156,7 +156,7 @@ static int mincore_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
 
 static inline bool can_do_mincore(struct vm_area_struct *vma)
 {
-	if (vma_is_anonymous(vma))
+	if (vma_is_private_anon(vma))
 		return true;
 	if (!vma->vm_file)
 		return false;
diff --git a/mm/mmap.c b/mm/mmap.c
index 3aa839f81e63..a2968669fd4e 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -3189,7 +3189,7 @@ int insert_vm_struct(struct mm_struct *mm, struct vm_area_struct *vma)
 	 * using the existing file pgoff checks and manipulations.
 	 * Similarly in do_mmap and in do_brk_flags.
 	 */
-	if (vma_is_anonymous(vma)) {
+	if (vma_is_private_anon(vma)) {
 		BUG_ON(vma->anon_vma);
 		vma->vm_pgoff = vma->vm_start >> PAGE_SHIFT;
 	}
@@ -3217,7 +3217,7 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
 	 * If anonymous vma has not yet been faulted, update new pgoff
 	 * to match new location, to increase its chance of merging.
 	 */
-	if (unlikely(vma_is_anonymous(vma) && !vma->anon_vma)) {
+	if (unlikely(vma_is_private_anon(vma) && !vma->anon_vma)) {
 		pgoff = addr >> PAGE_SHIFT;
 		faulted_in_anon_vma = false;
 	}
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 7ec38194f8e1..aa3e68934fcb 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -536,7 +536,7 @@ bool __oom_reap_task_mm(struct mm_struct *mm)
 		 * we do not want to block exit_mmap by keeping mm ref
 		 * count elevated without a good reason.
 		 */
-		if (vma_is_anonymous(vma) || !(vma->vm_flags & VM_SHARED)) {
+		if (vma_is_private_anon(vma) || !(vma->vm_flags & VM_SHARED)) {
 			struct mmu_notifier_range range;
 			struct mmu_gather tlb;
 
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index 0cb8e5ef1713..c94f81bd109b 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -540,9 +540,9 @@ static __always_inline ssize_t __mcopy_atomic(struct mm_struct *dst_mm,
 	err = -EINVAL;
 	/*
 	 * shmem_zero_setup is invoked in mmap for MAP_ANONYMOUS|MAP_SHARED but
-	 * it will overwrite vm_ops, so vma_is_anonymous must return false.
+	 * it will overwrite vm_ops, so vma_is_private_anon must return false.
 	 */
-	if (WARN_ON_ONCE(vma_is_anonymous(dst_vma) &&
+	if (WARN_ON_ONCE(vma_is_private_anon(dst_vma) &&
 	    dst_vma->vm_flags & VM_SHARED))
 		goto out_unlock;
 
@@ -561,7 +561,7 @@ static __always_inline ssize_t __mcopy_atomic(struct mm_struct *dst_mm,
 		return  __mcopy_atomic_hugetlb(dst_mm, dst_vma, dst_start,
 						src_start, len, mcopy_mode);
 
-	if (!vma_is_anonymous(dst_vma) && !vma_is_shmem(dst_vma))
+	if (!vma_is_private_anon(dst_vma) && !vma_is_shmem(dst_vma))
 		goto out_unlock;
 	if (!vma_is_shmem(dst_vma) && mcopy_mode == MCOPY_ATOMIC_CONTINUE)
 		goto out_unlock;
@@ -717,7 +717,7 @@ int mwriteprotect_range(struct mm_struct *dst_mm, unsigned long start,
 		goto out_unlock;
 	if (!userfaultfd_wp(dst_vma))
 		goto out_unlock;
-	if (!vma_is_anonymous(dst_vma))
+	if (!vma_is_private_anon(dst_vma))
 		goto out_unlock;
 
 	if (enable_wp)
-- 
2.35.1

[RFC 2/3] futex: exit: Print a warning when futex_cleanup fails

[Nico Pache]

The futex_cleanup routines currently fails silently.

Allow the futex_cleanup routines to fail more verbosely so we can
leave a message behind for easier debugging/error detecting.

Fixes: 0771dfefc9e5 ("[PATCH] lightweight robust futexes: core")
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Darren Hart <dvhart@infradead.org>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: "André Almeida" <andrealmeid@collabora.com>
Cc: Arjan van de Ven <arjan@infradead.org>
Cc: Ulrich Drepper <drepper@redhat.com>
Signed-off-by: Nico Pache <npache@redhat.com>
---
 kernel/futex/core.c | 44 ++++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/kernel/futex/core.c b/kernel/futex/core.c
index b22ef1efe751..8a62eb1b5d77 100644
--- a/kernel/futex/core.c
+++ b/kernel/futex/core.c
@@ -760,9 +760,9 @@ static inline int fetch_robust_entry(struct robust_list __user **entry,
  * Walk curr->robust_list (very carefully, it's a userspace list!)
  * and mark any locks found there dead, and notify any waiters.
  *
- * We silently return on any sign of list-walking problem.
+ * We return false on any sign of list-walking problem.
  */
-static void exit_robust_list(struct task_struct *curr)
+static bool exit_robust_list(struct task_struct *curr)
 {
 	struct robust_list_head __user *head = curr->robust_list;
 	struct robust_list __user *entry, *next_entry, *pending;
@@ -771,23 +771,25 @@ static void exit_robust_list(struct task_struct *curr)
 	unsigned long futex_offset;
 	int rc;
 
+	if (!futex_cmpxchg_enabled)
+		return false;
 	/*
 	 * Fetch the list head (which was registered earlier, via
 	 * sys_set_robust_list()):
 	 */
 	if (fetch_robust_entry(&entry, &head->list.next, &pi))
-		return;
+		return false;
 	/*
 	 * Fetch the relative futex offset:
 	 */
 	if (get_user(futex_offset, &head->futex_offset))
-		return;
+		return false;
 	/*
 	 * Fetch any possibly pending lock-add first, and handle it
 	 * if it exists:
 	 */
 	if (fetch_robust_entry(&pending, &head->list_op_pending, &pip))
-		return;
+		return false;
 
 	next_entry = NULL;	/* avoid warning with gcc */
 	while (entry != &head->list) {
@@ -803,10 +805,10 @@ static void exit_robust_list(struct task_struct *curr)
 		if (entry != pending) {
 			if (handle_futex_death((void __user *)entry + futex_offset,
 						curr, pi, HANDLE_DEATH_LIST))
-				return;
+				return false;
 		}
 		if (rc)
-			return;
+			return false;
 		entry = next_entry;
 		pi = next_pi;
 		/*
@@ -819,9 +821,10 @@ static void exit_robust_list(struct task_struct *curr)
 	}
 
 	if (pending) {
-		handle_futex_death((void __user *)pending + futex_offset,
+		return handle_futex_death((void __user *)pending + futex_offset,
 				   curr, pip, HANDLE_DEATH_PENDING);
 	}
+	return true;
 }
 
 #ifdef CONFIG_COMPAT
@@ -854,9 +857,9 @@ compat_fetch_robust_entry(compat_uptr_t *uentry, struct robust_list __user **ent
  * Walk curr->robust_list (very carefully, it's a userspace list!)
  * and mark any locks found there dead, and notify any waiters.
  *
- * We silently return on any sign of list-walking problem.
+ * We return false on any sign of list-walking problem.
  */
-static void compat_exit_robust_list(struct task_struct *curr)
+static bool compat_exit_robust_list(struct task_struct *curr)
 {
 	struct compat_robust_list_head __user *head = curr->compat_robust_list;
 	struct robust_list __user *entry, *next_entry, *pending;
@@ -866,24 +869,26 @@ static void compat_exit_robust_list(struct task_struct *curr)
 	compat_long_t futex_offset;
 	int rc;
 
+	if (!futex_cmpxchg_enabled)
+		return false;
 	/*
 	 * Fetch the list head (which was registered earlier, via
 	 * sys_set_robust_list()):
 	 */
 	if (compat_fetch_robust_entry(&uentry, &entry, &head->list.next, &pi))
-		return;
+		return false;
 	/*
 	 * Fetch the relative futex offset:
 	 */
 	if (get_user(futex_offset, &head->futex_offset))
-		return;
+		return false;
 	/*
 	 * Fetch any possibly pending lock-add first, and handle it
 	 * if it exists:
 	 */
 	if (compat_fetch_robust_entry(&upending, &pending,
 			       &head->list_op_pending, &pip))
-		return;
+		return false;
 
 	next_entry = NULL;	/* avoid warning with gcc */
 	while (entry != (struct robust_list __user *) &head->list) {
@@ -902,10 +907,10 @@ static void compat_exit_robust_list(struct task_struct *curr)
 
 			if (handle_futex_death(uaddr, curr, pi,
 					       HANDLE_DEATH_LIST))
-				return;
+				return false;
 		}
 		if (rc)
-			return;
+			return false;
 		uentry = next_uentry;
 		entry = next_entry;
 		pi = next_pi;
@@ -920,8 +925,9 @@ static void compat_exit_robust_list(struct task_struct *curr)
 	if (pending) {
 		void __user *uaddr = futex_uaddr(pending, futex_offset);
 
-		handle_futex_death(uaddr, curr, pip, HANDLE_DEATH_PENDING);
+		return handle_futex_death(uaddr, curr, pip, HANDLE_DEATH_PENDING);
 	}
+	return true;
 }
 #endif
 
@@ -1007,13 +1013,15 @@ static inline void exit_pi_state_list(struct task_struct *curr) { }
 static void futex_cleanup(struct task_struct *tsk)
 {
 	if (unlikely(tsk->robust_list)) {
-		exit_robust_list(tsk);
+		if (!exit_robust_list(tsk))
+			pr_info("futex: exit_robust_list failed");
 		tsk->robust_list = NULL;
 	}
 
 #ifdef CONFIG_COMPAT
 	if (unlikely(tsk->compat_robust_list)) {
-		compat_exit_robust_list(tsk);
+		if (!compat_exit_robust_list(tsk))
+			pr_info("futex: compat_exit_robust_list failed");
 		tsk->compat_robust_list = NULL;
 	}
 #endif
-- 
2.35.1

[RFC 3/3] exit: Check for MMF_OOM_SKIP in exit_mmap

[Nico Pache]

The MMF_OOM_SKIP bit is used to indicate weather a mm_struct can not be
invalided or has already been invalided. exit_mmap currently calls
__oom_reap_task_mm unconditionally despite the fact that the oom reaper
may have already called this.

Add a check for the MMF_OOM_SKIP bit being set in exit_mmap to avoid
unnessary calls to the invalidate code.

A slight race can occur on the MMF_OOM_SKIP bit that will still allow
this to run twice. My testing has shown an ~66% decrease in double calls
to _oom_reap_task_mm.

Fixes: 27ae357fa82b ("mm, oom: fix concurrent munlock and oom reaper unmap, v3")
Cc: David Rientjes <rientjes@google.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Signed-off-by: Nico Pache <npache@redhat.com>
---
 mm/mmap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index a2968669fd4e..b867f408dacd 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -3113,7 +3113,8 @@ void exit_mmap(struct mm_struct *mm)
 	/* mm's last user has gone, and its about to be pulled down */
 	mmu_notifier_release(mm);
 
-	if (unlikely(mm_is_oom_victim(mm))) {
+	if (unlikely(mm_is_oom_victim(mm)) &&
+			!test_bit(MMF_OOM_SKIP, &mm->flags)) {
 		/*
 		 * Manually reap the mm to free as much memory as possible.
 		 * Then, as the oom reaper does, set MMF_OOM_SKIP to disregard
-- 
2.35.1

Re: [RFC 1/3] mm: change vma_is_anonymous to vma_is_private_anon

[Matthew Wilcox]

On Thu, Apr 21, 2022 at 03:05:31PM -0400, Nico Pache wrote:
> The vma_is_anonymous function isn't fully indicative of what it checks.
> 
> Without having full knowledge of the mmap process, one may incorrectly
> assume this covers all types of anonymous memory; which is not the case.

Is your complaint that anonymous memory can also be found in file VMAs
that were mapped with MAP_PRIVATE?  ie COWed pages?

I don't think renaming this function is appropriate.  It's whether
the VMA is anonymous, not whether the VMA can contain anonymous
pages.

Re: [RFC 2/3] futex: exit: Print a warning when futex_cleanup fails

[Matthew Wilcox]

On Thu, Apr 21, 2022 at 03:05:32PM -0400, Nico Pache wrote:
> @@ -1007,13 +1013,15 @@ static inline void exit_pi_state_list(struct task_struct *curr) { }
>  static void futex_cleanup(struct task_struct *tsk)
>  {
>  	if (unlikely(tsk->robust_list)) {
> -		exit_robust_list(tsk);
> +		if (!exit_robust_list(tsk))
> +			pr_info("futex: exit_robust_list failed");

Doesn't this allow a malicious user process to spam the kernel logs
with messages?  There needs to be a ratelimit on this, at least.

Re: [RFC 2/3] futex: exit: Print a warning when futex_cleanup fails

[Thomas Gleixner]

On Thu, Apr 21 2022 at 15:05, Nico Pache wrote:

> The futex_cleanup routines currently fails silently.
>
> Allow the futex_cleanup routines to fail more verbosely so we can
> leave a message behind for easier debugging/error detecting.

This is a free ticket to spam dmesg for any unpriviledged user.

> Fixes: 0771dfefc9e5 ("[PATCH] lightweight robust futexes: core")

There is nothing to fix, really.

Robust futexes are best effort as I explained you before and spamming
dmesg won't help anything.

Thanks,

        tglx

Re: [RFC 2/3] futex: exit: Print a warning when futex_cleanup fails

[kernel test robot]

Hi Nico,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on powerpc/next]
[also build test ERROR on tip/locking/core linus/master v5.18-rc3]
[cannot apply to hnaz-mm/master linux/master next-20220421]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Nico-Pache/Slight-improvements-for-OOM-Futex/20220422-030740
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: hexagon-randconfig-r025-20220421 (https://download.01.org/0day-ci/archive/20220422/202204220841.5b2XtG9h-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 5bd87350a5ae429baf8f373cb226a57b62f87280)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/cd9fef152153fe60c67bb633118dd8adef51cadc
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Nico-Pache/Slight-improvements-for-OOM-Futex/20220422-030740
        git checkout cd9fef152153fe60c67bb633118dd8adef51cadc
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> kernel/futex/core.c:774:7: error: use of undeclared identifier 'futex_cmpxchg_enabled'
           if (!futex_cmpxchg_enabled)
                ^
   1 error generated.


vim +/futex_cmpxchg_enabled +774 kernel/futex/core.c

   758	
   759	/*
   760	 * Walk curr->robust_list (very carefully, it's a userspace list!)
   761	 * and mark any locks found there dead, and notify any waiters.
   762	 *
   763	 * We return false on any sign of list-walking problem.
   764	 */
   765	static bool exit_robust_list(struct task_struct *curr)
   766	{
   767		struct robust_list_head __user *head = curr->robust_list;
   768		struct robust_list __user *entry, *next_entry, *pending;
   769		unsigned int limit = ROBUST_LIST_LIMIT, pi, pip;
   770		unsigned int next_pi;
   771		unsigned long futex_offset;
   772		int rc;
   773	
 > 774		if (!futex_cmpxchg_enabled)
   775			return false;
   776		/*
   777		 * Fetch the list head (which was registered earlier, via
   778		 * sys_set_robust_list()):
   779		 */
   780		if (fetch_robust_entry(&entry, &head->list.next, &pi))
   781			return false;
   782		/*
   783		 * Fetch the relative futex offset:
   784		 */
   785		if (get_user(futex_offset, &head->futex_offset))
   786			return false;
   787		/*
   788		 * Fetch any possibly pending lock-add first, and handle it
   789		 * if it exists:
   790		 */
   791		if (fetch_robust_entry(&pending, &head->list_op_pending, &pip))
   792			return false;
   793	
   794		next_entry = NULL;	/* avoid warning with gcc */
   795		while (entry != &head->list) {
   796			/*
   797			 * Fetch the next entry in the list before calling
   798			 * handle_futex_death:
   799			 */
   800			rc = fetch_robust_entry(&next_entry, &entry->next, &next_pi);
   801			/*
   802			 * A pending lock might already be on the list, so
   803			 * don't process it twice:
   804			 */
   805			if (entry != pending) {
   806				if (handle_futex_death((void __user *)entry + futex_offset,
   807							curr, pi, HANDLE_DEATH_LIST))
   808					return false;
   809			}
   810			if (rc)
   811				return false;
   812			entry = next_entry;
   813			pi = next_pi;
   814			/*
   815			 * Avoid excessively long or circular lists:
   816			 */
   817			if (!--limit)
   818				break;
   819	
   820			cond_resched();
   821		}
   822	
   823		if (pending) {
   824			return handle_futex_death((void __user *)pending + futex_offset,
   825					   curr, pip, HANDLE_DEATH_PENDING);
   826		}
   827		return true;
   828	}
   829	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [RFC 1/3] mm: change vma_is_anonymous to vma_is_private_anon

[Nico Pache]

On 4/21/22 15:28, Matthew Wilcox wrote:
> On Thu, Apr 21, 2022 at 03:05:31PM -0400, Nico Pache wrote:
>> The vma_is_anonymous function isn't fully indicative of what it checks.
>>
>> Without having full knowledge of the mmap process, one may incorrectly
>> assume this covers all types of anonymous memory; which is not the case.
> 
> Is your complaint that anonymous memory can also be found in file VMAs
> that were mapped with MAP_PRIVATE?  ie COWed pages?
I should have been more descriptive in my commit msg about how I came to this
conclusion.

From my understanding of the mmap process, a vma->vm_ops field is only NULL when
mmapped as !file and !shared:

	if (file){
		...
	} else if (vm_flags & VM_SHARED) { 	//ANON SHARED
		error = shmem_zero_setup(vma);
	        if (error)
        		goto free_vma;
	} else { 				//ANON PRIVATE
		vma_set_anonymous(vma);		//set vma->vm_ops= NULL
	}

To me this means that the VMA is PRIVATE ANON memory. The vma_is_anonymous
function returns true when vm_ops == NULL. So my intentions were to more
accurately describe what we are checking for. I could be wrong though thats why
I started with an RFC :)

There could be some aspect of COW that I dont fully understand. It is not
something I've looked into much.

Cheers,
-- Nico

Re: [RFC 2/3] futex: exit: Print a warning when futex_cleanup fails

[Nico Pache]

On 4/21/22 15:30, Matthew Wilcox wrote:
> On Thu, Apr 21, 2022 at 03:05:32PM -0400, Nico Pache wrote:
>> @@ -1007,13 +1013,15 @@ static inline void exit_pi_state_list(struct task_struct *curr) { }
>>  static void futex_cleanup(struct task_struct *tsk)
>>  {
>>  	if (unlikely(tsk->robust_list)) {
>> -		exit_robust_list(tsk);
>> +		if (!exit_robust_list(tsk))
>> +			pr_info("futex: exit_robust_list failed");
> 
> Doesn't this allow a malicious user process to spam the kernel logs
> with messages?  There needs to be a ratelimit on this, at least.
Fair point, we'd need a ratelimited print if we want to continue forward with
this. Additionally we may want to limit this print to debug kernels, but thats
just a thought.

Thanks for the review :)
-- Nico

Re: [RFC 2/3] futex: exit: Print a warning when futex_cleanup fails

[Nico Pache]

On 4/21/22 16:53, Thomas Gleixner wrote:
> On Thu, Apr 21 2022 at 15:05, Nico Pache wrote:
> 
>> The futex_cleanup routines currently fails silently.
>>
>> Allow the futex_cleanup routines to fail more verbosely so we can
>> leave a message behind for easier debugging/error detecting.
> 
> This is a free ticket to spam dmesg for any unpriviledged user.
As Matthew pointed out, we'd want to rate limit this. I was also thinking we can
limit this to debug kernels so unpriviledged users can not cause dmesg spamming
in production configs.
> 
>> Fixes: 0771dfefc9e5 ("[PATCH] lightweight robust futexes: core")
> 
> There is nothing to fix, really.
Fair enough, its been part of the design since inceptions.
> 
> Robust futexes are best effort as I explained you before and spamming
> dmesg won't help anything.
It would have helped find the OOM/Robust Futex issue more quickly. It may also
help detect breakages in the pthread code (or any other users of robust futex)
if someone is doing something incorrectly.

Just a thought though. If you think its pointless to proceed down this path I
wont push forward.

Cheers,
-- Nico

Re: [RFC 2/3] futex: exit: Print a warning when futex_cleanup fails

[Thomas Gleixner]

On Fri, Apr 22 2022 at 10:23, Nico Pache wrote:
> On 4/21/22 16:53, Thomas Gleixner wrote:
>> On Thu, Apr 21 2022 at 15:05, Nico Pache wrote:
>> Robust futexes are best effort as I explained you before and spamming
>> dmesg won't help anything.
> It would have helped find the OOM/Robust Futex issue more quickly. It may also
> help detect breakages in the pthread code (or any other users of robust futex)
> if someone is doing something incorrectly.

If we follow that line of argumentation then we have to sprinkle printks
en masse all over the place as there are a gazillion ways to create hard
to debug issues.

Thanks,

        tglx

Re: [RFC 3/3] exit: Check for MMF_OOM_SKIP in exit_mmap

[Michal Hocko]

On Thu 21-04-22 15:05:33, Nico Pache wrote:
> The MMF_OOM_SKIP bit is used to indicate weather a mm_struct can not be
> invalided or has already been invalided. exit_mmap currently calls
> __oom_reap_task_mm unconditionally despite the fact that the oom reaper
> may have already called this.
> 
> Add a check for the MMF_OOM_SKIP bit being set in exit_mmap to avoid
> unnessary calls to the invalidate code.

Why do we care about this?
 
> A slight race can occur on the MMF_OOM_SKIP bit that will still allow
> this to run twice. My testing has shown an ~66% decrease in double calls
> to _oom_reap_task_mm.
> 
> Fixes: 27ae357fa82b ("mm, oom: fix concurrent munlock and oom reaper unmap, v3")

I do not see this would be fixing anything.

> Cc: David Rientjes <rientjes@google.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Andrea Arcangeli <aarcange@redhat.com>
> Signed-off-by: Nico Pache <npache@redhat.com>
> ---
>  mm/mmap.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index a2968669fd4e..b867f408dacd 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -3113,7 +3113,8 @@ void exit_mmap(struct mm_struct *mm)
>  	/* mm's last user has gone, and its about to be pulled down */
>  	mmu_notifier_release(mm);
>  
> -	if (unlikely(mm_is_oom_victim(mm))) {
> +	if (unlikely(mm_is_oom_victim(mm)) &&
> +			!test_bit(MMF_OOM_SKIP, &mm->flags)) {
>  		/*
>  		 * Manually reap the mm to free as much memory as possible.
>  		 * Then, as the oom reaper does, set MMF_OOM_SKIP to disregard
> -- 
> 2.35.1

-- 
Michal Hocko
SUSE Labs

Re: [RFC 3/3] exit: Check for MMF_OOM_SKIP in exit_mmap

[Nico Pache]

On 4/22/22 11:38, Michal Hocko wrote:
> On Thu 21-04-22 15:05:33, Nico Pache wrote:
>> The MMF_OOM_SKIP bit is used to indicate weather a mm_struct can not be
>> invalided or has already been invalided. exit_mmap currently calls
>> __oom_reap_task_mm unconditionally despite the fact that the oom reaper
>> may have already called this.
>>
>> Add a check for the MMF_OOM_SKIP bit being set in exit_mmap to avoid
>> unnessary calls to the invalidate code.
> 
> Why do we care about this?
Is there no cost to the MMU/TLB invalidation? The MMU notifier contains a lock
too so perhaps we can also avoids some unnecessary MMU notifier lock contention.
>  
>> A slight race can occur on the MMF_OOM_SKIP bit that will still allow
>> this to run twice. My testing has shown an ~66% decrease in double calls
>> to _oom_reap_task_mm.
>>
>> Fixes: 27ae357fa82b ("mm, oom: fix concurrent munlock and oom reaper unmap, v3")
> 
> I do not see this would be fixing anything.
Ok im just trying to make sure we are keeping an eye on what introduced this
double call. Davids commit above is what introduced the oom_reap_task_mm in the
exit_mmap code. It goes along with some other changes that I dont fully
understand without more studying, so that's why I was hoping he could provide
some input around that CVE (the main thing im concerned about re-introducing).
> 
>> Cc: David Rientjes <rientjes@google.com>
>> Cc: Michal Hocko <mhocko@suse.com>
>> Cc: Andrea Arcangeli <aarcange@redhat.com>
>> Signed-off-by: Nico Pache <npache@redhat.com>
>> ---
>>  mm/mmap.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/mmap.c b/mm/mmap.c
>> index a2968669fd4e..b867f408dacd 100644
>> --- a/mm/mmap.c
>> +++ b/mm/mmap.c
>> @@ -3113,7 +3113,8 @@ void exit_mmap(struct mm_struct *mm)
>>  	/* mm's last user has gone, and its about to be pulled down */
>>  	mmu_notifier_release(mm);
>>  
>> -	if (unlikely(mm_is_oom_victim(mm))) {
>> +	if (unlikely(mm_is_oom_victim(mm)) &&
>> +			!test_bit(MMF_OOM_SKIP, &mm->flags)) {
>>  		/*
>>  		 * Manually reap the mm to free as much memory as possible.
>>  		 * Then, as the oom reaper does, set MMF_OOM_SKIP to disregard
>> -- 
>> 2.35.1
> 

Re: [RFC 3/3] exit: Check for MMF_OOM_SKIP in exit_mmap

[Michal Hocko]

On Mon 25-04-22 15:00:24, Nico Pache wrote:
> 
> 
> On 4/22/22 11:38, Michal Hocko wrote:
> > On Thu 21-04-22 15:05:33, Nico Pache wrote:
> >> The MMF_OOM_SKIP bit is used to indicate weather a mm_struct can not be
> >> invalided or has already been invalided. exit_mmap currently calls
> >> __oom_reap_task_mm unconditionally despite the fact that the oom reaper
> >> may have already called this.
> >>
> >> Add a check for the MMF_OOM_SKIP bit being set in exit_mmap to avoid
> >> unnessary calls to the invalidate code.
> > 
> > Why do we care about this?
> Is there no cost to the MMU/TLB invalidation? The MMU notifier contains a lock
> too so perhaps we can also avoids some unnecessary MMU notifier lock contention.

I am pretty sure that this area can be micro optimized but I do not
really see a strong reason for that. OOM victims are/should be really
rare so I do not think that any performance optimization would be really
visible.

If you want to improve the code then I think a much better plan would be
to get rid of the whole oom special case altogether. This might be much
closer than ever after Hugh's recent m{un}lock changes. I didn't have
time to think that through though. I believe Suren Baghdasaryan has been
looking into that as well.
-- 
Michal Hocko
SUSE Labs

Re: [RFC 1/3] mm: change vma_is_anonymous to vma_is_private_anon

[David Hildenbrand]

On 22.04.22 16:00, Nico Pache wrote:
> 
> 
> On 4/21/22 15:28, Matthew Wilcox wrote:
>> On Thu, Apr 21, 2022 at 03:05:31PM -0400, Nico Pache wrote:
>>> The vma_is_anonymous function isn't fully indicative of what it checks.
>>>
>>> Without having full knowledge of the mmap process, one may incorrectly
>>> assume this covers all types of anonymous memory; which is not the case.
>>
>> Is your complaint that anonymous memory can also be found in file VMAs
>> that were mapped with MAP_PRIVATE?  ie COWed pages?
> I should have been more descriptive in my commit msg about how I came to this
> conclusion.
> 
> From my understanding of the mmap process, a vma->vm_ops field is only NULL when
> mmapped as !file and !shared:
> 
> 	if (file){
> 		...
> 	} else if (vm_flags & VM_SHARED) { 	//ANON SHARED
> 		error = shmem_zero_setup(vma);
> 	        if (error)
>         		goto free_vma;
> 	} else { 				//ANON PRIVATE
> 		vma_set_anonymous(vma);		//set vma->vm_ops= NULL
> 	}
> 
> To me this means that the VMA is PRIVATE ANON memory. The vma_is_anonymous
> function returns true when vm_ops == NULL. So my intentions were to more
> accurately describe what we are checking for. I could be wrong though thats why
> I started with an RFC :)

Shared anon in the kernel is really just shmem.  The user space notion
is MAP_ANON|MAP_SHARED, but that really just maps to shmem and the
kernel doesn't really call that thing anonymous memory.

So I agree, renaming this is not appropriate.

-- 
Thanks,

David / dhildenb