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

Marco Elver [off-list ref] writes:

On Wed, 24 Nov 2021 at 09:41, Huang, Ying [off-list ref] wrote:

quoted

Marco Elver [off-list ref] writes:

quoted

On Wed, 24 Nov 2021 at 02:44, Huang, Ying [off-list ref] wrote:

quoted

Marco Elver [off-list ref] writes:

quoted

On Tue, 23 Nov 2021 at 08:44, Huang Ying [off-list ref] wrote:

[...]

quoted

quoted

quoted

--- 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.

quoted

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel