Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3a7dd0c4-db90-62e4-d7c7-246a4e3b9325@redhat.com>
Date:   Wed, 9 Mar 2022 14:34:49 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v4] mm/oom_kill.c: futex: Don't OOM reap a process with a
 futex robust list
Content-Language: en-US
To:     Michal Hocko <mhocko@suse.com>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Rafael Aquini <aquini@redhat.com>, Baoquan He <bhe@redhat.com>,
        Christoph von Recklinghausen <crecklin@redhat.com>,
        Don Dutile <ddutile@redhat.com>,
        "Herton R . Krzesinski" <herton@redhat.com>,
        David Rientjes <rientjes@google.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Andrew Morton <akpm@linux-foundation.org>, tglx@linutronix.de,
        mingo@redhat.com, dvhart@infradead.org, dave@stgolabs.net,
        andrealmeid@collabora.com, peterz@infradead.org,
        Joel Savitz <jsavitz@redhat.com>,
        Waiman Long <longman@redhat.com>
References: <20220309002550.103786-1-npache@redhat.com>
 <YiinJ3A6WoTJLN8d@dhcp22.suse.cz>
From:   Nico Pache <npache@redhat.com>
In-Reply-To: <YiinJ3A6WoTJLN8d@dhcp22.suse.cz>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 3/9/22 06:09, Michal Hocko wrote:
> On Tue 08-03-22 17:25:50, Nico Pache wrote:
>> The pthread struct is allocated on PRIVATE|ANONYMOUS memory [1] which can
>> be targeted by the oom reaper. This mapping is also used to store the futex
>> robust list; the kernel does not keep a copy of the robust list and instead
>> references a userspace address to maintain the robustness during a process
>> death. A race can occur between exit_mm and the oom reaper that allows
>> the oom reaper to clear the memory of the futex robust list before the
>> exit path has handled the futex death.
> 
> The above is missing the important part of the problem description. So
> the oom_reaper frees the memory which is backing the robust list. It
> would be useful to link that to the lockup on the futex.
That is basically what that last sentence in the paragraph is saying. perhaps I
should change clear -> free to be more verbose. Do you mean provide the race in
the standard format of:

    CPU1                CPU2
-------------------------------------------
    page_fault
    do_exit "signal"
    wake_oom_reaper	
			oom_reaper
                        oom_reap_task_mm (invalidates the private|anon mapping)
    exit_mm
    exit_mm_release
    futex_exit_release
    futex_cleanup
    exit_robust_list
    get_user (tries to access the memory we invalidated)
> 
>> Prevent the OOM reaper from concurrently reaping the mappings if the dying
>> process contains a robust_list. If the dying task_struct does not contain
>> a pointer in tsk->robust_list, we can assume there was either never one
>> setup for this task struct, or futex_cleanup has properly handled the
>> futex death and we can safely reap this memory.
> 
> I do agree with Waiman that this should go into a helper function.This
Ok I will do so in my v5.
> would be a quick workaround but I believe that it would be much better
> to either do the futex cleanup in the oom_reaper context if that could
> be done without blocking.

The futex cleanup is protected by two locks: a sleeping mutex and a spin_lock,
and both seem to be protecting a different race condition. If we create a
function like futex_try_exit_release that does a non-sleeping try_mutex call we
could solve the issue, but in the case that we fail to acquire the lock then we
leave the race window open. So from a correctness standpoint this approach falls
short.

 If that is really not feasible for some reason
> then we could skip over vmas which are backing the robust list. Have you
> considered any of those solutions?

We did explore some other possibilities such as using:
'&& !vma->vm_mm->owner->robust_list' in  __oom_reap_task_mm which did not work
for some reason but should have the same effect as this patch but with wasting
cycles checking all the VMA under a given process for a robust_list that we
could just check for in the parent caller. The only benefit I could see from
this approach would be that if halfway through checking the VMAs the robust list
gets set to NULL, we could then clear the other half of memory more quickly with
the oom reaper.

Unless you know of some other way to determine if that VMA is the pthread
mapping or a way to hide a certain mmap from the OOM reaper then I think this is
our best approach.

We have also considered implementing a lock between the futex_cleanup and the
oom_reaper that would have a similar effect to this patch but instead of
skipping the oom on a robust list we wait for the cleanup to finish then
continue ooming. Not sure if we want to introduce that complexity though.

Cheers,
-- Nico