linux-kernel - Re: [Update][PATCH v5 7/9] mm/swap: Add cache for swap slots allocation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 17 Jan 2017 11:16:32 +0100
From:   Michal Hocko <mhocko@...nel.org>
To:     "Huang, Ying" <ying.huang@...el.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>, dave.hansen@...el.com,
        ak@...ux.intel.com, aaron.lu@...el.com, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, Hugh Dickins <hughd@...gle.com>,
        Shaohua Li <shli@...nel.org>, Minchan Kim <minchan@...nel.org>,
        Rik van Riel <riel@...hat.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Vladimir Davydov <vdavydov.dev@...il.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Hillf Danton <hillf.zj@...baba-inc.com>,
        Christian Borntraeger <borntraeger@...ibm.com>,
        Jonathan Corbet <corbet@....net>,
        Tim C Chen <tim.c.chen@...el.com>
Subject: Re: [Update][PATCH v5 7/9] mm/swap: Add cache for swap slots
 allocation

On Tue 17-01-17 10:55:47, Huang, Ying wrote:
[...]
> +int free_swap_slot(swp_entry_t entry)
> +{
> +	struct swap_slots_cache *cache;
> +
> +	BUG_ON(!swap_slot_cache_initialized);
> +
> +	cache = &get_cpu_var(swp_slots);
> +	if (use_swap_slot_cache && cache->slots_ret) {
> +		spin_lock_irq(&cache->free_lock);
> +		/* Swap slots cache may be deactivated before acquiring lock */
> +		if (!use_swap_slot_cache) {
> +			spin_unlock_irq(&cache->free_lock);
> +			goto direct_free;
> +		}
> +		if (cache->n_ret >= SWAP_SLOTS_CACHE_SIZE) {
> +			/*
> +			 * Return slots to global pool.
> +			 * The current swap_map value is SWAP_HAS_CACHE.
> +			 * Set it to 0 to indicate it is available for
> +			 * allocation in global pool
> +			 */
> +			swapcache_free_entries(cache->slots_ret, cache->n_ret);
> +			cache->n_ret = 0;
> +		}
> +		cache->slots_ret[cache->n_ret++] = entry;
> +		spin_unlock_irq(&cache->free_lock);
> +	} else {
> +direct_free:
> +		swapcache_free_entries(&entry, 1);
> +	}
> +	put_cpu_var(swp_slots);
> +
> +	return 0;
> +}
> +
> +swp_entry_t get_swap_page(void)
> +{
> +	swp_entry_t entry, *pentry;
> +	struct swap_slots_cache *cache;
> +
> +	/*
> +	 * Preemption need to be turned on here, because we may sleep
> +	 * in refill_swap_slots_cache().  But it is safe, because
> +	 * accesses to the per-CPU data structure are protected by a
> +	 * mutex.
> +	 */

the comment doesn't really explain why it is safe. THere are other users
which are not using the lock. E.g. just look at free_swap_slot above. 
How can
	cache->slots_ret[cache->n_ret++] = entry;
be safe wrt.
	pentry = &cache->slots[cache->cur++];
	entry = *pentry;

Both of them might touch the same slot, no? Btw. I would rather prefer
this would be a follow up fix with the trace and the detailed
explanation.

> +	cache = raw_cpu_ptr(&swp_slots);
> +
> +	entry.val = 0;
> +	if (check_cache_active()) {
> +		mutex_lock(&cache->alloc_lock);
> +		if (cache->slots) {
> +repeat:
> +			if (cache->nr) {
> +				pentry = &cache->slots[cache->cur++];
> +				entry = *pentry;
> +				pentry->val = 0;
> +				cache->nr--;
> +			} else {
> +				if (refill_swap_slots_cache(cache))
> +					goto repeat;
> +			}
> +		}
> +		mutex_unlock(&cache->alloc_lock);
> +		if (entry.val)
> +			return entry;
> +	}
> +
> +	get_swap_pages(1, &entry);
> +
> +	return entry;
> +}
-- 
Michal Hocko
SUSE Labs