* [PATCH 1/2] mm/swap: Fix race conditions in swap_slots cache init
@ 2017-07-21 22:45 Tim Chen
2017-07-21 22:45 ` [PATCH 2/2] mm/swap: Remove lock_initialized flag from swap_slots_cache Tim Chen
2017-10-03 22:27 ` [PATCH 1/2] mm/swap: Fix race conditions in swap_slots cache init Andrew Morton
0 siblings, 2 replies; 5+ messages in thread
From: Tim Chen @ 2017-07-21 22:45 UTC (permalink / raw)
To: Andrew Morton
Cc: Tim Chen, Ying Huang, Wenwei Tao, linux-mm, linux-kernel,
Minchan Kim, Rik van Riel, Andrea Arcangeli, Johannes Weiner,
Michal Hocko, Hillf Danton
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>
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] mm/swap: Remove lock_initialized flag from swap_slots_cache
2017-07-21 22:45 [PATCH 1/2] mm/swap: Fix race conditions in swap_slots cache init Tim Chen
@ 2017-07-21 22:45 ` Tim Chen
2017-07-24 2:15 ` Huang, Ying
2017-10-03 22:27 ` [PATCH 1/2] mm/swap: Fix race conditions in swap_slots cache init Andrew Morton
1 sibling, 1 reply; 5+ messages in thread
From: Tim Chen @ 2017-07-21 22:45 UTC (permalink / raw)
To: Andrew Morton
Cc: Tim Chen, Ying Huang, Wenwei Tao, linux-mm, linux-kernel,
Minchan Kim, Rik van Riel, Andrea Arcangeli, Johannes Weiner,
Michal Hocko, Hillf Danton
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>
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] mm/swap: Remove lock_initialized flag from swap_slots_cache
2017-07-21 22:45 ` [PATCH 2/2] mm/swap: Remove lock_initialized flag from swap_slots_cache Tim Chen
@ 2017-07-24 2:15 ` Huang, Ying
2017-07-24 16:54 ` Tim Chen
0 siblings, 1 reply; 5+ messages in thread
From: Huang, Ying @ 2017-07-24 2:15 UTC (permalink / raw)
To: Tim Chen
Cc: Andrew Morton, Ying Huang, Wenwei Tao, linux-mm, linux-kernel,
Minchan Kim, Rik van Riel, Andrea Arcangeli, Johannes Weiner,
Michal Hocko, Hillf Danton
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] mm/swap: Remove lock_initialized flag from swap_slots_cache
2017-07-24 2:15 ` Huang, Ying
@ 2017-07-24 16:54 ` Tim Chen
0 siblings, 0 replies; 5+ messages in thread
From: Tim Chen @ 2017-07-24 16:54 UTC (permalink / raw)
To: Huang, Ying
Cc: Andrew Morton, Wenwei Tao, linux-mm, linux-kernel, Minchan Kim,
Rik van Riel, Andrea Arcangeli, Johannes Weiner, Michal Hocko,
Hillf Danton
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] mm/swap: Fix race conditions in swap_slots cache init
2017-07-21 22:45 [PATCH 1/2] mm/swap: Fix race conditions in swap_slots cache init Tim Chen
2017-07-21 22:45 ` [PATCH 2/2] mm/swap: Remove lock_initialized flag from swap_slots_cache Tim Chen
@ 2017-10-03 22:27 ` Andrew Morton
1 sibling, 0 replies; 5+ messages in thread
From: Andrew Morton @ 2017-10-03 22:27 UTC (permalink / raw)
To: Tim Chen
Cc: Ying Huang, Wenwei Tao, linux-mm, linux-kernel, Minchan Kim,
Rik van Riel, Andrea Arcangeli, Johannes Weiner, Michal Hocko,
Hillf Danton
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-10-03 22:27 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-21 22:45 [PATCH 1/2] mm/swap: Fix race conditions in swap_slots cache init Tim Chen
2017-07-21 22:45 ` [PATCH 2/2] mm/swap: Remove lock_initialized flag from swap_slots_cache Tim Chen
2017-07-24 2:15 ` Huang, Ying
2017-07-24 16:54 ` Tim Chen
2017-10-03 22:27 ` [PATCH 1/2] mm/swap: Fix race conditions in swap_slots cache init Andrew Morton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).