Received-SPF: pass (google.com: domain of linux-kernel+bounces-57390-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <9dec8d00-c01b-44fe-983a-ec2b83d71e2a@linux.dev>
Date: Thu, 8 Feb 2024 10:34:54 +0800
Precedence: bulk
MIME-Version: 1.0
Subject: Re: [PATCH v4] mm/zswap: invalidate old entry when store fail or
 !zswap_enabled
Content-Language: en-US
To: Nhat Pham <nphamcs@gmail.com>
Cc: hannes@cmpxchg.org, yosryahmed@google.com, akpm@linux-foundation.org,
 linux-mm@kvack.org, linux-kernel@vger.kernel.org,
 Chengming Zhou <zhouchengming@bytedance.com>, stable@vger.kernel.org,
 Chris Li <chrisl@kernel.org>
References: <20240207115406.3865746-1-chengming.zhou@linux.dev>
 <CAKEwX=NR+QN1P960_Qz_sHACoD8FK=Kb=LQw==JGR+9h73SENg@mail.gmail.com>
From: Chengming Zhou <chengming.zhou@linux.dev>
In-Reply-To: <CAKEwX=NR+QN1P960_Qz_sHACoD8FK=Kb=LQw==JGR+9h73SENg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On 2024/2/8 07:06, Nhat Pham wrote:
> On Wed, Feb 7, 2024 at 3:54 AM <chengming.zhou@linux.dev> wrote:
>>
>> From: Chengming Zhou <zhouchengming@bytedance.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@vger.kernel.org>
>> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
>> Acked-by: Yosry Ahmed <yosryahmed@google.com>
>> Acked-by: Chris Li <chrisl@kernel.org>
>> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
> 
> Acked-by: Nhat Pham <nphamcs@gmail.com>
> 
> Sorry for being late to the party, and thanks for fixing this, Chengming!

Thanks for your review! :)