linux-kernel - Re: [PATCH v1] arm64: mm: Don't sleep in split_kernel_leaf

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]

Message-ID: <8df1d593-f176-422d-8b87-844986ac38a6@arm.com>
Date: Mon, 3 Nov 2025 17:28:31 +0000
From: Ryan Roberts <ryan.roberts@....com>
To: "David Hildenbrand (Red Hat)" <david@...nel.org>,
 catalin.marinas@....com, will@...nel.org, yang@...amperecomputing.com,
 ardb@...nel.org, dev.jain@....com, scott@...amperecomputing.com,
 cl@...two.org
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 Guenter Roeck <groeck@...gle.com>
Subject: Re: [PATCH v1] arm64: mm: Don't sleep in split_kernel_leaf_mapping()
 when in atomic context

On 03/11/2025 15:38, David Hildenbrand (Red Hat) wrote:
> 
>>   }
>>
>> +static inline bool force_pte_mapping(void)
>> +{
>> +    bool bbml2 = system_capabilities_finalized() ?
>> +        system_supports_bbml2_noabort() : cpu_supports_bbml2_noabort();
> 
> You are only moving this function. Still, there is some room for improvement I
> want to point out :)
> 
> bbml2 could be a const (or a helper function like bbml2_supported).
> 
>> +
>> +    return (!bbml2 && (rodata_full || arm64_kfence_can_set_direct_map() ||
>> +               is_realm_world())) ||
>> +        debug_pagealloc_enabled();
> 
> 
> I suspect this could be made a bit easier to read.
> 
>     if (debug_pagealloc_enabled())
>         return true;
>     if (bbml2)
>         return false;
>     return rodata_full || arm64_kfence_can_set_direct_map() || is_realm_world();

Yeah, I guess that's a bit nicer. I'd prefer to tidy it up in as separate commit
though. (feel free ;-) )

> 
> 
>> +}
>> +
>>   static DEFINE_MUTEX(pgtable_split_lock);
>>
>>   int split_kernel_leaf_mapping(unsigned long start, unsigned long end)
>> @@ -723,6 +733,16 @@ int split_kernel_leaf_mapping(unsigned long start,
>> unsigned long end)
>>       if (!system_supports_bbml2_noabort())
>>           return 0;
>>
>> +    /*
>> +     * If the region is within a pte-mapped area, there is no need to try to
>> +     * split. Additionally, CONFIG_DEBUG_PAGEALLOC and CONFIG_KFENCE may
>> +     * change permissions from softirq context so for those cases (which are
>> +     * always pte-mapped), we must not go any further because taking the
>> +     * mutex below may sleep.
>> +     */
>> +    if (force_pte_mapping() || is_kfence_address((void *)start))
>> +        return 0;
>> +
> 
> We're effectively performing two system_supports_bbml2_noabort() checks,
> similarly in
> arch_kfence_init_pool().
> 
> I wonder if there is a clean way to avoid that.

I thought about this too. But system_supports_bbml2_noabort() is actually a
magic alternatives patching thing; the code is updated so it's zero overhead. I
decided this was the simplest and clearest way to do it. But I'm open to other
ideas...

> 
> I'm not super up-to-date on that code. Nothing else jumped at me.

Thanks for the review!

>