Re: [PATCH v3 2/2] mm: attempt to batch free swap entries for zap_pte_range()

[Barry Song]

On Fri, Aug 16, 2024 at 6:29 AM Chris Li <chrisl@xxxxxxxxxx> wrote:
>
> Hi Barry,
>
> We got a crash report from syzbot that has been bisect into this change.
> > +static bool __swap_entries_free(struct swap_info_struct *si,
> > +               swp_entry_t entry, int nr)
> > +{
> > +       unsigned long offset = swp_offset(entry);
> > +       unsigned int type = swp_type(entry);
> > +       struct swap_cluster_info *ci;
> > +       bool has_cache = false;
> > +       unsigned char count;
> > +       int i;
> > +
> > +       if (nr <= 1 || swap_count(data_race(si->swap_map[offset])) != 1)
> > +               goto fallback;
> > +       /* cross into another cluster */
> > +       if (nr > SWAPFILE_CLUSTER - offset % SWAPFILE_CLUSTER)
> > +               goto fallback;
> > +
> > +       ci = lock_cluster_or_swap_info(si, offset);
> > +       if (!swap_is_last_map(si, offset, nr, &has_cache)) {
> > +               unlock_cluster_or_swap_info(si, ci);
> > +               goto fallback;
> > +       }
> > +       for (i = 0; i < nr; i++)
> > +               WRITE_ONCE(si->swap_map[offset + i], SWAP_HAS_CACHE);
> > +       unlock_cluster_or_swap_info(si, ci);
> > +
> > +       if (!has_cache) {
> > +               spin_lock(&si->lock);
> > +               swap_entry_range_free(si, entry, nr);
>
> Here it calls swap_entry_range_free() to free a range of the swap
> entry. However the swap_entry_range_free() has the assumption that all
> entries belong to the same folio and charge to the same memcg.
> It eventually pass down to swap_cgroup_record(), which BUG on this line:
>
> VM_BUG_ON(sc->id != old);
>
> The root cause is that the swap entries are not from the same memcg.
> Thankos Yosry for finding the root cause.
>
> > +               spin_unlock(&si->lock);
> > +       }
> > +       return has_cache;
> > +
> > +fallback:
> > +       for (i = 0; i < nr; i++) {
> > +               if (data_race(si->swap_map[offset + i])) {
> > +                       count = __swap_entry_free(si, swp_entry(type, offset + i));
> > +                       if (count == SWAP_HAS_CACHE)
> > +                               has_cache = true;
> > +               } else {
> > +                       WARN_ON_ONCE(1);
> > +               }
> > +       }
> > +       return has_cache;
> > +}
> > +
> >  /*
> >   * Drop the last HAS_CACHE flag of swap entries, caller have to
> >   * ensure all entries belong to the same cgroup.
> > @@ -1792,11 +1856,9 @@ void free_swap_and_cache_nr(swp_entry_t entry, int nr)
> >  {
> >         const unsigned long start_offset = swp_offset(entry);
> >         const unsigned long end_offset = start_offset + nr;
> > -       unsigned int type = swp_type(entry);
> >         struct swap_info_struct *si;
> >         bool any_only_cache = false;
> >         unsigned long offset;
> > -       unsigned char count;
> >
> >         if (non_swap_entry(entry))
> >                 return;
> > @@ -1811,15 +1873,7 @@ void free_swap_and_cache_nr(swp_entry_t entry, int nr)
> >         /*
> >          * First free all entries in the range.
> >          */
> > -       for (offset = start_offset; offset < end_offset; offset++) {
> > -               if (data_race(si->swap_map[offset])) {
> > -                       count = __swap_entry_free(si, swp_entry(type, offset));
> > -                       if (count == SWAP_HAS_CACHE)
> > -                               any_only_cache = true;
> > -               } else {
> > -                       WARN_ON_ONCE(1);
> > -               }
> > -       }
> > +       any_only_cache = __swap_entries_free(si, entry, nr);
>
> Here we are just doing a page table walk, there is no guarantee the
> 'nr' number of swap entries came from the same folio and previously
> charged to the same memcg. The swap_pte_batch() only checks they are
> the same swap type, does not check they charge to the same memcg.
>

Sorry for the trouble, thanks for the report, Yosry & Chris.
Does the below fix the problem? otherwise, we might remove
the assumption all swaps must belong to one swap_cgroup in
batch free?