Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 4 May 2023 00:22:53 -0700
From:   Chris Li <chrisl@kernel.org>
To:     Johannes Weiner <hannes@cmpxchg.org>
Cc:     Domenico Cerasuolo <cerasuolodomenico@gmail.com>,
        sjenning@redhat.com, ddstreet@ieee.org, vitaly.wool@konsulko.com,
        minchan@kernel.org, ngupta@vflare.org, akpm@linux-foundation.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] mm: fix zswap writeback race condition
Message-ID: <ZFNdTfki5HWmne2j@google.com>
References: <20230503151200.19707-1-cerasuolodomenico@gmail.com>
 <ZFLZQ/hJarV+iNYd@google.com>
 <20230504022904.GA202700@cmpxchg.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230504022904.GA202700@cmpxchg.org>
Precedence: bulk

On Wed, May 03, 2023 at 10:29:04PM -0400, Johannes Weiner wrote:
> > >  
> > >  	case ZSWAP_SWAPCACHE_NEW: /* page is locked */
> 
>                                   ^^^^^^^^^^^^^^^^^^^^
> 
> > > +		/*
> > > +		 * Having a local reference to the zswap entry doesn't exclude
> > > +		 * swapping from invalidating and recycling the swap slot. Once
> > > +		 * the swapcache is secured against concurrent swapping to and
> > > +		 * from the slot, recheck that the entry is still current before
> > > +		 * writing.
> > > +		 */
> > > +		spin_lock(&tree->lock);
> > > +		if (zswap_rb_search(&tree->rbroot, entry->offset) != entry) {
> > > +			spin_unlock(&tree->lock);
> > > +			delete_from_swap_cache(page_folio(page));
> > > +			ret = -ENOMEM;
> > > +			goto fail;
> > > +		}
> > > +		spin_unlock(&tree->lock);
> > > +
> > 
> > The race condition is still there, just making it much harder to hit.
> > What happens after you perform the rb tree search, release tree lock.
> > Then the entry gets invalid and recycled right here before the decompress
> > step?
> 
> Recyling can only happen up until we see ZSWAP_SWAPCACHE_NEW.
> 
> Once we see it, we're holding the page lock* on a new swapcache page
> for a valid, in-use** swp_entry_t.
> 
> The lock of the swapcache page prevents swapin, which would be
> required for the count to drop and the entry to be recycled.

Thanks for the explain. I miss the locked page will prevent swapin part.

> __read_swap_cache_async() checked that the entry is valid, so the slot
> cannot be allocated to someone else.
> 
> Now we just have to check if that entry is the right one, iow the slot
> wasn't recycled.
> 
> If the slot wasn't recycled, we know we have the right data and we can
> start the IO and unlock the page. (After that swapins can continue and
> the data can change, but regular writeback vs redirtying rules apply.)
> 
> If the slot was indeed recycled before we get ZSWAP_SWAPCACHE_NEW, we
> see the mismatch, delete the page from the swapcache and unlock it. A
> racing do_swap_page() may have found and reffed the page in swapcache,
> and acquire the page lock after us; but it'll see it's no longer in
> the swapcache, drop the reference (free the page) and retry the fault.

LGTM then. Please feel free to add:

Reviewed-by: Chris Li (Google) <chrisl@kernel.org>

Chris