Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v4 4/4] mm/shmem: fix shmem_swapin() race with swapoff
To:     "Huang, Ying" <ying.huang@intel.com>
CC:     <akpm@linux-foundation.org>, <dennis@kernel.org>,
        <tim.c.chen@linux.intel.com>, <hughd@google.com>,
        <hannes@cmpxchg.org>, <mhocko@suse.com>, <iamjoonsoo.kim@lge.com>,
        <alexs@kernel.org>, <willy@infradead.org>, <minchan@kernel.org>,
        <richard.weiyang@gmail.com>, <shy828301@gmail.com>,
        <david@redhat.com>, <linux-kernel@vger.kernel.org>,
        <linux-mm@kvack.org>
References: <20210425023806.3537283-1-linmiaohe@huawei.com>
 <20210425023806.3537283-5-linmiaohe@huawei.com>
 <87bla3xdt0.fsf@yhuang6-desk1.ccr.corp.intel.com>
From:   Miaohe Lin <linmiaohe@huawei.com>
Message-ID: <0213893e-2b05-8d2e-9a79-e8a71db23644@huawei.com>
Date:   Sun, 25 Apr 2021 11:33:42 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.0
MIME-Version: 1.0
In-Reply-To: <87bla3xdt0.fsf@yhuang6-desk1.ccr.corp.intel.com>
Content-Type: text/plain; charset="windows-1252"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2021/4/25 11:07, Huang, Ying wrote:
> Miaohe Lin <linmiaohe@huawei.com> writes:
> 
>> When I was investigating the swap code, I found the below possible race
>> window:
>>
>> CPU 1                                         CPU 2
>> -----                                         -----
>> shmem_swapin
>>   swap_cluster_readahead
>>     if (likely(si->flags & (SWP_BLKDEV | SWP_FS_OPS))) {
>>                                               swapoff
>>                                                 ..
>>                                                 si->swap_file = NULL;
>>                                                 ..
>>     struct inode *inode = si->swap_file->f_mapping->host;[oops!]
>>
>> Close this race window by using get/put_swap_device() to guard against
>> concurrent swapoff.
>>
>> Fixes: 8fd2e0b505d1 ("mm: swap: check if swap backing device is congested or not")
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>  mm/shmem.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 26c76b13ad23..be388d0cf8b5 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -1696,6 +1696,7 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index,
>>  	struct address_space *mapping = inode->i_mapping;
>>  	struct shmem_inode_info *info = SHMEM_I(inode);
>>  	struct mm_struct *charge_mm = vma ? vma->vm_mm : current->mm;
>> +	struct swap_info_struct *si;
>>  	struct page *page;
>>  	swp_entry_t swap;
>>  	int error;
>> @@ -1704,6 +1705,12 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index,
>>  	swap = radix_to_swp_entry(*pagep);
>>  	*pagep = NULL;
>>  
>> +	/* Prevent swapoff from happening to us. */
>> +	si = get_swap_device(swap);
>> +	if (unlikely(!si)) {
>> +		error = EINVAL;
>> +		goto failed;
>> +	}
>>  	/* Look it up and read it in.. */
>>  	page = lookup_swap_cache(swap, NULL, 0);
>>  	if (!page) {
>> @@ -1720,6 +1727,7 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index,
>>  			goto failed;
>>  		}
>>  	}
>> +	put_swap_device(si);
> 
> I think it's better to put_swap_device() just before returning from the
> function.  It's not a big issue to slow down swapoff() a little.  And
> this will make the logic easier to be understood.
> 

shmem_swapin_page() already has a methed, i.e. locked page, to prevent races. I was intended
to not mix with that. But your suggestion is good as this will make the logic easier to be
understood.

Just to make sure, is this what you mean? Many thanks!

diff --git a/mm/shmem.c b/mm/shmem.c
index 26c76b13ad23..737e5b3200c3 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1696,6 +1696,7 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index,
        struct address_space *mapping = inode->i_mapping;
        struct shmem_inode_info *info = SHMEM_I(inode);
        struct mm_struct *charge_mm = vma ? vma->vm_mm : current->mm;
+       struct swap_info_struct *si;
        struct page *page;
        swp_entry_t swap;
        int error;
@@ -1704,6 +1705,12 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index,
        swap = radix_to_swp_entry(*pagep);
        *pagep = NULL;

+       /* Prevent swapoff from happening to us. */
+       si = get_swap_device(swap);
+       if (unlikely(!si)) {
+               error = EINVAL;
+               goto failed;
+       }
        /* Look it up and read it in.. */
        page = lookup_swap_cache(swap, NULL, 0);
        if (!page) {
@@ -1765,6 +1772,8 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index,
        swap_free(swap);

        *pagep = page;
+       if (si)
+               put_swap_device(si);
        return 0;
 failed:
        if (!shmem_confirm_swap(mapping, index, swap))
@@ -1775,6 +1784,9 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index,
                put_page(page);
        }

+       if (si)
+               put_swap_device(si);
+
        return error;
 }

> Best Regards,
> Huang, Ying
> 
>>  
>>  	/* We have to do this with page locked to prevent races */
>>  	lock_page(page);
>> @@ -1775,6 +1783,9 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index,
>>  		put_page(page);
>>  	}
>>  
>> +	if (si)
>> +		put_swap_device(si);
>> +
>>  	return error;
>>  }
> .
>