linux-kernel - Re: [PATCH] mm: memcontrol: fix swap counter leak from offline cgroup

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: <CAMgjq7B5SyqYFbLhbgNCvQejqxVs5C6SaV_iot4P64EZLHZ8Gg@mail.gmail.com>
Date: Thu, 6 Mar 2025 10:54:12 +0800
From: Kairui Song <ryncsn@...il.com>
To: Muchun Song <songmuchun@...edance.com>
Cc: hannes@...xchg.org, mhocko@...nel.org, roman.gushchin@...ux.dev, 
	shakeel.butt@...ux.dev, muchun.song@...ux.dev, akpm@...ux-foundation.org, 
	chrisl@...nel.org, linux-kernel@...r.kernel.org, cgroups@...r.kernel.org, 
	linux-mm@...ck.org, stable@...r.kernel.org
Subject: Re: [PATCH] mm: memcontrol: fix swap counter leak from offline cgroup

On Thu, Mar 6, 2025 at 10:32 AM Muchun Song <songmuchun@...edance.com> wrote:
>
> The commit 6769183166b3 has removed the parameter of id from
> swap_cgroup_record() and get the memcg id from
> mem_cgroup_id(folio_memcg(folio)). However, the caller of it
> may update a different memcg's counter instead of
> folio_memcg(folio). E.g. in the caller of mem_cgroup_swapout(),
> @swap_memcg could be different with @memcg and update the counter
> of @swap_memcg, but swap_cgroup_record() records the wrong memcg's
> ID. When it is uncharged from __mem_cgroup_uncharge_swap(), the
> swap counter will leak since the wrong recorded ID. Fix it by
> bring the parameter of id back.
>
> Fixes: 6769183166b3 ("mm/swap_cgroup: decouple swap cgroup recording and clearing")
> Cc: <stable@...r.kernel.org>
> Signed-off-by: Muchun Song <songmuchun@...edance.com>
> ---
>  include/linux/swap_cgroup.h | 4 ++--
>  mm/memcontrol.c             | 4 ++--
>  mm/swap_cgroup.c            | 7 ++++---
>  3 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/swap_cgroup.h b/include/linux/swap_cgroup.h
> index b5ec038069dab..91cdf12190a03 100644
> --- a/include/linux/swap_cgroup.h
> +++ b/include/linux/swap_cgroup.h
> @@ -6,7 +6,7 @@
>
>  #if defined(CONFIG_MEMCG) && defined(CONFIG_SWAP)
>
> -extern void swap_cgroup_record(struct folio *folio, swp_entry_t ent);
> +extern void swap_cgroup_record(struct folio *folio, unsigned short id, swp_entry_t ent);
>  extern unsigned short swap_cgroup_clear(swp_entry_t ent, unsigned int nr_ents);
>  extern unsigned short lookup_swap_cgroup_id(swp_entry_t ent);
>  extern int swap_cgroup_swapon(int type, unsigned long max_pages);
> @@ -15,7 +15,7 @@ extern void swap_cgroup_swapoff(int type);
>  #else
>
>  static inline
> -void swap_cgroup_record(struct folio *folio, swp_entry_t ent)
> +void swap_cgroup_record(struct folio *folio, unsigned short id, swp_entry_t ent)
>  {
>  }
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index a5d870fbb4321..a5ab603806fbb 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4988,7 +4988,7 @@ void mem_cgroup_swapout(struct folio *folio, swp_entry_t entry)
>                 mem_cgroup_id_get_many(swap_memcg, nr_entries - 1);
>         mod_memcg_state(swap_memcg, MEMCG_SWAP, nr_entries);
>
> -       swap_cgroup_record(folio, entry);
> +       swap_cgroup_record(folio, mem_cgroup_id(swap_memcg), entry);
>
>         folio_unqueue_deferred_split(folio);
>         folio->memcg_data = 0;
> @@ -5050,7 +5050,7 @@ int __mem_cgroup_try_charge_swap(struct folio *folio, swp_entry_t entry)
>                 mem_cgroup_id_get_many(memcg, nr_pages - 1);
>         mod_memcg_state(memcg, MEMCG_SWAP, nr_pages);
>
> -       swap_cgroup_record(folio, entry);
> +       swap_cgroup_record(folio, mem_cgroup_id(memcg), entry);
>
>         return 0;
>  }
> diff --git a/mm/swap_cgroup.c b/mm/swap_cgroup.c
> index be39078f255be..1007c30f12e2c 100644
> --- a/mm/swap_cgroup.c
> +++ b/mm/swap_cgroup.c
> @@ -58,9 +58,11 @@ static unsigned short __swap_cgroup_id_xchg(struct swap_cgroup *map,
>   * entries must not have been charged
>   *
>   * @folio: the folio that the swap entry belongs to
> + * @id: mem_cgroup ID to be recorded
>   * @ent: the first swap entry to be recorded
>   */
> -void swap_cgroup_record(struct folio *folio, swp_entry_t ent)
> +void swap_cgroup_record(struct folio *folio, unsigned short id,
> +                       swp_entry_t ent)
>  {
>         unsigned int nr_ents = folio_nr_pages(folio);
>         struct swap_cgroup *map;
> @@ -72,8 +74,7 @@ void swap_cgroup_record(struct folio *folio, swp_entry_t ent)
>         map = swap_cgroup_ctrl[swp_type(ent)].map;
>
>         do {
> -               old = __swap_cgroup_id_xchg(map, offset,
> -                                           mem_cgroup_id(folio_memcg(folio)));
> +               old = __swap_cgroup_id_xchg(map, offset, id);
>                 VM_BUG_ON(old);
>         } while (++offset != end);
>  }
> --
> 2.20.1
>

Good catch, Thanks!

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