[PATCH 1/2] mm/swap: Fix race conditions in swap_slots cache init

[PATCH 1/2] mm/swap: Fix race conditions in swap_slots cache init

[Tim Chen]

Memory allocations can happen before the swap_slots cache initialization
is completed during cpu bring up.  If we are low on memory, we could call
get_swap_page and access swap_slots_cache before it is fully initialized.

Add a check in get_swap_page for initialized swap_slots_cache
to prevent this condition.  Similar check already exists in
free_swap_slot.  Also annotate the checks to indicate the likely
condition.

We also added a memory barrier to make sure that the locks
initialization are done before the assignment of cache->slots
and cache->slots_ret pointers. This ensures the assumption
that it is safe to acquire the slots cache locks and use the slots
cache when the corresponding cache->slots or cache->slots_ret
pointers are non null.

Reported-by: Wenwei Tao <wenwei.tww@alibaba-inc.com>
Acked-by: Ying Huang <ying.huang@intel.com>
Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 mm/swap_slots.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/mm/swap_slots.c b/mm/swap_slots.c
index 58f6c78..4c5457c 100644
--- a/mm/swap_slots.c
+++ b/mm/swap_slots.c
@@ -148,6 +148,14 @@ static int alloc_swap_slot_cache(unsigned int cpu)
 	cache->nr = 0;
 	cache->cur = 0;
 	cache->n_ret = 0;
+	/*
+	 * We intialized alloc_lock and free_lock earlier.
+	 * We use !cache->slots or !cache->slots_ret
+	 * to know if it is safe to acquire the corresponding
+	 * lock and use the cache.  Memory barrier
+	 * below ensures the assumption.
+	 */
+	mb();
 	cache->slots = slots;
 	slots = NULL;
 	cache->slots_ret = slots_ret;
@@ -273,7 +281,7 @@ int free_swap_slot(swp_entry_t entry)
 	struct swap_slots_cache *cache;
 
 	cache = &get_cpu_var(swp_slots);
