Received-SPF: pass (google.com: domain of linux-kernel+bounces-99700-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: "Huang, Ying" <ying.huang@intel.com>
To: Ryan Roberts <ryan.roberts@arm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,  <linux-mm@kvack.org>,
  <linux-kernel@vger.kernel.org>,  David Hildenbrand <david@redhat.com>,
  <stable@vger.kernel.org>
Subject: Re: [PATCH -v3] mm: swap: fix race between free_swap_and_cache()
 and swapoff()
In-Reply-To: <1d27e93b-1c6b-4909-859f-e0756974a640@arm.com> (Ryan Roberts's
	message of "Mon, 11 Mar 2024 09:44:47 +0000")
References: <20240311084426.447164-1-ying.huang@intel.com>
	<1d27e93b-1c6b-4909-859f-e0756974a640@arm.com>
Date: Tue, 12 Mar 2024 08:27:36 +0800
Message-ID: <87v85s47lz.fsf@yhuang6-desk2.ccr.corp.intel.com>
User-Agent: Gnus/5.13 (Gnus v5.13)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=ascii

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