linux-kernel - Re: [PATCH] mm/rmap: fix potential batched TLB flush race

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]

Message-ID: <87pmqoaghq.fsf@yhuang6-desk2.ccr.corp.intel.com>
Date:   Thu, 25 Nov 2021 14:36:49 +0800
From:   "Huang, Ying" <ying.huang@...el.com>
To:     Marco Elver <elver@...gle.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org,
        syzbot+aa5bebed695edaccf0df@...kaller.appspotmail.com,
        Nadav Amit <namit@...are.com>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Will Deacon <will@...nel.org>, Yu Zhao <yuzhao@...gle.com>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] mm/rmap: fix potential batched TLB flush race

Marco Elver <elver@...gle.com> writes:

> On Wed, 24 Nov 2021 at 09:41, Huang, Ying <ying.huang@...el.com> wrote:
>>
>> Marco Elver <elver@...gle.com> writes:
>>
>> > On Wed, 24 Nov 2021 at 02:44, Huang, Ying <ying.huang@...el.com> wrote:
>> >>
>> >> Marco Elver <elver@...gle.com> writes:
>> >>
>> >> > On Tue, 23 Nov 2021 at 08:44, Huang Ying <ying.huang@...el.com> wrote:
>> > [...]
>> >> >> --- a/mm/rmap.c
>> >> >> +++ b/mm/rmap.c
>> >> >> @@ -633,7 +633,7 @@ static void set_tlb_ubc_flush_pending(struct mm_struct *mm, bool writable)
>> >> >>          * before the PTE is cleared.
>> >> >>          */
>> >> >>         barrier();
>> >> >> -       mm->tlb_flush_batched = true;
>> >> >> +       atomic_inc(&mm->tlb_flush_batched);
>> >> >
>> >> > The use of barrier() and atomic needs some clarification.
>> >>
>> >> There are some comments above barrier() to describe why it is needed.
>> >> For atomic, because the type of mm->tlb_flush_batched is atomic_t, do we
>> >> need extra clarification?
>> >
>> > Apologies, maybe I wasn't clear enough: the existing comment tells me
>> > the clearing of PTE should never happen after tlb_flush_batched is
>> > set, but only the compiler is considered. However, I become suspicious
>> > when I see barrier() paired with an atomic. barrier() is purely a
>> > compiler-barrier and does not prevent the CPU from reordering things.
>> > atomic_inc() does not return anything and is therefore unordered per
>> > Documentation/atomic_t.txt.
>> >
>> >> > Is there a
>> >> > requirement that the CPU also doesn't reorder anything after this
>> >> > atomic_inc() (which is unordered)? I.e. should this be
>> >> > atomic_inc_return_release() and remove barrier()?
>> >>
>> >> We don't have an atomic_xx_acquire() to pair with this.  So I guess we
>> >> don't need atomic_inc_return_release()?
>> >
>> > You have 2 things stronger than unordered: atomic_read() which result
>> > is used in a conditional branch, thus creating a control-dependency
>> > ordering later dependent writes; and the atomic_cmpxchg() is fully
>> > ordered.
>> >
>> > But before all that, I'd still want to understand what ordering
>> > requirements you have. The current comments say only the compiler
>> > needs taming, but does that mean we're fine with the CPU wildly
>> > reordering things?
>>
>> Per my understanding, atomic_cmpxchg() is fully ordered, so we have
>> strong ordering in flush_tlb_batched_pending().  And we use xchg() in
>> ptep_get_and_clear() (at least for x86) which is called before
>> set_tlb_ubc_flush_pending().  So we have strong ordering there too.
>>
>> So at least for x86, barrier() in set_tlb_ubc_flush_pending() appears
>> unnecessary.  Is it needed by other architectures?
>
> Hmm, this is not arch/ code -- this code needs to be portable.
> atomic_t accessors provide arch-independent guarantees. But do the
> other operations here provide any guarantees? If they don't, then I
> think we have to assume unordered.

Yes.  The analysis is for x86 only.  For other architectures, we need to
make sure the order of ptep_get_and_clear().

But anyway, that should be another patch.  This patch doesn't make the
original ordering weaker.

Best Regards,
Huang, Ying