Re: [PATCH] zswap: do not crash the kernel on decompression failure

[Yosry Ahmed <yosry.ahmed@linux.dev>]

On Tue, Feb 25, 2025 at 10:57:30PM -0500, Johannes Weiner wrote:
> On Wed, Feb 26, 2025 at 02:45:41AM +0000, Yosry Ahmed wrote:
> > On Tue, Feb 25, 2025 at 07:51:49PM -0500, Johannes Weiner wrote:
> > > On Tue, Feb 25, 2025 at 01:32:00PM -0800, Nhat Pham wrote:
> > > > +	}
> > > >  	mutex_unlock(&acomp_ctx->mutex);
> > > >  
> > > >  	if (src != acomp_ctx->buffer)
> > > >  		zpool_unmap_handle(zpool, entry->handle);
> > > > +	return ret;
> > > >  }
> > > >  
> > > >  /*********************************
> > > > @@ -1018,6 +1028,7 @@ static int zswap_writeback_entry(struct zswap_entry *entry,
> > > >  	struct writeback_control wbc = {
> > > >  		.sync_mode = WB_SYNC_NONE,
> > > >  	};
> > > > +	int ret = 0;
> > > >  
> > > >  	/* try to allocate swap cache folio */
> > > >  	mpol = get_task_policy(current);
> > > > @@ -1034,8 +1045,8 @@ static int zswap_writeback_entry(struct zswap_entry *entry,
> > > >  	 * and freed when invalidated by the concurrent shrinker anyway.
> > > >  	 */
> > > >  	if (!folio_was_allocated) {
> > > > -		folio_put(folio);
> > > > -		return -EEXIST;
> > > > +		ret = -EEXIST;
> > > > +		goto put_folio;
> > > >  	}
> > > >  
> > > >  	/*
> > > > @@ -1048,14 +1059,17 @@ static int zswap_writeback_entry(struct zswap_entry *entry,
> > > >  	 * be dereferenced.
> > > >  	 */
> > > >  	tree = swap_zswap_tree(swpentry);
> > > > -	if (entry != xa_cmpxchg(tree, offset, entry, NULL, GFP_KERNEL)) {
> > > > -		delete_from_swap_cache(folio);
> > > > -		folio_unlock(folio);
> > > > -		folio_put(folio);
> > > > -		return -ENOMEM;
> > > > +	if (entry != xa_load(tree, offset)) {
> > > > +		ret = -ENOMEM;
> > > > +		goto fail;
> > > >  	}
> > > >  
> > > > -	zswap_decompress(entry, folio);
> > > > +	if (!zswap_decompress(entry, folio)) {
> > > > +		ret = -EIO;
> > > > +		goto fail;
> > > > +	}
> > > > +
> > > > +	xa_erase(tree, offset);
> > > >  
> > > >  	count_vm_event(ZSWPWB);
> > > >  	if (entry->objcg)
> > > > @@ -1071,9 +1085,14 @@ static int zswap_writeback_entry(struct zswap_entry *entry,
> > > >  
> > > >  	/* start writeback */
> > > >  	__swap_writepage(folio, &wbc);
> > > > -	folio_put(folio);
> > > > +	goto put_folio;
> > > >  
> > > > -	return 0;
> > > > +fail:
> > > > +	delete_from_swap_cache(folio);
> > > > +	folio_unlock(folio);
> > > > +put_folio:
> > > > +	folio_put(folio);
> > > > +	return ret;
> > > >  }
> > > 
> > > Nice, yeah it's time for factoring out the error unwinding. If you
> > > write it like this, you can save a jump in the main sequence:
> > > 
> > > 	__swap_writepage(folio, &wbc);
> > > 	ret = 0;
> > > put:
> > > 	folio_put(folio);
> > > 	return ret;
> > > delete_unlock:
> > 
> > (I like how you sneaked the label rename in here, I didn't like 'fail'
> > either :P)
> > 
> > > 	delete_from_swap_cache(folio);
> > > 	folio_unlock(folio);
> > > 	goto put;
> > 
> > I would go even further and avoid gotos completely (and make it super
> > clear what gets executed in the normal path vs the failure path):
> > 
> > 	__swap_writepage(folio, &wbc);
> > 	folio_put(folio);
> > 	if (ret) {
> > 		delete_from_swap_cache(folio);
> > 		folio_unlock(folio);
> > 	}
> > 	return ret;
> 
> The !folio_was_allocated case only needs the put. I guess that could
> stay open-coded.

We also do:

	if (ret && ret != -EEXIST) {
		...
	}

or

	if (ret && !folio_was_allocated) {
		...
	}

Probably the latter is clearer. If the folio was already there, we
didn't add it to the swapcache or lock it ourselves so we don't unwind
that.

> 
> And I think you still need one goto for the other two error legs to
> jump past the __swap_writepage.

Oh yeah I meant eliminate the jumps within the cleanup/return code, not
entirely. Sorry for the confusion. We still need an 'out' label or
similar after __swap_writepage().

> 
> > > Something like this?
> > > 
> > > 	if (!zswap_decompress(entry, folio)) {
> > > 		/*
> > > 		 * The zswap_load() return value doesn't indicate success or
> > > 		 * failure, but whether zswap owns the swapped out contents.
> > > 		 * This MUST return true here, otherwise swap_readpage() will
> > > 		 * read garbage from the backend.
> > > 		 *
> > > 		 * Success is signaled by marking the folio uptodate.
> > > 		 */
> > 
> > We use the same trick in the folio_test_large() branch, so maybe this
> > should be moved to above the function definition. Then we can perhaps
> > refer to it in places where we return true wihout setting uptodate for
> > added clarity if needed.
> 
> That makes sense to me. Nhat, what do you think?