From: Ryan Roberts <[email protected]>
There was previously a theoretical window where swapoff() could run and
teardown a swap_info_struct while a call to free_swap_and_cache() was
running in another thread. This could cause, amongst other bad
possibilities, swap_page_trans_huge_swapped() (called by
free_swap_and_cache()) to access the freed memory for swap_map.
This is a theoretical problem and I haven't been able to provoke it from a
test case. But there has been agreement based on code review that this is
possible (see link below).
Fix it by using get_swap_device()/put_swap_device(), which will stall
swapoff(). There was an extra check in _swap_info_get() to confirm that
the swap entry was not free. This isn't present in get_swap_device()
because it doesn't make sense in general due to the race between getting
the reference and swapoff. So I've added an equivalent check directly in
free_swap_and_cache().
Details of how to provoke one possible issue:
--8<-----
CPU0 CPU1
---- ----
shmem_undo_range
shmem_free_swap
xa_cmpxchg_irq
free_swap_and_cache
__swap_entry_free
/* swap_count() become 0 */
swapoff
try_to_unuse
shmem_unuse /* cannot find swap entry */
find_next_to_unuse
filemap_get_folio
folio_free_swap
/* remove swap cache */
/* free si->swap_map[] */
swap_page_trans_huge_swapped <-- access freed si->swap_map !!!
--8<-----
Link: https://lkml.kernel.org/r/[email protected]
Closes: https://lore.kernel.org/linux-mm/[email protected]/
Signed-off-by: Ryan Roberts <[email protected]>
Signed-off-by: "Huang, Ying" <[email protected]> [patch description]
Cc: David Hildenbrand <[email protected]>
Cc: <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---
Hi, Andrew,
If it's not too late. Please replace v2 of this patch in mm-stable
with this version.
Changes since v2:
- Remove comments for get_swap_device() because it's not correct.
- Revised patch description about the race condition description.
Changes since v1:
- Added comments for get_swap_device() as suggested by David
- Moved check that swap entry is not free from get_swap_device() to
free_swap_and_cache() since there are some paths that legitimately call with
a free offset.
Best Regards,
Huang, Ying
mm/swapfile.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 2b3a2d85e350..9e0691276f5e 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1609,13 +1609,19 @@ int free_swap_and_cache(swp_entry_t entry)
if (non_swap_entry(entry))
return 1;
- p = _swap_info_get(entry);
+ p = get_swap_device(entry);
if (p) {
+ if (WARN_ON(data_race(!p->swap_map[swp_offset(entry)]))) {
+ put_swap_device(p);
+ return 0;
+ }
+
count = __swap_entry_free(p, entry);
if (count == SWAP_HAS_CACHE &&
!swap_page_trans_huge_swapped(p, entry))
__try_to_reclaim_swap(p, swp_offset(entry),
TTRS_UNMAPPED | TTRS_FULL);
+ put_swap_device(p);
}
return p != NULL;
}
--
2.39.2
On 11/03/2024 08:44, Huang Ying wrote:
> From: Ryan Roberts <[email protected]>
>
> There was previously a theoretical window where swapoff() could run and
> teardown a swap_info_struct while a call to free_swap_and_cache() was
> running in another thread. This could cause, amongst other bad
> possibilities, swap_page_trans_huge_swapped() (called by
> free_swap_and_cache()) to access the freed memory for swap_map.
>
> This is a theoretical problem and I haven't been able to provoke it from a
> test case. But there has been agreement based on code review that this is
> possible (see link below).
>
> Fix it by using get_swap_device()/put_swap_device(), which will stall
> swapoff(). There was an extra check in _swap_info_get() to confirm that
> the swap entry was not free. This isn't present in get_swap_device()
> because it doesn't make sense in general due to the race between getting
> the reference and swapoff. So I've added an equivalent check directly in
> free_swap_and_cache().
>
> Details of how to provoke one possible issue:
>
> --8<-----
>
> CPU0 CPU1
> ---- ----
> shmem_undo_range
> shmem_free_swap
> xa_cmpxchg_irq
> free_swap_and_cache
> __swap_entry_free
> /* swap_count() become 0 */
> swapoff
> try_to_unuse
> shmem_unuse /* cannot find swap entry */
> find_next_to_unuse
> filemap_get_folio
> folio_free_swap
> /* remove swap cache */
> /* free si->swap_map[] */
> swap_page_trans_huge_swapped <-- access freed si->swap_map !!!
>
> --8<-----
>
> Link: https://lkml.kernel.org/r/[email protected]
> Closes: https://lore.kernel.org/linux-mm/[email protected]/
> Signed-off-by: Ryan Roberts <[email protected]>
> Signed-off-by: "Huang, Ying" <[email protected]> [patch description]
> Cc: David Hildenbrand <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
> ---
> Hi, Andrew,
>
> If it's not too late. Please replace v2 of this patch in mm-stable
> with this version.
Thanks for sorting this out, Huang, Ying! I saw your note asking if I could do
it, and it was on my list, but I've been busy debugging other urgent issues in
mm-stable. That should be solved now so unblocks me finishing the testing on my
large folios swap-out v4 series. Hopefully that will be incomming in the next
couple of days.
You did previously suggest you wanted some comments around synchronise_rcu() in
swapoff(), but I don't see those here. I don't think that should hold this up
though.
Thanks,
Ryan
>
> Changes since v2:
>
> - Remove comments for get_swap_device() because it's not correct.
> - Revised patch description about the race condition description.
>
> Changes since v1:
>
> - Added comments for get_swap_device() as suggested by David
> - Moved check that swap entry is not free from get_swap_device() to
> free_swap_and_cache() since there are some paths that legitimately call with
> a free offset.
>
> Best Regards,
> Huang, Ying
>
> mm/swapfile.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 2b3a2d85e350..9e0691276f5e 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1609,13 +1609,19 @@ int free_swap_and_cache(swp_entry_t entry)
> if (non_swap_entry(entry))
> return 1;
>
> - p = _swap_info_get(entry);
> + p = get_swap_device(entry);
> if (p) {
> + if (WARN_ON(data_race(!p->swap_map[swp_offset(entry)]))) {
> + put_swap_device(p);
> + return 0;
> + }
> +
> count = __swap_entry_free(p, entry);
> if (count == SWAP_HAS_CACHE &&
> !swap_page_trans_huge_swapped(p, entry))
> __try_to_reclaim_swap(p, swp_offset(entry),
> TTRS_UNMAPPED | TTRS_FULL);
> + put_swap_device(p);
> }
> return p != NULL;
> }
Ryan Roberts <[email protected]> writes:
> On 11/03/2024 08:44, Huang Ying wrote:
>> From: Ryan Roberts <[email protected]>
>>
>> There was previously a theoretical window where swapoff() could run and
>> teardown a swap_info_struct while a call to free_swap_and_cache() was
>> running in another thread. This could cause, amongst other bad
>> possibilities, swap_page_trans_huge_swapped() (called by
>> free_swap_and_cache()) to access the freed memory for swap_map.
>>
>> This is a theoretical problem and I haven't been able to provoke it from a
>> test case. But there has been agreement based on code review that this is
>> possible (see link below).
>>
>> Fix it by using get_swap_device()/put_swap_device(), which will stall
>> swapoff(). There was an extra check in _swap_info_get() to confirm that
>> the swap entry was not free. This isn't present in get_swap_device()
>> because it doesn't make sense in general due to the race between getting
>> the reference and swapoff. So I've added an equivalent check directly in
>> free_swap_and_cache().
>>
>> Details of how to provoke one possible issue:
>>
>> --8<-----
>>
>> CPU0 CPU1
>> ---- ----
>> shmem_undo_range
>> shmem_free_swap
>> xa_cmpxchg_irq
>> free_swap_and_cache
>> __swap_entry_free
>> /* swap_count() become 0 */
>> swapoff
>> try_to_unuse
>> shmem_unuse /* cannot find swap entry */
>> find_next_to_unuse
>> filemap_get_folio
>> folio_free_swap
>> /* remove swap cache */
>> /* free si->swap_map[] */
>> swap_page_trans_huge_swapped <-- access freed si->swap_map !!!
>>
>> --8<-----
>>
>> Link: https://lkml.kernel.org/r/[email protected]
>> Closes: https://lore.kernel.org/linux-mm/[email protected]/
>> Signed-off-by: Ryan Roberts <[email protected]>
>> Signed-off-by: "Huang, Ying" <[email protected]> [patch description]
>> Cc: David Hildenbrand <[email protected]>
>> Cc: <[email protected]>
>> Signed-off-by: Andrew Morton <[email protected]>
>> ---
>> Hi, Andrew,
>>
>> If it's not too late. Please replace v2 of this patch in mm-stable
>> with this version.
>
> Thanks for sorting this out, Huang, Ying! I saw your note asking if I could do
> it, and it was on my list, but I've been busy debugging other urgent issues in
> mm-stable. That should be solved now so unblocks me finishing the testing on my
> large folios swap-out v4 series. Hopefully that will be incomming in the next
> couple of days.
You are welcome!
> You did previously suggest you wanted some comments around synchronise_rcu() in
> swapoff(), but I don't see those here. I don't think that should hold this up
> though.
I will send a separate patch for that, including comments for
get_swap_device() and around synchronize_rcu() in swapoff().
--
Best Regards,
Huang, Ying
> Thanks,
> Ryan
>
>
>>
>> Changes since v2:
>>
>> - Remove comments for get_swap_device() because it's not correct.
>> - Revised patch description about the race condition description.
>>
>> Changes since v1:
>>
>> - Added comments for get_swap_device() as suggested by David
>> - Moved check that swap entry is not free from get_swap_device() to
>> free_swap_and_cache() since there are some paths that legitimately call with
>> a free offset.
>>
>> Best Regards,
>> Huang, Ying
>>
>> mm/swapfile.c | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>> index 2b3a2d85e350..9e0691276f5e 100644
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -1609,13 +1609,19 @@ int free_swap_and_cache(swp_entry_t entry)
>> if (non_swap_entry(entry))
>> return 1;
>>
>> - p = _swap_info_get(entry);
>> + p = get_swap_device(entry);
>> if (p) {
>> + if (WARN_ON(data_race(!p->swap_map[swp_offset(entry)]))) {
>> + put_swap_device(p);
>> + return 0;
>> + }
>> +
>> count = __swap_entry_free(p, entry);
>> if (count == SWAP_HAS_CACHE &&
>> !swap_page_trans_huge_swapped(p, entry))
>> __try_to_reclaim_swap(p, swp_offset(entry),
>> TTRS_UNMAPPED | TTRS_FULL);
>> + put_swap_device(p);
>> }
>> return p != NULL;
>> }