-	if (use_swap_slot_cache && cache->slots_ret) {
+	if (likely(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) {
@@ -318,7 +326,7 @@ swp_entry_t get_swap_page(void)
 	cache = raw_cpu_ptr(&swp_slots);
 
 	entry.val = 0;
-	if (check_cache_active()) {
+	if (likely(check_cache_active() && cache->slots)) {
 		mutex_lock(&cache->alloc_lock);
 		if (cache->slots) {
 repeat:
-- 
2.9.4

--
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>

[PATCH 2/2] mm/swap: Remove lock_initialized flag from swap_slots_cache

[Tim Chen]

We will only reach the lock initialization code
in alloc_swap_slot_cache when the cpu's swap_slots_cache's slots
have not been allocated and swap_slots_cache has not been initialized
previously.  So the lock_initialized check is redundant and unnecessary.
Remove lock_initialized flag from swap_slots_cache to save memory.

Reported-by: Wenwei Tao <wenwei.tww@alibaba-inc.com>
Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 include/linux/swap_slots.h | 1 -
 mm/swap_slots.c            | 9 ++++-----
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/include/linux/swap_slots.h b/include/linux/swap_slots.h
index 6ef92d1..a75c30b 100644
--- a/include/linux/swap_slots.h
+++ b/include/linux/swap_slots.h
@@ -10,7 +10,6 @@
 #define THRESHOLD_DEACTIVATE_SWAP_SLOTS_CACHE	(2*SWAP_SLOTS_CACHE_SIZE)
 
 struct swap_slots_cache {
-	bool		lock_initialized;
 	struct mutex	alloc_lock; /* protects slots, nr, cur */
 	swp_entry_t	*slots;
 	int		nr;
diff --git a/mm/swap_slots.c b/mm/swap_slots.c
index 4c5457c..c039e6c 100644
--- a/mm/swap_slots.c
+++ b/mm/swap_slots.c
@@ -140,11 +140,10 @@ static int alloc_swap_slot_cache(unsigned int cpu)
 	if (cache->slots || cache->slots_ret)
 		/* cache already allocated */
 		goto out;
-	if (!cache->lock_initialized) {
-		mutex_init(&cache->alloc_lock);
-		spin_lock_init(&cache->free_lock);
-		cache->lock_initialized = true;
-	}
+
+	mutex_init(&cache->alloc_lock);
+	spin_lock_init(&cache->free_lock);
+
 	cache->nr = 0;
 	cache->cur = 0;
 	cache->n_ret = 0;
-- 
2.9.4

--
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>

Re: [PATCH 2/2] mm/swap: Remove lock_initialized flag from swap_slots_cache

[Huang, Ying]

Hi, Tim,

Tim Chen <tim.c.chen@linux.intel.com> writes:

> We will only reach the lock initialization code
> in alloc_swap_slot_cache when the cpu's swap_slots_cache's slots
> have not been allocated and swap_slots_cache has not been initialized
> previously.  So the lock_initialized check is redundant and unnecessary.
> Remove lock_initialized flag from swap_slots_cache to save memory.

Is there a race condition with CPU offline/online when preempt is enabled?

CPU A                                   CPU B
-----                                   -----
                                        get_swap_page()
                                          get cache[B], cache[B]->slots != NULL
                                          preempted and moved to CPU A
                                        be offlined
                                        be onlined
                                          alloc_swap_slot_cache()
mutex_lock(cache[B]->alloc_lock)
                                            mutex_init(cache[B]->alloc_lock) !!!

The cache[B]->alloc_lock will be reinitialized when it is still held.

Best Regards,
Huang, Ying

> Reported-by: Wenwei Tao <wenwei.tww@alibaba-inc.com>
> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
> ---
>  include/linux/swap_slots.h | 1 -
>  mm/swap_slots.c            | 9 ++++-----
>  2 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/swap_slots.h b/include/linux/swap_slots.h
> index 6ef92d1..a75c30b 100644
> --- a/include/linux/swap_slots.h
> +++ b/include/linux/swap_slots.h
> @@ -10,7 +10,6 @@
>  #define THRESHOLD_DEACTIVATE_SWAP_SLOTS_CACHE	(2*SWAP_SLOTS_CACHE_SIZE)
>  
>  struct swap_slots_cache {
> -	bool		lock_initialized;
>  	struct mutex	alloc_lock; /* protects slots, nr, cur */
>  	swp_entry_t	*slots;
>  	int		nr;
> diff --git a/mm/swap_slots.c b/mm/swap_slots.c
> index 4c5457c..c039e6c 100644
> --- a/mm/swap_slots.c
> +++ b/mm/swap_slots.c
> @@ -140,11 +140,10 @@ static int alloc_swap_slot_cache(unsigned int cpu)
>  	if (cache->slots || cache->slots_ret)
>  		/* cache already allocated */
>  		goto out;
> -	if (!cache->lock_initialized) {
> -		mutex_init(&cache->alloc_lock);
> -		spin_lock_init(&cache->free_lock);
> -		cache->lock_initialized = true;
> -	}
> +
> +	mutex_init(&cache->alloc_lock);
> +	spin_lock_init(&cache->free_lock);
> +
>  	cache->nr = 0;
>  	cache->cur = 0;
>  	cache->n_ret = 0;

--
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>

Re: [PATCH 2/2] mm/swap: Remove lock_initialized flag from swap_slots_cache

[Tim Chen]

On 07/23/2017 07:15 PM, Huang, Ying wrote:
> Hi, Tim,
> 
> Tim Chen <tim.c.chen@linux.intel.com> writes:
> 
>> We will only reach the lock initialization code
>> in alloc_swap_slot_cache when the cpu's swap_slots_cache's slots
>> have not been allocated and swap_slots_cache has not been initialized
>> previously.  So the lock_initialized check is redundant and unnecessary.
>> Remove lock_initialized flag from swap_slots_cache to save memory.
> 
> Is there a race condition with CPU offline/online when preempt is enabled?
> 
> CPU A                                   CPU B
> -----                                   -----
>                                         get_swap_page()
>                                           get cache[B], cache[B]->slots != NULL
>                                           preempted and moved to CPU A
>                                         be offlined
>                                         be onlined
>                                           alloc_swap_slot_cache()
> mutex_lock(cache[B]->alloc_lock)
>                                             mutex_init(cache[B]->alloc_lock) !!!
> 
> The cache[B]->alloc_lock will be reinitialized when it is still held.

Looks like for this case the lock_initialized flag is still needed
to prevent such races and prevent re-initialization of taken locks.

Okay, let's scrap patch 2.

Thanks.

Tim

--
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>

Re: [PATCH 1/2] mm/swap: Fix race conditions in swap_slots cache init

[Andrew Morton]

On Fri, 21 Jul 2017 15:45:00 -0700 Tim Chen <tim.c.chen@linux.intel.com> wrote:

> Memory allocations can happen before the swap_slots cache initialization
> is completed during cpu bring up.  If we are low on memory, we could call
> get_swap_page and access swap_slots_cache before it is fully initialized.
> 
> Add a check in get_swap_page for initialized swap_slots_cache
> to prevent this condition.  Similar check already exists in
> free_swap_slot.  Also annotate the checks to indicate the likely
> condition.
> 
> We also added a memory barrier to make sure that the locks
> initialization are done before the assignment of cache->slots
> and cache->slots_ret pointers. This ensures the assumption
> that it is safe to acquire the slots cache locks and use the slots
> cache when the corresponding cache->slots or cache->slots_ret
> pointers are non null.

I guess that the user-visible effect is "crash on boot on large
machine".  Or something.  Please don't make me guess!

Which kernel version(s) do you believe need this patch, and why?

--
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>