From: Huang Ying <[email protected]>
When swapin is performed, after getting the swap entry information from
the page table, system will swap in the swap entry, without any lock held
to prevent the swap device from being swapoff. This may cause the race
like below,
CPU 1 CPU 2
----- -----
do_swap_page
swapin_readahead
__read_swap_cache_async
swapoff swapcache_prepare
p->swap_map = NULL __swap_duplicate
p->swap_map[?] /* !!! NULL pointer access */
Because swapoff is usually done when system shutdown only, the race may
not hit many people in practice. But it is still a race need to be fixed.
To fix the race, get_swap_device() is added to check whether the specified
swap entry is valid in its swap device. If so, it will keep the swap
entry valid via preventing the swap device from being swapoff, until
put_swap_device() is called.
Because swapoff() is very rare code path, to make the normal path runs as
fast as possible, disabling preemption + stop_machine() instead of
reference count is used to implement get/put_swap_device(). From
get_swap_device() to put_swap_device(), the preemption is disabled, so
stop_machine() in swapoff() will wait until put_swap_device() is called.
In addition to swap_map, cluster_info, etc. data structure in the struct
swap_info_struct, the swap cache radix tree will be freed after swapoff,
so this patch fixes the race between swap cache looking up and swapoff
too.
Races between some other swap cache usages protected via disabling
preemption and swapoff are fixed too via calling stop_machine() between
clearing PageSwapCache() and freeing swap cache data structure.
Alternative implementation could be replacing disable preemption with
rcu_read_lock_sched and stop_machine() with synchronize_sched().
Signed-off-by: "Huang, Ying" <[email protected]>
Not-Nacked-by: Hugh Dickins <[email protected]>
Cc: Paul E. McKenney <[email protected]>
Cc: Minchan Kim <[email protected]>
Cc: Johannes Weiner <[email protected]>
Cc: Tim Chen <[email protected]>
Cc: Mel Gorman <[email protected]>
Cc: Jérôme Glisse <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: Andrea Arcangeli <[email protected]>
Cc: David Rientjes <[email protected]>
Cc: Rik van Riel <[email protected]>
Cc: Jan Kara <[email protected]>
Cc: Dave Jiang <[email protected]>
Cc: Daniel Jordan <[email protected]>
Cc: Andrea Parri <[email protected]>
Changelog:
v7:
- Rebased on patch: "mm, swap: bounds check swap_info accesses to avoid NULL derefs"
v6:
- Add more comments to get_swap_device() to make it more clear about
possible swapoff or swapoff+swapon.
v5:
- Replace RCU with stop_machine()
v4:
- Use synchronize_rcu() in enable_swap_info() to reduce overhead of
normal paths further.
v3:
- Re-implemented with RCU to reduce the overhead of normal paths
v2:
- Re-implemented with SRCU to reduce the overhead of normal paths.
- Avoid to check whether the swap device has been swapoff in
get_swap_device(). Because we can check the origin of the swap
entry to make sure the swap device hasn't bee swapoff.
---
include/linux/swap.h | 13 +++-
mm/memory.c | 2 +-
mm/swap_state.c | 16 ++++-
mm/swapfile.c | 150 ++++++++++++++++++++++++++++++++++---------
4 files changed, 145 insertions(+), 36 deletions(-)
diff --git a/include/linux/swap.h b/include/linux/swap.h
index 649529be91f2..aecd1430d0a9 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -175,8 +175,9 @@ enum {
SWP_PAGE_DISCARD = (1 << 10), /* freed swap page-cluster discards */
SWP_STABLE_WRITES = (1 << 11), /* no overwrite PG_writeback pages */
SWP_SYNCHRONOUS_IO = (1 << 12), /* synchronous IO is efficient */
+ SWP_VALID = (1 << 13), /* swap is valid to be operated on? */
/* add others here before... */
- SWP_SCANNING = (1 << 13), /* refcount in scan_swap_map */
+ SWP_SCANNING = (1 << 14), /* refcount in scan_swap_map */
};
#define SWAP_CLUSTER_MAX 32UL
@@ -460,7 +461,7 @@ extern unsigned int count_swap_pages(int, int);
extern sector_t map_swap_page(struct page *, struct block_device **);
extern sector_t swapdev_block(int, pgoff_t);
extern int page_swapcount(struct page *);
-extern int __swap_count(struct swap_info_struct *si, swp_entry_t entry);
+extern int __swap_count(swp_entry_t entry);
extern int __swp_swapcount(swp_entry_t entry);
extern int swp_swapcount(swp_entry_t entry);
extern struct swap_info_struct *page_swap_info(struct page *);
@@ -470,6 +471,12 @@ extern int try_to_free_swap(struct page *);
struct backing_dev_info;
extern int init_swap_address_space(unsigned int type, unsigned long nr_pages);
extern void exit_swap_address_space(unsigned int type);
+extern struct swap_info_struct *get_swap_device(swp_entry_t entry);
+
+static inline void put_swap_device(struct swap_info_struct *si)
+{
+ preempt_enable();
+}
#else /* CONFIG_SWAP */
@@ -576,7 +583,7 @@ static inline int page_swapcount(struct page *page)
return 0;
}
-static inline int __swap_count(struct swap_info_struct *si, swp_entry_t entry)
+static inline int __swap_count(swp_entry_t entry)
{
return 0;
}
diff --git a/mm/memory.c b/mm/memory.c
index 34ced1369883..9c0743c17c6c 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2719,7 +2719,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
struct swap_info_struct *si = swp_swap_info(entry);
if (si->flags & SWP_SYNCHRONOUS_IO &&
- __swap_count(si, entry) == 1) {
+ __swap_count(entry) == 1) {
/* skip swapcache */
page = alloc_page_vma(GFP_HIGHUSER_MOVABLE, vma,
vmf->address);
diff --git a/mm/swap_state.c b/mm/swap_state.c
index 85245fdec8d9..61453f1faf72 100644
--- a/mm/swap_state.c
+++ b/mm/swap_state.c
@@ -310,8 +310,13 @@ struct page *lookup_swap_cache(swp_entry_t entry, struct vm_area_struct *vma,
unsigned long addr)
{
struct page *page;
+ struct swap_info_struct *si;
+ si = get_swap_device(entry);
+ if (!si)
+ return NULL;
page = find_get_page(swap_address_space(entry), swp_offset(entry));
+ put_swap_device(si);
INC_CACHE_INFO(find_total);
if (page) {
@@ -354,8 +359,8 @@ struct page *__read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
struct vm_area_struct *vma, unsigned long addr,
bool *new_page_allocated)
{
- struct page *found_page, *new_page = NULL;
- struct address_space *swapper_space = swap_address_space(entry);
+ struct page *found_page = NULL, *new_page = NULL;
+ struct swap_info_struct *si;
int err;
*new_page_allocated = false;
@@ -365,7 +370,12 @@ struct page *__read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
* called after lookup_swap_cache() failed, re-calling
* that would confuse statistics.
*/
- found_page = find_get_page(swapper_space, swp_offset(entry));
+ si = get_swap_device(entry);
+ if (!si)
+ break;
+ found_page = find_get_page(swap_address_space(entry),
+ swp_offset(entry));
+ put_swap_device(si);
if (found_page)
break;
diff --git a/mm/swapfile.c b/mm/swapfile.c
index cca8420b12db..8f92f36814fb 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -38,6 +38,7 @@
#include <linux/export.h>
#include <linux/swap_slots.h>
#include <linux/sort.h>
+#include <linux/stop_machine.h>
#include <asm/pgtable.h>
#include <asm/tlbflush.h>
@@ -1186,6 +1187,64 @@ static unsigned char __swap_entry_free_locked(struct swap_info_struct *p,
return usage;
}
+/*
+ * Check whether swap entry is valid in the swap device. If so,
+ * return pointer to swap_info_struct, and keep the swap entry valid
+ * via preventing the swap device from being swapoff, until
+ * put_swap_device() is called. Otherwise return NULL.
+ *
+ * Notice that swapoff or swapoff+swapon can still happen before the
+ * preempt_disable() in get_swap_device() or after the
+ * preempt_enable() in put_swap_device() if there isn't any other way
+ * to prevent swapoff, such as page lock, page table lock, etc. The
+ * caller must be prepared for that. For example, the following
+ * situation is possible.
+ *
+ * CPU1 CPU2
+ * do_swap_page()
+ * ... swapoff+swapon
+ * __read_swap_cache_async()
+ * swapcache_prepare()
+ * __swap_duplicate()
+ * // check swap_map
+ * // verify PTE not changed
+ *
+ * In __swap_duplicate(), the swap_map need to be checked before
+ * changing partly because the specified swap entry may be for another
+ * swap device which has been swapoff. And in do_swap_page(), after
+ * the page is read from the swap device, the PTE is verified not
+ * changed with the page table locked to check whether the swap device
+ * has been swapoff or swapoff+swapon.
+ */
+struct swap_info_struct *get_swap_device(swp_entry_t entry)
+{
+ struct swap_info_struct *si;
+ unsigned long type, offset;
+
+ if (!entry.val)
+ goto out;
+ type = swp_type(entry);
+ si = swap_type_to_swap_info(type);
+ if (!si)
+ goto bad_nofile;
+
+ preempt_disable();
+ if (!(si->flags & SWP_VALID))
+ goto unlock_out;
+ offset = swp_offset(entry);
+ if (offset >= si->max)
+ goto unlock_out;
+
+ return si;
+bad_nofile:
+ pr_err("%s: %s%08lx\n", __func__, Bad_file, entry.val);
+out:
+ return NULL;
+unlock_out:
+ preempt_enable();
+ return NULL;
+}
+
static unsigned char __swap_entry_free(struct swap_info_struct *p,
swp_entry_t entry, unsigned char usage)
{
@@ -1357,11 +1416,18 @@ int page_swapcount(struct page *page)
return count;
}
-int __swap_count(struct swap_info_struct *si, swp_entry_t entry)
+int __swap_count(swp_entry_t entry)
{
+ struct swap_info_struct *si;
pgoff_t offset = swp_offset(entry);
+ int count = 0;
- return swap_count(si->swap_map[offset]);
+ si = get_swap_device(entry);
+ if (si) {
+ count = swap_count(si->swap_map[offset]);
+ put_swap_device(si);
+ }
+ return count;
}
static int swap_swapcount(struct swap_info_struct *si, swp_entry_t entry)
@@ -1386,9 +1452,11 @@ int __swp_swapcount(swp_entry_t entry)
int count = 0;
struct swap_info_struct *si;
- si = __swap_info_get(entry);
- if (si)
+ si = get_swap_device(entry);
+ if (si) {
count = swap_swapcount(si, entry);
+ put_swap_device(si);
+ }
return count;
}
@@ -2332,9 +2400,9 @@ static int swap_node(struct swap_info_struct *p)
return bdev ? bdev->bd_disk->node_id : NUMA_NO_NODE;
}
-static void _enable_swap_info(struct swap_info_struct *p, int prio,
- unsigned char *swap_map,
- struct swap_cluster_info *cluster_info)
+static void setup_swap_info(struct swap_info_struct *p, int prio,
+ unsigned char *swap_map,
+ struct swap_cluster_info *cluster_info)
{
int i;
@@ -2359,7 +2427,11 @@ static void _enable_swap_info(struct swap_info_struct *p, int prio,
}
p->swap_map = swap_map;
p->cluster_info = cluster_info;
- p->flags |= SWP_WRITEOK;
+}
+
+static void _enable_swap_info(struct swap_info_struct *p)
+{
+ p->flags |= SWP_WRITEOK | SWP_VALID;
atomic_long_add(p->pages, &nr_swap_pages);
total_swap_pages += p->pages;
@@ -2378,6 +2450,11 @@ static void _enable_swap_info(struct swap_info_struct *p, int prio,
add_to_avail_list(p);
}
+static int swap_onoff_stop(void *arg)
+{
+ return 0;
+}
+
static void enable_swap_info(struct swap_info_struct *p, int prio,
unsigned char *swap_map,
struct swap_cluster_info *cluster_info,
@@ -2386,7 +2463,17 @@ static void enable_swap_info(struct swap_info_struct *p, int prio,
frontswap_init(p->type, frontswap_map);
spin_lock(&swap_lock);
spin_lock(&p->lock);
- _enable_swap_info(p, prio, swap_map, cluster_info);
+ setup_swap_info(p, prio, swap_map, cluster_info);
+ spin_unlock(&p->lock);
+ spin_unlock(&swap_lock);
+ /*
+ * Guarantee swap_map, cluster_info, etc. fields are used
+ * between get/put_swap_device() only if SWP_VALID bit is set
+ */
+ stop_machine(swap_onoff_stop, NULL, cpu_online_mask);
+ spin_lock(&swap_lock);
+ spin_lock(&p->lock);
+ _enable_swap_info(p);
spin_unlock(&p->lock);
spin_unlock(&swap_lock);
}
@@ -2395,7 +2482,8 @@ static void reinsert_swap_info(struct swap_info_struct *p)
{
spin_lock(&swap_lock);
spin_lock(&p->lock);
- _enable_swap_info(p, p->prio, p->swap_map, p->cluster_info);
+ setup_swap_info(p, p->prio, p->swap_map, p->cluster_info);
+ _enable_swap_info(p);
spin_unlock(&p->lock);
spin_unlock(&swap_lock);
}
@@ -2498,6 +2586,17 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile)
reenable_swap_slots_cache_unlock();
+ spin_lock(&swap_lock);
+ spin_lock(&p->lock);
+ p->flags &= ~SWP_VALID; /* mark swap device as invalid */
+ spin_unlock(&p->lock);
+ spin_unlock(&swap_lock);
+ /*
+ * wait for swap operations protected by get/put_swap_device()
+ * to complete
+ */
+ stop_machine(swap_onoff_stop, NULL, cpu_online_mask);
+
flush_work(&p->discard_work);
destroy_swap_extents(p);
@@ -3263,17 +3362,11 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage)
unsigned char has_cache;
int err = -EINVAL;
- if (non_swap_entry(entry))
- goto out;
-
- p = swp_swap_info(entry);
+ p = get_swap_device(entry);
if (!p)
- goto bad_file;
-
- offset = swp_offset(entry);
- if (unlikely(offset >= p->max))
goto out;
+ offset = swp_offset(entry);
ci = lock_cluster_or_swap_info(p, offset);
count = p->swap_map[offset];
@@ -3319,11 +3412,9 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage)
unlock_out:
unlock_cluster_or_swap_info(p, ci);
out:
+ if (p)
+ put_swap_device(p);
return err;
-
-bad_file:
- pr_err("swap_dup: %s%08lx\n", Bad_file, entry.val);
- goto out;
}
/*
@@ -3415,6 +3506,7 @@ int add_swap_count_continuation(swp_entry_t entry, gfp_t gfp_mask)
struct page *list_page;
pgoff_t offset;
unsigned char count;
+ int ret = 0;
/*
* When debugging, it's easier to use __GFP_ZERO here; but it's better
@@ -3422,15 +3514,15 @@ int add_swap_count_continuation(swp_entry_t entry, gfp_t gfp_mask)
*/
page = alloc_page(gfp_mask | __GFP_HIGHMEM);
- si = swap_info_get(entry);
+ si = get_swap_device(entry);
if (!si) {
/*
* An acceptable race has occurred since the failing
- * __swap_duplicate(): the swap entry has been freed,
- * perhaps even the whole swap_map cleared for swapoff.
+ * __swap_duplicate(): the swap device may be swapoff
*/
goto outer;
}
+ spin_lock(&si->lock);
offset = swp_offset(entry);
@@ -3448,9 +3540,8 @@ int add_swap_count_continuation(swp_entry_t entry, gfp_t gfp_mask)
}
if (!page) {
- unlock_cluster(ci);
- spin_unlock(&si->lock);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto out;
}
/*
@@ -3502,10 +3593,11 @@ int add_swap_count_continuation(swp_entry_t entry, gfp_t gfp_mask)
out:
unlock_cluster(ci);
spin_unlock(&si->lock);
+ put_swap_device(si);
outer:
if (page)
__free_page(page);
- return 0;
+ return ret;
}
/*
--
2.20.1
On Mon, Feb 11, 2019 at 04:38:46PM +0800, Huang, Ying wrote:
> +struct swap_info_struct *get_swap_device(swp_entry_t entry)
> +{
> + struct swap_info_struct *si;
> + unsigned long type, offset;
> +
> + if (!entry.val)
> + goto out;
> + type = swp_type(entry);
> + si = swap_type_to_swap_info(type);
These lines can be collapsed into swp_swap_info if you want.
> + if (!si)
> + goto bad_nofile;
> +
> + preempt_disable();
> + if (!(si->flags & SWP_VALID))
> + goto unlock_out;
After Hugh alluded to barriers, it seems the read of SWP_VALID could be
reordered with the write in preempt_disable at runtime. Without smp_mb()
between the two, couldn't this happen, however unlikely a race it is?
CPU0 CPU1
__swap_duplicate()
get_swap_device()
// sees SWP_VALID set
swapoff
p->flags &= ~SWP_VALID;
spin_unlock(&p->lock); // pair w/ smp_mb
...
stop_machine(...)
p->swap_map = NULL;
preempt_disable()
read NULL p->swap_map
> > + if (!si)
> > + goto bad_nofile;
> > +
> > + preempt_disable();
> > + if (!(si->flags & SWP_VALID))
> > + goto unlock_out;
>
> After Hugh alluded to barriers, it seems the read of SWP_VALID could be
> reordered with the write in preempt_disable at runtime. Without smp_mb()
> between the two, couldn't this happen, however unlikely a race it is?
>
> CPU0 CPU1
>
> __swap_duplicate()
> get_swap_device()
> // sees SWP_VALID set
> swapoff
> p->flags &= ~SWP_VALID;
> spin_unlock(&p->lock); // pair w/ smp_mb
> ...
> stop_machine(...)
> p->swap_map = NULL;
> preempt_disable()
> read NULL p->swap_map
I don't think that that smp_mb() is necessary. I elaborate:
An important piece of information, I think, that is missing in the
diagram above is the stopper thread which executes the work queued
by stop_machine(). We have two cases to consider, that is,
1) the stopper is "executed before" the preempt-disable section
CPU0
cpu_stopper_thread()
...
preempt_disable()
...
preempt_enable()
2) the stopper is "executed after" the preempt-disable section
CPU0
preempt_disable()
...
preempt_enable()
...
cpu_stopper_thread()
Notice that the reads from p->flags and p->swap_map in CPU0 cannot
cross cpu_stopper_thread(). The claim is that CPU0 sees SWP_VALID
unset in (1) and that it sees a non-NULL p->swap_map in (2).
I consider the two cases separately:
1) CPU1 unsets SPW_VALID, it locks the stopper's lock, and it
queues the stopper work; CPU0 locks the stopper's lock, it
dequeues this work, and it reads from p->flags.
Diagrammatically, we have the following MP-like pattern:
CPU0 CPU1
lock(stopper->lock) p->flags &= ~SPW_VALID
get @work lock(stopper->lock)
unlock(stopper->lock) add @work
reads p->flags unlock(stopper->lock)
where CPU0 must see SPW_VALID unset (if CPU0 sees the work
added by CPU1).
2) CPU0 reads from p->swap_map, it locks the completion lock,
and it signals completion; CPU1 locks the completion lock,
it checks for completion, and it writes to p->swap_map.
(If CPU0 doesn't signal the completion, or CPU1 doesn't see
the completion, then CPU1 will have to iterate the read and
to postpone the control-dependent write to p->swap_map.)
Diagrammatically, we have the following LB-like pattern:
CPU0 CPU1
reads p->swap_map lock(completion)
lock(completion) read completion->done
completion->done++ unlock(completion)
unlock(completion) p->swap_map = NULL
where CPU0 must see a non-NULL p->swap_map if CPU1 sees the
completion from CPU0.
Does this make sense?
Andrea
Daniel Jordan <[email protected]> writes:
> On Mon, Feb 11, 2019 at 04:38:46PM +0800, Huang, Ying wrote:
>> +struct swap_info_struct *get_swap_device(swp_entry_t entry)
>> +{
>> + struct swap_info_struct *si;
>> + unsigned long type, offset;
>> +
>> + if (!entry.val)
>> + goto out;
>
>> + type = swp_type(entry);
>> + si = swap_type_to_swap_info(type);
>
> These lines can be collapsed into swp_swap_info if you want.
Yes. I can use that function to reduce another line from the patch.
Thanks! Will do that.
>> + if (!si)
>> + goto bad_nofile;
>> +
>> + preempt_disable();
>> + if (!(si->flags & SWP_VALID))
>> + goto unlock_out;
>
> After Hugh alluded to barriers, it seems the read of SWP_VALID could be
> reordered with the write in preempt_disable at runtime. Without smp_mb()
> between the two, couldn't this happen, however unlikely a race it is?
>
> CPU0 CPU1
>
> __swap_duplicate()
> get_swap_device()
> // sees SWP_VALID set
> swapoff
> p->flags &= ~SWP_VALID;
> spin_unlock(&p->lock); // pair w/ smp_mb
> ...
> stop_machine(...)
> p->swap_map = NULL;
> preempt_disable()
> read NULL p->swap_map
Andrea has helped to explain this.
Best Regards,
Huang, Ying
> Alternative implementation could be replacing disable preemption with
> rcu_read_lock_sched and stop_machine() with synchronize_sched().
JFYI, starting with v4.20-rc1, synchronize_rcu{,expedited}() also wait
for preempt-disable sections (the intent seems to retire the RCU-sched
update-side API), so that here you could instead use preempt-disable +
synchronize_rcu{,expedited}(). This LWN article gives an overview of
the latest RCU API/semantics changes: https://lwn.net/Articles/777036/.
Andrea
On 2/11/19 10:47 PM, Huang, Ying wrote:
> Andrea Parri <[email protected]> writes:
>
>>>> + if (!si)
>>>> + goto bad_nofile;
>>>> +
>>>> + preempt_disable();
>>>> + if (!(si->flags & SWP_VALID))
>>>> + goto unlock_out;
>>>
>>> After Hugh alluded to barriers, it seems the read of SWP_VALID could be
>>> reordered with the write in preempt_disable at runtime. Without smp_mb()
>>> between the two, couldn't this happen, however unlikely a race it is?
>>>
>>> CPU0 CPU1
>>>
>>> __swap_duplicate()
>>> get_swap_device()
>>> // sees SWP_VALID set
>>> swapoff
>>> p->flags &= ~SWP_VALID;
>>> spin_unlock(&p->lock); // pair w/ smp_mb
>>> ...
>>> stop_machine(...)
>>> p->swap_map = NULL;
>>> preempt_disable()
>>> read NULL p->swap_map
>>
>>
>> I don't think that that smp_mb() is necessary. I elaborate:
>>
>> An important piece of information, I think, that is missing in the
>> diagram above is the stopper thread which executes the work queued
>> by stop_machine(). We have two cases to consider, that is,
>>
>> 1) the stopper is "executed before" the preempt-disable section
>>
>> CPU0
>>
>> cpu_stopper_thread()
>> ...
>> preempt_disable()
>> ...
>> preempt_enable()
>>
>> 2) the stopper is "executed after" the preempt-disable section
>>
>> CPU0
>>
>> preempt_disable()
>> ...
>> preempt_enable()
>> ...
>> cpu_stopper_thread()
>>
>> Notice that the reads from p->flags and p->swap_map in CPU0 cannot
>> cross cpu_stopper_thread(). The claim is that CPU0 sees SWP_VALID
>> unset in (1) and that it sees a non-NULL p->swap_map in (2).
>>
>> I consider the two cases separately:
>>
>> 1) CPU1 unsets SPW_VALID, it locks the stopper's lock, and it
>> queues the stopper work; CPU0 locks the stopper's lock, it
>> dequeues this work, and it reads from p->flags.
>>
>> Diagrammatically, we have the following MP-like pattern:
>>
>> CPU0 CPU1
>>
>> lock(stopper->lock) p->flags &= ~SPW_VALID
>> get @work lock(stopper->lock)
>> unlock(stopper->lock) add @work
>> reads p->flags unlock(stopper->lock)
>>
>> where CPU0 must see SPW_VALID unset (if CPU0 sees the work
>> added by CPU1).
>>
>> 2) CPU0 reads from p->swap_map, it locks the completion lock,
>> and it signals completion; CPU1 locks the completion lock,
>> it checks for completion, and it writes to p->swap_map.
>>
>> (If CPU0 doesn't signal the completion, or CPU1 doesn't see
>> the completion, then CPU1 will have to iterate the read and
>> to postpone the control-dependent write to p->swap_map.)
>>
>> Diagrammatically, we have the following LB-like pattern:
>>
>> CPU0 CPU1
>>
>> reads p->swap_map lock(completion)
>> lock(completion) read completion->done
>> completion->done++ unlock(completion)
>> unlock(completion) p->swap_map = NULL
>>
>> where CPU0 must see a non-NULL p->swap_map if CPU1 sees the
>> completion from CPU0.
>>
>> Does this make sense?
>
> Thanks a lot for detailed explanation!
This is certainly a non-trivial explanation of why memory barrier is not
needed. Can we put it in the commit log and mention something in
comments on why we don't need memory barrier?
Thanks.
Tim
On Tue, Feb 12, 2019 at 04:21:21AM +0100, Andrea Parri wrote:
> > > + if (!si)
> > > + goto bad_nofile;
> > > +
> > > + preempt_disable();
> > > + if (!(si->flags & SWP_VALID))
> > > + goto unlock_out;
> >
> > After Hugh alluded to barriers, it seems the read of SWP_VALID could be
> > reordered with the write in preempt_disable at runtime. Without smp_mb()
> > between the two, couldn't this happen, however unlikely a race it is?
> >
> > CPU0 CPU1
> >
> > __swap_duplicate()
> > get_swap_device()
> > // sees SWP_VALID set
> > swapoff
> > p->flags &= ~SWP_VALID;
> > spin_unlock(&p->lock); // pair w/ smp_mb
> > ...
> > stop_machine(...)
> > p->swap_map = NULL;
> > preempt_disable()
> > read NULL p->swap_map
>
>
> I don't think that that smp_mb() is necessary. I elaborate:
>
> An important piece of information, I think, that is missing in the
> diagram above is the stopper thread which executes the work queued
> by stop_machine(). We have two cases to consider, that is,
>
> 1) the stopper is "executed before" the preempt-disable section
>
> CPU0
>
> cpu_stopper_thread()
> ...
> preempt_disable()
> ...
> preempt_enable()
>
> 2) the stopper is "executed after" the preempt-disable section
>
> CPU0
>
> preempt_disable()
> ...
> preempt_enable()
> ...
> cpu_stopper_thread()
>
> Notice that the reads from p->flags and p->swap_map in CPU0 cannot
> cross cpu_stopper_thread(). The claim is that CPU0 sees SWP_VALID
> unset in (1) and that it sees a non-NULL p->swap_map in (2).
>
> I consider the two cases separately:
>
> 1) CPU1 unsets SPW_VALID, it locks the stopper's lock, and it
> queues the stopper work; CPU0 locks the stopper's lock, it
> dequeues this work, and it reads from p->flags.
>
> Diagrammatically, we have the following MP-like pattern:
>
> CPU0 CPU1
>
> lock(stopper->lock) p->flags &= ~SPW_VALID
> get @work lock(stopper->lock)
> unlock(stopper->lock) add @work
> reads p->flags unlock(stopper->lock)
>
> where CPU0 must see SPW_VALID unset (if CPU0 sees the work
> added by CPU1).
>
> 2) CPU0 reads from p->swap_map, it locks the completion lock,
> and it signals completion; CPU1 locks the completion lock,
> it checks for completion, and it writes to p->swap_map.
>
> (If CPU0 doesn't signal the completion, or CPU1 doesn't see
> the completion, then CPU1 will have to iterate the read and
> to postpone the control-dependent write to p->swap_map.)
>
> Diagrammatically, we have the following LB-like pattern:
>
> CPU0 CPU1
>
> reads p->swap_map lock(completion)
> lock(completion) read completion->done
> completion->done++ unlock(completion)
> unlock(completion) p->swap_map = NULL
>
> where CPU0 must see a non-NULL p->swap_map if CPU1 sees the
> completion from CPU0.
>
> Does this make sense?
Yes, thanks for this, Andrea! Good that smp_mb isn't needed.
Tim Chen <[email protected]> writes:
> On 2/11/19 10:47 PM, Huang, Ying wrote:
>> Andrea Parri <[email protected]> writes:
>>
>>>>> + if (!si)
>>>>> + goto bad_nofile;
>>>>> +
>>>>> + preempt_disable();
>>>>> + if (!(si->flags & SWP_VALID))
>>>>> + goto unlock_out;
>>>>
>>>> After Hugh alluded to barriers, it seems the read of SWP_VALID could be
>>>> reordered with the write in preempt_disable at runtime. Without smp_mb()
>>>> between the two, couldn't this happen, however unlikely a race it is?
>>>>
>>>> CPU0 CPU1
>>>>
>>>> __swap_duplicate()
>>>> get_swap_device()
>>>> // sees SWP_VALID set
>>>> swapoff
>>>> p->flags &= ~SWP_VALID;
>>>> spin_unlock(&p->lock); // pair w/ smp_mb
>>>> ...
>>>> stop_machine(...)
>>>> p->swap_map = NULL;
>>>> preempt_disable()
>>>> read NULL p->swap_map
>>>
>>>
>>> I don't think that that smp_mb() is necessary. I elaborate:
>>>
>>> An important piece of information, I think, that is missing in the
>>> diagram above is the stopper thread which executes the work queued
>>> by stop_machine(). We have two cases to consider, that is,
>>>
>>> 1) the stopper is "executed before" the preempt-disable section
>>>
>>> CPU0
>>>
>>> cpu_stopper_thread()
>>> ...
>>> preempt_disable()
>>> ...
>>> preempt_enable()
>>>
>>> 2) the stopper is "executed after" the preempt-disable section
>>>
>>> CPU0
>>>
>>> preempt_disable()
>>> ...
>>> preempt_enable()
>>> ...
>>> cpu_stopper_thread()
>>>
>>> Notice that the reads from p->flags and p->swap_map in CPU0 cannot
>>> cross cpu_stopper_thread(). The claim is that CPU0 sees SWP_VALID
>>> unset in (1) and that it sees a non-NULL p->swap_map in (2).
>>>
>>> I consider the two cases separately:
>>>
>>> 1) CPU1 unsets SPW_VALID, it locks the stopper's lock, and it
>>> queues the stopper work; CPU0 locks the stopper's lock, it
>>> dequeues this work, and it reads from p->flags.
>>>
>>> Diagrammatically, we have the following MP-like pattern:
>>>
>>> CPU0 CPU1
>>>
>>> lock(stopper->lock) p->flags &= ~SPW_VALID
>>> get @work lock(stopper->lock)
>>> unlock(stopper->lock) add @work
>>> reads p->flags unlock(stopper->lock)
>>>
>>> where CPU0 must see SPW_VALID unset (if CPU0 sees the work
>>> added by CPU1).
>>>
>>> 2) CPU0 reads from p->swap_map, it locks the completion lock,
>>> and it signals completion; CPU1 locks the completion lock,
>>> it checks for completion, and it writes to p->swap_map.
>>>
>>> (If CPU0 doesn't signal the completion, or CPU1 doesn't see
>>> the completion, then CPU1 will have to iterate the read and
>>> to postpone the control-dependent write to p->swap_map.)
>>>
>>> Diagrammatically, we have the following LB-like pattern:
>>>
>>> CPU0 CPU1
>>>
>>> reads p->swap_map lock(completion)
>>> lock(completion) read completion->done
>>> completion->done++ unlock(completion)
>>> unlock(completion) p->swap_map = NULL
>>>
>>> where CPU0 must see a non-NULL p->swap_map if CPU1 sees the
>>> completion from CPU0.
>>>
>>> Does this make sense?
>>
>> Thanks a lot for detailed explanation!
>
> This is certainly a non-trivial explanation of why memory barrier is not
> needed. Can we put it in the commit log and mention something in
> comments on why we don't need memory barrier?
Good idea! Will do this.
Best Regards,
Huang, Ying
> Thanks.
>
> Tim
Hello everyone,
On Mon, Feb 11, 2019 at 04:38:46PM +0800, Huang, Ying wrote:
> @@ -2386,7 +2463,17 @@ static void enable_swap_info(struct swap_info_struct *p, int prio,
> frontswap_init(p->type, frontswap_map);
> spin_lock(&swap_lock);
> spin_lock(&p->lock);
> - _enable_swap_info(p, prio, swap_map, cluster_info);
> + setup_swap_info(p, prio, swap_map, cluster_info);
> + spin_unlock(&p->lock);
> + spin_unlock(&swap_lock);
> + /*
> + * Guarantee swap_map, cluster_info, etc. fields are used
> + * between get/put_swap_device() only if SWP_VALID bit is set
> + */
> + stop_machine(swap_onoff_stop, NULL, cpu_online_mask);
Should cpu_online_mask be read while holding cpus_read_lock?
cpus_read_lock();
err = __stop_machine(swap_onoff_stop, NULL, cpu_online_mask);
cpus_read_unlock();
I missed what the exact motivation was for the switch from
rcu_read_lock()/syncrhonize_rcu() to preempt_disable()/stop_machine().
It looks like the above stop_machine all it does is to reach a
quiescent point, when you've RCU that already can reach the quiescent
point without an explicit stop_machine.
The reason both implementations are basically looking the same is that
stop_machine dummy call of swap_onoff_stop() { /* noop */ } will only
reach a quiescent point faster than RCU, but it's otherwise
functionally identical to RCU, but it's extremely more expensive. If
it wasn't functionally identical stop_machine() couldn't be used as a
drop in replacement of synchronize_sched() in the previous patch.
I don't see the point of worrying about the synchronize_rcu latency in
swapoff when RCU is basically identical and not more complex.
So to be clear, I'm not against stop_machine() but with stop_machine()
method invoked in all CPUs, you can actually do more than RCU and you
can remove real locking not just reach a quiescent point.
With stop_machine() the code would need reshuffling around so that the
actual p->swap_map = NULL happens inside stop_machine, not outside
like with RCU.
With RCU all code stays concurrent at all times, simply the race is
controlled, as opposed with stop_machine() you can make fully
serialize and run like in UP temporarily (vs all preempt_disable()
section at least).
For example nr_swapfiles could in theory become a constant under
preempt_disable() with stop_machine() without having to take a
swap_lock.
swap_onoff_stop can be implemented like this:
enum {
FIRST_STOP_MACHINE_INIT,
FIRST_STOP_MACHINE_START,
FIRST_STOP_MACHINE_END,
};
static int first_stop_machine;
static int swap_onoff_stop(void *data)
{
struct swap_stop_machine *swsm = (struct swap_stop_machine *)data;
int first;
first = cmpxchg(&first_stop_machine, FIRST_STOP_MACHINE_INIT,
FIRST_STOP_MACHINE_START);
if (first == FIRST_STOP_MACHINE_INIT) {
swsm->p->swap_map = NULL;
/* add more stuff here until swap_lock goes away */
smp_wmb();
WRITE_ONCE(first_stop_machine, FIRST_STOP_MACHINE_END);
} else {
do {
cpu_relax();
} while (READ_ONCE(first_stop_machine) !=
FIRST_STOP_MACHINE_END);
smp_rmb();
}
return 0;
}
stop_machine invoked with a method like above, will guarantee while we
set p->swap_map to NULL (and while we do nr_swapfiles++) nothing else
can run, no even interrupts, so some lock may just disappear. Only NMI
and SMI could possibly run concurrently with the swsm->p->swap_map =
NULL operation.
If we've to keep swap_onoff_stop() a dummy function run on all CPUs
just to reach a quiescent point, then I don't see why
the synchronize_rcu() (or synchronize_sched or synchronize_kernel or
whatever it is called right now, but still RCU) solution isn't
preferable.
Thanks,
Andrea
On Mon 11-02-19 16:38:46, Huang, Ying wrote:
> From: Huang Ying <[email protected]>
>
> When swapin is performed, after getting the swap entry information from
> the page table, system will swap in the swap entry, without any lock held
> to prevent the swap device from being swapoff. This may cause the race
> like below,
>
> CPU 1 CPU 2
> ----- -----
> do_swap_page
> swapin_readahead
> __read_swap_cache_async
> swapoff swapcache_prepare
> p->swap_map = NULL __swap_duplicate
> p->swap_map[?] /* !!! NULL pointer access */
>
> Because swapoff is usually done when system shutdown only, the race may
> not hit many people in practice. But it is still a race need to be fixed.
>
> To fix the race, get_swap_device() is added to check whether the specified
> swap entry is valid in its swap device. If so, it will keep the swap
> entry valid via preventing the swap device from being swapoff, until
> put_swap_device() is called.
>
> Because swapoff() is very rare code path, to make the normal path runs as
> fast as possible, disabling preemption + stop_machine() instead of
> reference count is used to implement get/put_swap_device(). From
> get_swap_device() to put_swap_device(), the preemption is disabled, so
> stop_machine() in swapoff() will wait until put_swap_device() is called.
>
> In addition to swap_map, cluster_info, etc. data structure in the struct
> swap_info_struct, the swap cache radix tree will be freed after swapoff,
> so this patch fixes the race between swap cache looking up and swapoff
> too.
>
> Races between some other swap cache usages protected via disabling
> preemption and swapoff are fixed too via calling stop_machine() between
> clearing PageSwapCache() and freeing swap cache data structure.
>
> Alternative implementation could be replacing disable preemption with
> rcu_read_lock_sched and stop_machine() with synchronize_sched().
using stop_machine is generally discouraged. It is a gross
synchronization.
Besides that, since when do we have this problem?
> Signed-off-by: "Huang, Ying" <[email protected]>
> Not-Nacked-by: Hugh Dickins <[email protected]>
> Cc: Paul E. McKenney <[email protected]>
> Cc: Minchan Kim <[email protected]>
> Cc: Johannes Weiner <[email protected]>
> Cc: Tim Chen <[email protected]>
> Cc: Mel Gorman <[email protected]>
> Cc: J?r?me Glisse <[email protected]>
> Cc: Michal Hocko <[email protected]>
> Cc: Andrea Arcangeli <[email protected]>
> Cc: David Rientjes <[email protected]>
> Cc: Rik van Riel <[email protected]>
> Cc: Jan Kara <[email protected]>
> Cc: Dave Jiang <[email protected]>
> Cc: Daniel Jordan <[email protected]>
> Cc: Andrea Parri <[email protected]>
>
> Changelog:
>
> v7:
>
> - Rebased on patch: "mm, swap: bounds check swap_info accesses to avoid NULL derefs"
>
> v6:
>
> - Add more comments to get_swap_device() to make it more clear about
> possible swapoff or swapoff+swapon.
>
> v5:
>
> - Replace RCU with stop_machine()
>
> v4:
>
> - Use synchronize_rcu() in enable_swap_info() to reduce overhead of
> normal paths further.
>
> v3:
>
> - Re-implemented with RCU to reduce the overhead of normal paths
>
> v2:
>
> - Re-implemented with SRCU to reduce the overhead of normal paths.
>
> - Avoid to check whether the swap device has been swapoff in
> get_swap_device(). Because we can check the origin of the swap
> entry to make sure the swap device hasn't bee swapoff.
> ---
> include/linux/swap.h | 13 +++-
> mm/memory.c | 2 +-
> mm/swap_state.c | 16 ++++-
> mm/swapfile.c | 150 ++++++++++++++++++++++++++++++++++---------
> 4 files changed, 145 insertions(+), 36 deletions(-)
>
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index 649529be91f2..aecd1430d0a9 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -175,8 +175,9 @@ enum {
> SWP_PAGE_DISCARD = (1 << 10), /* freed swap page-cluster discards */
> SWP_STABLE_WRITES = (1 << 11), /* no overwrite PG_writeback pages */
> SWP_SYNCHRONOUS_IO = (1 << 12), /* synchronous IO is efficient */
> + SWP_VALID = (1 << 13), /* swap is valid to be operated on? */
> /* add others here before... */
> - SWP_SCANNING = (1 << 13), /* refcount in scan_swap_map */
> + SWP_SCANNING = (1 << 14), /* refcount in scan_swap_map */
> };
>
> #define SWAP_CLUSTER_MAX 32UL
> @@ -460,7 +461,7 @@ extern unsigned int count_swap_pages(int, int);
> extern sector_t map_swap_page(struct page *, struct block_device **);
> extern sector_t swapdev_block(int, pgoff_t);
> extern int page_swapcount(struct page *);
> -extern int __swap_count(struct swap_info_struct *si, swp_entry_t entry);
> +extern int __swap_count(swp_entry_t entry);
> extern int __swp_swapcount(swp_entry_t entry);
> extern int swp_swapcount(swp_entry_t entry);
> extern struct swap_info_struct *page_swap_info(struct page *);
> @@ -470,6 +471,12 @@ extern int try_to_free_swap(struct page *);
> struct backing_dev_info;
> extern int init_swap_address_space(unsigned int type, unsigned long nr_pages);
> extern void exit_swap_address_space(unsigned int type);
> +extern struct swap_info_struct *get_swap_device(swp_entry_t entry);
> +
> +static inline void put_swap_device(struct swap_info_struct *si)
> +{
> + preempt_enable();
> +}
>
> #else /* CONFIG_SWAP */
>
> @@ -576,7 +583,7 @@ static inline int page_swapcount(struct page *page)
> return 0;
> }
>
> -static inline int __swap_count(struct swap_info_struct *si, swp_entry_t entry)
> +static inline int __swap_count(swp_entry_t entry)
> {
> return 0;
> }
> diff --git a/mm/memory.c b/mm/memory.c
> index 34ced1369883..9c0743c17c6c 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -2719,7 +2719,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
> struct swap_info_struct *si = swp_swap_info(entry);
>
> if (si->flags & SWP_SYNCHRONOUS_IO &&
> - __swap_count(si, entry) == 1) {
> + __swap_count(entry) == 1) {
> /* skip swapcache */
> page = alloc_page_vma(GFP_HIGHUSER_MOVABLE, vma,
> vmf->address);
> diff --git a/mm/swap_state.c b/mm/swap_state.c
> index 85245fdec8d9..61453f1faf72 100644
> --- a/mm/swap_state.c
> +++ b/mm/swap_state.c
> @@ -310,8 +310,13 @@ struct page *lookup_swap_cache(swp_entry_t entry, struct vm_area_struct *vma,
> unsigned long addr)
> {
> struct page *page;
> + struct swap_info_struct *si;
>
> + si = get_swap_device(entry);
> + if (!si)
> + return NULL;
> page = find_get_page(swap_address_space(entry), swp_offset(entry));
> + put_swap_device(si);
>
> INC_CACHE_INFO(find_total);
> if (page) {
> @@ -354,8 +359,8 @@ struct page *__read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
> struct vm_area_struct *vma, unsigned long addr,
> bool *new_page_allocated)
> {
> - struct page *found_page, *new_page = NULL;
> - struct address_space *swapper_space = swap_address_space(entry);
> + struct page *found_page = NULL, *new_page = NULL;
> + struct swap_info_struct *si;
> int err;
> *new_page_allocated = false;
>
> @@ -365,7 +370,12 @@ struct page *__read_swap_cache_async(swp_entry_t entry, gfp_t gfp_mask,
> * called after lookup_swap_cache() failed, re-calling
> * that would confuse statistics.
> */
> - found_page = find_get_page(swapper_space, swp_offset(entry));
> + si = get_swap_device(entry);
> + if (!si)
> + break;
> + found_page = find_get_page(swap_address_space(entry),
> + swp_offset(entry));
> + put_swap_device(si);
> if (found_page)
> break;
>
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index cca8420b12db..8f92f36814fb 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -38,6 +38,7 @@
> #include <linux/export.h>
> #include <linux/swap_slots.h>
> #include <linux/sort.h>
> +#include <linux/stop_machine.h>
>
> #include <asm/pgtable.h>
> #include <asm/tlbflush.h>
> @@ -1186,6 +1187,64 @@ static unsigned char __swap_entry_free_locked(struct swap_info_struct *p,
> return usage;
> }
>
> +/*
> + * Check whether swap entry is valid in the swap device. If so,
> + * return pointer to swap_info_struct, and keep the swap entry valid
> + * via preventing the swap device from being swapoff, until
> + * put_swap_device() is called. Otherwise return NULL.
> + *
> + * Notice that swapoff or swapoff+swapon can still happen before the
> + * preempt_disable() in get_swap_device() or after the
> + * preempt_enable() in put_swap_device() if there isn't any other way
> + * to prevent swapoff, such as page lock, page table lock, etc. The
> + * caller must be prepared for that. For example, the following
> + * situation is possible.
> + *
> + * CPU1 CPU2
> + * do_swap_page()
> + * ... swapoff+swapon
> + * __read_swap_cache_async()
> + * swapcache_prepare()
> + * __swap_duplicate()
> + * // check swap_map
> + * // verify PTE not changed
> + *
> + * In __swap_duplicate(), the swap_map need to be checked before
> + * changing partly because the specified swap entry may be for another
> + * swap device which has been swapoff. And in do_swap_page(), after
> + * the page is read from the swap device, the PTE is verified not
> + * changed with the page table locked to check whether the swap device
> + * has been swapoff or swapoff+swapon.
> + */
> +struct swap_info_struct *get_swap_device(swp_entry_t entry)
> +{
> + struct swap_info_struct *si;
> + unsigned long type, offset;
> +
> + if (!entry.val)
> + goto out;
> + type = swp_type(entry);
> + si = swap_type_to_swap_info(type);
> + if (!si)
> + goto bad_nofile;
> +
> + preempt_disable();
> + if (!(si->flags & SWP_VALID))
> + goto unlock_out;
> + offset = swp_offset(entry);
> + if (offset >= si->max)
> + goto unlock_out;
> +
> + return si;
> +bad_nofile:
> + pr_err("%s: %s%08lx\n", __func__, Bad_file, entry.val);
> +out:
> + return NULL;
> +unlock_out:
> + preempt_enable();
> + return NULL;
> +}
> +
> static unsigned char __swap_entry_free(struct swap_info_struct *p,
> swp_entry_t entry, unsigned char usage)
> {
> @@ -1357,11 +1416,18 @@ int page_swapcount(struct page *page)
> return count;
> }
>
> -int __swap_count(struct swap_info_struct *si, swp_entry_t entry)
> +int __swap_count(swp_entry_t entry)
> {
> + struct swap_info_struct *si;
> pgoff_t offset = swp_offset(entry);
> + int count = 0;
>
> - return swap_count(si->swap_map[offset]);
> + si = get_swap_device(entry);
> + if (si) {
> + count = swap_count(si->swap_map[offset]);
> + put_swap_device(si);
> + }
> + return count;
> }
>
> static int swap_swapcount(struct swap_info_struct *si, swp_entry_t entry)
> @@ -1386,9 +1452,11 @@ int __swp_swapcount(swp_entry_t entry)
> int count = 0;
> struct swap_info_struct *si;
>
> - si = __swap_info_get(entry);
> - if (si)
> + si = get_swap_device(entry);
> + if (si) {
> count = swap_swapcount(si, entry);
> + put_swap_device(si);
> + }
> return count;
> }
>
> @@ -2332,9 +2400,9 @@ static int swap_node(struct swap_info_struct *p)
> return bdev ? bdev->bd_disk->node_id : NUMA_NO_NODE;
> }
>
> -static void _enable_swap_info(struct swap_info_struct *p, int prio,
> - unsigned char *swap_map,
> - struct swap_cluster_info *cluster_info)
> +static void setup_swap_info(struct swap_info_struct *p, int prio,
> + unsigned char *swap_map,
> + struct swap_cluster_info *cluster_info)
> {
> int i;
>
> @@ -2359,7 +2427,11 @@ static void _enable_swap_info(struct swap_info_struct *p, int prio,
> }
> p->swap_map = swap_map;
> p->cluster_info = cluster_info;
> - p->flags |= SWP_WRITEOK;
> +}
> +
> +static void _enable_swap_info(struct swap_info_struct *p)
> +{
> + p->flags |= SWP_WRITEOK | SWP_VALID;
> atomic_long_add(p->pages, &nr_swap_pages);
> total_swap_pages += p->pages;
>
> @@ -2378,6 +2450,11 @@ static void _enable_swap_info(struct swap_info_struct *p, int prio,
> add_to_avail_list(p);
> }
>
> +static int swap_onoff_stop(void *arg)
> +{
> + return 0;
> +}
> +
> static void enable_swap_info(struct swap_info_struct *p, int prio,
> unsigned char *swap_map,
> struct swap_cluster_info *cluster_info,
> @@ -2386,7 +2463,17 @@ static void enable_swap_info(struct swap_info_struct *p, int prio,
> frontswap_init(p->type, frontswap_map);
> spin_lock(&swap_lock);
> spin_lock(&p->lock);
> - _enable_swap_info(p, prio, swap_map, cluster_info);
> + setup_swap_info(p, prio, swap_map, cluster_info);
> + spin_unlock(&p->lock);
> + spin_unlock(&swap_lock);
> + /*
> + * Guarantee swap_map, cluster_info, etc. fields are used
> + * between get/put_swap_device() only if SWP_VALID bit is set
> + */
> + stop_machine(swap_onoff_stop, NULL, cpu_online_mask);
> + spin_lock(&swap_lock);
> + spin_lock(&p->lock);
> + _enable_swap_info(p);
> spin_unlock(&p->lock);
> spin_unlock(&swap_lock);
> }
> @@ -2395,7 +2482,8 @@ static void reinsert_swap_info(struct swap_info_struct *p)
> {
> spin_lock(&swap_lock);
> spin_lock(&p->lock);
> - _enable_swap_info(p, p->prio, p->swap_map, p->cluster_info);
> + setup_swap_info(p, p->prio, p->swap_map, p->cluster_info);
> + _enable_swap_info(p);
> spin_unlock(&p->lock);
> spin_unlock(&swap_lock);
> }
> @@ -2498,6 +2586,17 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile)
>
> reenable_swap_slots_cache_unlock();
>
> + spin_lock(&swap_lock);
> + spin_lock(&p->lock);
> + p->flags &= ~SWP_VALID; /* mark swap device as invalid */
> + spin_unlock(&p->lock);
> + spin_unlock(&swap_lock);
> + /*
> + * wait for swap operations protected by get/put_swap_device()
> + * to complete
> + */
> + stop_machine(swap_onoff_stop, NULL, cpu_online_mask);
> +
> flush_work(&p->discard_work);
>
> destroy_swap_extents(p);
> @@ -3263,17 +3362,11 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage)
> unsigned char has_cache;
> int err = -EINVAL;
>
> - if (non_swap_entry(entry))
> - goto out;
> -
> - p = swp_swap_info(entry);
> + p = get_swap_device(entry);
> if (!p)
> - goto bad_file;
> -
> - offset = swp_offset(entry);
> - if (unlikely(offset >= p->max))
> goto out;
>
> + offset = swp_offset(entry);
> ci = lock_cluster_or_swap_info(p, offset);
>
> count = p->swap_map[offset];
> @@ -3319,11 +3412,9 @@ static int __swap_duplicate(swp_entry_t entry, unsigned char usage)
> unlock_out:
> unlock_cluster_or_swap_info(p, ci);
> out:
> + if (p)
> + put_swap_device(p);
> return err;
> -
> -bad_file:
> - pr_err("swap_dup: %s%08lx\n", Bad_file, entry.val);
> - goto out;
> }
>
> /*
> @@ -3415,6 +3506,7 @@ int add_swap_count_continuation(swp_entry_t entry, gfp_t gfp_mask)
> struct page *list_page;
> pgoff_t offset;
> unsigned char count;
> + int ret = 0;
>
> /*
> * When debugging, it's easier to use __GFP_ZERO here; but it's better
> @@ -3422,15 +3514,15 @@ int add_swap_count_continuation(swp_entry_t entry, gfp_t gfp_mask)
> */
> page = alloc_page(gfp_mask | __GFP_HIGHMEM);
>
> - si = swap_info_get(entry);
> + si = get_swap_device(entry);
> if (!si) {
> /*
> * An acceptable race has occurred since the failing
> - * __swap_duplicate(): the swap entry has been freed,
> - * perhaps even the whole swap_map cleared for swapoff.
> + * __swap_duplicate(): the swap device may be swapoff
> */
> goto outer;
> }
> + spin_lock(&si->lock);
>
> offset = swp_offset(entry);
>
> @@ -3448,9 +3540,8 @@ int add_swap_count_continuation(swp_entry_t entry, gfp_t gfp_mask)
> }
>
> if (!page) {
> - unlock_cluster(ci);
> - spin_unlock(&si->lock);
> - return -ENOMEM;
> + ret = -ENOMEM;
> + goto out;
> }
>
> /*
> @@ -3502,10 +3593,11 @@ int add_swap_count_continuation(swp_entry_t entry, gfp_t gfp_mask)
> out:
> unlock_cluster(ci);
> spin_unlock(&si->lock);
> + put_swap_device(si);
> outer:
> if (page)
> __free_page(page);
> - return 0;
> + return ret;
> }
>
> /*
> --
> 2.20.1
>
--
Michal Hocko
SUSE Labs
On Thu, 14 Feb 2019 15:33:18 +0100 Michal Hocko <[email protected]> wrote:
> > Because swapoff() is very rare code path, to make the normal path runs as
> > fast as possible, disabling preemption + stop_machine() instead of
> > reference count is used to implement get/put_swap_device(). From
> > get_swap_device() to put_swap_device(), the preemption is disabled, so
> > stop_machine() in swapoff() will wait until put_swap_device() is called.
> >
> > In addition to swap_map, cluster_info, etc. data structure in the struct
> > swap_info_struct, the swap cache radix tree will be freed after swapoff,
> > so this patch fixes the race between swap cache looking up and swapoff
> > too.
> >
> > Races between some other swap cache usages protected via disabling
> > preemption and swapoff are fixed too via calling stop_machine() between
> > clearing PageSwapCache() and freeing swap cache data structure.
> >
> > Alternative implementation could be replacing disable preemption with
> > rcu_read_lock_sched and stop_machine() with synchronize_sched().
>
> using stop_machine is generally discouraged. It is a gross
> synchronization.
This was discussed to death and I think the changelog explains the
conclusions adequately. swapoff is super-rare so a stop_machine() in
that path is appropriate if its use permits more efficiency in the
regular swap code paths.
> Besides that, since when do we have this problem?
What problem??
Hello,
On Thu, Feb 14, 2019 at 12:30:02PM -0800, Andrew Morton wrote:
> This was discussed to death and I think the changelog explains the
> conclusions adequately. swapoff is super-rare so a stop_machine() in
> that path is appropriate if its use permits more efficiency in the
> regular swap code paths.
The problem is precisely that the way the stop_machine callback is
implemented right now (a dummy noop), makes the stop_machine()
solution fully equivalent to RCU from the fast path point of view. It
does not permit more efficiency in the fast path which is all we care
about.
For the slow path point of view the only difference is possibly that
stop_machine will reach the quiescent state faster (i.e. swapoff may
return a few dozen milliseconds faster), but nobody cares about the
latency of swapoff and it's actually nicer if swapoff doesn't stop all
CPUs on large systems and it uses less CPU overall.
This is why I suggested if we keep using stop_machine() we should not
use a dummy function whose only purpose is to reach a queiscent state
(which is something that is more efficiently achieved with the
syncronize_rcu/sched/kernel RCU API of the day) but we should instead
try to leverage the UP-like serialization to remove more spinlocks
from the fast path and convert them to preempt_disable(). However the
current dummy callback cannot achieve that higher efficiency in the
fast paths, the code would need to be reshuffled to try to remove at
least the swap_lock.
If no spinlock is converted to preempt_disable() RCU I don't see the
point of stop_machine().
On a side note, the cmpxchge machinery I posted to run the function
simultaneously on all CPUs I think may actually be superflous if using
cpus=NULL like Hing suggested. Implementation details aside, still the
idea of stop_machine would be to do those p->swap_map = NULL and
everything protected by the swap_lock, should be executed inside the
callback that runs like in a UP system to speedup the fast path
further.
Thanks,
Andrea
On Thu, Feb 14, 2019 at 04:07:37PM +0800, Huang, Ying wrote:
> Before, we choose to use stop_machine() to reduce the overhead of hot
> path (page fault handler) as much as possible. But now, I found
> rcu_read_lock_sched() is just a wrapper of preempt_disable(). So maybe
> we can switch to RCU version now.
rcu_read_lock looks more efficient than rcu_read_lock_sched. So for
this purpose in the fast path rcu_read_lock()/unlock() should be the
preferred methods, no need to force preempt_disable() (except for
debug purposes if sleep debug is enabled). Server builds are done with
voluntary preempt (no preempt shouldn't even exist as config option)
and there rcu_read_lock might be just a noop.
Against a fast path rcu_read_lock/unlock before the consolidation
synchronize_rcu would have been enough, now after the consolidation
even more certain that it's enough because it's equivalent to _mult.
Andrea Arcangeli <[email protected]> writes:
> On Thu, Feb 14, 2019 at 04:07:37PM +0800, Huang, Ying wrote:
>> Before, we choose to use stop_machine() to reduce the overhead of hot
>> path (page fault handler) as much as possible. But now, I found
>> rcu_read_lock_sched() is just a wrapper of preempt_disable(). So maybe
>> we can switch to RCU version now.
>
> rcu_read_lock looks more efficient than rcu_read_lock_sched. So for
> this purpose in the fast path rcu_read_lock()/unlock() should be the
> preferred methods, no need to force preempt_disable() (except for
> debug purposes if sleep debug is enabled). Server builds are done with
> voluntary preempt (no preempt shouldn't even exist as config option)
> and there rcu_read_lock might be just a noop.
If
CONFIG_PREEMPT_VOLUNTARY=y
CONFIG_PREEMPT=n
CONFIG_PREEMPT_COUNT=n
which is common for servers,
rcu_read_lock() will be a noop, rcu_read_lock_sched() and
preempt_disable() will be barrier(). So rcu_read_lock() is a little
better.
> Against a fast path rcu_read_lock/unlock before the consolidation
> synchronize_rcu would have been enough, now after the consolidation
> even more certain that it's enough because it's equivalent to _mult.
Yes. Will change to rcu_read_lock/unlock based method.
Best Regards,
Huang, Ying
On Fri 15-02-19 15:08:36, Huang, Ying wrote:
> Michal Hocko <[email protected]> writes:
>
> > On Mon 11-02-19 16:38:46, Huang, Ying wrote:
> >> From: Huang Ying <[email protected]>
> >>
> >> When swapin is performed, after getting the swap entry information from
> >> the page table, system will swap in the swap entry, without any lock held
> >> to prevent the swap device from being swapoff. This may cause the race
> >> like below,
> >>
> >> CPU 1 CPU 2
> >> ----- -----
> >> do_swap_page
> >> swapin_readahead
> >> __read_swap_cache_async
> >> swapoff swapcache_prepare
> >> p->swap_map = NULL __swap_duplicate
> >> p->swap_map[?] /* !!! NULL pointer access */
> >>
> >> Because swapoff is usually done when system shutdown only, the race may
> >> not hit many people in practice. But it is still a race need to be fixed.
> >>
> >> To fix the race, get_swap_device() is added to check whether the specified
> >> swap entry is valid in its swap device. If so, it will keep the swap
> >> entry valid via preventing the swap device from being swapoff, until
> >> put_swap_device() is called.
> >>
> >> Because swapoff() is very rare code path, to make the normal path runs as
> >> fast as possible, disabling preemption + stop_machine() instead of
> >> reference count is used to implement get/put_swap_device(). From
> >> get_swap_device() to put_swap_device(), the preemption is disabled, so
> >> stop_machine() in swapoff() will wait until put_swap_device() is called.
> >>
> >> In addition to swap_map, cluster_info, etc. data structure in the struct
> >> swap_info_struct, the swap cache radix tree will be freed after swapoff,
> >> so this patch fixes the race between swap cache looking up and swapoff
> >> too.
> >>
> >> Races between some other swap cache usages protected via disabling
> >> preemption and swapoff are fixed too via calling stop_machine() between
> >> clearing PageSwapCache() and freeing swap cache data structure.
> >>
> >> Alternative implementation could be replacing disable preemption with
> >> rcu_read_lock_sched and stop_machine() with synchronize_sched().
> >
> > using stop_machine is generally discouraged. It is a gross
> > synchronization.
> >
> > Besides that, since when do we have this problem?
>
> For problem, you mean the race between swapoff and the page fault
> handler?
yes
> The problem is introduced in v4.11 when we avoid to replace
> swap_info_struct->lock with swap_cluster_info->lock in
> __swap_duplicate() if possible to improve the scalability of swap
> operations. But because swapoff is a really rare operation, I don't
> think it's necessary to backport the fix.
Well, a lack of any bug reports would support your theory that this is
unlikely to hit in practice. Fixes tag would be nice to have regardless
though.
Thanks!
--
Michal Hocko
SUSE Labs