linux-kernel - Re: [PATCH] mm/sparsemem: fix race in accessing memory

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 17 Oct 2023 16:53:46 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Charan Teja Kalla <quic_charante@...cinc.com>,
        Pavan Kondeti <quic_pkondeti@...cinc.com>
Cc:     akpm@...ux-foundation.org, osalvador@...e.de,
        dan.j.williams@...el.com, vbabka@...e.cz,
        mgorman@...hsingularity.net, aneesh.kumar@...ux.ibm.com,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm/sparsemem: fix race in accessing memory_section->usage

On 17.10.23 16:10, Charan Teja Kalla wrote:
> 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.

Sound reasonable to me; most PFN walkers shouldn't deal with pageblock 
flags. alloc_contig_range() is certainly interesting, I suspect it's 
fine but we better double-check.

-- 
Cheers,

David / dhildenb