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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Wed, 30 May 2018 13:51:42 -0700 (PDT)
From:   David Rientjes <rientjes@...gle.com>
To:     Mike Kravetz <mike.kravetz@...cle.com>
cc:     Andrew Morton <akpm@...ux-foundation.org>,
        "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

Hi Mike,

On Tue, 29 May 2018, Mike Kravetz 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".
> 

Yes, I mentioned the backwards compatibility issue and I'm not sure there 
is a likely way around it.  But it's rather unfortunate that applications 
can become constrained in such a way that SIGBUS may be unavoidable if 
alloc_buddy_huge_page_with_mpol() cannot allocate from surplus and/or the 
hugetlb_cgroup limit is reached.  Not only are both racy, but applications 
prior to hugetlb_cgroup was introduced may have avoided SIGBUS by checking 
global hstate and are now limited to hugetlb_cgroup constraints 
unknowingly.  It's also not possible to avoid the SIGBUS by trying to 
terminate a lower priority process that has hugetlb reservations.

I'm not sure there is a path forward that can make this more 
deterministic.  We have customers who have reported receiving SIGBUS deep 
in their allocation stack using MAP_HUGETLB and were checking global 
hstate but were unaware of any hugetlb_cgroup restriction.

Andrew, please drop the patch.  I'd like to know if anybody has any ideas 
on how this can be more userspace friendly, however.