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

[Huang, Ying]

Marco Elver <elver@xxxxxxxxxx> writes:

> On Wed, 24 Nov 2021 at 02:44, Huang, Ying <ying.huang@xxxxxxxxx> wrote:
>>
>> Marco Elver <elver@xxxxxxxxxx> writes:
>>
>> > On Tue, 23 Nov 2021 at 08:44, Huang Ying <ying.huang@xxxxxxxxx> 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?

Best Regards,
Huang, Ying