Messages in this thread | | | Date | Wed, 26 Jan 2022 11:30:52 +0100 | Subject | Re: [PATCH v3] arm64/mm: avoid fixmap race condition when create pud mapping | From | David Hildenbrand <> |
| |
On 26.01.22 11:28, Jianyong Wu wrote: > Hi David, > >> -----Original Message----- >> From: David Hildenbrand <david@redhat.com> >> Sent: Wednesday, January 26, 2022 6:18 PM >> To: Ard Biesheuvel <ardb@kernel.org>; Jianyong Wu >> <Jianyong.Wu@arm.com> >> Cc: Justin He <Justin.He@arm.com>; Catalin Marinas >> <Catalin.Marinas@arm.com>; will@kernel.org; Anshuman Khandual >> <Anshuman.Khandual@arm.com>; akpm@linux-foundation.org; >> quic_qiancai@quicinc.com; linux-kernel@vger.kernel.org; linux-arm- >> kernel@lists.infradead.org; gshan@redhat.com; nd <nd@arm.com> >> Subject: Re: [PATCH v3] arm64/mm: avoid fixmap race condition when create >> pud mapping >> >> On 26.01.22 11:12, Ard Biesheuvel wrote: >>> On Wed, 26 Jan 2022 at 11:09, Jianyong Wu <Jianyong.Wu@arm.com> >> wrote: >>>> >>>> Hi Ard, >>>> >>>>> -----Original Message----- >>>>> From: Ard Biesheuvel <ardb@kernel.org> >>>>> Sent: Wednesday, January 26, 2022 4:37 PM >>>>> To: Justin He <Justin.He@arm.com> >>>>> Cc: Catalin Marinas <Catalin.Marinas@arm.com>; Jianyong Wu >>>>> <Jianyong.Wu@arm.com>; will@kernel.org; Anshuman Khandual >>>>> <Anshuman.Khandual@arm.com>; akpm@linux-foundation.org; >>>>> david@redhat.com; quic_qiancai@quicinc.com; linux- >>>>> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; >>>>> gshan@redhat.com; nd <nd@arm.com> >>>>> Subject: Re: [PATCH v3] arm64/mm: avoid fixmap race condition when >>>>> create pud mapping >>>>> >>>>> On Wed, 26 Jan 2022 at 05:21, Justin He <Justin.He@arm.com> wrote: >>>>>> >>>>>> Hi Catalin >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: Catalin Marinas <catalin.marinas@arm.com> >>>>>>> Sent: Friday, January 7, 2022 6:43 PM >>>>>>> To: Jianyong Wu <Jianyong.Wu@arm.com> >>>>>>> Cc: will@kernel.org; Anshuman Khandual >>>>> <Anshuman.Khandual@arm.com>; >>>>>>> akpm@linux-foundation.org; david@redhat.com; >>>>>>> quic_qiancai@quicinc.com; ardb@kernel.org; >>>>>>> linux-kernel@vger.kernel.org; linux-arm- >>>>>>> kernel@lists.infradead.org; gshan@redhat.com; Justin He >>>>>>> <Justin.He@arm.com>; nd <nd@arm.com> >>>>>>> Subject: Re: [PATCH v3] arm64/mm: avoid fixmap race condition when >>>>>>> create pud mapping >>>>>>> >>>>>>> On Fri, Jan 07, 2022 at 09:10:57AM +0000, Jianyong Wu wrote: >>>>>>>> Hi Catalin, >>>>>>>> >>>>>>>> I roughly find the root cause. >>>>>>>> alloc_init_pud will be called at the very beginning of kernel >>>>>>>> boot in >>>>>>> create_mapping_noalloc where no memory allocator is initialized. >>>>>>> But lockdep check may need allocate memory. So, kernel take >>>>>>> exception when acquire lock.(I have not found the exact code that >>>>>>> cause this >>>>>>> issue) that's say we may not be able to use a lock so early. >>>>>>>> >>>>>>>> I come up with 2 methods to address it. >>>>>>>> 1) skip dead lock check at the very beginning of kernel boot in >>>>>>>> lockdep >>>>>>> code. >>>>>>>> 2) provided 2 two versions of __create_pgd_mapping, one with lock >>>>>>>> in it and the other without. There may be no possible of race for >>>>>>>> memory mapping at the very beginning time of kernel boot, thus we >>>>>>>> can use the no lock version of __create_pgd_mapping safely. >>>>>>>> In my test, this issue is gone if there is no lock held in >>>>>>>> create_mapping_noalloc. I think create_mapping_noalloc is called >>>>>>>> early enough to avoid the race conditions of memory mapping, >>>>>>>> however, I have not proved it. >>>>>>> >>>>>>> I think method 2 would work better but rather than implementing >>>>>>> new nolock functions I'd add a NO_LOCK flag and check it in >>>>>>> alloc_init_pud() before mutex_lock/unlock. Also add a comment >> when >>>>>>> passing the NO_LOCK flag on why it's needed and why there wouldn't >>>>>>> be any races at that stage (early boot etc.) >>>>>>> >>>>>> The problematic code path is: >>>>>> __primary_switched >>>>>> early_fdt_map->fixmap_remap_fdt >>>>>> create_mapping_noalloc->alloc_init_pud >>>>>> mutex_lock (with Jianyong's patch) >>>>>> >>>>>> The problem seems to be that we will clear BSS segment twice if >>>>>> kaslr is enabled. Hence, some of the static variables in lockdep >>>>>> init process were messed up. That is to said, with kaslr enabled we >>>>>> might initialize lockdep twice if we add mutex_lock/unlock in >> alloc_init_pud(). >>>>>> >>>>> >>>>> Thanks for tracking that down. >>>>> >>>>> Note that clearing the BSS twice is not the root problem here. The >>>>> root problem is that we set global state while the kernel runs at >>>>> the default link time address, and then refer to it again after the >>>>> entire kernel has been shifted in the kernel VA space. Such global >>>>> state could consist of mutable pointers to statically allocated data >>>>> (which would be reset to their default values after the relocation code >> runs again), or global pointer variables in BSS. >>>>> In either case, relying on such a global variable after the second >>>>> relocation performed by KASLR would be risky, and so we should avoid >>>>> manipulating global state at all if it might involve pointer to >>>>> statically allocated data structures. >>>>> >>>>>> In other ways, if we invoke mutex_lock/unlock in such a early booting >> stage. >>>>>> It might be unsafe because lockdep inserts lock_acquire/release as >>>>>> the complex hooks. >>>>>> >>>>>> In summary, would it better if Jianyong splits these early boot and >>>>>> late boot case? e.g. introduce a nolock version for >>>>> create_mapping_noalloc(). >>>>>> >>>>>> What do you think of it? >>>>>> >>>>> >>>>> The pre-KASLR case definitely doesn't need a lock. But given that >>>>> create_mapping_noalloc() is only used to map the FDT, which happens >>>>> very early one way or the other, wouldn't it be better to move the >>>>> lock/unlock into other callers of __create_pgd_mapping()? (and make >>>>> sure no other users of the fixmap slots exist) >>>> >>>> There are server callers of __create_pgd_mapping. I think some of them >> need no fixmap lock as they are called so early. I figure out all of them here: >>>> create_mapping_noalloc: no lock >>>> create_pgd_mapping: no lock >>>> __map_memblock: no lock >>>> map_kernel_segment: no lock >>>> map_entry_trampoline: no lock >>>> update_mapping_prot: need lock >>>> arch_add_memory: need lock >>>> >>>> WDYT? >>>> >>> >>> That seems reasonable, but it needs to be documented clearly in the code. >>> >> >> Just a random thought, could we rely on system_state to do the locking >> conditionally? > > I can't see the point. At the early stages of kernel boot, we definitely need no lock. Also, I think we should keep it simple. >
Is e.g.,
if (system_state < SYSTEM_RUNNING) /* lock */
if (system_state < SYSTEM_RUNNING) /* unlock */
more complicated than checking individual users and eventually getting it wrong?
> Thanks > Jianyong
-- Thanks,
David / dhildenb
| |