linux-kernel - Re: [PATCH] mm: zswap: add VM_BUG_ON() if large folio swapin is attempted

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]

Date: Fri, 7 Jun 2024 20:52:47 +0200
From: David Hildenbrand <david@...hat.com>
To: Yosry Ahmed <yosryahmed@...gle.com>
Cc: Barry Song <21cnbao@...il.com>, Andrew Morton
 <akpm@...ux-foundation.org>, Johannes Weiner <hannes@...xchg.org>,
 Nhat Pham <nphamcs@...il.com>, Chengming Zhou <chengming.zhou@...ux.dev>,
 Baolin Wang <baolin.wang@...ux.alibaba.com>, Chris Li <chrisl@...nel.org>,
 Ryan Roberts <ryan.roberts@....com>, Matthew Wilcox <willy@...radead.org>,
 linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm: zswap: add VM_BUG_ON() if large folio swapin is
 attempted

>> I have no strong opinion on this one, but likely a VM_WARN_ON would also
>> be sufficient to find such issues early during testing. No need to crash
>> the machine.
> 
> I thought VM_BUG_ON() was less frowned-upon than BUG_ON(), but after
> some digging I found your patches to checkpatch and Linus clearly
> stating that it isn't.

:) yes.

VM_BUG_ON is not particularly helpful IMHO. If you want something to be 
found early during testing, VM_WARN_ON is good enough.

Ever since Fedora stopped enabling CONFIG_DEBUG_VM, VM_* friends are 
primarily reported during early/development testing only. But maybe some 
distro out there still sets it.

> 
> How about something like the following (untested), it is the minimal
> recovery we can do but should work for a lot of cases, and does
> nothing beyond a warning if we can swapin the large folio from disk:
> 
> diff --git a/mm/page_io.c b/mm/page_io.c
> index f1a9cfab6e748..8f441dd8e109f 100644
> --- a/mm/page_io.c
> +++ b/mm/page_io.c
> @@ -517,7 +517,6 @@ void swap_read_folio(struct folio *folio, struct
> swap_iocb **plug)
>          delayacct_swapin_start();
> 
>          if (zswap_load(folio)) {
> -               folio_mark_uptodate(folio);
>                  folio_unlock(folio);
>          } else if (data_race(sis->flags & SWP_FS_OPS)) {
>                  swap_read_folio_fs(folio, plug);
> diff --git a/mm/zswap.c b/mm/zswap.c
> index 6007252429bb2..cc04db6bb217e 100644
> --- a/mm/zswap.c
> +++ b/mm/zswap.c
> @@ -1557,6 +1557,22 @@ bool zswap_load(struct folio *folio)
> 
>          VM_WARN_ON_ONCE(!folio_test_locked(folio));
> 
> +       /*
> +        * Large folios should not be swapped in while zswap is being used, as
> +        * they are not properly handled.
> +        *
> +        * If any of the subpages are in zswap, reading from disk would result
> +        * in data corruption, so return true without marking the folio uptodate
> +        * so that an IO error is emitted (e.g. do_swap_page() will sigfault).
> +        *
> +        * Otherwise, return false and read the folio from disk.
> +        */
> +       if (WARN_ON_ONCE(folio_test_large(folio))) {
> +               if (xa_find(tree, &offset, offset +
> folio_nr_pages(folio) - 1, 0))
> +                       return true;
> +               return false;
> +       }
> +
>          /*
>           * When reading into the swapcache, invalidate our entry. The
>           * swapcache can be the authoritative owner of the page and
> @@ -1593,7 +1609,7 @@ bool zswap_load(struct folio *folio)
>                  zswap_entry_free(entry);
>                  folio_mark_dirty(folio);
>          }
> -
> +       folio_mark_uptodate(folio);
>          return true;
>   }
> 
> One problem is that even if zswap was never enabled, the warning will
> be emitted just if CONFIG_ZSWAP is on. Perhaps we need a variable or
> static key if zswap was "ever" enabled.

We should use WARN_ON_ONCE() only for things that cannot happen. So if 
there are cases where this could be triggered today, it would be 
problematic -- especially if it can be triggered from unprivileged user 
space. But if we're concerned of other code messing up our invariant in 
the future (e.g., enabling large folios without taking proper care about 
zswap etc), we're good to add it.

-- 
Cheers,

David / dhildenb