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: Mon, 25 Mar 2024 12:30:03 -0400
From: Johannes Weiner <hannes@...xchg.org>
To: Yosry Ahmed <yosryahmed@...gle.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
	Zhongkun He <hezhongkun.hzk@...edance.com>,
	Chengming Zhou <zhouchengming@...edance.com>,
	Barry Song <21cnbao@...il.com>, Chris Li <chrisl@...nel.org>,
	Nhat Pham <nphamcs@...il.com>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm: zswap: fix data loss on SWP_SYNCHRONOUS_IO devices

On Sun, Mar 24, 2024 at 02:22:46PM -0700, Yosry Ahmed wrote:
> On Sun, Mar 24, 2024 at 2:04 PM Johannes Weiner <hannes@...xchg.org> wrote:
> >
> > Zhongkun He reports data corruption when combining zswap with zram.
> >
> > The issue is the exclusive loads we're doing in zswap. They assume
> > that all reads are going into the swapcache, which can assume
> > authoritative ownership of the data and so the zswap copy can go.
> >
> > However, zram files are marked SWP_SYNCHRONOUS_IO, and faults will try
> > to bypass the swapcache. This results in an optimistic read of the
> > swap data into a page that will be dismissed if the fault fails due to
> > races. In this case, zswap mustn't drop its authoritative copy.
> >
> > Link: https://lore.kernel.org/all/CACSyD1N+dUvsu8=zV9P691B9bVq33erwOXNTmEaUbi9DrDeJzw@mail.gmail.com/
> > Reported-by: Zhongkun He <hezhongkun.hzk@...edance.com>
> > Fixes: b9c91c43412f ("mm: zswap: support exclusive loads")
> > Cc: stable@...r.kernel.org      [6.5+]
> > Signed-off-by: Johannes Weiner <hannes@...xchg.org>
> > Tested-by: Zhongkun He <hezhongkun.hzk@...edance.com>
> 
> Do we also want to mention somewhere (commit log or comment) that
> keeping the entry in the tree is fine because we are still protected
> from concurrent loads/invalidations/writeback by swapcache_prepare()
> setting SWAP_HAS_CACHE or so?

I don't think it's necessary, as zswap isn't doing anything special
here. It's up to the caller to follow the generic swap exclusion
protocol that zswap also adheres to. So IMO the relevant comment
should be, and is, above that swapcache_prepare() in do_swap_page().

> Anyway, this LGTM.
> 
> Acked-by: Yosry Ahmed <yosryahmed@...gle.com>

Thanks!

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