Re: [PATCH v2] mm: fix zswap writeback race condition

[Chris Li <chrisl@kernel.org>]

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