Re: [PATCH] mm/secretmem: disable under HIGHMEM

[Brendan Jackman]

On Mon Jul 6, 2026 at 8:42 AM UTC, David Hildenbrand (Arm) wrote:
> On 7/5/26 13:34, Mike Rapoport wrote:
>> On Sun, Jul 05, 2026 at 10:46:19AM +0000, Brendan Jackman wrote:
>>> On Sun Jul 5, 2026 at 2:26 AM UTC, Andrew Morton wrote:
>>>>
>>>> Well OK, but the secretmem code is still wrong.  The patch protects
>>>> people from hitting the bug but leaves the bug in place.  Surely it would be
>>>> better to fix the bug?
>>>
>>> I don't think the code is wrong if highmem is disabled. Certainly
>>> there is an implicit coupling between the .c file and the Kconfig file,
>>> but we could always add a BUILD_BUG_ON(IS_ENABLED(CONFIG_SECRETMEM)) to
>>> the relevant bit of code to make it explicit.
>>>
>>>> Is that as simple as adding the folio_test_highmem() test?  
>>>
>>> This would fix the WARN+SIGBUS but I don't think it resolves the fact
>>> that this configuration is completely untested - there are likely other
>>> functional bugs? But more importantly, I am not sure if secretmem
>>> actually does its security job if kmap_local_page() isn't a NOP. I
>>> think shipping a "security feature" that doesn't do what it says would
>>> be really terrible. (It might work totally fine, I dunno, but it would
>>> require some research and deep thinking that I don't really want to do
>>> for a configuration with no users).
>>>
>>>> Or switching to GFP_KERNEL?  
>>>
>>> ... Oh, that's a nice idea though :)
>> 
>> GFP_USER if anything :)
>
> Right.
>
>> 
>> But still with kmap() and friends not being an NOP the promise "kernel does
>> not map this memory" does not hold.
>> 
>> I think that keeping SECRETMEM and HIGHMEM mutually exclusive is
>> conceptually correct.
>
> We could even limit it to 64BIT ;)

I fear this in limbo, we have quite a wide range of opinions from "fix
the broken configuration" all the way to "disable secretmem completely on
32bit". I'm not passionately committed to any one answer but I do think
we need to pick something.

Mike's position seems to be roughly "in the middle" of the spectrum, and
(conveniently for me) happens to align with what I did in the [PATCH].
So... could we go ahead with this?