| Message ID | 20240311084426.447164-1-ying.huang@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [-v3] mm: swap: fix race between free_swap_and_cache() and swapoff() | expand |
On 11/03/2024 08:44, Huang Ying wrote: > From: Ryan Roberts <ryan.roberts@arm.com> > > 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/20240306140356.3974886-1-ryan.roberts@arm.com > Closes: https://lore.kernel.org/linux-mm/8734t27awd.fsf@yhuang6-desk2.ccr.corp.intel.com/ > Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> > Signed-off-by: "Huang, Ying" <ying.huang@intel.com> [patch description] > Cc: David Hildenbrand <david@redhat.com> > Cc: <stable@vger.kernel.org> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > --- > 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 <ryan.roberts@arm.com> writes: > On 11/03/2024 08:44, Huang Ying wrote: >> From: Ryan Roberts <ryan.roberts@arm.com> >> >> 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/20240306140356.3974886-1-ryan.roberts@arm.com >> Closes: https://lore.kernel.org/linux-mm/8734t27awd.fsf@yhuang6-desk2.ccr.corp.intel.com/ >> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> >> Signed-off-by: "Huang, Ying" <ying.huang@intel.com> [patch description] >> Cc: David Hildenbrand <david@redhat.com> >> Cc: <stable@vger.kernel.org> >> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> >> --- >> 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; >> }
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; }