linux-kernel - Re: [patch] mm, hugetlb_cgroup: suppress SIGBUS when hugetlb

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 for Android: free password hash cracker in your pocket

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

Date:   Tue, 29 May 2018 11:13:06 -0700
From:   Mike Kravetz <mike.kravetz@...cle.com>
To:     David Rientjes <rientjes@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Cc:     "Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>,
        Naoya Horiguchi <n-horiguchi@...jp.nec.com>,
        Vlastimil Babka <vbabka@...e.cz>, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org
Subject: Re: [patch] mm, hugetlb_cgroup: suppress SIGBUS when hugetlb_cgroup
 charge fails

On 05/25/2018 01:16 PM, David Rientjes wrote:
> When charging to a hugetlb_cgroup fails, alloc_huge_page() returns
> ERR_PTR(-ENOSPC) which will cause VM_FAULT_SIGBUS to be returned to the
> page fault handler.
> 
> Instead, return the proper error code, ERR_PTR(-ENOMEM), so VM_FAULT_OOM
> is handled correctly.  This is consistent with failing mem cgroup charges
> in the non-hugetlb fault path.

Apologies for the late reply.

I am not %100 sure we want to make this change.  When hugetlb cgroup support
was added by Aneesh, the intention was for the application to get SIGBUS.

commit 2bc64a204697
https://lwn.net/Articles/499255/

Since the code has always caused SIGBUS when exceeding cgroup limit, there
may be applications depending on this behavior.  I would be especially
concerned with HPC applications which were the original purpose for adding
the feature.

Perhaps, the original code should have returned ENOMEM to be consistent as
in your patch.  That does seem to be the more correct behavior.  But, do we
want to change behavior now (admittedly undocumented) and potentially break
some application?

I echo Michal's question about the reason for the change.  If there is a
real problem or issue to solve, that makes more of a case for making the
change.  If it is simply code/behavior cleanup for consistency then I would
suggest not making the change, but rather documenting this as another
hugetlbfs "special behavior".

As a quick test, I added a shell to a hugetlb cgroup with 2 huge page
limit and started a task using huge pages.

Current behavior
----------------
ssh_to_dbg # sudo ./test_mmap 4
mapping 4 huge pages
address 7f62bba00000 read (-)
address 7f62bbc00000 read (-)
Bus error
ssh_to_dbg #

Behavior with patch
-------------------
ssh_to_dbg # sudo ./test_mmap 4
mapping 4 huge pages
address 7f62bba00000 read (-)
address 7f62bbc00000 read (-)
Connection to dbg closed by remote host.
Connection to dbf closed.

OOM did kick in (lots of console/log output) and killed the shell
as well.
-- 
Mike Kravetz