linux-kernel - Re: [PATCH resend] mm/gup: fix try_grab_compound_head() race with split_huge

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] [day] [month] [year] [list]

Date:   Mon, 14 Jun 2021 17:38:18 -0700
From:   John Hubbard <jhubbard@...dia.com>
To:     Jann Horn <jannh@...gle.com>
CC:     Andrew Morton <akpm@...ux-foundation.org>,
        Linux-MM <linux-mm@...ck.org>,
        kernel list <linux-kernel@...r.kernel.org>,
        Matthew Wilcox <willy@...radead.org>,
        "Kirill A . Shutemov" <kirill@...temov.name>,
        Jan Kara <jack@...e.cz>, stable <stable@...r.kernel.org>
Subject: Re: [PATCH resend] mm/gup: fix try_grab_compound_head() race with
 split_huge_page()

On 6/13/21 9:47 PM, Jann Horn wrote:
...
>> The VM_WARN_ON_ONCE_PAGE() is not implemented exactly right
>> in the !CONFIG_DEBUG_VM case. IMHO it should follow the WARN*()
>> behavior, and return the original condition and keep going
>> in that case.
> 
> But the point of the existing definition is that the compiler can
> avoid generating code for the condition in !DEBUG_VM builds, even if
> it can't prove that the condition is free of side effects, right? If
> VM_WARN_ON_ONCE_PAGE() was changed as you propose, then I think that
> in mem_cgroup_page_lruvec(), the compiler would have to generate code
> for mem_cgroup_disabled(), which calls static_branch_likely(), which
> ends up in "asm volatile" statements; so the compiler probably won't
> be able to eliminate the condition.
> 
>> Then you could use it directly here.
> 
> Depending on whether the intended behavior here is to skip the check
> in !DEBUG_VM builds (which was the case before) or also perform the
> check in DEBUG_VM builds. And if DEBUG_VM is a config option because
> it might have some performance impact, isn't the cost of the check
> probably quite large compared to the cost of printing the warning on a
> codpath that should never execute?
> 

That's true for these VM_WARN*() macros, but not true for the more widely
used WARN*() macros. And I was hoping to bring VM macros closer to the
WARN macros. But as you point out, pre-existing callers expect to have
zero impact in !DEBUG_VM builds, and so some caution is required.

I feel like a separate set of macros would be reasonable. Something that
has WARN*() type of behavior, and accepts a struct page (which typically
means that WARN_ON_ONCE is required, because for pages you have to limit
it to that pretty much always).

thanks,
-- 
John Hubbard
NVIDIA