Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Message-ID: <b4a2bbe8-4181-68ad-1a6c-463fbe64d8f8@quicinc.com>
Date:   Tue, 17 Oct 2023 19:40:15 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH] mm/sparsemem: fix race in accessing memory_section->usage
To:     Pavan Kondeti <quic_pkondeti@quicinc.com>
CC:     <akpm@linux-foundation.org>, <osalvador@suse.de>,
        <dan.j.williams@intel.com>, <david@redhat.com>, <vbabka@suse.cz>,
        <mgorman@techsingularity.net>, <aneesh.kumar@linux.ibm.com>,
        <linux-mm@kvack.org>, <linux-kernel@vger.kernel.org>
References: <1697202267-23600-1-git-send-email-quic_charante@quicinc.com>
 <e1102c1b-1773-4e59-8a70-00a7deff2bde@quicinc.com>
Content-Language: en-US
From:   Charan Teja Kalla <quic_charante@quicinc.com>
In-Reply-To: <e1102c1b-1773-4e59-8a70-00a7deff2bde@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk

Thanks Pavan!!

On 10/16/2023 4:03 PM, Pavan Kondeti wrote:
>> Fix this issue by the below steps:
>> a) Clear SECTION_HAS_MEM_MAP before freeing the ->usage.
>> b) RCU protected read side critical section will either return NULL when
>> SECTION_HAS_MEM_MAP is cleared or can successfully access ->usage.
>> c) Synchronize the rcu on the write side and free the ->usage. No
>> attempt will be made to access ->usage after this as the
>> SECTION_HAS_MEM_MAP is cleared thus valid_section() return false.
>>
>> Since the section_deactivate() is a rare operation and will come in the
>> hot remove path, impact of synchronize_rcu() should be negligble.
> struct mem_section_usage has other field like pageblock_flags. Do we
> need to protect its readers with RCU? Also can we annotate usage field
> in struct mem_section with __rcu and use RCU accessors like
> rcu_dereference() while using memsection::usage field?

Good question about the pageblock_flags!! I think we rely on the
get_pageblock_bitmap() to read the ms->usage->pageblock_flags by passing
struct page*.

1) All the functions that I have come across calling
get_pageblock_bitmap()/get_pfnblock_flags_mask() passing the page* which
it get from buddy. I think we are safe here as the device pages(from
which the problem is coming will never be onlined/added to buddy)

2) There are functions in compaction which directly operate on the pfn's
through pfn_to_online_page(). As for device pages, it is going to return
NULL, I think we are safe here too.

3) alloc_contig_range() which also operate on the pfn's directly, and
TMK, we will not pass the [start , end) values of this function to
contains the hole/offlined pages. I think we are safe here too.

May be David/other reviewers can help in commenting if there are some
mistakes above.

I am currently working on to use the __rcu accessors.

> 
>>  		/*
>> +		 * Mark the section invalid so that valid_section()
>> +		 * return false. This prevents code from dereferencing
>> +		 * ms->usage array.
>> +		 */
>> +		ms->section_mem_map &= ~SECTION_HAS_MEM_MAP;
>> +
> This trick may not be needed if we add proper NULL checks around ms->usage. We are anyway
> introducing a new rule this check needs to be done under RCU lock, so why not revisit it?
> 

I think this is for valid_section().

>>  		 * was allocated during boot.
>>  		 */
>>  		if (!PageReserved(virt_to_page(ms->usage))) {
>> +			synchronize_rcu();
>>  			kfree(ms->usage);
>>  			ms->usage = NULL;
>>  		}
> If we add NULL checks around ms->usage, this becomes
> 
> tmp = rcu_replace_pointer(ms->usage, NULL, hotplug_locked());
> syncrhonize_rcu();
> kfree(tmp);
Per David input, I am working on using kfree_rcu().

Thanks,
Charan