[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <defcaf0b-21a9-454f-b44a-c91820f541c8@linux.dev>
Date: Thu, 8 Feb 2024 10:41:49 +0800
From: Chengming Zhou <chengming.zhou@...ux.dev>
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: hannes@...xchg.org, yosryahmed@...gle.com, nphamcs@...il.com,
linux-mm@...ck.org, linux-kernel@...r.kernel.org,
Chengming Zhou <zhouchengming@...edance.com>, stable@...r.kernel.org,
Chris Li <chrisl@...nel.org>
Subject: Re: [PATCH v4] mm/zswap: invalidate old entry when store fail or
!zswap_enabled
On 2024/2/8 07:43, Andrew Morton wrote:
> On Wed, 7 Feb 2024 11:54:06 +0000 chengming.zhou@...ux.dev wrote:
>
>> From: Chengming Zhou <zhouchengming@...edance.com>
>>
>> We may encounter duplicate entry in the zswap_store():
>>
>> 1. swap slot that freed to per-cpu swap cache, doesn't invalidate
>> the zswap entry, then got reused. This has been fixed.
>>
>> 2. !exclusive load mode, swapin folio will leave its zswap entry
>> on the tree, then swapout again. This has been removed.
>>
>> 3. one folio can be dirtied again after zswap_store(), so need to
>> zswap_store() again. This should be handled correctly.
>>
>> So we must invalidate the old duplicate entry before insert the
>> new one, which actually doesn't have to be done at the beginning
>> of zswap_store(). And this is a normal situation, we shouldn't
>> WARN_ON(1) in this case, so delete it. (The WARN_ON(1) seems want
>> to detect swap entry UAF problem? But not very necessary here.)
>>
>> The good point is that we don't need to lock tree twice in the
>> store success path.
>>
>> Note we still need to invalidate the old duplicate entry in the
>> store failure path, otherwise the new data in swapfile could be
>> overwrite by the old data in zswap pool when lru writeback.
>>
>> We have to do this even when !zswap_enabled since zswap can be
>> disabled anytime. If the folio store success before, then got
>> dirtied again but zswap disabled, we won't invalidate the old
>> duplicate entry in the zswap_store(). So later lru writeback
>> may overwrite the new data in swapfile.
>>
>> Fixes: 42c06a0e8ebe ("mm: kill frontswap")
>> Cc: <stable@...r.kernel.org>
>
> We have a patch ordering issue.
>
> As a cc:stable hotfix, this should be merged into 6.8-rcX and later
> backported into -stable trees. So it will go
> mm-hotfixes-unstable->mm-hotfixes-stable->mainline. So someone has to
> make this patch merge and work against latest mm-hotfixes-unstable.
Ah, right. I just sent a fix based on mm-hotfixes-unstable [1], which
is split from this patch to only include bugfix, so easy to backport.
This patch actually include two parts: bugfix and a little optimization
for the zswap_store() normal case.
Should I split this patch into two small patches and resend based on
mm-unstable?
[1] https://lore.kernel.org/all/20240208023254.3873823-1-chengming.zhou@linux.dev/
>
> The patch you sent appears to be based on linux-next, so it has
> dependencies upon mm-unstable patches which won't be merged into
> mainline until the next merge window.
>
> So can you please redo and retest this against mm.git's
> mm-hotfixes-unstable branch? Then I'll try to figure out how to merge
> the gigentic pile of mm-unstable zswap changes on top of that.
>
> Thanks.
Powered by blists - more mailing lists