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

[Mike Rapoport]

On Wed, Jul 15, 2026 at 11:22:33AM +0000, Brendan Jackman wrote:
> 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

Right :)
Supporting 32-bit without HIGHMEM is easy, so I don't think we should limit
it to 64BIT at this point.

> (conveniently for me) happens to align with what I did in the [PATCH].
> So... could we go ahead with this?


-- 
Sincerely yours,
Mike.