From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id EBAB5C433F5 for ; Thu, 7 Apr 2022 22:32:25 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 7CC6E6B0072; Thu, 7 Apr 2022 18:32:15 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 77B246B0073; Thu, 7 Apr 2022 18:32:15 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 66A6C6B0074; Thu, 7 Apr 2022 18:32:15 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0076.hostedemail.com [216.40.44.76]) by kanga.kvack.org (Postfix) with ESMTP id 55BDA6B0072 for ; Thu, 7 Apr 2022 18:32:15 -0400 (EDT) Received: from smtpin29.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 114E5AA17D for ; Thu, 7 Apr 2022 22:32:05 +0000 (UTC) X-FDA: 79331532210.29.25E5DCF Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf04.hostedemail.com (Postfix) with ESMTP id 47E8140004 for ; Thu, 7 Apr 2022 22:32:04 +0000 (UTC) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 69FCC61FB7; Thu, 7 Apr 2022 22:32:03 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 32DEFC385A4; Thu, 7 Apr 2022 22:32:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linux-foundation.org; s=korg; t=1649370722; bh=fQt1MnSysX/EHNJtZXA6Yk6KIqVhFKrn01VGpcgiLqg=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=j+bQv2bTZlMfLeJRhLqw0Aa3Rrwh0Zp/ijpEFh5imqLNVymbm+DyPCuJhA5tiJ/2e /9PJ9r1+QBKVRr6k6F5rvvTi1m3zt0uK+i5/KCg15fK4hkMGVE0OgpY76wxQ1cTLYz NuTVvf+kIYzHVu54niX2Rdihf0S44LRv0vpH4JJ8= Date: Thu, 7 Apr 2022 15:32:01 -0700 From: Andrew Morton To: Nico Pache Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Rafael Aquini , Waiman Long , Baoquan He , Christoph von Recklinghausen , Don Dutile , "Herton R . Krzesinski" , David Rientjes , Michal Hocko , Andrea Arcangeli , Davidlohr Bueso , Thomas Gleixner , Peter Zijlstra , Ingo Molnar , Joel Savitz , Darren Hart Subject: Re: [PATCH v6] oom_kill.c: futex: Don't OOM reap the VMA containing the robust_list_head Message-Id: <20220407153201.cdcab0a2d4a58ce358fb7c47@linux-foundation.org> In-Reply-To: References: <20220407184254.3612387-1-npache@redhat.com> <20220407131809.f2d256541e2c039c434c0d72@linux-foundation.org> X-Mailer: Sylpheed 3.7.0 (GTK+ 2.24.33; x86_64-redhat-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 47E8140004 X-Stat-Signature: 4guugnkdff95itqg6xfoc4u3gn3pdakn Authentication-Results: imf04.hostedemail.com; dkim=pass header.d=linux-foundation.org header.s=korg header.b=j+bQv2bT; dmarc=none; spf=pass (imf04.hostedemail.com: domain of akpm@linux-foundation.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=akpm@linux-foundation.org X-Rspam-User: X-HE-Tag: 1649370724-799031 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu, 7 Apr 2022 17:52:31 -0400 Nico Pache wrote: > >> > >> The following case can still fail: > >> robust head (skipped) -> private lock (reaped) -> shared lock (skipped) > >> > >> Reproducer: https://gitlab.com/jsavitz/oom_futex_reproducer > > > > Should this fix be backported into -stable kernels? > > Yes I believe so. This is caused by the commit marked under 'Fixes:' which is in > stable branch. OK. The MM team don't like the promiscuous backporting of things which we didn't ask to be backported. So -stable maintainers have been trained (we hope) to only backport things which we explicitly marked cc:stable. > >> --- a/include/linux/oom.h > >> +++ b/include/linux/oom.h > >> @@ -106,7 +106,8 @@ static inline vm_fault_t check_stable_address_space(struct mm_struct *mm) > >> return 0; > >> } > >> > >> -bool __oom_reap_task_mm(struct mm_struct *mm); > >> +bool __oom_reap_task_mm(struct mm_struct *mm, struct robust_list_head > >> + __user *robust_list); > > > > Should explicitly include futex.h > Good point. On second thought I think we also need to surround some of the > changes with a ifdef CONFIG_FUTEX. current->robust_list is undefined if we turn > that config option off. Ah. > > > >> long oom_badness(struct task_struct *p, > >> unsigned long totalpages); > >> diff --git a/mm/mmap.c b/mm/mmap.c > >> index 3aa839f81e63..c14fe6f8e9a5 100644 > >> --- a/mm/mmap.c > >> +++ b/mm/mmap.c > >> @@ -3126,7 +3126,8 @@ void exit_mmap(struct mm_struct *mm) > >> * to mmu_notifier_release(mm) ensures mmu notifier callbacks in > >> * __oom_reap_task_mm() will not block. > >> */ > >> - (void)__oom_reap_task_mm(mm); > >> + (void)__oom_reap_task_mm(mm, current->robust_list); > >> + > >> set_bit(MMF_OOM_SKIP, &mm->flags); > >> } > >> > >> diff --git a/mm/oom_kill.c b/mm/oom_kill.c > >> index 7ec38194f8e1..727cfc3bd284 100644 > >> --- a/mm/oom_kill.c > >> +++ b/mm/oom_kill.c > >> @@ -509,9 +509,11 @@ static DECLARE_WAIT_QUEUE_HEAD(oom_reaper_wait); > >> static struct task_struct *oom_reaper_list; > >> static DEFINE_SPINLOCK(oom_reaper_lock); > >> > >> -bool __oom_reap_task_mm(struct mm_struct *mm) > >> +bool __oom_reap_task_mm(struct mm_struct *mm, struct robust_list_head > >> + __user *robust_list) > >> { > > > > It's pretty sad to make such a low-level function aware of futex > > internals. How about making it a more general `void *skip_area'? > Yes we can make this change. My concern is that the caller may now have to cast > the type: __oom_reap_task_mm(mm_struct, (void*) current->robust_list). But I > doubt that is a big concern. No cast needed - the compiler will happily cast an anything* to a void*. > > > >> struct vm_area_struct *vma; > >> + unsigned long head = (unsigned long) robust_list; > >> bool ret = true; > >> > >> /* > >> @@ -526,6 +528,11 @@ bool __oom_reap_task_mm(struct mm_struct *mm) > >> if (vma->vm_flags & (VM_HUGETLB|VM_PFNMAP)) > >> continue; > >> > >> + if (vma->vm_start <= head && vma->vm_end > head) { > > > > This check as you have it is making assumptions about the length of the > > area at *robust_list and about that area's relation to the area > > represented by the vma. > > > > So if this is to be made more generic, we'd also need skip_area_len so > > we can perform a full overlap check. > Im not sure I follow here. Can a single MMAP call span multiple VMAs? The > address would be part of the pthread_t struct which is mmapped by the userspace > code. We are simply looking for that VMA and skipping the oom of it. It does not > try to find the individual locks (allocated separately and represented on a > LinkedList), it just prevents the reaping of the robust_list_head (part of > pthread_t) which stores the start of this LL. If some of the locks are private > (shared locks are not reaped) we may run into a case where this still fails; > however, we haven't been able to reproduce this. Well I was thinking that if it were to become a generic "ignore this region" then the code would need to be taught to skip any vma which had any form of overlap with that region. Which sounds like way overdesign until there's a demonstrated need.