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

[Marco Elver]

On Tue, 23 Nov 2021 at 08:44, Huang Ying <ying.huang@xxxxxxxxx> wrote:
>
> In theory, the following race is possible for batched TLB flushing.
>
> CPU0                               CPU1
> ----                               ----
> shrink_page_list()
>                                    unmap
>                                      zap_pte_range()
>                                        flush_tlb_batched_pending()
>                                          flush_tlb_mm()
>   try_to_unmap()
>     set_tlb_ubc_flush_pending()
>       mm->tlb_flush_batched = true
>                                          mm->tlb_flush_batched = false
>
> After the TLB is flushed on CPU1 via flush_tlb_mm() and before
> mm->tlb_flush_batched is set to false, some PTE is unmapped on CPU0
> and the TLB flushing is pended.  Then the pended TLB flushing will be
> lost.  Although both set_tlb_ubc_flush_pending() and
> flush_tlb_batched_pending() are called with PTL locked, different PTL
> instances may be used.
>
> Because the race window is really small, and the lost TLB flushing
> will cause problem only if a TLB entry is inserted before the
> unmapping in the race window, the race is only theoretical.  But the
> fix is simple and cheap too.

Thanks for fixing this!

> Syzbot has reported this too as follows,
>
> ==================================================================
> BUG: KCSAN: data-race in flush_tlb_batched_pending / try_to_unmap_one
[...]
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index c3a6e6209600..789778067db9 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -632,7 +632,7 @@ struct mm_struct {
>                 atomic_t tlb_flush_pending;
>  #ifdef CONFIG_ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH
>                 /* See flush_tlb_batched_pending() */
> -               bool tlb_flush_batched;
> +               atomic_t tlb_flush_batched;
>  #endif
>                 struct uprobes_state uprobes_state;
>  #ifdef CONFIG_PREEMPT_RT
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 163ac4e6bcee..60902c3cfb4a 100644
> --- 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. 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()?

>         /*
>          * If the PTE was dirty then it's best to assume it's writable. The
> @@ -680,15 +680,16 @@ static bool should_defer_flush(struct mm_struct *mm, enum ttu_flags flags)
>   */
>  void flush_tlb_batched_pending(struct mm_struct *mm)
>  {
> -       if (data_race(mm->tlb_flush_batched)) {
> -               flush_tlb_mm(mm);
> +       int batched = atomic_read(&mm->tlb_flush_batched);
>
> +       if (batched) {
> +               flush_tlb_mm(mm);
>                 /*
> -                * Do not allow the compiler to re-order the clearing of
> -                * tlb_flush_batched before the tlb is flushed.
> +                * If the new TLB flushing is pended during flushing,
> +                * leave mm->tlb_flush_batched as is, to avoid to lose
> +                * flushing.
>                  */
> -               barrier();
> -               mm->tlb_flush_batched = false;
> +               atomic_cmpxchg(&mm->tlb_flush_batched, batched, 0);
>         }
>  }
>  #else
> --
> 2.30.2
>