linux-kernel - Re: [PATCH 1/6] mm: swap: avoid losting cluster in swap_reclaim_full

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMgjq7Axk-rh+3hMpSisxKRU6W1tApgVA1H5zswC7z0q-t0-eg@mail.gmail.com>
Date: Sun, 23 Feb 2025 01:19:45 +0800
From: Kairui Song <ryncsn@...il.com>
To: Kemeng Shi <shikemeng@...weicloud.com>, akpm@...ux-foundation.org
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/6] mm: swap: avoid losting cluster in swap_reclaim_full_clusters()

On Sat, Feb 22, 2025 at 3:12 PM Kemeng Shi <shikemeng@...weicloud.com> wrote:
>
> If no swap cache is reclaimed, cluster taken off from full_clusters list
> will not be put in any list and may not be reused. Do relocate_cluster
> for such cluster to fix the issue.
>
> Fixes: 3b644773eefda ("mm, swap: reduce contention on device lock")
> Signed-off-by: Kemeng Shi <shikemeng@...weicloud.com>
> ---
>  mm/swapfile.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 34baefb000b5..e5f58ab86329 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -861,6 +861,10 @@ static void swap_reclaim_full_clusters(struct swap_info_struct *si, bool force)
>                         offset++;
>                 }
>
> +               /* in case no swap cache is reclaimed */
> +               if (ci->flags == CLUSTER_FLAG_NONE)
> +                       relocate_cluster(si, ci);
> +
>                 unlock_cluster(ci);
>                 if (to_scan <= 0)
>                         break;
> --
> 2.30.0

Thanks. A little nick pick, "losting" is not a word, I think you mean "leaking".

And BTW maybe it's better to describe the result of this leak in a bit
more details, "cluster leaking from lists" and "will not be reused"
looked a bit scary at a glance to me. But realizing it's full
clusters, they will be moved back to a list if any slots on them are
freed, so the worst result is inefficiently reclaiming of HAS_CACHE
slots, we didn't really lose these clusters.

We do need to fix it though. So other than the commit summary and
message nitpick:

Reviewed-by: Kairui Song <kasong@...cent.com>