Re: [BUG REPORT] ZSWAP: theoretical race condition issues

[Bob Liu <bob.liu@oracle.com>]

Hi Weijie,

On 08/19/2013 12:14 AM, Weijie Yang wrote:
> I found a few bugs in zswap when I review Linux-3.11-rc5, and I have
> also some questions about it, described as following:
> 
> BUG:
> 1. A race condition when reclaim a page
> when a handle alloced from zbud, zbud considers this handle is used
> validly by upper(zswap) and can be a candidate for reclaim.
> But zswap has to initialize it such as setting swapentry and addding
> it to rbtree. so there is a race condition, such as:
> thread 0: obtain handle x from zbud_alloc
> thread 1: zbud_reclaim_page is called
> thread 1: callback zswap_writeback_entry to reclaim handle x
> thread 1: get swpentry from handle x (it is random value now)
> thread 1: bad thing may happen
> thread 0: initialize handle x with swapentry

Yes, this may happen potentially but in rare case.
Because we have a LRU list for page frames, after Thread 0 called
zbud_alloc the corresponding page will be add to the head of LRU
list,While zbud_reclaim_page(Thread 1 called) is started from the tail
of LRU list.

> Of course, this situation almost never happen, it is a "theoretical
> race condition" issue.
> 
> 2. Pollute swapcache data by add a pre-invalided swap page
> when a swap_entry is invalidated, it will be reused by other anon
> page. At the same time, zswap is reclaiming old page, pollute
> swapcache of new page as a result, because old page and new page use
> the same swap_entry, such as:
> thread 1: zswap reclaim entry x
> thread 0: zswap_frontswap_invalidate_page entry x
> thread 0: entry x reused by other anon page
> thread 1: add old data to swapcache of entry x

I didn't get your idea here, why thread1 will add old data to entry x?

> thread 0: swapcache of entry x is polluted
> Of course, this situation almost never happen, it is another
> "theoretical race condition" issue.
> 
> 3. Frontswap uses frontswap_map bitmap to track page in "backend"
> implementation, when zswap reclaim a
> page, the corresponding bitmap record is not cleared.
>

That's true, but I don't think it's a big problem.
Only waste little time to search rbtree during zswap_frontswap_load().

> 4. zswap_tree is not freed when swapoff, and it got re-kzalloc in
> swapon, memory leak occurs.

Nice catch! I think it should be freed in zswap_frontswap_invalidate_area().

> 
> questions:
> 1. How about SetPageReclaim befor __swap_writepage, so that move it to
> the tail of the inactive list?

It will be added to inactive now.

> 2. zswap uses GFP_KERNEL flag to alloc things in store and reclaim
> function, does this lead to these function called recursively?

Yes, that's a potential problem.

> 3. for reclaiming one zbud page which contains two buddies, zswap
> needs to alloc two pages. Does this reclaim cost-efficient?
> 

Yes, that's a problem too. And that's why we use zbud as the default
allocator instead of zsmalloc.
I think improving the write back path of zswap is the next important
step for zswap.

-- 
Regards,
-Bob

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>