| 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: <CAMgjq7AcMtsS-EX0065jvucLR=YiAvPkjp+rmr=hfxyv9JVW5g@mail.gmail.com>
Date: Mon, 26 May 2025 01:08:56 +0800
From: Kairui Song <ryncsn@...il.com>
To: Kemeng Shi <shikemeng@...weicloud.com>
Cc: akpm@...ux-foundation.org, bhe@...hat.com, hannes@...xchg.org,
linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] mm: swap: correctly use maxpages in swapon syscall to
avoid potensial deadloop
On Thu, May 22, 2025 at 11:32 AM Kemeng Shi <shikemeng@...weicloud.com> wrote:
>
> We use maxpages from read_swap_header() to initialize swap_info_struct,
> however the maxpages might be reduced in setup_swap_extents() and the
> si->max is assigned with the reduced maxpages from the
> setup_swap_extents().
>
> Obviously, this could lead to memory waste as we allocated memory based on
> larger maxpages, besides, this could lead to a potensial deadloop as
> following:
> 1) When calling setup_clusters() with larger maxpages, unavailable pages
> within range [si->max, larger maxpages) are not accounted with
> inc_cluster_info_page(). As a result, these pages are assumed available
> but can not be allocated. The cluster contains these pages can be moved
> to frag_clusters list after it's all available pages were allocated.
> 2) When the cluster mentioned in 1) is the only cluster in frag_clusters
> list, cluster_alloc_swap_entry() assume order 0 allocation will never
> failed and will enter a deadloop by keep trying to allocate page from the
> only cluster in frag_clusters which contains no actually available page.
>
> Call setup_swap_extents() to get the final maxpages before swap_info_struct
> initialization to fix the issue.
>
> Fixes: 661383c6111a3 ("mm: swap: relaim the cached parts that got scanned")
>
> Signed-off-by: Kemeng Shi <shikemeng@...weicloud.com>
> ---
> mm/swapfile.c | 47 ++++++++++++++++++++---------------------------
> 1 file changed, 20 insertions(+), 27 deletions(-)
>
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 75b69213c2e7..a82f4ebefca3 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -3141,43 +3141,30 @@ static unsigned long read_swap_header(struct swap_info_struct *si,
> return maxpages;
> }
>
> -static int setup_swap_map_and_extents(struct swap_info_struct *si,
> - union swap_header *swap_header,
> - unsigned char *swap_map,
> - unsigned long maxpages,
> - sector_t *span)
> +static int setup_swap_map(struct swap_info_struct *si,
> + union swap_header *swap_header,
> + unsigned char *swap_map,
> + unsigned long maxpages)
> {
> - unsigned int nr_good_pages;
> unsigned long i;
> - int nr_extents;
> -
> - nr_good_pages = maxpages - 1; /* omit header page */
>
> + swap_map[0] = SWAP_MAP_BAD; /* omit header page */
> for (i = 0; i < swap_header->info.nr_badpages; i++) {
> unsigned int page_nr = swap_header->info.badpages[i];
> if (page_nr == 0 || page_nr > swap_header->info.last_page)
> return -EINVAL;
> if (page_nr < maxpages) {
> swap_map[page_nr] = SWAP_MAP_BAD;
> - nr_good_pages--;
> + si->pages--;
> }
> }
>
> - if (nr_good_pages) {
> - swap_map[0] = SWAP_MAP_BAD;
> - si->max = maxpages;
> - si->pages = nr_good_pages;
> - nr_extents = setup_swap_extents(si, span);
> - if (nr_extents < 0)
> - return nr_extents;
> - nr_good_pages = si->pages;
> - }
> - if (!nr_good_pages) {
> + if (!si->pages) {
> pr_warn("Empty swap-file\n");
> return -EINVAL;
> }
>
>
> - return nr_extents;
> + return 0;
> }
>
> #define SWAP_CLUSTER_INFO_COLS \
> @@ -3217,7 +3204,7 @@ static struct swap_cluster_info *setup_clusters(struct swap_info_struct *si,
> * Mark unusable pages as unavailable. The clusters aren't
> * marked free yet, so no list operations are involved yet.
> *
> - * See setup_swap_map_and_extents(): header page, bad pages,
> + * See setup_swap_map(): header page, bad pages,
> * and the EOF part of the last cluster.
> */
> inc_cluster_info_page(si, cluster_info, 0);
> @@ -3354,6 +3341,15 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
> goto bad_swap_unlock_inode;
> }
>
> + si->max = maxpages;
> + si->pages = maxpages - 1;
> + nr_extents = setup_swap_extents(si, &span);
> + if (nr_extents < 0) {
> + error = nr_extents;
> + goto bad_swap_unlock_inode;
> + }
> + maxpages = si->max;
There seems to be a trivial problem here, previously the si->pages
will be seen by swap_activate after bad blocks have been counted and
si->pages means the actual available slots. But now si->pages will be
seen by swap_active as `maxpages - 1`.
One current side effect now is the span value will not be updated
properly so the pr_info in swap on may print a larger value, if the
swap header contains badblocks and swapfile is on nfs/cifs.
This should not be a problem but it's better to mention or add
comments about it.
And I think it's better to add a sanity check here to check if
si->pages still equal to si->max - 1, setup_swap_map_and_extents /
setup_swap_map assumes the header section was already counted. This
also helps indicate the setup_swap_extents may shrink and modify these
two values.
BTW, I was thinking that we should get rid of the whole extents design
after the swap table series is ready, so mTHP allocation will be
usable for swap over fs too.
> /* OK, set up the swap map and apply the bad block list */
> swap_map = vzalloc(maxpages);
> if (!swap_map) {
> @@ -3365,12 +3361,9 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
> if (error)
> goto bad_swap_unlock_inode;
>
> - nr_extents = setup_swap_map_and_extents(si, swap_header, swap_map,
> - maxpages, &span);
> - if (unlikely(nr_extents < 0)) {
> - error = nr_extents;
> + error = setup_swap_map(si, swap_header, swap_map, maxpages);
> + if (error)
> goto bad_swap_unlock_inode;
> - }
>
> /*
> * Use kvmalloc_array instead of bitmap_zalloc as the allocation order might
> --
> 2.30.0
>
Other than that:
Reviewed-by: Kairui Song <kasong@...cent.com>
Powered by blists - more mailing lists