Re: [PATCH] arm64/mm: Drop redundant BUG_ON(!pgtable_alloc)

[Robin Murphy <robin.murphy@arm.com>]

On 2022-11-21 12:27, Mark Rutland wrote:
> On Mon, Nov 21, 2022 at 11:00:42AM +0530, Anshuman Khandual wrote:
>> Hello Nathan,
>>
>> Thanks for the report.
>>
>> On 11/20/22 21:46, Nathan Chancellor wrote:
>>> Hi Anshuman,
> 
>>> I just bisected a boot failure in our QEMU-based continuous integration
>>> setup to this change as commit 9ed2b4616d4e ("arm64/mm: Drop redundant
>>> BUG_ON(!pgtable_alloc)") in the arm64 tree. There is no output so the
>>> panic clearly happens early at boot. If I move back to the previous
>>> commit and add a WARN_ON() like so:
>>>
>>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>>> index d386033a074c..9280a92ff920 100644
>>> --- a/arch/arm64/mm/mmu.c
>>> +++ b/arch/arm64/mm/mmu.c
>>> @@ -383,6 +383,7 @@ static void __create_pgd_mapping_locked(pgd_t *pgdir, phys_addr_t phys,
>>>   	phys &= PAGE_MASK;
>>>   	addr = virt & PAGE_MASK;
>>>   	end = PAGE_ALIGN(virt + size);
>>> +	WARN_ON(!pgtable_alloc);
>>>   
>>>   	do {
>>>   		next = pgd_addr_end(addr, end);
>>>
>>> I do see some stacktraces. I have attached the boot log from QEMU.
>>>
>>> If there is any additional information I can provide or patches I can
>>> test, I am more than happy to do so.
>>
>> There are couple of instances, where __create_pgd_mapping() function gets called
>> without a valid pgtable alloc function (NULL is passed on instead), as it is not
>> expected to allocate page table pages, during the mapping process. The following
>> change after this patch should solve the reported problem.
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index 9ea8e9039992..a00563122fcb 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -42,6 +42,7 @@
>>   #define NO_BLOCK_MAPPINGS      BIT(0)
>>   #define NO_CONT_MAPPINGS       BIT(1)
>>   #define NO_EXEC_MAPPINGS       BIT(2)  /* assumes FEAT_HPDS is not used */
>> +#define NO_ALLOC_MAPPINGS      BIT(3)  /* does not allocate page table pages */
>>   
>>   int idmap_t0sz __ro_after_init;
>>   
>> @@ -380,7 +381,7 @@ static void __create_pgd_mapping_locked(pgd_t *pgdir, phys_addr_t phys,
>>          phys &= PAGE_MASK;
>>          addr = virt & PAGE_MASK;
>>          end = PAGE_ALIGN(virt + size);
>> -       BUG_ON(!pgtable_alloc);
>> +       BUG_ON(!(flags & NO_ALLOC_MAPPINGS) && !pgtable_alloc);
>>   
>>          do {
>>                  next = pgd_addr_end(addr, end);
>> @@ -453,7 +454,7 @@ static void __init create_mapping_noalloc(phys_addr_t phys, unsigned long virt,
>>                  return;
>>          }
>>          __create_pgd_mapping(init_mm.pgd, phys, virt, size, prot, NULL,
>> -                            NO_CONT_MAPPINGS);
>> +                            NO_CONT_MAPPINGS | NO_ALLOC_MAPPINGS);
>>   }
>>   
>>   void __init create_pgd_mapping(struct mm_struct *mm, phys_addr_t phys,
>> @@ -481,7 +482,7 @@ static void update_mapping_prot(phys_addr_t phys, unsigned long virt,
>>          }
>>   
>>          __create_pgd_mapping(init_mm.pgd, phys, virt, size, prot, NULL,
>> -                            NO_CONT_MAPPINGS);
>> +                            NO_CONT_MAPPINGS | NO_ALLOC_MAPPINGS);
>>   
>>          /* flush the TLBs after updating live kernel mappings */
>>          flush_tlb_kernel_range(virt, virt + size);
> 
> This is now more complicated than what we had originally, and it doesn't catch
> the case where the caller sets NO_ALLOC_MAPPINGS but the callee ends up needing
> to perform an allocation, which the old code would have caught.

Well, it's still "caught" as such - all that BUG_ON(!pgtable_alloc) does 
in these cases is encode the source location in the backtrace, vs. 
having to decode it (if necessary) from the LR in a backtrace from 
immediately dereferencing pgtable_alloc(). If that happens before the 
user has a console up then the difference is moot anyway.

Cheers,
Robin.