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;
Message-ID: <cbe8d847-8a2d-4220-c6a3-775d517e2edd@redhat.com>
Date:   Wed, 20 Oct 2021 11:05:11 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.1.0
Content-Language: en-US
To:     Jianyong Wu <Jianyong.Wu@arm.com>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "mhiramat@kernel.org" <mhiramat@kernel.org>,
        "peterz@infradead.org" <peterz@infradead.org>
Cc:     "rostedt@goodmis.org" <rostedt@goodmis.org>,
        "vbabka@suse.cz" <vbabka@suse.cz>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Anshuman Khandual <Anshuman.Khandual@arm.com>,
        Justin He <Justin.He@arm.com>, nd <nd@arm.com>
References: <20211020020712.90240-1-jianyong.wu@arm.com>
 <4858258b-8fb9-b060-8724-25c89ba2ce0e@redhat.com>
 <AM9PR08MB7276FEF0AE524A9728F63A4CF4BE9@AM9PR08MB7276.eurprd08.prod.outlook.com>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
Subject: Re: [PATCH v1] init: avoid race condition of update page table in
 kernel init
In-Reply-To: <AM9PR08MB7276FEF0AE524A9728F63A4CF4BE9@AM9PR08MB7276.eurprd08.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

>> But why does it matter on arm64? Can you describe how the exact race
>> triggers?
> 
> I don't know much about how x86 does in memory map. Let me show you how the race happens on arm64.
> When virtio-mem workqueue is triggered, arch_memory_add will be called where the related page table will be created. The call chain is arch_memory_add->__create_pgd_mapping->alloc_init_pud. As the memory add may take for serval seconds, it may be concurrent with mark_rodata_ro, in which page tables are created either.

> The race can occur in alloc_init_pud. See below:
> /***************************************************************************/
> Virtio-mem workqueue thread                                                               mark_rodata_ro thread
> {                                                                                                                         
> ...
>         pudp = pud_set_fixmap_offset(p4dp, addr); // set fixmap
>         do {
>                 pud_t old_pud = READ_ONCE(*pudp);                                         
> ...
>         } while (pudp++, addr = next, addr != end);                                   pudp = pud_set_fixmap_offset;  //set fixmap                                
>         pud_clear_fixmap();  // clear fixmap                                              do {
> }                                                                                                                              pud_t old_pud = READ_ONCE(*pudp);//CRASH
> 

I still don't quite understand how that race can even exist. I assume
it's due to the weird semantics of the "fixmap". (whatever that is :) )
I don't see anything similar happen on other archs, especially x86-64
and s390x, which I'm familiar with.

s390x similarly to x86-64 code uses a vmem_mutex to serialize add/remove
in the direct map and a cpa_mutex to serialize attribute changes (and
splitting of large mappings).

The right should be to teach arm64 mmu code that direct mapping updates
might be concurrent, and that two instances might try messing with the
fixmap concurrently.


On a similar topic: I think we might want to reclaim compeltely empty
page tables when unplugging memory; I suspect that we also have to mess
with the fixmap then, whem removing page tables. But I feel like the
whole fixmap machinery is still a big black box for me.

-- 
Thanks,

David / dhildenb