linux-kernel - Re: [PATCH v9] oom_kill.c: futex: Delay the OOM reaper to allow time for proper futex cleanup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5653e751-f81d-f64e-b4b5-b251949d13d9@redhat.com>
Date:   Thu, 21 Apr 2022 12:25:58 -0400
From:   Nico Pache <npache@...hat.com>
To:     Thomas Gleixner <tglx@...utronix.de>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>
Cc:     Rafael Aquini <aquini@...hat.com>,
        Waiman Long <longman@...hat.com>,
        "Herton R . Krzesinski" <herton@...hat.com>,
        Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        David Rientjes <rientjes@...gle.com>,
        Michal Hocko <mhocko@...e.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Davidlohr Bueso <dave@...olabs.net>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Joel Savitz <jsavitz@...hat.com>,
        Darren Hart <dvhart@...radead.org>, stable@...nel.org
Subject: Re: [PATCH v9] oom_kill.c: futex: Delay the OOM reaper to allow time
 for proper futex cleanup



On 4/21/22 10:40, Thomas Gleixner wrote:
> On Thu, Apr 14 2022 at 10:40, Nico Pache wrote:
>> The pthread struct is allocated on PRIVATE|ANONYMOUS memory [1] which can
>> be targeted by the oom reaper. This mapping is used to store the futex
>> robust list head; 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 free the memory of the futex robust list before
>> the exit path has handled the futex death:
>>
>>     CPU1                               CPU2
>> ------------------------------------------------------------------------
>>     page_fault
>>     do_exit "signal"
>>     wake_oom_reaper
>>                                         oom_reaper
>>                                         oom_reap_task_mm (invalidates mm)
>>     exit_mm
>>     exit_mm_release
>>     futex_exit_release
>>     futex_cleanup
>>     exit_robust_list
>>     get_user (EFAULT- can't access memory)
>>
>> If the get_user EFAULT's, the kernel will be unable to recover the
>> waiters on the robust_list, leaving userspace mutexes hung indefinitely.
>>
>> Delay the OOM reaper, allowing more time for the exit path to perform
>> the futex cleanup.
>>
>> Reproducer: https://gitlab.com/jsavitz/oom_futex_reproducer
>>
>> [1] https://elixir.bootlin.com/glibc/latest/source/nptl/allocatestack.c#L370
> 
> A link to the original discussion about this would be more useful than a
> code reference which is stale tomorrow. The above explanation is good
> enough to describe the problem.

Hi Andrew,

can you please update the link when you add the ACKs.

Here is a more stable link:
[1] https://elixir.bootlin.com/glibc/glibc-2.35/source/nptl/allocatestack.c#L370

> 
>>  
>> +/*
>> + * Give the OOM victim time to exit naturally before invoking the oom_reaping.
>> + * The timers timeout is arbitrary... the longer it is, the longer the worst
>> + * case scenario for the OOM can take. If it is too small, the oom_reaper can
>> + * get in the way and release resources needed by the process exit path.
>> + * e.g. The futex robust list can sit in Anon|Private memory that gets reaped
>> + * before the exit path is able to wake the futex waiters.
>> + */
>> +#define OOM_REAPER_DELAY (2*HZ)
>> +static void queue_oom_reaper(struct task_struct *tsk)
> 
> Bah. Did you run out of newlines? Glueing that define between the
> comment and the function is unreadable.

My Enter key hit its cgroup limit for newlines.

Andrew, would it be possible to also add a new line when you make the other
changes. Sorry about that.

> 
> Other than that.
> 
> Acked-by: Thomas Gleixner <tglx@...utronix.de>

Thanks!