| 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
| ||
|
Message-Id: <20240815215308.55233-1-21cnbao@gmail.com>
Date: Fri, 16 Aug 2024 09:53:08 +1200
From: Barry Song <21cnbao@...il.com>
To: chrisl@...nel.org
Cc: akpm@...ux-foundation.org,
david@...hat.com,
hughd@...gle.com,
justinjiang@...o.com,
kaleshsingh@...gle.com,
kasong@...cent.com,
linux-kernel@...r.kernel.org,
linux-mm@...ck.org,
ryan.roberts@....com,
v-songbaohua@...o.com,
ying.huang@...el.com,
yosryahmed@...gle.com
Subject: Re: [PATCH v3 2/2] mm: attempt to batch free swap entries for zap_pte_range()
On Fri, Aug 16, 2024 at 6:29 AM Chris Li <chrisl@...nel.org> 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?
>From c68e0d780ba808da4bb682b753e3fa77c4f96e13 Mon Sep 17 00:00:00 2001
From: Barry Song <v-songbaohua@...o.com>
Date: Fri, 16 Aug 2024 09:36:23 +1200
Subject: [PATCH] mm: check all swaps belong to same swap_cgroup in
swap_pte_batch()
Right now, it is possible two folios are contiguous in swap slots
but they don't belong to one memcg. In this case, even we return
a large nr, we can't really batch free all slots.
Reported-by: Yosry Ahmed <yosryahmed@...gle.com>
Reported-by: Chris Li <chrisl@...nel.org>
Signed-off-by: Barry Song <v-songbaohua@...o.com>
---
mm/internal.h | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/mm/internal.h b/mm/internal.h
index adbf8c88c9df..d1f1e221212d 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -15,6 +15,7 @@
#include <linux/rmap.h>
#include <linux/swap.h>
#include <linux/swapops.h>
+#include <linux/swap_cgroup.h>
#include <linux/tracepoint-defs.h>
/* Internal core VMA manipulation functions. */
@@ -275,18 +276,22 @@ static inline int swap_pte_batch(pte_t *start_ptep, int max_nr, pte_t pte)
{
pte_t expected_pte = pte_next_swp_offset(pte);
const pte_t *end_ptep = start_ptep + max_nr;
+ swp_entry_t entry = pte_to_swp_entry(pte);
pte_t *ptep = start_ptep + 1;
+ unsigned short cgroup_id;
VM_WARN_ON(max_nr < 1);
VM_WARN_ON(!is_swap_pte(pte));
- VM_WARN_ON(non_swap_entry(pte_to_swp_entry(pte)));
+ VM_WARN_ON(non_swap_entry(entry));
+ cgroup_id = lookup_swap_cgroup_id(entry);
while (ptep < end_ptep) {
pte = ptep_get(ptep);
if (!pte_same(pte, expected_pte))
break;
-
+ if (lookup_swap_cgroup_id(pte_to_swp_entry(pte)) != cgroup_id)
+ break;
expected_pte = pte_next_swp_offset(expected_pte);
ptep++;
}
--
2.34.1
> Chris
>
> >
> > /*
> > * Short-circuit the below loop if none of the entries had their
> > --
> > 2.34.1
> >
Thanks
Barry
Powered by blists - more mailing lists